fix(grpc): linking multiple proto files with the same package name

tailcallhq · Apr 15, 2024 · 15e648c · 15e648c
1 parent ede9a3a
commit 15e648c
Show file tree

Hide file tree

Showing 12 changed files with 232 additions and 40 deletions.
diff --git a/src/blueprint/operators/grpc.rs b/src/blueprint/operators/grpc.rs
@@ -166,8 +166,8 @@ 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(&method),
-                format!("File descriptor not found for method: {}", grpc.method),
+                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))

diff --git a/src/config/config_module.rs b/src/config/config_module.rs
@@ -7,9 +7,9 @@ use jsonwebtoken::jwk::JwkSet;
 use prost_reflect::prost_types::FileDescriptorSet;
 use rustls_pki_types::{CertificateDer, PrivateKeyDer};
 
-use crate::blueprint::GrpcMethod;
 use crate::config::Config;
 use crate::merge_right::MergeRight;
+use crate::proto_reader::ProtoMetadata;
 use crate::rest::{EndpointSet, Unchecked};
 use crate::scalar;
 
@@ -41,8 +41,8 @@ impl<A> Deref for Content<A> {
 /// an IO operation, i.e., reading a file, making an HTTP call, etc.
 #[derive(Clone, Debug, Default)]
 pub struct Extensions {
-    /// Contains the file descriptor sets resolved from the links
-    pub grpc_file_descriptors: Vec<Content<FileDescriptorSet>>,
+    /// Contains the file descriptor set resolved from the links to proto files
+    pub grpc_file_descriptor_set: Option<FileDescriptorSet>,
 
     /// Contains the contents of the JS file
     pub script: Option<String>,
@@ -62,28 +62,38 @@ pub struct Extensions {
 }
 
 impl Extensions {
-    pub fn get_file_descriptor_set(&self, grpc: &GrpcMethod) -> Option<&FileDescriptorSet> {
-        self.grpc_file_descriptors
-            .iter()
-            .find(|content| {
-                content
-                    .file
-                    .iter()
-                    .any(|file| file.package == Some(grpc.package.to_owned()))
-            })
-            .map(|a| &a.content)
+    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);
+        }
+    }
+
+    pub fn get_file_descriptor_set(&self) -> Option<&FileDescriptorSet> {
+        self.grpc_file_descriptor_set.as_ref()
     }
 
     pub fn has_auth(&self) -> bool {
         !self.htpasswd.is_empty() || !self.jwks.is_empty()
     }
 }
 
+impl MergeRight for FileDescriptorSet {
+    fn merge_right(mut self, other: Self) -> Self {
+        self.file.extend(other.file);
+
+        self
+    }
+}
+
 impl MergeRight for Extensions {
     fn merge_right(mut self, mut other: Self) -> Self {
-        self.grpc_file_descriptors = self
-            .grpc_file_descriptors
-            .merge_right(other.grpc_file_descriptors);
+        self.grpc_file_descriptor_set = self
+            .grpc_file_descriptor_set
+            .merge_right(other.grpc_file_descriptor_set);
         self.script = self.script.merge_right(other.script.take());
         self.cert = self.cert.merge_right(other.cert);
         self.keys = if !other.keys.is_empty() {

diff --git a/src/config/reader.rs b/src/config/reader.rs
@@ -81,13 +81,7 @@ impl ConfigReader {
                 LinkType::Protobuf => {
                     let path = Self::resolve_path(&link.src, parent_dir);
                     let meta = self.proto_reader.read(path).await?;
-                    config_module
-                        .extensions
-                        .grpc_file_descriptors
-                        .push(Content {
-                            id: link.id.clone(),
-                            content: meta.descriptor_set.clone(),
-                        });
+                    config_module.extensions.add_proto(meta);
                 }
                 LinkType::Script => {
                     config_module.extensions.script = Some(content);

diff --git a/src/grpc/data_loader_request.rs b/src/grpc/data_loader_request.rs
@@ -98,7 +98,7 @@ mod tests {
         let protobuf_set = ProtobufSet::from_proto_file(
             config_module
                 .extensions
-                .get_file_descriptor_set(&method)
+                .get_file_descriptor_set()
                 .unwrap()
                 .clone(),
         )

diff --git a/src/grpc/protobuf.rs b/src/grpc/protobuf.rs
@@ -79,17 +79,12 @@ impl ProtobufSet {
     }
 
     pub fn find_service(&self, grpc_method: &GrpcMethod) -> Result<ProtobufService> {
+        let service_name = format!("{}.{}", grpc_method.package, grpc_method.service);
+
         let service_descriptor = self
             .descriptor_pool
-            .get_service_by_name(
-                format!("{}.{}", grpc_method.package, grpc_method.service).as_str(),
-            )
-            .with_context(|| {
-                format!(
-                    "Couldn't find definitions for service {}",
-                    grpc_method.service
-                )
-            })?;
+            .get_service_by_name(&service_name)
+            .with_context(|| format!("Couldn't find definitions for service {service_name}"))?;
 
         Ok(ProtobufService { service_descriptor })
     }
@@ -304,7 +299,7 @@ pub mod tests {
             .resolve(config, None)
             .await?
             .extensions
-            .get_file_descriptor_set(&method)
+            .get_file_descriptor_set()
             .unwrap()
             .to_owned())
     }
@@ -353,7 +348,7 @@ pub mod tests {
 
         assert_eq!(
             error.to_string(),
-            "Couldn't find definitions for service _unknown"
+            "Couldn't find definitions for service greetings._unknown"
         );
 
         Ok(())

diff --git a/src/grpc/request_template.rs b/src/grpc/request_template.rs
@@ -169,7 +169,7 @@ mod tests {
                 .await
                 .unwrap()
                 .extensions
-                .get_file_descriptor_set(&method)
+                .get_file_descriptor_set()
                 .unwrap()
                 .clone(),
         )

diff --git a/tests/execution/grpc-proto-with-same-package.md b/tests/execution/grpc-proto-with-same-package.md
@@ -0,0 +1,97 @@
+# Grpc when multiple proto files have the same package name
+
+```protobuf @file:errors.proto
+syntax = "proto3";
+
+package news;
+
+// The error message definition.
+message ErrValidation {
+  string error = 1;
+}
+```
+
+```protobuf @file:news.proto
+syntax = "proto3";
+
+import "google/protobuf/empty.proto";
+
+package news;
+
+message News {
+    int32 id = 1;
+    string title = 2;
+    string body = 3;
+    string postImage = 4;
+}
+
+service NewsService {
+    rpc GetAllNews (google.protobuf.Empty) returns (NewsList) {}
+    rpc GetNews (NewsId) returns (News) {}
+    rpc GetMultipleNews (MultipleNewsId) returns (NewsList) {}
+    rpc DeleteNews (NewsId) returns (google.protobuf.Empty) {}
+    rpc EditNews (News) returns (News) {}
+    rpc AddNews (News) returns (News) {}
+}
+
+message NewsId {
+    int32 id = 1;
+}
+
+message MultipleNewsId {
+    repeated NewsId ids = 1;
+}
+
+message NewsList {
+    repeated News news = 1;
+}
+```
+
+```graphql @server
+schema
+  @server(port: 8000, graphiql: true)
+  @upstream(httpCache: true, batch: {delay: 10})
+  @link(id: "errors", src: "errors.proto", type: Protobuf)
+  @link(id: "news", src: "news.proto", type: Protobuf) {
+  query: Query
+}
+
+type Query {
+  news: NewsData! @grpc(method: "news.NewsService.GetAllNews", baseURL: "http://localhost:50051")
+  newsById(news: NewsInput!): News!
+    @grpc(method: "news.NewsService.GetNews", baseURL: "http://localhost:50051", body: "{{args.news}}")
+}
+input NewsInput {
+  id: Int
+  title: String
+  body: String
+  postImage: String
+}
+type NewsData {
+  news: [News]!
+}
+
+type News {
+  id: Int
+  title: String
+  body: String
+  postImage: String
+}
+```
+
+```yml @mock
+- request:
+    method: POST
+    url: http://localhost:50051/news.NewsService/GetAllNews
+    body: null
+  response:
+    status: 200
+    body: \0\0\0\0t\n#\x08\x01\x12\x06Note 1\x1a\tContent 1\"\x0cPost image 1\n#\x08\x02\x12\x06Note 2\x1a\tContent 2\"\x0cPost image 2
+```
+
+```yml @assert
+- method: POST
+  url: http://localhost:8080/graphql
+  body:
+    query: query { news {news{ id }} }
+```
diff --git a/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_assert_0.snap b/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_assert_0.snap
@@ -0,0 +1,24 @@
+---
+source: tests/execution_spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "news": {
+        "news": [
+          {
+            "id": 1
+          },
+          {
+            "id": 2
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_client.snap b/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_client.snap
@@ -0,0 +1,42 @@
+---
+source: tests/execution_spec.rs
+expression: client
+---
+scalar Date
+
+scalar Email
+
+scalar Empty
+
+scalar JSON
+
+type News {
+  body: String
+  id: Int
+  postImage: String
+  title: String
+}
+
+type NewsData {
+  news: [News]!
+}
+
+input NewsInput {
+  body: String
+  id: Int
+  postImage: String
+  title: String
+}
+
+scalar PhoneNumber
+
+type Query {
+  news: NewsData!
+  newsById(news: NewsInput!): News!
+}
+
+scalar Url
+
+schema {
+  query: Query
+}
diff --git a/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_merged.snap b/tests/snapshots/execution_spec__grpc-proto-with-same-package.md_merged.snap
@@ -0,0 +1,30 @@
+---
+source: tests/execution_spec.rs
+expression: merged
+---
+schema @server(graphiql: true, port: 8000) @upstream(batch: {delay: 10, headers: [], maxSize: 100}, httpCache: true) @link(id: "errors", src: "errors.proto", type: Protobuf) @link(id: "news", src: "news.proto", type: Protobuf) {
+  query: Query
+}
+
+input NewsInput {
+  body: String
+  id: Int
+  postImage: String
+  title: String
+}
+
+type News {
+  body: String
+  id: Int
+  postImage: String
+  title: String
+}
+
+type NewsData {
+  news: [News]!
+}
+
+type Query {
+  news: NewsData! @grpc(baseURL: "http://localhost:50051", method: "news.NewsService.GetAllNews")
+  newsById(news: NewsInput!): News! @grpc(baseURL: "http://localhost:50051", body: "{{args.news}}", method: "news.NewsService.GetNews")
+}
diff --git a/tests/snapshots/execution_spec__test-grpc-invalid-proto-id.md_errors.snap b/tests/snapshots/execution_spec__test-grpc-invalid-proto-id.md_errors.snap
@@ -4,7 +4,7 @@ expression: errors
 ---
 [
   {
-    "message": "File descriptor not found for method: abc.NewsService.GetAllNews",
+    "message": "Protobuf files were not specified in the config",
     "trace": [
       "Query",
       "news",

diff --git a/tests/snapshots/execution_spec__test-grpc-service.md_errors.snap b/tests/snapshots/execution_spec__test-grpc-service.md_errors.snap
@@ -4,7 +4,7 @@ expression: errors
 ---
 [
   {
-    "message": "Couldn't find definitions for service YourServiceName",
+    "message": "Couldn't find definitions for service news.YourServiceName",
     "trace": [
       "Query",
       "news",