Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: grpc dependency path resolution #1697

Closed
wants to merge 10 commits into from
Closed

fix: grpc dependency path resolution #1697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e6917f7
--wip-- [skip ci]
tusharmath Apr 10, 2024
0a48f7a
fixing proto relative path bug
shashitnak Apr 10, 2024
8ed500a
chore(deps): bump h2 from 0.3.24 to 0.3.26 (#1685)
dependabot[bot] Apr 10, 2024
ee580c3
removing includes
shashitnak Apr 10, 2024
7509e65
fixing some tests
shashitnak Apr 10, 2024
f0dc98c
Revert "removing includes"
shashitnak Apr 10, 2024
5b813b1
manually loading proto files for handling urls
shashitnak Apr 10, 2024
bab61c4
fixing tests
shashitnak Apr 11, 2024
246b208
style: fmt
amitksingh1490 Apr 11, 2024
58107e6
adding back the comment
shashitnak Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions src/config/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,13 @@ use crate::runtime::TargetRuntime;
pub struct ConfigReader {
runtime: TargetRuntime,
resource_reader: ResourceReader,
proto_reader: ProtoReader,
}

impl ConfigReader {
pub fn init(runtime: TargetRuntime) -> Self {
Self {
runtime: runtime.clone(),
resource_reader: ResourceReader::init(runtime.clone()),
proto_reader: ProtoReader::init(runtime),
}
}

Expand Down Expand Up @@ -80,7 +78,12 @@ impl ConfigReader {
}
LinkType::Protobuf => {
let path = Self::resolve_path(&link.src, parent_dir);
let meta = self.proto_reader.read(path).await?;
let meta = ProtoReader::init(self.runtime.clone(), &[path])
.load()
.await?
.into_iter()
.next()
.ok_or(anyhow::anyhow!("No Proto file found"))?;
config_module
.extensions
.grpc_file_descriptors
Expand Down
16 changes: 9 additions & 7 deletions src/generator/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ use crate::proto_reader::ProtoReader;
use crate::runtime::TargetRuntime;

pub struct Generator {
proto_reader: ProtoReader,
runtime: TargetRuntime,
}
impl Generator {
pub fn init(runtime: TargetRuntime) -> Self {
Self { proto_reader: ProtoReader::init(runtime) }
Self { runtime }
}

pub async fn read_all<T: AsRef<str>>(
Expand All @@ -21,13 +21,15 @@ impl Generator {
files: &[T],
query: &str,
) -> Result<Config> {
let mut config = Config::default();
let mut links = vec![];
let proto_metadata = self.proto_reader.read_all(files).await?;

let mut config = Config::default();
for metadata in proto_metadata {
match input_source {
Source::PROTO => {
match input_source {
Source::PROTO => {
let mut proto_reader = ProtoReader::init(self.runtime.clone(), files);

let proto_metadata_list = proto_reader.load().await?;
for metadata in proto_metadata_list {
links.push(Link { id: None, src: metadata.path, type_of: LinkType::Protobuf });
config = config.merge_right(from_proto(&[metadata.descriptor_set], query));
}
Expand Down
7 changes: 7 additions & 0 deletions src/generator/proto/greetings.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
syntax = "proto3";

package greetings;
import "greetings_message.proto";
service Greeter {
rpc SayHello (greetings.HelloRequest) returns (greetings.HelloReply) {}
}
11 changes: 11 additions & 0 deletions src/generator/proto/greetings_message.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
syntax = "proto3";

package greetings;

message HelloRequest {
string name = 1;
}

message HelloReply {
string message = 1;
}
86 changes: 63 additions & 23 deletions src/proto_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::collections::{HashMap, VecDeque};
use std::path::{Path, PathBuf};

use anyhow::Context;
use futures_util::future::join_all;
Expand All @@ -10,6 +11,9 @@ use crate::runtime::TargetRuntime;

pub struct ProtoReader {
resource_reader: ResourceReader,
descriptors: HashMap<String, FileDescriptorProto>,
files: Vec<String>,
includes: Vec<PathBuf>,
}

pub struct ProtoMetadata {
Expand All @@ -18,61 +22,94 @@ pub struct ProtoMetadata {
}

impl ProtoReader {
pub fn init(runtime: TargetRuntime) -> Self {
Self { resource_reader: ResourceReader::init(runtime) }
pub fn init<T: AsRef<str>>(runtime: TargetRuntime, paths: &[T]) -> Self {
Self {
resource_reader: ResourceReader::init(runtime),
descriptors: HashMap::new(),
files: paths.iter().map(|path| path.as_ref().to_string()).collect(),
includes: paths
.iter()
.filter_map(|path| {
Path::new(path.as_ref())
.ancestors()
.nth(1)
.map(Path::to_path_buf)
})
.collect(),
}
Comment on lines +25 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the includes computation in the init method for clarity. Consider using a helper function to encapsulate the logic of extracting the parent directory, which will make the init method cleaner and easier to understand.

-            includes: paths
-                .iter()
-                .filter_map(|path| {
-                    Path::new(path.as_ref())
-                        .ancestors().nth(1)
-                        .map(Path::to_path_buf)
-                })
-                .collect(),
+            includes: paths.iter().map(|path| get_parent_directory(path)).collect(),
+
+fn get_parent_directory<T: AsRef<str>>(path: &T) -> PathBuf {
+    Path::new(path.as_ref()).ancestors().nth(1).unwrap().to_path_buf()
+}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
pub fn init<T: AsRef<str>>(runtime: TargetRuntime, paths: &[T]) -> Self {
Self {
resource_reader: ResourceReader::init(runtime),
descriptors: HashMap::new(),
files: paths.iter().map(|path| path.as_ref().to_string()).collect(),
includes: paths
.iter()
.filter_map(|path| {
Path::new(path.as_ref())
.ancestors()
.nth(1)
.map(Path::to_path_buf)
})
.collect(),
}
pub fn init<T: AsRef<str>>(runtime: TargetRuntime, paths: &[T]) -> Self {
Self {
resource_reader: ResourceReader::init(runtime),
descriptors: HashMap::new(),
files: paths.iter().map(|path| path.as_ref().to_string()).collect(),
includes: paths.iter().map(|path| get_parent_directory(path)).collect(),
}
}
fn get_parent_directory<T: AsRef<str>>(path: &T) -> PathBuf {
Path::new(path.as_ref()).ancestors().nth(1).unwrap().to_path_buf()
}

}

pub async fn read_all<T: AsRef<str>>(&self, paths: &[T]) -> anyhow::Result<Vec<ProtoMetadata>> {
let resolved_protos = join_all(paths.iter().map(|v| self.read(v.as_ref())))
.await
.into_iter()
.collect::<anyhow::Result<Vec<_>>>()?;
Ok(resolved_protos)
pub async fn load(&mut self) -> anyhow::Result<Vec<ProtoMetadata>> {
let mut metadata = Vec::new();

for file in self.files.clone().iter() {
metadata.push(self.read(file).await?);
}

Ok(metadata)
Comment on lines +42 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize the load method by avoiding unnecessary cloning of the files vector. Use iter() directly since the vector does not need to be owned in this context.

-        for file in self.files.clone().iter() {
+        for file in self.files.iter() {
            metadata.push(self.read(file).await?);
        }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
pub async fn load(&mut self) -> anyhow::Result<Vec<ProtoMetadata>> {
let mut metadata = Vec::new();
for file in self.files.clone().iter() {
metadata.push(self.read(file).await?);
}
Ok(metadata)
pub async fn load(&mut self) -> anyhow::Result<Vec<ProtoMetadata>> {
let mut metadata = Vec::new();
for file in self.files.iter() {
metadata.push(self.read(file).await?);
}
Ok(metadata)

}

pub async fn read<T: AsRef<str>>(&self, path: T) -> anyhow::Result<ProtoMetadata> {
let file_read = self.read_proto(path.as_ref()).await?;
if file_read.package.is_none() {
pub async fn read(&mut self, path: &str) -> anyhow::Result<ProtoMetadata> {
let proto = self.read_proto(path).await?;
if proto.package.is_none() {
anyhow::bail!("Package name is required");
}

let descriptors = self.resolve_descriptors(file_read).await?;
let descriptors = self.resolve_dependencies(proto, path).await?;
let metadata = ProtoMetadata {
descriptor_set: FileDescriptorSet { file: descriptors },
path: path.as_ref().to_string(),
path: path.to_string(),
Comment on lines +52 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that the error message in the read method is clear and actionable when the package name is missing from the proto file.

-            anyhow::bail!("Package name is required");
+            anyhow::bail!("Package name is required in the proto file: {}", path);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
pub async fn read(&mut self, path: &str) -> anyhow::Result<ProtoMetadata> {
let proto = self.read_proto(path).await?;
if proto.package.is_none() {
anyhow::bail!("Package name is required");
}
let descriptors = self.resolve_descriptors(file_read).await?;
let descriptors = self.resolve_dependencies(proto, path).await?;
let metadata = ProtoMetadata {
descriptor_set: FileDescriptorSet { file: descriptors },
path: path.as_ref().to_string(),
path: path.to_string(),
pub async fn read(&mut self, path: &str) -> anyhow::Result<ProtoMetadata> {
let proto = self.read_proto(path).await?;
if proto.package.is_none() {
anyhow::bail!("Package name is required in the proto file: {}", path);
}
let descriptors = self.resolve_dependencies(proto, path).await?;
let metadata = ProtoMetadata {
descriptor_set: FileDescriptorSet { file: descriptors },
path: path.to_string(),

};
Ok(metadata)
}

/// Performs BFS to import all nested proto files
async fn resolve_descriptors(
&self,
async fn resolve_dependencies(
&mut self,
parent_proto: FileDescriptorProto,
parent_path: &str,
) -> anyhow::Result<Vec<FileDescriptorProto>> {
let mut descriptors: HashMap<String, FileDescriptorProto> = HashMap::new();
let mut queue = VecDeque::new();
let parent_path = Path::new(parent_path).ancestors().nth(1).unwrap();
println!("{}", parent_path.display());

queue.push_back(parent_proto.clone());

while let Some(file) = queue.pop_front() {
let futures: Vec<_> = file
.dependency
.iter()
.map(|import| self.read_proto(import))
.map(|import| async {
let import = import.clone();
let import = self
.includes
.iter()
.find_map(|include| {
let path = include.join(&import);
path.exists().then_some(path)
})
.unwrap_or_else(|| PathBuf::from(import));
self.read_proto(import.to_str().unwrap()).await
})
.collect();

let results = join_all(futures).await;

for result in results {
let proto = result?;
let descriptors = &mut self.descriptors;
if descriptors.get(proto.name()).is_none() {
queue.push_back(proto.clone());
descriptors.insert(proto.name().to_string(), proto);
}
}
}

let mut descriptors_vec = descriptors
.into_values()
let hash_map = &mut self.descriptors;

let mut descriptors_vec = hash_map
.values()
.cloned()
Comment on lines +67 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling in the resolve_dependencies method to prevent potential runtime panics. Specifically, handle cases where ancestors().nth(1) might return None, which currently leads to an unwrap() call that could panic.

-        let parent_path = Path::new(parent_path).ancestors().nth(1).unwrap();
+        let parent_path = Path::new(parent_path).ancestors().nth(1).ok_or_else(|| anyhow::Error::msg("Parent path not found"))?;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
async fn resolve_dependencies(
&mut self,
parent_proto: FileDescriptorProto,
parent_path: &str,
) -> anyhow::Result<Vec<FileDescriptorProto>> {
let mut descriptors: HashMap<String, FileDescriptorProto> = HashMap::new();
let mut queue = VecDeque::new();
let parent_path = Path::new(parent_path).ancestors().nth(1).unwrap();
println!("{}", parent_path.display());
queue.push_back(parent_proto.clone());
while let Some(file) = queue.pop_front() {
let futures: Vec<_> = file
.dependency
.iter()
.map(|import| self.read_proto(import))
.map(|import| async {
let import = import.clone();
let import = self
.includes
.iter()
.find_map(|include| {
let path = include.join(&import);
path.exists().then_some(path)
})
.unwrap_or_else(|| PathBuf::from(import));
self.read_proto(import.to_str().unwrap()).await
})
.collect();
let results = join_all(futures).await;
for result in results {
let proto = result?;
let descriptors = &mut self.descriptors;
if descriptors.get(proto.name()).is_none() {
queue.push_back(proto.clone());
descriptors.insert(proto.name().to_string(), proto);
}
}
}
let mut descriptors_vec = descriptors
.into_values()
let hash_map = &mut self.descriptors;
let mut descriptors_vec = hash_map
.values()
.cloned()
async fn resolve_dependencies(
&mut self,
parent_proto: FileDescriptorProto,
parent_path: &str,
) -> anyhow::Result<Vec<FileDescriptorProto>> {
let mut queue = VecDeque::new();
let parent_path = Path::new(parent_path).ancestors().nth(1).ok_or_else(|| anyhow::Error::msg("Parent path not found"))?;
println!("{}", parent_path.display());
queue.push_back(parent_proto.clone());
while let Some(file) = queue.pop_front() {
let futures: Vec<_> = file
.dependency
.iter()
.map(|import| async {
let import = import.clone();
let import = self
.includes
.iter()
.find_map(|include| {
let path = include.join(&import);
path.exists().then_some(path)
})
.unwrap_or_else(|| PathBuf::from(import));
self.read_proto(import.to_str().unwrap()).await
})
.collect();
let results = join_all(futures).await;
for result in results {
let proto = result?;
let descriptors = &mut self.descriptors;
if descriptors.get(proto.name()).is_none() {
queue.push_back(proto.clone());
descriptors.insert(proto.name().to_string(), proto);
}
}
}
let hash_map = &mut self.descriptors;
let mut descriptors_vec = hash_map
.values()
.cloned()

.collect::<Vec<FileDescriptorProto>>();
descriptors_vec.push(parent_proto);
Ok(descriptors_vec)
Expand Down Expand Up @@ -104,7 +141,10 @@ mod test_proto_config {
#[tokio::test]
async fn test_resolve() {
// Skipping IO tests as they are covered in reader.rs
let reader = ProtoReader::init(crate::runtime::test::init(None));
let reader = ProtoReader::init(
crate::runtime::test::init(None),
&["google/protobuf/empty.proto"],
);
reader
.read_proto("google/protobuf/empty.proto")
.await
Expand Down Expand Up @@ -133,9 +173,9 @@ mod test_proto_config {
let runtime = crate::runtime::test::init(None);
let file_rt = runtime.file.clone();

let reader = ProtoReader::init(runtime);
let mut reader = ProtoReader::init(runtime, &[&test_file]);
let helper_map = reader
.resolve_descriptors(reader.read_proto(&test_file).await?)
.resolve_dependencies(reader.read_proto(&test_file).await?, &test_file)
.await?;
let files = test_dir.read_dir()?;
for file in files {
Expand Down Expand Up @@ -177,9 +217,9 @@ mod test_proto_config {
#[tokio::test]
async fn test_proto_no_pkg() -> Result<()> {
let runtime = crate::runtime::test::init(None);
let reader = ProtoReader::init(runtime);
let mut proto_no_pkg = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
proto_no_pkg.push("src/grpc/tests/proto_no_pkg.graphql");
let mut reader = ProtoReader::init(runtime, &[proto_no_pkg.display().to_string()]);
let config_module = reader.read(proto_no_pkg.to_str().unwrap()).await;
assert!(config_module.is_err());
Ok(())
Expand Down
Loading