Skip to content

Commit

Permalink
fix(grpc): import the same proto from multiple linked proto files (#1749
Browse files Browse the repository at this point in the history
)
  • Loading branch information
meskill authored Apr 18, 2024
1 parent 3450eff commit f89a888
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 129 deletions.
20 changes: 11 additions & 9 deletions src/blueprint/operators/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,15 +165,17 @@ pub fn compile_grpc(inputs: CompileGrpc) -> Valid<Expression, String> {

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 {
Expand Down
109 changes: 8 additions & 101 deletions src/config/config_module.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -43,7 +43,7 @@ impl<A> Deref for Content<A> {
#[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<FileDescriptorSet>,
pub grpc_file_descriptors: HashMap<String, FileDescriptorProto>,

/// Contains the contents of the JS file
pub script: Option<String>,
Expand All @@ -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 {
Expand Down Expand Up @@ -163,93 +160,3 @@ impl From<Config> 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()
})
);
}
}
}
}
11 changes: 3 additions & 8 deletions src/grpc/data_loader_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
4 changes: 1 addition & 3 deletions src/grpc/protobuf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,7 @@ pub mod tests {
.resolve(config, None)
.await?
.extensions
.get_file_descriptor_set()
.unwrap()
.to_owned())
.get_file_descriptor_set())
}

#[test]
Expand Down
4 changes: 1 addition & 3 deletions src/grpc/request_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,7 @@ mod tests {
.await
.unwrap()
.extensions
.get_file_descriptor_set()
.unwrap()
.clone(),
.get_file_descriptor_set(),
)
.unwrap();

Expand Down
12 changes: 11 additions & 1 deletion src/merge_right.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::sync::Arc;

pub trait MergeRight {
Expand Down Expand Up @@ -67,3 +67,13 @@ where
self
}
}

impl<K, V> MergeRight for HashMap<K, V>
where
K: Eq + std::hash::Hash,
{
fn merge_right(mut self, other: Self) -> Self {
self.extend(other);
self
}
}
7 changes: 3 additions & 4 deletions tests/execution/grpc-proto-with-same-package.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
}
```

Expand Down

1 comment on commit f89a888

@github-actions
Copy link

Choose a reason for hiding this comment

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

Running 30s test @ http://localhost:8000/graphql

4 threads and 100 connections

Thread Stats Avg Stdev Max +/- Stdev
Latency 7.32ms 3.41ms 150.88ms 72.91%
Req/Sec 3.46k 117.31 3.85k 86.75%

412844 requests in 30.01s, 2.07GB read

Requests/sec: 13756.02

Transfer/sec: 70.61MB

Please sign in to comment.