fix: grpc dependency path resolution #1697
Conversation
github-actions
bot
added
the
type: fix
WalkthroughWalkthroughThe changes primarily involve updating the Changes
Assessment against linked issues
The changes effectively address the issues related to Proto file resolution and dependency management by incorporating Protox's capabilities, ensuring that Proto files are found and handled correctly. Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (5)
Files skipped from review as they are similar to previous changes (4)
Additional Context UsedPath-based Instructions (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1697 +/- ##
==========================================
+ Coverage 87.18% 87.21% +0.03%
==========================================
Files 149 149
Lines 15275 15314 +39
==========================================
+ Hits 13317 13356 +39
Misses 1958 1958 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
src/proto_reader.rs
Outdated
| 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(), | ||
| } |
There was a problem hiding this comment.
Refactor the init method to improve clarity and maintainability. Consider using more descriptive variable names and possibly splitting complex logic into smaller functions.
- 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.
| 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() | |
| } |
amitksingh1490
force-pushed
the
grpc-fix-2
branch
from
4da93dc to
da59077
Compare
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This reverts commit 9660900.
amitksingh1490
force-pushed
the
grpc-fix-2
branch
from
da59077 to
58107e6
Compare
| 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(), | ||
| } |
There was a problem hiding this comment.
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.
| 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 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) |
There was a problem hiding this comment.
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.
| 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) |
| 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() |
There was a problem hiding this comment.
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.
| 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() |
| 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(), |
There was a problem hiding this comment.
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.
| 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(), |
|
Action required: PR inactive for 2 days. |
github-actions
bot
added
the
state: inactive
|
fixed in #1705 |
Summary:
fixes #1686
Fixing the path resolution for imported files in the proto file
Build & Testing:
cargo testsuccessfully../lint.sh --mode=fixto fix all linting issues raised by./lint.sh --mode=check.Checklist:
<type>(<optional scope>): <title>