From 7c7633cdbfb060dfbbbeb9ae74f8071ab0ed730e Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Tue, 21 May 2024 10:56:49 +0200 Subject: [PATCH] fix(grpc): map type support (#1979) Co-authored-by: Tushar Mathur Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- src/core/blueprint/from_config.rs | 2 +- src/core/generator/from_proto.rs | 69 +++++++++++++------ ...enerator__from_proto__test__map_types.snap | 19 +++++ ...tor__from_proto__test__required_types.snap | 2 +- src/core/grpc/protobuf.rs | 28 ++++++++ src/core/json/json_schema.rs | 5 ++ tailcall-fixtures/fixtures/protobuf/map.proto | 15 ++++ tests/core/snapshots/grpc-map.md_0.snap | 19 +++++ tests/core/snapshots/grpc-map.md_client.snap | 31 +++++++++ tests/core/snapshots/grpc-map.md_merged.snap | 19 +++++ tests/execution/grpc-map.md | 67 ++++++++++++++++++ 11 files changed, 253 insertions(+), 23 deletions(-) create mode 100644 src/core/generator/snapshots/tailcall__core__generator__from_proto__test__map_types.snap create mode 100644 tailcall-fixtures/fixtures/protobuf/map.proto create mode 100644 tests/core/snapshots/grpc-map.md_0.snap create mode 100644 tests/core/snapshots/grpc-map.md_client.snap create mode 100644 tests/core/snapshots/grpc-map.md_merged.snap create mode 100644 tests/execution/grpc-map.md diff --git a/src/core/blueprint/from_config.rs b/src/core/blueprint/from_config.rs index 74ed90febf..68747c76b0 100644 --- a/src/core/blueprint/from_config.rs +++ b/src/core/blueprint/from_config.rs @@ -103,7 +103,7 @@ where "Int" => JsonSchema::Num, "Boolean" => JsonSchema::Bool, "Empty" => JsonSchema::Empty, - "JSON" => JsonSchema::Obj(HashMap::new()), + "JSON" => JsonSchema::Any, _ => JsonSchema::Any, } }; diff --git a/src/core/generator/from_proto.rs b/src/core/generator/from_proto.rs index bf4532f6b9..8ba454926b 100644 --- a/src/core/generator/from_proto.rs +++ b/src/core/generator/from_proto.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashSet}; use anyhow::{bail, Result}; use derive_setters::Setters; @@ -21,6 +21,9 @@ struct Context { /// Root GraphQL query type query: String, + + /// Set of visited map types + map_types: HashSet, } impl Context { @@ -29,6 +32,7 @@ impl Context { query: query.to_string(), namespace: Default::default(), config: Default::default(), + map_types: Default::default(), } } @@ -64,10 +68,23 @@ impl Context { fn append_msg_type(mut self, messages: &Vec) -> Result { for message in messages { let msg_name = message.name(); - if let Some(options) = message.options.as_ref() { - if options.map_entry.unwrap_or_default() { - continue; - } + + let msg_type = GraphQLType::new(msg_name) + .extend(self.namespace.as_slice()) + .into_object_type(); + + if message + .options + .as_ref() + .and_then(|opt| opt.map_entry) + .unwrap_or_default() + { + // map types in protobuf are encoded as nested type + // https://protobuf.dev/programming-guides/encoding/#maps + // since we encode it as JSON scalar type in graphQL + // record that this type is map and ignore it + self.map_types.insert(msg_type.id()); + continue; } // first append the name of current message as namespace @@ -77,10 +94,6 @@ impl Context { // then drop it after handling nested types self.namespace.pop(); - let msg_type = GraphQLType::new(msg_name) - .extend(self.namespace.as_slice()) - .into_object_type(); - let mut ty = Type::default(); for field in message.field.iter() { let field_name = GraphQLType::new(field.name()) @@ -93,19 +106,27 @@ impl Context { cfg_field.list = label.contains("repeated"); cfg_field.required = label.contains("required") || cfg_field.list; - if field.type_name.is_some() { - // for non-primitive types - let type_of = graphql_type_from_ref(field.type_name())? - .into_object_type() - .to_string(); + if let Some(type_name) = &field.type_name { + // check that current field is map. + // it's done by checking that we've seen this type before + // inside the nested type. It works only if we explore nested types + // before the current type + if self.map_types.contains(&type_name[1..]) { + cfg_field.type_of = "JSON".to_string(); + // drop list option since it is not relevant + // when using JSON representation + cfg_field.list = false; + } else { + // for non-primitive types + let type_of = graphql_type_from_ref(type_name)? + .into_object_type() + .to_string(); - cfg_field.type_of = type_of; + cfg_field.type_of = type_of; + } } else { let type_of = convert_primitive_type(field.r#type().as_str_name()); - if type_of.eq("JSON") { - cfg_field.list = false; - cfg_field.required = false; - } + cfg_field.type_of = type_of; } @@ -207,8 +228,6 @@ fn convert_primitive_type(proto_ty: &str) -> String { "int32" | "int64" | "fixed32" | "fixed64" | "uint32" | "uint64" => "Int", "bool" => "Boolean", "string" | "bytes" => "String", - "message" => "JSON", // JSON scalar is preloaded by tailcall, so there is no need to - // explicitly define it in the config. x => x, } .to_string() @@ -365,4 +384,12 @@ mod test { insta::assert_snapshot!(config); Ok(()) } + + #[test] + fn test_map_types() -> Result<()> { + let set = compile_protobuf(&[protobuf::MAP])?; + let config = from_proto(&[set], "Query")?.to_sdl(); + insta::assert_snapshot!(config); + Ok(()) + } } diff --git a/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__map_types.snap b/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__map_types.snap new file mode 100644 index 0000000000..ffea5b7e7f --- /dev/null +++ b/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__map_types.snap @@ -0,0 +1,19 @@ +--- +source: src/core/generator/from_proto.rs +expression: config +--- +schema @server @upstream { + query: Query +} + +input map__MapRequest @tag(id: "map.MapRequest") { + map: JSON! +} + +type Query { + map__MapService__GetMap(mapRequest: map__MapRequest!): map__MapResponse! @grpc(body: "{{.args.mapRequest}}", method: "map.MapService.GetMap") +} + +type map__MapResponse @tag(id: "map.MapResponse") { + map: JSON! +} diff --git a/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__required_types.snap b/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__required_types.snap index 1edae7cc34..a270761b1c 100644 --- a/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__required_types.snap +++ b/src/core/generator/snapshots/tailcall__core__generator__from_proto__test__required_types.snap @@ -15,7 +15,7 @@ type person__Person @tag(id: "person.Person") { id: Int! name: String! phone: [person__PhoneNumber]! - stringMap: [person__Person__StringMapEntry]! + stringMap: JSON! } type person__PhoneNumber @tag(id: "person.PhoneNumber") { diff --git a/src/core/grpc/protobuf.rs b/src/core/grpc/protobuf.rs index 801b2ad3ed..2f3fe1e178 100644 --- a/src/core/grpc/protobuf.rs +++ b/src/core/grpc/protobuf.rs @@ -430,4 +430,32 @@ pub mod tests { Ok(()) } + + #[tokio::test] + async fn map_proto_file() -> Result<()> { + let grpc_method = GrpcMethod::try_from("map.MapService.GetMap").unwrap(); + + let file = ProtobufSet::from_proto_file(get_proto_file(protobuf::MAP).await?)?; + let service = file.find_service(&grpc_method)?; + let operation = service.find_operation(&grpc_method)?; + + // only single key-value in json since the converted output can change the + // ordering on every run + let input = operation.convert_input(r#"{ "map": { "key": "value" } }"#)?; + + assert_eq!(input, b"\0\0\0\0\x0e\n\x0c\n\x03key\x12\x05value"); + + let output = b"\0\0\0\0\x12\n\t\x08\x01\x12\x05value\n\x05\x08\x02\x12\x01v"; + + let parsed = operation.convert_output::(output)?; + + assert_eq!( + serde_json::to_value(parsed)?, + json!({ + "map": { "1": "value", "2": "v" } + }) + ); + + Ok(()) + } } diff --git a/src/core/json/json_schema.rs b/src/core/json/json_schema.rs index 0a08c53fb0..92dc3a635b 100644 --- a/src/core/json/json_schema.rs +++ b/src/core/json/json_schema.rs @@ -183,6 +183,11 @@ impl TryFrom<&MessageDescriptor> for JsonSchema { type Error = crate::core::valid::ValidationError; fn try_from(value: &MessageDescriptor) -> Result { + if value.is_map_entry() { + // we encode protobuf's map as JSON scalar + return Ok(JsonSchema::Any); + } + let mut map = std::collections::HashMap::new(); let fields = value.fields(); diff --git a/tailcall-fixtures/fixtures/protobuf/map.proto b/tailcall-fixtures/fixtures/protobuf/map.proto new file mode 100644 index 0000000000..19f10a1ac9 --- /dev/null +++ b/tailcall-fixtures/fixtures/protobuf/map.proto @@ -0,0 +1,15 @@ +syntax = "proto3"; + +package map; + +message MapRequest { + map map = 1; +} + +message MapResponse { + map map = 1; +} + +service MapService { + rpc GetMap (MapRequest) returns (MapResponse) {} +} diff --git a/tests/core/snapshots/grpc-map.md_0.snap b/tests/core/snapshots/grpc-map.md_0.snap new file mode 100644 index 0000000000..e4157fdb54 --- /dev/null +++ b/tests/core/snapshots/grpc-map.md_0.snap @@ -0,0 +1,19 @@ +--- +source: tests/core/spec.rs +expression: response +--- +{ + "status": 200, + "headers": { + "content-type": "application/json" + }, + "body": { + "data": { + "map__MapService__GetMap": { + "map": { + "1": "value" + } + } + } + } +} diff --git a/tests/core/snapshots/grpc-map.md_client.snap b/tests/core/snapshots/grpc-map.md_client.snap new file mode 100644 index 0000000000..841ab58b81 --- /dev/null +++ b/tests/core/snapshots/grpc-map.md_client.snap @@ -0,0 +1,31 @@ +--- +source: tests/core/spec.rs +expression: client +--- +scalar Date + +scalar Email + +scalar Empty + +scalar JSON + +scalar PhoneNumber + +type Query { + map__MapService__GetMap(mapRequest: map__MapRequest!): map__MapResponse! +} + +scalar Url + +input map__MapRequest { + map: JSON! +} + +type map__MapResponse { + map: JSON! +} + +schema { + query: Query +} diff --git a/tests/core/snapshots/grpc-map.md_merged.snap b/tests/core/snapshots/grpc-map.md_merged.snap new file mode 100644 index 0000000000..2b1fd8cb85 --- /dev/null +++ b/tests/core/snapshots/grpc-map.md_merged.snap @@ -0,0 +1,19 @@ +--- +source: tests/core/spec.rs +expression: merged +--- +schema @server(port: 8000) @upstream(baseURL: "http://localhost:50051", batch: {delay: 10, headers: []}, httpCache: true) @link(src: "map.proto", type: Protobuf) { + query: Query +} + +input map__MapRequest { + map: JSON! +} + +type Query { + map__MapService__GetMap(mapRequest: map__MapRequest!): map__MapResponse! @grpc(body: "{{.args.mapRequest}}", method: "map.MapService.GetMap") +} + +type map__MapResponse @tag(id: "map.MapResponse") { + map: JSON! +} diff --git a/tests/execution/grpc-map.md b/tests/execution/grpc-map.md new file mode 100644 index 0000000000..2ae69a5296 --- /dev/null +++ b/tests/execution/grpc-map.md @@ -0,0 +1,67 @@ +# Grpc map type + +```protobuf @file:map.proto +syntax = "proto3"; + +package map; + +message MapRequest { + map map = 1; +} + +message MapResponse { + map map = 1; +} + +service MapService { + rpc GetMap (MapRequest) returns (MapResponse) {} +} + +``` + +```graphql @config +schema + @server(port: 8000) + @upstream(baseURL: "http://localhost:50051", httpCache: true, batch: {delay: 10}) + @link(src: "map.proto", type: Protobuf) { + query: Query +} + +schema @server @upstream { + query: Query +} + +input map__MapRequest @tag(id: "map.MapRequest") { + map: JSON! +} + +type Query { + map__MapService__GetMap(mapRequest: map__MapRequest!): map__MapResponse! + @grpc(body: "{{.args.mapRequest}}", method: "map.MapService.GetMap") +} + +type map__MapResponse @tag(id: "map.MapResponse") { + map: JSON! +} +``` + +```yml @mock +- request: + method: POST + url: http://localhost:50051/map.MapService/GetMap + response: + status: 200 + textBody: \0\0\0\0\x12\n\t\x08\x01\x12\x05value +``` + +```yml @test +- method: POST + url: http://localhost:8080/graphql + body: + query: > + query { + map__MapService__GetMap(mapRequest: { map: { key: "value" } }) { + map + } + } +```