From f89a888433e08fcf0b9f42e3f654314798a2974d Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Thu, 18 Apr 2024 11:48:25 +0200 Subject: [PATCH] fix(grpc): import the same proto from multiple linked proto files (#1749) --- src/blueprint/operators/grpc.rs | 20 ++-- src/config/config_module.rs | 109 ++---------------- src/grpc/data_loader_request.rs | 11 +- src/grpc/protobuf.rs | 4 +- src/grpc/request_template.rs | 4 +- src/merge_right.rs | 12 +- .../execution/grpc-proto-with-same-package.md | 7 +- 7 files changed, 38 insertions(+), 129 deletions(-) diff --git a/src/blueprint/operators/grpc.rs b/src/blueprint/operators/grpc.rs index e33cfd0cab..3029469519 100644 --- a/src/blueprint/operators/grpc.rs +++ b/src/blueprint/operators/grpc.rs @@ -165,15 +165,17 @@ pub fn compile_grpc(inputs: CompileGrpc) -> Valid { Valid::from(GrpcMethod::try_from(grpc.method.as_str())) .and_then(|method| { - Valid::from_option( - config_module.extensions.get_file_descriptor_set(), - "Protobuf files were not specified in the config".to_string(), - ) - .and_then(|file_descriptor_set| to_operation(&method, file_descriptor_set.clone())) - .fuse(to_url(grpc, &method, config_module)) - .fuse(helpers::headers::to_mustache_headers(&grpc.headers)) - .fuse(helpers::body::to_body(grpc.body.as_deref())) - .into() + let file_descriptor_set = config_module.extensions.get_file_descriptor_set(); + + if file_descriptor_set.file.is_empty() { + return Valid::fail("Protobuf files were not specified in the config".to_string()); + } + + to_operation(&method, file_descriptor_set) + .fuse(to_url(grpc, &method, config_module)) + .fuse(helpers::headers::to_mustache_headers(&grpc.headers)) + .fuse(helpers::body::to_body(grpc.body.as_deref())) + .into() }) .and_then(|(operation, url, headers, body)| { let validation = if validate_with_schema { diff --git a/src/config/config_module.rs b/src/config/config_module.rs index bab6be8642..12328ac5c8 100644 --- a/src/config/config_module.rs +++ b/src/config/config_module.rs @@ -1,10 +1,10 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::ops::Deref; use std::sync::Arc; use derive_setters::Setters; use jsonwebtoken::jwk::JwkSet; -use prost_reflect::prost_types::FileDescriptorSet; +use prost_reflect::prost_types::{FileDescriptorProto, FileDescriptorSet}; use rustls_pki_types::{CertificateDer, PrivateKeyDer}; use crate::config::Config; @@ -43,7 +43,7 @@ impl Deref for Content { #[derive(Clone, Debug, Default, MergeRight)] pub struct Extensions { /// Contains the file descriptor set resolved from the links to proto files - pub grpc_file_descriptor_set: Option, + pub grpc_file_descriptors: HashMap, /// Contains the contents of the JS file pub script: Option, @@ -64,17 +64,14 @@ pub struct Extensions { impl Extensions { pub fn add_proto(&mut self, metadata: ProtoMetadata) { - if let Some(set) = self.grpc_file_descriptor_set.as_mut() { - set.file.extend(metadata.descriptor_set.file); - } else { - let _ = self - .grpc_file_descriptor_set - .insert(metadata.descriptor_set); + for file in metadata.descriptor_set.file { + self.grpc_file_descriptors + .insert(file.name().to_string(), file); } } - pub fn get_file_descriptor_set(&self) -> Option<&FileDescriptorSet> { - self.grpc_file_descriptor_set.as_ref() + pub fn get_file_descriptor_set(&self) -> FileDescriptorSet { + FileDescriptorSet { file: self.grpc_file_descriptors.values().cloned().collect() } } pub fn has_auth(&self) -> bool { @@ -163,93 +160,3 @@ impl From for ConfigModule { ConfigModule { config, input_types, output_types, ..Default::default() } } } - -#[cfg(test)] -mod tests { - mod extensions { - mod merge_right { - use std::path::Path; - - use prost_reflect::prost_types::FileDescriptorSet; - - use crate::config::Extensions; - use crate::merge_right::MergeRight; - - #[test] - fn grpc_file_descriptor_set_none() { - let extensions1 = Extensions::default(); - let extensions2 = Extensions::default(); - - assert_eq!( - extensions1 - .merge_right(extensions2) - .grpc_file_descriptor_set, - None - ); - } - - #[test] - fn grpc_file_descriptor_set_single() { - let greetings_path = Path::new("src/grpc/tests/proto/greetings.proto"); - - let file_descriptor_set = protox::compile([greetings_path], ["."]).unwrap(); - let extensions1 = Extensions { - grpc_file_descriptor_set: Some(file_descriptor_set.clone()), - ..Default::default() - }; - let extensions2 = Extensions::default(); - - assert_eq!( - extensions1 - .merge_right(extensions2) - .grpc_file_descriptor_set, - Some(file_descriptor_set.clone()) - ); - - let extensions1 = Extensions::default(); - let extensions2 = Extensions { - grpc_file_descriptor_set: Some(file_descriptor_set.clone()), - ..Default::default() - }; - - assert_eq!( - extensions1 - .merge_right(extensions2) - .grpc_file_descriptor_set, - Some(file_descriptor_set) - ); - } - - #[test] - fn grpc_file_descriptor_set_both() { - let greetings_path = Path::new("src/grpc/tests/proto/greetings.proto"); - let news_path = Path::new("src/grpc/tests/proto/news.proto"); - - let file_descriptor_set_greetings = - protox::compile([greetings_path], ["."]).unwrap(); - let file_descriptor_set_news = protox::compile([news_path], ["."]).unwrap(); - let extensions1 = Extensions { - grpc_file_descriptor_set: Some(file_descriptor_set_greetings.clone()), - ..Default::default() - }; - let extensions2 = Extensions { - grpc_file_descriptor_set: Some(file_descriptor_set_news.clone()), - ..Default::default() - }; - - assert_eq!( - extensions1 - .merge_right(extensions2) - .grpc_file_descriptor_set, - Some(FileDescriptorSet { - file: file_descriptor_set_greetings - .file - .into_iter() - .chain(file_descriptor_set_news.file) - .collect() - }) - ); - } - } - } -} diff --git a/src/grpc/data_loader_request.rs b/src/grpc/data_loader_request.rs index 70c9a15751..b36e2e9177 100644 --- a/src/grpc/data_loader_request.rs +++ b/src/grpc/data_loader_request.rs @@ -95,14 +95,9 @@ mod tests { let reader = ConfigReader::init(runtime); let config_module = reader.resolve(config, None).await.unwrap(); - let protobuf_set = ProtobufSet::from_proto_file( - config_module - .extensions - .get_file_descriptor_set() - .unwrap() - .clone(), - ) - .unwrap(); + let protobuf_set = + ProtobufSet::from_proto_file(config_module.extensions.get_file_descriptor_set()) + .unwrap(); let service = protobuf_set.find_service(&method).unwrap(); diff --git a/src/grpc/protobuf.rs b/src/grpc/protobuf.rs index ec9ed9fca6..05295e736e 100644 --- a/src/grpc/protobuf.rs +++ b/src/grpc/protobuf.rs @@ -299,9 +299,7 @@ pub mod tests { .resolve(config, None) .await? .extensions - .get_file_descriptor_set() - .unwrap() - .to_owned()) + .get_file_descriptor_set()) } #[test] diff --git a/src/grpc/request_template.rs b/src/grpc/request_template.rs index c09049340f..64b37aa988 100644 --- a/src/grpc/request_template.rs +++ b/src/grpc/request_template.rs @@ -169,9 +169,7 @@ mod tests { .await .unwrap() .extensions - .get_file_descriptor_set() - .unwrap() - .clone(), + .get_file_descriptor_set(), ) .unwrap(); diff --git a/src/merge_right.rs b/src/merge_right.rs index 43df242828..1c04d67b6a 100644 --- a/src/merge_right.rs +++ b/src/merge_right.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::sync::Arc; pub trait MergeRight { @@ -67,3 +67,13 @@ where self } } + +impl MergeRight for HashMap +where + K: Eq + std::hash::Hash, +{ + fn merge_right(mut self, other: Self) -> Self { + self.extend(other); + self + } +} diff --git a/tests/execution/grpc-proto-with-same-package.md b/tests/execution/grpc-proto-with-same-package.md index 93d6d613e4..fb44b0f99e 100644 --- a/tests/execution/grpc-proto-with-same-package.md +++ b/tests/execution/grpc-proto-with-same-package.md @@ -19,18 +19,17 @@ service FooService { ```protobuf @file:bar.proto syntax = "proto3"; -package test; +import "google/protobuf/empty.proto"; -message Input { +package test; -} message Bar { string bar = 1; } service BarService { - rpc GetBar (Input) returns (Bar) {} + rpc GetBar (google.protobuf.Empty) returns (Bar) {} } ```