From 361f33bc3ca6ac1dcd9566f32a5813bbaad44785 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Tue, 19 Nov 2024 10:57:37 +0000 Subject: [PATCH 01/15] mark resolvable directives as repeatable --- generated/.tailcallrc.graphql | 12 ++++++------ src/core/config/directives/call.rs | 2 +- src/core/config/directives/expr.rs | 2 +- src/core/config/directives/graphql.rs | 2 +- src/core/config/directives/grpc.rs | 2 +- src/core/config/directives/http.rs | 2 +- src/core/config/directives/js.rs | 2 +- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/generated/.tailcallrc.graphql b/generated/.tailcallrc.graphql index 9dc5ed9dc1..5c93271be9 100644 --- a/generated/.tailcallrc.graphql +++ b/generated/.tailcallrc.graphql @@ -47,7 +47,7 @@ directive @call( of the previous step is passed as input to the next step. """ steps: [Step] -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT """ The `@expr` operators allows you to specify an expression that can evaluate to a @@ -55,7 +55,7 @@ value. The expression can be a static value or built form a Mustache template. s """ directive @expr( body: JSON -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT """ The @graphQL operator allows to specify GraphQL API server request to fetch data @@ -95,7 +95,7 @@ directive @graphQL( This refers URL of the API. """ url: String! -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT """ The @grpc operator indicates that a field or node is backed by a gRPC API.For instance, @@ -149,7 +149,7 @@ directive @grpc( This refers to URL of the API. """ url: String! -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT """ The @http operator indicates that a field or node is backed by a REST API.For instance, @@ -229,11 +229,11 @@ directive @http( This refers to URL of the API. """ url: String! -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT directive @js( name: String! -) on FIELD_DEFINITION | OBJECT +) repeatable on FIELD_DEFINITION | OBJECT """ The @link directive allows you to import external resources, such as configuration diff --git a/src/core/config/directives/call.rs b/src/core/config/directives/call.rs index 377b711833..c1f1f28b7a 100644 --- a/src/core/config/directives/call.rs +++ b/src/core/config/directives/call.rs @@ -38,7 +38,7 @@ pub struct Step { schemars::JsonSchema, DirectiveDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object")] +#[directive_definition(repeatable, locations = "FieldDefinition, Object")] pub struct Call { /// Steps are composed together to form a call. /// If you have multiple steps, the output of the previous step is passed as diff --git a/src/core/config/directives/expr.rs b/src/core/config/directives/expr.rs index 39dcced1ce..5a60ecea09 100644 --- a/src/core/config/directives/expr.rs +++ b/src/core/config/directives/expr.rs @@ -13,7 +13,7 @@ use tailcall_macros::{DirectiveDefinition, InputDefinition}; DirectiveDefinition, InputDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object")] +#[directive_definition(repeatable, locations = "FieldDefinition, Object")] #[serde(deny_unknown_fields)] /// The `@expr` operators allows you to specify an expression that can evaluate /// to a value. The expression can be a static value or built form a Mustache diff --git a/src/core/config/directives/graphql.rs b/src/core/config/directives/graphql.rs index e366bbaaa2..509c2a0646 100644 --- a/src/core/config/directives/graphql.rs +++ b/src/core/config/directives/graphql.rs @@ -16,7 +16,7 @@ use crate::core::is_default; DirectiveDefinition, InputDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object")] +#[directive_definition(repeatable, locations = "FieldDefinition, Object")] #[serde(deny_unknown_fields)] /// The @graphQL operator allows to specify GraphQL API server request to fetch /// data from. diff --git a/src/core/config/directives/grpc.rs b/src/core/config/directives/grpc.rs index d770ce6502..919ae53027 100644 --- a/src/core/config/directives/grpc.rs +++ b/src/core/config/directives/grpc.rs @@ -17,7 +17,7 @@ use crate::core::is_default; InputDefinition, DirectiveDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object")] +#[directive_definition(repeatable, locations = "FieldDefinition, Object")] #[serde(rename_all = "camelCase")] #[serde(deny_unknown_fields)] /// The @grpc operator indicates that a field or node is backed by a gRPC API. diff --git a/src/core/config/directives/http.rs b/src/core/config/directives/http.rs index 8d13eb44ac..91e0ecc985 100644 --- a/src/core/config/directives/http.rs +++ b/src/core/config/directives/http.rs @@ -19,7 +19,7 @@ use crate::core::json::JsonSchema; DirectiveDefinition, InputDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object")] +#[directive_definition(repeatable, locations = "FieldDefinition, Object")] #[serde(deny_unknown_fields)] /// The @http operator indicates that a field or node is backed by a REST API. /// diff --git a/src/core/config/directives/js.rs b/src/core/config/directives/js.rs index 60f307befc..e27891a78d 100644 --- a/src/core/config/directives/js.rs +++ b/src/core/config/directives/js.rs @@ -12,7 +12,7 @@ use tailcall_macros::{DirectiveDefinition, InputDefinition}; DirectiveDefinition, InputDefinition, )] -#[directive_definition(locations = "FieldDefinition, Object", lowercase_name)] +#[directive_definition(repeatable, locations = "FieldDefinition, Object", lowercase_name)] pub struct JS { pub name: String, } From c93602d47b32840fef3fd972b56d42055f83de20 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Tue, 19 Nov 2024 12:51:56 +0000 Subject: [PATCH 02/15] specify multiple resolvers in config --- src/cli/tc/init.rs | 2 +- src/core/blueprint/definitions.rs | 30 +++++++++------- src/core/blueprint/from_config.rs | 2 +- src/core/config/config.rs | 23 ++++++------ src/core/config/config_module/merge.rs | 8 ++--- src/core/config/from_document.rs | 8 ++--- src/core/config/into_document.rs | 30 ++++++---------- src/core/generator/from_proto.rs | 4 +-- .../generator/json/operation_generator.rs | 14 ++++---- src/core/grpc/data_loader_request.rs | 2 +- src/core/grpc/protobuf.rs | 2 +- src/core/grpc/request_template.rs | 2 +- tailcall-macros/src/resolver.rs | 36 ++++++------------- 13 files changed, 74 insertions(+), 89 deletions(-) diff --git a/src/cli/tc/init.rs b/src/cli/tc/init.rs index 2a9e537f1c..a483f6d74a 100644 --- a/src/cli/tc/init.rs +++ b/src/cli/tc/init.rs @@ -87,7 +87,7 @@ async fn confirm_and_write_yml( fn main_config() -> Config { let field = Field { type_of: Type::from("String".to_owned()).into_required(), - resolver: Some(Resolver::Expr(Expr { body: "Hello, World!".into() })), + resolvers: vec![Resolver::Expr(Expr { body: "Hello, World!".into() })], ..Default::default() }; diff --git a/src/core/blueprint/definitions.rs b/src/core/blueprint/definitions.rs index 15dc40bb41..c09804ed38 100644 --- a/src/core/blueprint/definitions.rs +++ b/src/core/blueprint/definitions.rs @@ -107,14 +107,19 @@ fn process_field_within_type(context: ProcessFieldWithinTypeContext) -> Valid add_field.path[1..] + let added_field_path = if source_field.resolvers.is_empty() { + add_field.path.clone() + } else { + add_field.path[1..] .iter() .map(|s| s.to_owned()) - .collect::>(), - None => add_field.path.clone(), + .collect::>() }; let invalid_path_handler = |field_name: &str, _added_field_path: &[String], diff --git a/src/core/blueprint/from_config.rs b/src/core/blueprint/from_config.rs index e9a8d094b3..c2d7b47c4b 100644 --- a/src/core/blueprint/from_config.rs +++ b/src/core/blueprint/from_config.rs @@ -88,7 +88,7 @@ pub fn to_json_schema(type_of: &Type, config: &Config) -> JsonSchema { if let Some(type_) = type_ { let mut schema_fields = BTreeMap::new(); for (name, field) in type_.fields.iter() { - if field.resolver.is_none() { + if field.resolvers.is_empty() { schema_fields.insert(name.clone(), to_json_schema(&field.type_of, config)); } } diff --git a/src/core/config/config.rs b/src/core/config/config.rs index 070ec7499b..23560625ef 100644 --- a/src/core/config/config.rs +++ b/src/core/config/config.rs @@ -117,7 +117,7 @@ pub struct Type { /// /// Apollo federation entity resolver. #[serde(flatten, default, skip_serializing_if = "is_default")] - pub resolver: Option, + pub resolvers: Vec, /// /// Any additional directives #[serde(default, skip_serializing_if = "is_default")] @@ -226,7 +226,7 @@ pub struct Field { /// /// Resolver for the field #[serde(flatten, default, skip_serializing_if = "is_default")] - pub resolver: Option, + pub resolvers: Vec, /// /// Any additional directives @@ -243,14 +243,15 @@ impl MergeRight for Field { impl Field { pub fn has_resolver(&self) -> bool { - self.resolver.is_some() + !self.resolvers.is_empty() } pub fn has_batched_resolver(&self) -> bool { - self.resolver - .as_ref() - .map(Resolver::is_batched) - .unwrap_or(false) + if self.resolvers.is_empty() { + true + } else { + self.resolvers.iter().all(Resolver::is_batched) + } } pub fn int() -> Self { @@ -700,18 +701,18 @@ mod tests { let f1 = Field { ..Default::default() }; let f2 = Field { - resolver: Some(Resolver::Http(Http { + resolvers: vec![Resolver::Http(Http { batch_key: vec!["id".to_string()], ..Default::default() - })), + })], ..Default::default() }; let f3 = Field { - resolver: Some(Resolver::Http(Http { + resolvers: vec![Resolver::Http(Http { batch_key: vec![], ..Default::default() - })), + })], ..Default::default() }; diff --git a/src/core/config/config_module/merge.rs b/src/core/config/config_module/merge.rs index 0c145234d2..5fc7fff0d7 100644 --- a/src/core/config/config_module/merge.rs +++ b/src/core/config/config_module/merge.rs @@ -101,7 +101,7 @@ impl Contravariant for Field { default_value: self.default_value.or(other.default_value), protected: self.protected.merge_right(other.protected), discriminate: self.discriminate.merge_right(other.discriminate), - resolver: self.resolver.merge_right(other.resolver), + resolvers: self.resolvers.merge_right(other.resolvers), directives: self.directives.merge_right(other.directives), }) } @@ -123,7 +123,7 @@ impl Covariant for Field { default_value: self.default_value.or(other.default_value), protected: self.protected.merge_right(other.protected), discriminate: self.discriminate.merge_right(other.discriminate), - resolver: self.resolver.merge_right(other.resolver), + resolvers: self.resolvers.merge_right(other.resolvers), directives: self.directives.merge_right(other.directives), }) } @@ -139,7 +139,7 @@ impl Contravariant for Type { implements: self.implements.merge_right(other.implements), cache: self.cache.merge_right(other.cache), protected: self.protected.merge_right(other.protected), - resolver: self.resolver.merge_right(other.resolver), + resolvers: self.resolvers.merge_right(other.resolvers), directives: self.directives.merge_right(other.directives), }) } @@ -155,7 +155,7 @@ impl Covariant for Type { implements: self.implements.merge_right(other.implements), cache: self.cache.merge_right(other.cache), protected: self.protected.merge_right(other.protected), - resolver: self.resolver.merge_right(other.resolver), + resolvers: self.resolvers.merge_right(other.resolvers), directives: self.directives.merge_right(other.directives), }) } diff --git a/src/core/config/from_document.rs b/src/core/config/from_document.rs index 3ccd7b72b6..b05757900f 100644 --- a/src/core/config/from_document.rs +++ b/src/core/config/from_document.rs @@ -245,7 +245,7 @@ where .fuse(to_add_fields_from_directives(directives)) .fuse(to_federation_directives(directives)) .map( - |(resolver, cache, fields, protected, added_fields, unknown_directives)| { + |(resolvers, cache, fields, protected, added_fields, unknown_directives)| { let doc = description.to_owned().map(|pos| pos.node); let implements = implements.iter().map(|pos| pos.node.to_string()).collect(); config::Type { @@ -255,7 +255,7 @@ where implements, cache, protected, - resolver, + resolvers, directives: unknown_directives, } }, @@ -339,7 +339,7 @@ where .fuse(to_federation_directives(directives)) .map( |( - resolver, + resolvers, cache, omit, modify, @@ -357,7 +357,7 @@ where protected, discriminate, default_value, - resolver, + resolvers, directives, }, ) diff --git a/src/core/config/into_document.rs b/src/core/config/into_document.rs index b06ff84311..5df4f034ae 100644 --- a/src/core/config/into_document.rs +++ b/src/core/config/into_document.rs @@ -213,21 +213,14 @@ fn into_directives( } fn field_directives(field: &crate::core::config::Field) -> Vec> { - let directives = vec![ - field - .resolver - .as_ref() - .and_then(|d| d.to_directive()) - .map(pos), - field.modify.as_ref().map(|d| pos(d.to_directive())), - field.omit.as_ref().map(|d| pos(d.to_directive())), - field.cache.as_ref().map(|d| pos(d.to_directive())), - field.protected.as_ref().map(|d| pos(d.to_directive())), - ]; - - directives - .into_iter() - .flatten() + field + .resolvers + .iter() + .filter_map(|resolver| resolver.to_directive().map(pos)) + .chain(field.modify.as_ref().map(|d| pos(d.to_directive()))) + .chain(field.omit.as_ref().map(|d| pos(d.to_directive()))) + .chain(field.cache.as_ref().map(|d| pos(d.to_directive()))) + .chain(field.protected.as_ref().map(|d| pos(d.to_directive()))) .chain(into_directives(&field.directives)) .collect() } @@ -251,10 +244,9 @@ fn type_directives(type_def: &crate::core::config::Type) -> Vec>() diff --git a/src/core/generator/from_proto.rs b/src/core/generator/from_proto.rs index b85aca8efa..4275a56cbc 100644 --- a/src/core/generator/from_proto.rs +++ b/src/core/generator/from_proto.rs @@ -367,7 +367,7 @@ impl Context { .to_string(); cfg_field.type_of = cfg_field.type_of.with_name(output_ty); - cfg_field.resolver = Some(Resolver::Grpc(Grpc { + cfg_field.resolvers = vec![Resolver::Grpc(Grpc { url: url.to_string(), body, batch_key: vec![], @@ -375,7 +375,7 @@ impl Context { method: field_name.id(), dedupe: None, select: None, - })); + })]; let method_path = PathBuilder::new(&path).extend(PathField::Method, method_index as i32); diff --git a/src/core/generator/json/operation_generator.rs b/src/core/generator/json/operation_generator.rs index f30b489a6d..27d57ab94c 100644 --- a/src/core/generator/json/operation_generator.rs +++ b/src/core/generator/json/operation_generator.rs @@ -30,9 +30,7 @@ impl OperationTypeGenerator { // generate required http directive. let http_directive_gen = HttpDirectiveGenerator::new(&request_sample.url); - field.resolver = Some(Resolver::Http( - http_directive_gen.generate_http_directive(&mut field), - )); + let mut http_resolver = http_directive_gen.generate_http_directive(&mut field); if let GraphQLOperationType::Mutation = request_sample.operation_type { // generate the input type. @@ -42,16 +40,18 @@ impl OperationTypeGenerator { let prefix = format!("{}Input", PREFIX); let arg_name_gen = NameGenerator::new(prefix.as_str()); let arg_name = arg_name_gen.next(); - if let Some(Resolver::Http(http)) = &mut field.resolver { - http.body = Some(format!("{{{{.args.{}}}}}", arg_name)); - http.method = request_sample.method.to_owned(); - } + + http_resolver.body = Some(format!("{{{{.args.{}}}}}", arg_name)); + http_resolver.method = request_sample.method.to_owned(); + field.args.insert( arg_name, Arg { type_of: root_ty.into(), ..Default::default() }, ); } + field.resolvers = vec![Resolver::Http(http_resolver)]; + // if type is already present, then append the new field to it else create one. let req_op = request_sample .operation_type diff --git a/src/core/grpc/data_loader_request.rs b/src/core/grpc/data_loader_request.rs index a8af211bde..e5a09681c0 100644 --- a/src/core/grpc/data_loader_request.rs +++ b/src/core/grpc/data_loader_request.rs @@ -85,7 +85,7 @@ mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolver(Resolver::Grpc(grpc)), + Field::default().resolvers(vec![Resolver::Grpc(grpc)]), )]), ); diff --git a/src/core/grpc/protobuf.rs b/src/core/grpc/protobuf.rs index ca0012b46b..2707fde967 100644 --- a/src/core/grpc/protobuf.rs +++ b/src/core/grpc/protobuf.rs @@ -276,7 +276,7 @@ pub mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolver(Resolver::Grpc(grpc)), + Field::default().resolvers(vec![Resolver::Grpc(grpc)]), )]), ); Ok(reader diff --git a/src/core/grpc/request_template.rs b/src/core/grpc/request_template.rs index f4901f5cc5..f4579ba8ee 100644 --- a/src/core/grpc/request_template.rs +++ b/src/core/grpc/request_template.rs @@ -171,7 +171,7 @@ mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolver(Resolver::Grpc(grpc)), + Field::default().resolvers(vec![Resolver::Grpc(grpc)]), )]), ); diff --git a/tailcall-macros/src/resolver.rs b/tailcall-macros/src/resolver.rs index 99cd446c4f..871e647b30 100644 --- a/tailcall-macros/src/resolver.rs +++ b/tailcall-macros/src/resolver.rs @@ -60,15 +60,9 @@ pub fn expand_resolver_derive(input: DeriveInput) -> syn::Result { } Some(quote! { - valid = valid.and(<#ty>::from_directives(directives.iter()).map(|resolver| { - if let Some(resolver) = resolver { - let directive_name = <#ty>::trace_name(); - if !resolvable_directives.contains(&directive_name) { - resolvable_directives.push(directive_name); - } - result = Some(Self::#variant_name(resolver)); - } - })); + if <#ty>::directive_name() == directive.node.name.node { + return <#ty>::from_directive(&directive.node).map(|x| Some(Self::#variant_name(x))) + } }) }); @@ -100,23 +94,15 @@ pub fn expand_resolver_derive(input: DeriveInput) -> syn::Result { impl #name { pub fn from_directives( directives: &[Positioned], - ) -> Valid, String> { - let mut result = None; - let mut resolvable_directives = Vec::new(); - let mut valid = Valid::succeed(()); - - #(#variant_parsers)* - - valid.and_then(|_| { - if resolvable_directives.len() > 1 { - Valid::fail(format!( - "Multiple resolvers detected [{}]", - resolvable_directives.join(", ") - )) - } else { - Valid::succeed(result) - } + ) -> Valid, String> { + Valid::from_iter(directives.iter(), |directive| { + #(#variant_parsers)* + + Valid::succeed(None) }) + .map(|resolvers| { + resolvers.into_iter().filter_map(std::convert::identity).collect() + }) } pub fn to_directive(&self) -> Option { From 9d43b7567f0b3f17d21f873a9bf0a29c2b6a2fcd Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:08:23 +0000 Subject: [PATCH 03/15] feat: add merge_right for ConstValue --- src/core/merge_right.rs | 143 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 1 deletion(-) diff --git a/src/core/merge_right.rs b/src/core/merge_right.rs index b0c928575b..71b40061ea 100644 --- a/src/core/merge_right.rs +++ b/src/core/merge_right.rs @@ -1,5 +1,6 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use indexmap::IndexMap; use prost_reflect::prost_types::FileDescriptorProto; pub trait MergeRight { @@ -78,12 +79,55 @@ where } } +impl MergeRight for IndexMap +where + K: Eq + std::hash::Hash, + V: MergeRight + Default, +{ + fn merge_right(mut self, other: Self) -> Self { + use indexmap::map::Entry; + + for (other_name, other_value) in other { + match self.entry(other_name) { + Entry::Occupied(mut occupied_entry) => { + // try to support insertion order while merging index maps. + // if value is present on left, present it's position + // and if value is present only on the right then + // add it to the end of left map preserving the iteration order of the right map + let value = std::mem::take(occupied_entry.get_mut()); + + *occupied_entry.get_mut() = value.merge_right(other_value); + } + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(other_value); + } + } + } + self + } +} + impl MergeRight for FileDescriptorProto { fn merge_right(self, other: Self) -> Self { other } } +impl MergeRight for async_graphql_value::ConstValue { + fn merge_right(self, other: Self) -> Self { + use async_graphql_value::ConstValue; + match (self, other) { + (ConstValue::List(a), ConstValue::List(b)) => ConstValue::List(a.merge_right(b)), + (ConstValue::List(mut vec), other) => { + vec.push(other); + ConstValue::List(vec) + } + (ConstValue::Object(a), ConstValue::Object(b)) => ConstValue::Object(a.merge_right(b)), + (_, other) => other, + } + } +} + impl MergeRight for serde_yaml::Value { fn merge_right(self, other: Self) -> Self { use serde_yaml::Value; @@ -135,9 +179,11 @@ impl MergeRight for serde_yaml::Value { mod tests { use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; + use serde_json::json; + use super::MergeRight; - #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] struct Test(u32); impl From for Test { @@ -336,4 +382,99 @@ mod tests { ]) ); } + + #[test] + fn test_index_map() { + use indexmap::IndexMap; + + let l: IndexMap = IndexMap::from_iter(vec![]); + let r: IndexMap = IndexMap::from_iter(vec![]); + assert_eq!(l.merge_right(r), IndexMap::<_, _>::from_iter(vec![])); + + let l: IndexMap = + IndexMap::from_iter(vec![(1, Test::from(1)), (2, Test::from(2))]); + let r: IndexMap = IndexMap::from_iter(vec![]); + assert_eq!( + l.merge_right(r), + IndexMap::<_, _>::from_iter(vec![(1, Test::from(1)), (2, Test::from(2))]) + ); + + let l: IndexMap = IndexMap::from_iter(vec![]); + let r: IndexMap = + IndexMap::from_iter(vec![(3, Test::from(3)), (4, Test::from(4))]); + assert_eq!( + l.merge_right(r), + IndexMap::<_, _>::from_iter(vec![(3, Test::from(3)), (4, Test::from(4))]) + ); + + let l: IndexMap = + IndexMap::from_iter(vec![(1, Test::from(1)), (2, Test::from(2))]); + let r: IndexMap = IndexMap::from_iter(vec![ + (2, Test::from(5)), + (3, Test::from(3)), + (4, Test::from(4)), + ]); + assert_eq!( + l.merge_right(r), + IndexMap::<_, _>::from_iter(vec![ + (1, Test::from(1)), + (2, Test::from(7)), + (3, Test::from(3)), + (4, Test::from(4)) + ]) + ); + } + + #[test] + fn test_const_value() { + use async_graphql_value::ConstValue; + + let a: ConstValue = serde_json::from_value(json!({ + "a": null, + "b": "string", + "c": 32, + "d": [1, 2, 3], + "e": { + "ea": null, + "eb": "string e", + "ec": 88, + "ed": {} + } + })) + .unwrap(); + + let b: ConstValue = serde_json::from_value(json!({ + "a": true, + "b": "another", + "c": 48, + "d": [4, 5, 6], + "e": { + "ec": 108, + "ed": { + "eda": false + } + }, + "f": "new f" + })) + .unwrap(); + + let expected: ConstValue = serde_json::from_value(json!({ + "a": true, + "b": "another", + "c": 48, + "d": [1, 2, 3, 4, 5, 6], + "e": { + "ea": null, + "eb": "string e", + "ec": 108, + "ed": { + "eda": false + } + }, + "f": "new f" + })) + .unwrap(); + + assert_eq!(a.merge_right(b), expected); + } } From c6493ecfb3d39d3e1b4c3f2d7a5cea6f875a9240 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:52:40 +0000 Subject: [PATCH 04/15] feat: add merge variant for IR --- src/core/ir/eval.rs | 88 +++++++++++++++++++++++ src/core/ir/model.rs | 5 ++ src/core/jit/transform/check_cache.rs | 17 +++-- src/core/jit/transform/check_const.rs | 1 + src/core/jit/transform/check_dedupe.rs | 1 + src/core/jit/transform/check_protected.rs | 1 + 6 files changed, 106 insertions(+), 7 deletions(-) diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index f1d315aa09..a3acd1ad58 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -10,6 +10,7 @@ use super::eval_io::eval_io; use super::model::{Cache, CacheKey, Map, IR}; use super::{Error, EvalContext, ResolverContextLike, TypedValue}; use crate::core::json::{JsonLike, JsonObjectLike}; +use crate::core::merge_right::MergeRight; use crate::core::serde_value_ext::ValueExt; impl IR { @@ -96,6 +97,21 @@ impl IR { let ctx = &mut ctx.with_args(args); second.eval(ctx).await } + IR::Merge(vec) => { + let results: Vec<_> = join_all(vec.iter().map(|ir| { + let mut ctx = ctx.clone(); + + async move { ir.eval(&mut ctx).await } + })) + .await + .into_iter() + .collect::>()?; + + Ok(results + .into_iter() + .reduce(|acc, result| acc.merge_right(result)) + .unwrap_or_default()) + } IR::Discriminate(discriminator, expr) => expr .eval(ctx) .await @@ -151,3 +167,75 @@ impl IR { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + mod merge { + use serde_json::json; + + use crate::core::{ + blueprint::{Blueprint, DynamicValue}, + http::RequestContext, + ir::EmptyResolverContext, + }; + + use super::*; + + #[tokio::test] + async fn test_const_values() { + let a = DynamicValue::Value( + ConstValue::from_json(json!({ + "a": 1, + "c": { + "ca": false + } + })) + .unwrap(), + ); + + let b = DynamicValue::Value( + ConstValue::from_json(json!({ + "b": 2, + "c": { + "cb": 23 + } + })) + .unwrap(), + ); + + let c = DynamicValue::Value( + ConstValue::from_json(json!({ + "c" : { + "ca": true, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(), + ); + + let ir = IR::Merge([a, b, c].into_iter().map(IR::Dynamic).collect()); + let runtime = crate::cli::runtime::init(&Blueprint::default()); + let req_ctx = RequestContext::new(runtime); + let res_ctx = EmptyResolverContext {}; + let mut eval_ctx = EvalContext::new(&req_ctx, &res_ctx); + + let actual = ir.eval(&mut eval_ctx).await.unwrap(); + let expected = ConstValue::from_json(json!({ + "a": 1, + "b": 2, + "c": { + "ca": true, + "cb": 23, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(); + + assert_eq!(actual, expected); + } + } +} diff --git a/src/core/ir/model.rs b/src/core/ir/model.rs index ecab192987..6fad3219a0 100644 --- a/src/core/ir/model.rs +++ b/src/core/ir/model.rs @@ -25,6 +25,8 @@ pub enum IR { Protect(Box), Map(Map), Pipe(Box, Box), + /// Merges the result of multiple IRs together + Merge(Vec), Discriminate(Discriminator, Box), /// Apollo Federation _entities resolver Entity(HashMap), @@ -174,6 +176,9 @@ impl IR { .collect(), ), IR::Service(sdl) => IR::Service(sdl), + IR::Merge(vec) => { + IR::Merge(vec.into_iter().map(|ir| ir.modify(modifier)).collect()) + } } } } diff --git a/src/core/jit/transform/check_cache.rs b/src/core/jit/transform/check_cache.rs index 2bc82e2637..8f704733c3 100644 --- a/src/core/jit/transform/check_cache.rs +++ b/src/core/jit/transform/check_cache.rs @@ -27,14 +27,17 @@ fn check_cache(ir: &IR) -> Option { (Some(age1), Some(age2)) => Some(age1.min(age2)), _ => None, }, + IR::Merge(vec) => vec + .iter() + .map(|ir| check_cache(ir)) + .min() + .unwrap_or_default(), IR::Discriminate(_, ir) => check_cache(ir), - IR::Entity(hash_map) => { - let mut ttl = Some(NonZeroU64::MAX); - for ir in hash_map.values() { - ttl = std::cmp::min(ttl, check_cache(ir)); - } - ttl - } + IR::Entity(hash_map) => hash_map + .values() + .map(|ir| check_cache(ir)) + .min() + .unwrap_or_default(), IR::Dynamic(_) | IR::ContextPath(_) | IR::Map(_) | IR::Service(_) => None, } } diff --git a/src/core/jit/transform/check_const.rs b/src/core/jit/transform/check_const.rs index 0db2ca1bf9..5b7dbf01a1 100644 --- a/src/core/jit/transform/check_const.rs +++ b/src/core/jit/transform/check_const.rs @@ -25,6 +25,7 @@ pub fn is_const(ir: &IR) -> bool { IR::Protect(ir) => is_const(ir), IR::Map(map) => is_const(&map.input), IR::Pipe(ir, ir1) => is_const(ir) && is_const(ir1), + IR::Merge(vec) => vec.iter().all(is_const), IR::Discriminate(_, ir) => is_const(ir), IR::Entity(hash_map) => hash_map.values().all(is_const), IR::Service(_) => true, diff --git a/src/core/jit/transform/check_dedupe.rs b/src/core/jit/transform/check_dedupe.rs index e95be51434..dda36b1f3b 100644 --- a/src/core/jit/transform/check_dedupe.rs +++ b/src/core/jit/transform/check_dedupe.rs @@ -21,6 +21,7 @@ fn check_dedupe(ir: &IR) -> bool { IR::Path(ir, _) => check_dedupe(ir), IR::Protect(ir) => check_dedupe(ir), IR::Pipe(ir, ir1) => check_dedupe(ir) && check_dedupe(ir1), + IR::Merge(vec) => vec.iter().all(check_dedupe), IR::Discriminate(_, ir) => check_dedupe(ir), IR::Entity(hash_map) => hash_map.values().all(check_dedupe), IR::Dynamic(_) => true, diff --git a/src/core/jit/transform/check_protected.rs b/src/core/jit/transform/check_protected.rs index e55bbb3214..9aeaad0bec 100644 --- a/src/core/jit/transform/check_protected.rs +++ b/src/core/jit/transform/check_protected.rs @@ -25,6 +25,7 @@ pub fn is_protected(ir: &IR) -> bool { IR::Protect(_) => true, IR::Map(map) => is_protected(&map.input), IR::Pipe(ir, ir1) => is_protected(ir) || is_protected(ir1), + IR::Merge(vec) => vec.iter().all(is_protected), IR::Discriminate(_, ir) => is_protected(ir), IR::Entity(hash_map) => hash_map.values().any(is_protected), IR::Service(_) => false, From 76ae503b02675d126d2e52c5999fab1642091306 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Thu, 21 Nov 2024 15:35:53 +0000 Subject: [PATCH 05/15] convert resolvers to blueprint --- src/core/blueprint/definitions.rs | 7 +- src/core/blueprint/mustache.rs | 1 + .../blueprint/operators/apollo_federation.rs | 55 +++++------- src/core/blueprint/operators/call.rs | 20 +---- src/core/blueprint/operators/expr.rs | 18 +--- src/core/blueprint/operators/graphql.rs | 22 +---- src/core/blueprint/operators/grpc.rs | 33 +------ src/core/blueprint/operators/http.rs | 22 ----- src/core/blueprint/operators/js.rs | 20 +---- src/core/blueprint/operators/mod.rs | 2 + src/core/blueprint/operators/resolver.rs | 90 +++++++++++++++++++ src/core/config/config.rs | 6 +- src/core/config/transformer/subgraph.rs | 15 +++- 13 files changed, 135 insertions(+), 176 deletions(-) create mode 100644 src/core/blueprint/operators/resolver.rs diff --git a/src/core/blueprint/definitions.rs b/src/core/blueprint/definitions.rs index c09804ed38..3b1bfb4845 100644 --- a/src/core/blueprint/definitions.rs +++ b/src/core/blueprint/definitions.rs @@ -505,13 +505,8 @@ pub fn to_field_definition( name: &str, ) -> Valid { update_args() - .and(update_http().trace(config::Http::trace_name().as_str())) - .and(update_grpc(operation_type).trace(config::Grpc::trace_name().as_str())) - .and(update_const_field().trace(config::Expr::trace_name().as_str())) - .and(update_js_field().trace(config::JS::trace_name().as_str())) - .and(update_graphql(operation_type).trace(config::GraphQL::trace_name().as_str())) + .and(update_resolver(operation_type, object_name)) .and(update_modify().trace(config::Modify::trace_name().as_str())) - .and(update_call(operation_type, object_name).trace(config::Call::trace_name().as_str())) .and(fix_dangling_resolvers()) .and(update_cache_resolvers()) .and(update_protected(object_name).trace(Protected::trace_name().as_str())) diff --git a/src/core/blueprint/mustache.rs b/src/core/blueprint/mustache.rs index 8845aaa5ac..42e2610eff 100644 --- a/src/core/blueprint/mustache.rs +++ b/src/core/blueprint/mustache.rs @@ -103,6 +103,7 @@ impl<'a> MustachePartsValidator<'a> { } impl FieldDefinition { + // TODO: need validation for multiple resolvers pub fn validate_field(&self, type_of: &config::Type, config: &Config) -> Valid<(), String> { // XXX we could use `Mustache`'s `render` method with a mock // struct implementing the `PathString` trait encapsulating `validation_map` diff --git a/src/core/blueprint/operators/apollo_federation.rs b/src/core/blueprint/operators/apollo_federation.rs index 46c4a8e770..49e7a54fba 100644 --- a/src/core/blueprint/operators/apollo_federation.rs +++ b/src/core/blueprint/operators/apollo_federation.rs @@ -4,7 +4,7 @@ use std::fmt::Write; use async_graphql::parser::types::ServiceDocument; use tailcall_valid::{Valid, Validator}; -use super::{compile_call, compile_expr, compile_graphql, compile_grpc, compile_http, compile_js}; +use super::{compile_resolver, CompileResolver}; use crate::core::blueprint::{Blueprint, Definition, TryFoldConfig}; use crate::core::config::{ ApolloFederation, ConfigModule, EntityResolver, Field, GraphQLOperationType, Resolver, @@ -13,8 +13,8 @@ use crate::core::ir::model::IR; use crate::core::Type; pub struct CompileEntityResolver<'a> { - config_module: &'a ConfigModule, - entity_resolver: &'a EntityResolver, + pub config_module: &'a ConfigModule, + pub entity_resolver: &'a EntityResolver, } pub fn compile_entity_resolver(inputs: CompileEntityResolver<'_>) -> Valid { @@ -31,36 +31,6 @@ pub fn compile_entity_resolver(inputs: CompileEntityResolver<'_>) -> Valid compile_http( - config_module, - http, - // inner resolver should resolve only single instance of type, not a list - false, - ), - Resolver::Grpc(grpc) => compile_grpc(super::CompileGrpc { - config_module, - operation_type: &GraphQLOperationType::Query, - field, - grpc, - validate_with_schema: true, - }), - Resolver::Graphql(graphql) => compile_graphql( - config_module, - &GraphQLOperationType::Query, - type_name, - graphql, - ), - Resolver::Call(call) => { - compile_call(config_module, call, &GraphQLOperationType::Query, type_name) - } - Resolver::Js(js) => { - compile_js(super::CompileJs { js, script: &config_module.extensions().script }) - } - Resolver::Expr(expr) => { - compile_expr(super::CompileExpr { config_module, field, expr, validate: true }) - } Resolver::ApolloFederation(federation) => match federation { ApolloFederation::EntityResolver(entity_resolver) => { compile_entity_resolver(CompileEntityResolver { entity_resolver, ..inputs }) @@ -70,6 +40,18 @@ pub fn compile_entity_resolver(inputs: CompileEntityResolver<'_>) -> Valid { + let inputs = CompileResolver { + config_module, + field, + operation_type: &GraphQLOperationType::Query, + object_name: type_name, + }; + + compile_resolver(&inputs, resolver).and_then(|resolver| { + Valid::from_option(resolver, "Resolver is empty".to_string()) + }) + } }; ir.map(|ir| { @@ -126,7 +108,12 @@ pub fn update_federation<'a>() -> TryFoldConfig<'a, Blueprint> { format!("Cannot find field {name} in the type"), ) .and_then(|field| { - let Some(Resolver::ApolloFederation(federation)) = &field.resolver else { + let federation = field + .resolvers + .iter() + .find(|resolver| matches!(resolver, Resolver::ApolloFederation(_))); + + let Some(Resolver::ApolloFederation(federation)) = federation else { return Valid::succeed(b_field); }; diff --git a/src/core/blueprint/operators/call.rs b/src/core/blueprint/operators/call.rs index 73afa763f7..a496eea479 100644 --- a/src/core/blueprint/operators/call.rs +++ b/src/core/blueprint/operators/call.rs @@ -3,26 +3,8 @@ use tailcall_valid::{Valid, ValidationError, Validator}; use crate::core::blueprint::*; use crate::core::config; -use crate::core::config::{Field, GraphQLOperationType, Resolver}; +use crate::core::config::{Field, GraphQLOperationType}; use crate::core::ir::model::IR; -use crate::core::try_fold::TryFold; - -pub fn update_call<'a>( - operation_type: &'a GraphQLOperationType, - object_name: &'a str, -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> -{ - TryFold::<(&ConfigModule, &Field, &config::Type, &str), FieldDefinition, String>::new( - move |(config, field, _, _), b_field| { - let Some(Resolver::Call(call)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_call(config, call, operation_type, object_name) - .map(|resolver| b_field.resolver(Some(resolver))) - }, - ) -} pub fn compile_call( config_module: &ConfigModule, diff --git a/src/core/blueprint/operators/expr.rs b/src/core/blueprint/operators/expr.rs index 42d767487c..510e8945c1 100644 --- a/src/core/blueprint/operators/expr.rs +++ b/src/core/blueprint/operators/expr.rs @@ -3,10 +3,9 @@ use tailcall_valid::{Valid, ValidationError, Validator}; use crate::core::blueprint::*; use crate::core::config; -use crate::core::config::{Expr, Field, Resolver}; +use crate::core::config::Expr; use crate::core::ir::model::IR; use crate::core::ir::model::IR::Dynamic; -use crate::core::try_fold::TryFold; fn validate_data_with_schema( config: &config::Config, @@ -58,18 +57,3 @@ pub fn compile_expr(inputs: CompileExpr) -> Valid { } }) } - -pub fn update_const_field<'a>( -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> -{ - TryFold::<(&ConfigModule, &Field, &config::Type, &str), FieldDefinition, String>::new( - |(config_module, field, _, _), b_field| { - let Some(Resolver::Expr(expr)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_expr(CompileExpr { config_module, field, expr, validate: true }) - .map(|resolver| b_field.resolver(Some(resolver))) - }, - ) -} diff --git a/src/core/blueprint/operators/graphql.rs b/src/core/blueprint/operators/graphql.rs index ee25eb5fd1..92b2b9f61f 100644 --- a/src/core/blueprint/operators/graphql.rs +++ b/src/core/blueprint/operators/graphql.rs @@ -2,15 +2,11 @@ use std::collections::{HashMap, HashSet}; use tailcall_valid::{Valid, ValidationError, Validator}; -use crate::core::blueprint::FieldDefinition; -use crate::core::config::{ - Config, ConfigModule, Field, GraphQL, GraphQLOperationType, Resolver, Type, -}; +use crate::core::config::{Config, ConfigModule, GraphQL, GraphQLOperationType}; use crate::core::graphql::RequestTemplate; use crate::core::helpers; use crate::core::ir::model::{IO, IR}; use crate::core::ir::RelatedFields; -use crate::core::try_fold::TryFold; fn create_related_fields( config: &Config, @@ -85,19 +81,3 @@ pub fn compile_graphql( IR::IO(IO::GraphQL { req_template, field_name, batch, dl_id: None, dedupe }) }) } - -pub fn update_graphql<'a>( - operation_type: &'a GraphQLOperationType, -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a Type, &'a str), FieldDefinition, String> { - TryFold::<(&ConfigModule, &Field, &Type, &'a str), FieldDefinition, String>::new( - |(config, field, type_of, _), b_field| { - let Some(Resolver::Graphql(graphql)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_graphql(config, operation_type, field.type_of.name(), graphql) - .map(|resolver| b_field.resolver(Some(resolver))) - .and_then(|b_field| b_field.validate_field(type_of, config).map_to(b_field)) - }, - ) -} diff --git a/src/core/blueprint/operators/grpc.rs b/src/core/blueprint/operators/grpc.rs index 3f730b109c..108c8cbf84 100644 --- a/src/core/blueprint/operators/grpc.rs +++ b/src/core/blueprint/operators/grpc.rs @@ -5,16 +5,14 @@ use prost_reflect::FieldDescriptor; use tailcall_valid::{Valid, ValidationError, Validator}; use super::apply_select; -use crate::core::blueprint::FieldDefinition; use crate::core::config::group_by::GroupBy; -use crate::core::config::{Config, ConfigModule, Field, GraphQLOperationType, Grpc, Resolver}; +use crate::core::config::{Config, ConfigModule, Field, GraphQLOperationType, Grpc}; use crate::core::grpc::protobuf::{ProtobufOperation, ProtobufSet}; use crate::core::grpc::request_template::RequestTemplate; use crate::core::ir::model::{IO, IR}; use crate::core::json::JsonSchema; use crate::core::mustache::Mustache; -use crate::core::try_fold::TryFold; -use crate::core::{config, helpers}; +use crate::core::helpers; fn to_url(grpc: &Grpc, method: &GrpcMethod) -> Valid { Valid::succeed(grpc.url.as_str()).and_then(|base_url| { @@ -210,33 +208,6 @@ pub fn compile_grpc(inputs: CompileGrpc) -> Valid { .and_then(apply_select) } -pub fn update_grpc<'a>( - operation_type: &'a GraphQLOperationType, -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> -{ - TryFold::<(&ConfigModule, &Field, &config::Type, &'a str), FieldDefinition, String>::new( - |(config_module, field, type_of, _name), b_field| { - let Some(Resolver::Grpc(grpc)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_grpc(CompileGrpc { - config_module, - operation_type, - field, - grpc, - validate_with_schema: true, - }) - .map(|resolver| b_field.resolver(Some(resolver))) - .and_then(|b_field| { - b_field - .validate_field(type_of, config_module) - .map_to(b_field) - }) - }, - ) -} - #[cfg(test)] mod tests { use std::convert::TryFrom; diff --git a/src/core/blueprint/operators/http.rs b/src/core/blueprint/operators/http.rs index 6518388845..b181c48afa 100644 --- a/src/core/blueprint/operators/http.rs +++ b/src/core/blueprint/operators/http.rs @@ -2,11 +2,9 @@ use tailcall_valid::{Valid, ValidationError, Validator}; use crate::core::blueprint::*; use crate::core::config::group_by::GroupBy; -use crate::core::config::{Field, Resolver}; use crate::core::endpoint::Endpoint; use crate::core::http::{HttpFilter, Method, RequestTemplate}; use crate::core::ir::model::{IO, IR}; -use crate::core::try_fold::TryFold; use crate::core::{config, helpers, Mustache}; pub fn compile_http( @@ -92,23 +90,3 @@ pub fn compile_http( }) .and_then(apply_select) } - -pub fn update_http<'a>( -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> -{ - TryFold::<(&ConfigModule, &Field, &config::Type, &'a str), FieldDefinition, String>::new( - |(config_module, field, type_of, _), b_field| { - let Some(Resolver::Http(http)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_http(config_module, http, field.type_of.is_list()) - .map(|resolver| b_field.resolver(Some(resolver))) - .and_then(|b_field| { - b_field - .validate_field(type_of, config_module) - .map_to(b_field) - }) - }, - ) -} diff --git a/src/core/blueprint/operators/js.rs b/src/core/blueprint/operators/js.rs index 2eb35ee423..c65c735383 100644 --- a/src/core/blueprint/operators/js.rs +++ b/src/core/blueprint/operators/js.rs @@ -1,10 +1,7 @@ use tailcall_valid::{Valid, Validator}; -use crate::core::blueprint::FieldDefinition; -use crate::core::config; -use crate::core::config::{ConfigModule, Field, Resolver, JS}; +use crate::core::config::JS; use crate::core::ir::model::{IO, IR}; -use crate::core::try_fold::TryFold; pub struct CompileJs<'a> { pub js: &'a JS, @@ -16,18 +13,3 @@ pub fn compile_js(inputs: CompileJs) -> Valid { Valid::from_option(inputs.script.as_ref(), "script is required".to_string()) .map(|_| IR::IO(IO::Js { name: name.to_string() })) } - -pub fn update_js_field<'a>( -) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> -{ - TryFold::<(&ConfigModule, &Field, &config::Type, &str), FieldDefinition, String>::new( - |(module, field, _, _), b_field| { - let Some(Resolver::Js(js)) = &field.resolver else { - return Valid::succeed(b_field); - }; - - compile_js(CompileJs { script: &module.extensions().script, js }) - .map(|resolver| b_field.resolver(Some(resolver))) - }, - ) -} diff --git a/src/core/blueprint/operators/mod.rs b/src/core/blueprint/operators/mod.rs index 77947e9571..0548e74111 100644 --- a/src/core/blueprint/operators/mod.rs +++ b/src/core/blueprint/operators/mod.rs @@ -8,6 +8,7 @@ mod http; mod js; mod modify; mod protected; +mod resolver; mod select; pub use apollo_federation::*; @@ -20,4 +21,5 @@ pub use http::*; pub use js::*; pub use modify::*; pub use protected::*; +pub use resolver::*; pub use select::*; diff --git a/src/core/blueprint/operators/resolver.rs b/src/core/blueprint/operators/resolver.rs new file mode 100644 index 0000000000..e842a45598 --- /dev/null +++ b/src/core/blueprint/operators/resolver.rs @@ -0,0 +1,90 @@ +use tailcall_valid::{Valid, Validator}; + +use super::{compile_call, compile_expr, compile_graphql, compile_grpc, compile_http, compile_js}; +use crate::core::blueprint::FieldDefinition; +use crate::core::config::{self, ConfigModule, Field, GraphQLOperationType, Resolver}; +use crate::core::directive::DirectiveCodec; +use crate::core::ir::model::IR; +use crate::core::try_fold::TryFold; + +pub struct CompileResolver<'a> { + pub config_module: &'a ConfigModule, + pub field: &'a Field, + pub operation_type: &'a GraphQLOperationType, + pub object_name: &'a str, +} + +pub fn compile_resolver( + inputs: &CompileResolver, + resolver: &Resolver, +) -> Valid, String> { + let CompileResolver { config_module, field, operation_type, object_name } = inputs; + + match resolver { + Resolver::Http(http) => compile_http( + config_module, + http, + // inner resolver should resolve only single instance of type, not a list + field.type_of.is_list(), + ) + .trace(config::Http::trace_name().as_str()), + Resolver::Grpc(grpc) => compile_grpc(super::CompileGrpc { + config_module, + operation_type, + field, + grpc, + validate_with_schema: true, + }) + .trace(config::Grpc::trace_name().as_str()), + Resolver::Graphql(graphql) => { + compile_graphql(config_module, operation_type, field.type_of.name(), graphql) + .trace(config::GraphQL::trace_name().as_str()) + } + Resolver::Call(call) => compile_call(config_module, call, operation_type, object_name) + .trace(config::Call::trace_name().as_str()), + Resolver::Js(js) => { + compile_js(super::CompileJs { js, script: &config_module.extensions().script }) + .trace(config::JS::trace_name().as_str()) + } + Resolver::Expr(expr) => { + compile_expr(super::CompileExpr { config_module, field, expr, validate: true }) + .trace(config::Expr::trace_name().as_str()) + } + Resolver::ApolloFederation(_) => { + // ignore the Federation resolvers since they have special meaning + // and should be executed only after the other config processing + return Valid::succeed(None); + } + } + .map(Some) +} + +pub fn update_resolver<'a>( + operation_type: &'a GraphQLOperationType, + object_name: &'a str, +) -> TryFold<'a, (&'a ConfigModule, &'a Field, &'a config::Type, &'a str), FieldDefinition, String> +{ + TryFold::<(&ConfigModule, &Field, &config::Type, &str), FieldDefinition, String>::new( + |(config_module, field, type_of, _), b_field| { + let inputs = CompileResolver { config_module, field, operation_type, object_name }; + + Valid::from_iter(field.resolvers.iter(), |resolver| { + compile_resolver(&inputs, resolver) + }) + .map(|mut resolvers| match resolvers.len() { + 0 => None, + 1 => resolvers.pop().unwrap(), + _ => Some(IR::Merge(resolvers.into_iter().flatten().collect())), + }) + .map(|resolver| b_field.resolver(resolver)) + .and_then(|b_field| { + // TODO: handle multiple resolvers in validation + b_field + // TODO: there are `validate_field` for field, but not for types + // when we use federations's entities + .validate_field(type_of, config_module) + .map_to(b_field) + }) + }, + ) +} diff --git a/src/core/config/config.rs b/src/core/config/config.rs index 23560625ef..e16e6b0c9b 100644 --- a/src/core/config/config.rs +++ b/src/core/config/config.rs @@ -116,7 +116,7 @@ pub struct Type { pub protected: Option, /// /// Apollo federation entity resolver. - #[serde(flatten, default, skip_serializing_if = "is_default")] + #[serde(default, skip_serializing_if = "is_default")] pub resolvers: Vec, /// /// Any additional directives @@ -225,7 +225,7 @@ pub struct Field { /// /// Resolver for the field - #[serde(flatten, default, skip_serializing_if = "is_default")] + #[serde(default, skip_serializing_if = "is_default")] pub resolvers: Vec, /// @@ -248,7 +248,7 @@ impl Field { pub fn has_batched_resolver(&self) -> bool { if self.resolvers.is_empty() { - true + false } else { self.resolvers.iter().all(Resolver::is_batched) } diff --git a/src/core/config/transformer/subgraph.rs b/src/core/config/transformer/subgraph.rs index b95dc2d5a3..e2b058fbb9 100644 --- a/src/core/config/transformer/subgraph.rs +++ b/src/core/config/transformer/subgraph.rs @@ -43,7 +43,14 @@ impl Transform for Subgraph { let mut resolver_by_type = BTreeMap::new(); let valid = Valid::from_iter(config.types.iter_mut(), |(type_name, ty)| { - if let Some(resolver) = &ty.resolver { + if ty.resolvers.len() > 1 { + // TODO: should support multiple different resolvers actually, see https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/entities/define-keys#multiple-keys + return Valid::fail( + "Only single resolver for entity is currently supported".to_string(), + ); + } + + if let Some(resolver) = ty.resolvers.get(0) { resolver_by_type.insert(type_name.clone(), resolver.clone()); KeysExtractor::validate(&config_types, resolver, type_name).and_then(|_| { @@ -95,7 +102,7 @@ impl Transform for Subgraph { Field { type_of: Type::from(SERVICE_TYPE_NAME.to_owned()).into_required(), doc: Some("Apollo federation Query._service resolver".to_string()), - resolver: Some(Resolver::ApolloFederation(ApolloFederation::Service)), + resolvers: vec![Resolver::ApolloFederation(ApolloFederation::Service)], ..Default::default() }, ); @@ -135,9 +142,9 @@ impl Transform for Subgraph { .into_required(), args: [(ENTITIES_ARG_NAME.to_owned(), arg)].into_iter().collect(), doc: Some("Apollo federation Query._entities resolver".to_string()), - resolver: Some(Resolver::ApolloFederation( + resolvers: vec![Resolver::ApolloFederation( ApolloFederation::EntityResolver(entity_resolver), - )), + )], ..Default::default() }, ); From fcb62172fa67771b64bd34a77c03f43c0bee1193 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Sat, 23 Nov 2024 12:53:59 +0000 Subject: [PATCH 06/15] fix conversion from json/yaml --- src/cli/tc/init.rs | 2 +- src/core/blueprint/definitions.rs | 6 +- src/core/blueprint/mustache.rs | 8 +- .../blueprint/operators/apollo_federation.rs | 2 +- src/core/config/config.rs | 24 +++--- src/core/config/resolver.rs | 85 +++++++++++++++++++ src/core/config/transformer/subgraph.rs | 9 +- src/core/generator/from_proto.rs | 5 +- .../generator/json/operation_generator.rs | 2 +- src/core/grpc/data_loader_request.rs | 2 +- src/core/grpc/protobuf.rs | 2 +- src/core/grpc/request_template.rs | 2 +- tailcall-macros/src/resolver.rs | 4 +- 13 files changed, 122 insertions(+), 31 deletions(-) diff --git a/src/cli/tc/init.rs b/src/cli/tc/init.rs index a483f6d74a..89f8885bd3 100644 --- a/src/cli/tc/init.rs +++ b/src/cli/tc/init.rs @@ -87,7 +87,7 @@ async fn confirm_and_write_yml( fn main_config() -> Config { let field = Field { type_of: Type::from("String".to_owned()).into_required(), - resolvers: vec![Resolver::Expr(Expr { body: "Hello, World!".into() })], + resolvers: Resolver::Expr(Expr { body: "Hello, World!".into() }).into(), ..Default::default() }; diff --git a/src/core/blueprint/definitions.rs b/src/core/blueprint/definitions.rs index 3b1bfb4845..f83ae833f2 100644 --- a/src/core/blueprint/definitions.rs +++ b/src/core/blueprint/definitions.rs @@ -107,10 +107,10 @@ fn process_field_within_type(context: ProcessFieldWithinTypeContext) -> Valid { type_of: &'a config::Type, @@ -126,6 +129,7 @@ impl FieldDefinition { }) })) .unit() + .trace(config::Http::trace_name().as_str()) } Some(IR::IO(IO::GraphQL { req_template, .. })) => { Valid::from_iter(req_template.headers.clone(), |(_, mustache)| { @@ -145,6 +149,7 @@ impl FieldDefinition { } }) .unit() + .trace(config::GraphQL::trace_name().as_str()) } Some(IR::IO(IO::Grpc { req_template, .. })) => { Valid::from_iter(req_template.url.expression_segments(), |parts| { @@ -173,6 +178,7 @@ impl FieldDefinition { } }) .unit() + .trace(config::Grpc::trace_name().as_str()) } _ => Valid::succeed(()), } diff --git a/src/core/blueprint/operators/apollo_federation.rs b/src/core/blueprint/operators/apollo_federation.rs index 49e7a54fba..085ecb548c 100644 --- a/src/core/blueprint/operators/apollo_federation.rs +++ b/src/core/blueprint/operators/apollo_federation.rs @@ -111,7 +111,7 @@ pub fn update_federation<'a>() -> TryFoldConfig<'a, Blueprint> { let federation = field .resolvers .iter() - .find(|resolver| matches!(resolver, Resolver::ApolloFederation(_))); + .find(|&resolver| matches!(resolver, Resolver::ApolloFederation(_))); let Some(Resolver::ApolloFederation(federation)) = federation else { return Valid::succeed(b_field); diff --git a/src/core/config/config.rs b/src/core/config/config.rs index e16e6b0c9b..17e0255d31 100644 --- a/src/core/config/config.rs +++ b/src/core/config/config.rs @@ -14,10 +14,10 @@ use tailcall_typedefs_common::ServiceDocumentBuilder; use tailcall_valid::{Valid, Validator}; use super::directive::Directive; -use super::from_document::from_document; +use super::{from_document::from_document, Resolver, Resolvers}; use super::{ AddField, Alias, Cache, Call, Discriminate, Expr, GraphQL, Grpc, Http, Link, Modify, Omit, - Protected, Resolver, Server, Telemetry, Upstream, JS, + Protected, Server, Telemetry, Upstream, JS, }; use crate::core::config::npo::QueryPath; use crate::core::config::source::Source; @@ -116,8 +116,8 @@ pub struct Type { pub protected: Option, /// /// Apollo federation entity resolver. - #[serde(default, skip_serializing_if = "is_default")] - pub resolvers: Vec, + #[serde(flatten, default, skip_serializing_if = "is_default")] + pub resolvers: Resolvers, /// /// Any additional directives #[serde(default, skip_serializing_if = "is_default")] @@ -225,8 +225,8 @@ pub struct Field { /// /// Resolver for the field - #[serde(default, skip_serializing_if = "is_default")] - pub resolvers: Vec, + #[serde(flatten, default, skip_serializing_if = "is_default")] + pub resolvers: Resolvers, /// /// Any additional directives @@ -694,25 +694,23 @@ mod tests { use pretty_assertions::assert_eq; use super::*; - use crate::core::directive::DirectiveCodec; + use crate::core::{config::Resolver, directive::DirectiveCodec}; #[test] fn test_field_has_or_not_batch_resolver() { let f1 = Field { ..Default::default() }; let f2 = Field { - resolvers: vec![Resolver::Http(Http { + resolvers: Resolver::Http(Http { batch_key: vec!["id".to_string()], ..Default::default() - })], + }) + .into(), ..Default::default() }; let f3 = Field { - resolvers: vec![Resolver::Http(Http { - batch_key: vec![], - ..Default::default() - })], + resolvers: Resolver::Http(Http { batch_key: vec![], ..Default::default() }).into(), ..Default::default() }; diff --git a/src/core/config/resolver.rs b/src/core/config/resolver.rs index e30d3150ed..611f520511 100644 --- a/src/core/config/resolver.rs +++ b/src/core/config/resolver.rs @@ -1,3 +1,5 @@ +use std::ops::Deref; + use async_graphql::parser::types::ConstDirective; use async_graphql::Positioned; use serde::{Deserialize, Serialize}; @@ -6,6 +8,7 @@ use tailcall_valid::{Valid, Validator}; use super::{Call, EntityResolver, Expr, GraphQL, Grpc, Http, JS}; use crate::core::directive::DirectiveCodec; +use crate::core::merge_right::MergeRight; #[derive(Clone, Debug, PartialEq, Eq)] pub enum ApolloFederation { @@ -53,3 +56,85 @@ impl Resolver { } } } + +#[derive(Default, Clone, Debug, PartialEq, Eq, schemars::JsonSchema)] +pub struct Resolvers(pub Vec); + +// Implement custom serializer to provide backward compatibility for JSON/YAML formats +// when converting config to config file. In case the only one resolver is defined +// serialize it as flatten structure instead of `resolvers: []` +// TODO: this is not required in case Tailcall drop defining type schema in json/yaml files +impl Serialize for Resolvers { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + let resolvers = &self.0; + + if resolvers.len() == 1 { + resolvers.get(0).unwrap().serialize(serializer) + } else { + resolvers.serialize(serializer) + } + } +} + +// Implement custom deserializer to provide backward compatibility for JSON/YAML formats +// when parsing config files. In case the `resolvers` field is defined in config +// parse it as vec of [Resolver] and otherwise try to parse it as single [Resolver] +// TODO: this is not required in case Tailcall drop defining type schema in json/yaml files +impl<'de> Deserialize<'de> for Resolvers { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error; + use serde_json::Value; + + let mut value = Value::deserialize(deserializer)?; + + if let Value::Object(obj) = &mut value { + if obj.len() == 0 { + return Ok(Resolvers::default()); + } + + if let Some(value) = obj.remove("resolvers") { + let resolvers = serde_json::from_value(value).map_err(Error::custom)?; + + return Ok(Self(resolvers)); + } + } + + let resolver: Resolver = serde_json::from_value(value).map_err(Error::custom)?; + + Ok(Resolvers::from(resolver)) + } +} + +impl From for Resolvers { + fn from(value: Resolver) -> Self { + Self(vec![value]) + } +} + +impl Deref for Resolvers { + type Target = Vec; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +// Custom implementation for MergeRight in order +// to provide compatibility with old tests where +// the same resolver could be defined in multiple configs +// leading to duplicated resolver in such tests +impl MergeRight for Resolvers { + fn merge_right(self, other: Self) -> Self { + if other.is_empty() { + self + } else { + other + } + } +} diff --git a/src/core/config/transformer/subgraph.rs b/src/core/config/transformer/subgraph.rs index e2b058fbb9..aff870e981 100644 --- a/src/core/config/transformer/subgraph.rs +++ b/src/core/config/transformer/subgraph.rs @@ -102,7 +102,7 @@ impl Transform for Subgraph { Field { type_of: Type::from(SERVICE_TYPE_NAME.to_owned()).into_required(), doc: Some("Apollo federation Query._service resolver".to_string()), - resolvers: vec![Resolver::ApolloFederation(ApolloFederation::Service)], + resolvers: Resolver::ApolloFederation(ApolloFederation::Service).into(), ..Default::default() }, ); @@ -142,9 +142,10 @@ impl Transform for Subgraph { .into_required(), args: [(ENTITIES_ARG_NAME.to_owned(), arg)].into_iter().collect(), doc: Some("Apollo federation Query._entities resolver".to_string()), - resolvers: vec![Resolver::ApolloFederation( - ApolloFederation::EntityResolver(entity_resolver), - )], + resolvers: Resolver::ApolloFederation(ApolloFederation::EntityResolver( + entity_resolver, + )) + .into(), ..Default::default() }, ); diff --git a/src/core/generator/from_proto.rs b/src/core/generator/from_proto.rs index 4275a56cbc..93bc676a82 100644 --- a/src/core/generator/from_proto.rs +++ b/src/core/generator/from_proto.rs @@ -367,7 +367,7 @@ impl Context { .to_string(); cfg_field.type_of = cfg_field.type_of.with_name(output_ty); - cfg_field.resolvers = vec![Resolver::Grpc(Grpc { + cfg_field.resolvers = Resolver::Grpc(Grpc { url: url.to_string(), body, batch_key: vec![], @@ -375,7 +375,8 @@ impl Context { method: field_name.id(), dedupe: None, select: None, - })]; + }) + .into(); let method_path = PathBuilder::new(&path).extend(PathField::Method, method_index as i32); diff --git a/src/core/generator/json/operation_generator.rs b/src/core/generator/json/operation_generator.rs index 27d57ab94c..fe0aeb36aa 100644 --- a/src/core/generator/json/operation_generator.rs +++ b/src/core/generator/json/operation_generator.rs @@ -50,7 +50,7 @@ impl OperationTypeGenerator { ); } - field.resolvers = vec![Resolver::Http(http_resolver)]; + field.resolvers = Resolver::Http(http_resolver).into(); // if type is already present, then append the new field to it else create one. let req_op = request_sample diff --git a/src/core/grpc/data_loader_request.rs b/src/core/grpc/data_loader_request.rs index e5a09681c0..1f49188d6f 100644 --- a/src/core/grpc/data_loader_request.rs +++ b/src/core/grpc/data_loader_request.rs @@ -85,7 +85,7 @@ mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolvers(vec![Resolver::Grpc(grpc)]), + Field::default().resolvers(Resolver::Grpc(grpc).into()), )]), ); diff --git a/src/core/grpc/protobuf.rs b/src/core/grpc/protobuf.rs index 2707fde967..979b7afb2f 100644 --- a/src/core/grpc/protobuf.rs +++ b/src/core/grpc/protobuf.rs @@ -276,7 +276,7 @@ pub mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolvers(vec![Resolver::Grpc(grpc)]), + Field::default().resolvers(Resolver::Grpc(grpc).into()), )]), ); Ok(reader diff --git a/src/core/grpc/request_template.rs b/src/core/grpc/request_template.rs index f4579ba8ee..b1cb5653e4 100644 --- a/src/core/grpc/request_template.rs +++ b/src/core/grpc/request_template.rs @@ -171,7 +171,7 @@ mod tests { "foo".to_string(), Type::default().fields(vec![( "bar", - Field::default().resolvers(vec![Resolver::Grpc(grpc)]), + Field::default().resolvers(Resolver::Grpc(grpc).into()), )]), ); diff --git a/tailcall-macros/src/resolver.rs b/tailcall-macros/src/resolver.rs index 871e647b30..2efc733b91 100644 --- a/tailcall-macros/src/resolver.rs +++ b/tailcall-macros/src/resolver.rs @@ -94,14 +94,14 @@ pub fn expand_resolver_derive(input: DeriveInput) -> syn::Result { impl #name { pub fn from_directives( directives: &[Positioned], - ) -> Valid, String> { + ) -> Valid { Valid::from_iter(directives.iter(), |directive| { #(#variant_parsers)* Valid::succeed(None) }) .map(|resolvers| { - resolvers.into_iter().filter_map(std::convert::identity).collect() + crate::core::config::Resolvers(resolvers.into_iter().flatten().collect()) }) } From 20d68e0c534ec33495fd8509a04cd89399627478 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Sat, 23 Nov 2024 13:09:57 +0000 Subject: [PATCH 07/15] add tests and improve validations --- src/core/blueprint/mustache.rs | 56 +++++++------ src/core/blueprint/operators/resolver.rs | 1 - .../test-http-with-inline.md_error.snap | 19 ++++- ...rectives-on-field-validation.md_error.snap | 47 +++++++++++ ...e-resolvable-directives-on-field.md_0.snap | 36 +++++++++ ...olvable-directives-on-field.md_client.snap | 25 ++++++ ...solvable-directives-on-field.md_error.snap | 22 ------ ...olvable-directives-on-field.md_merged.snap | 27 +++++++ ...solvable-directives-on-field-validation.md | 28 +++++++ ...multiple-resolvable-directives-on-field.md | 78 +++++++++++++++++-- 10 files changed, 283 insertions(+), 56 deletions(-) create mode 100644 tests/core/snapshots/test-multiple-resolvable-directives-on-field-validation.md_error.snap create mode 100644 tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_0.snap create mode 100644 tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_client.snap delete mode 100644 tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_error.snap create mode 100644 tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_merged.snap create mode 100644 tests/execution/test-multiple-resolvable-directives-on-field-validation.md diff --git a/src/core/blueprint/mustache.rs b/src/core/blueprint/mustache.rs index 64fc430b97..b09e60f52b 100644 --- a/src/core/blueprint/mustache.rs +++ b/src/core/blueprint/mustache.rs @@ -103,45 +103,37 @@ impl<'a> MustachePartsValidator<'a> { Valid::succeed(()) } -} - -impl FieldDefinition { - // TODO: need validation for multiple resolvers - pub fn validate_field(&self, type_of: &config::Type, config: &Config) -> Valid<(), String> { - // XXX we could use `Mustache`'s `render` method with a mock - // struct implementing the `PathString` trait encapsulating `validation_map` - // but `render` simply falls back to the default value for a given - // type if it doesn't exist, so we wouldn't be able to get enough - // context from that method alone - // So we must duplicate some of that logic here :( - let parts_validator = MustachePartsValidator::new(type_of, config, self); - match &self.resolver { - Some(IR::IO(IO::Http { req_template, .. })) => { + fn validate_resolver(&self, resolver: &IR) -> Valid<(), String> { + match resolver { + IR::Merge(resolvers) => { + Valid::from_iter(resolvers, |resolver| self.validate_resolver(resolver)).unit() + } + IR::IO(IO::Http { req_template, .. }) => { Valid::from_iter(req_template.root_url.expression_segments(), |parts| { - parts_validator.validate(parts, false).trace("path") + self.validate(parts, false).trace("path") }) .and(Valid::from_iter(req_template.query.clone(), |query| { let mustache = &query.value; Valid::from_iter(mustache.expression_segments(), |parts| { - parts_validator.validate(parts, true).trace("query") + self.validate(parts, true).trace("query") }) })) .unit() .trace(config::Http::trace_name().as_str()) } - Some(IR::IO(IO::GraphQL { req_template, .. })) => { + IR::IO(IO::GraphQL { req_template, .. }) => { Valid::from_iter(req_template.headers.clone(), |(_, mustache)| { Valid::from_iter(mustache.expression_segments(), |parts| { - parts_validator.validate(parts, true).trace("headers") + self.validate(parts, true).trace("headers") }) }) .and_then(|_| { if let Some(args) = &req_template.operation_arguments { Valid::from_iter(args, |(_, mustache)| { Valid::from_iter(mustache.expression_segments(), |parts| { - parts_validator.validate(parts, true).trace("args") + self.validate(parts, true).trace("args") }) }) } else { @@ -151,14 +143,14 @@ impl FieldDefinition { .unit() .trace(config::GraphQL::trace_name().as_str()) } - Some(IR::IO(IO::Grpc { req_template, .. })) => { + IR::IO(IO::Grpc { req_template, .. }) => { Valid::from_iter(req_template.url.expression_segments(), |parts| { - parts_validator.validate(parts, false).trace("path") + self.validate(parts, false).trace("path") }) .and( Valid::from_iter(req_template.headers.clone(), |(_, mustache)| { Valid::from_iter(mustache.expression_segments(), |parts| { - parts_validator.validate(parts, true).trace("headers") + self.validate(parts, true).trace("headers") }) }) .unit(), @@ -167,7 +159,7 @@ impl FieldDefinition { if let Some(body) = &req_template.body { if let Some(mustache) = &body.mustache { Valid::from_iter(mustache.expression_segments(), |parts| { - parts_validator.validate(parts, true).trace("body") + self.validate(parts, true).trace("body") }) } else { // TODO: needs review @@ -180,11 +172,29 @@ impl FieldDefinition { .unit() .trace(config::Grpc::trace_name().as_str()) } + // TODO: add validation for @expr _ => Valid::succeed(()), } } } +impl FieldDefinition { + pub fn validate_field(&self, type_of: &config::Type, config: &Config) -> Valid<(), String> { + // XXX we could use `Mustache`'s `render` method with a mock + // struct implementing the `PathString` trait encapsulating `validation_map` + // but `render` simply falls back to the default value for a given + // type if it doesn't exist, so we wouldn't be able to get enough + // context from that method alone + // So we must duplicate some of that logic here :( + let parts_validator = MustachePartsValidator::new(type_of, config, self); + + match &self.resolver { + Some(resolver) => parts_validator.validate_resolver(resolver), + None => Valid::succeed(()), + } + } +} + #[cfg(test)] mod test { use tailcall_valid::Validator; diff --git a/src/core/blueprint/operators/resolver.rs b/src/core/blueprint/operators/resolver.rs index e842a45598..394ba90d4b 100644 --- a/src/core/blueprint/operators/resolver.rs +++ b/src/core/blueprint/operators/resolver.rs @@ -78,7 +78,6 @@ pub fn update_resolver<'a>( }) .map(|resolver| b_field.resolver(resolver)) .and_then(|b_field| { - // TODO: handle multiple resolvers in validation b_field // TODO: there are `validate_field` for field, but not for types // when we use federations's entities diff --git a/tests/core/snapshots/test-http-with-inline.md_error.snap b/tests/core/snapshots/test-http-with-inline.md_error.snap index f1751bc827..ff0c14bb99 100644 --- a/tests/core/snapshots/test-http-with-inline.md_error.snap +++ b/tests/core/snapshots/test-http-with-inline.md_error.snap @@ -1,14 +1,27 @@ --- source: tests/core/spec.rs expression: errors +snapshot_kind: text --- [ { - "message": "Cannot add field", + "message": "no value 'userId' found", "trace": [ "Query", - "@addField" + "post", + "@http", + "path" ], - "description": "Path: [post, user, name] contains resolver http at [Post.user]" + "description": null + }, + { + "message": "no value 'userId' found", + "trace": [ + "Query", + "@addField", + "@http", + "path" + ], + "description": null } ] diff --git a/tests/core/snapshots/test-multiple-resolvable-directives-on-field-validation.md_error.snap b/tests/core/snapshots/test-multiple-resolvable-directives-on-field-validation.md_error.snap new file mode 100644 index 0000000000..3d1c41589d --- /dev/null +++ b/tests/core/snapshots/test-multiple-resolvable-directives-on-field-validation.md_error.snap @@ -0,0 +1,47 @@ +--- +source: tests/core/spec.rs +expression: errors +snapshot_kind: text +--- +[ + { + "message": "no value 'id' found", + "trace": [ + "Query", + "user1", + "@http", + "query" + ], + "description": null + }, + { + "message": "no value 'name' found", + "trace": [ + "Query", + "user2", + "@http", + "query" + ], + "description": null + }, + { + "message": "no value 'address' found", + "trace": [ + "Query", + "user3", + "@http", + "query" + ], + "description": null + }, + { + "message": "no argument 'id' found", + "trace": [ + "Query", + "user3", + "@graphQL", + "args" + ], + "description": null + } +] diff --git a/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_0.snap b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_0.snap new file mode 100644 index 0000000000..9f53757f38 --- /dev/null +++ b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_0.snap @@ -0,0 +1,36 @@ +--- +source: tests/core/spec.rs +expression: response +snapshot_kind: text +--- +{ + "status": 200, + "headers": { + "content-type": "application/json" + }, + "body": { + "data": { + "user1": { + "name": "from request 1", + "address": { + "street": "street request 1", + "city": "city request 1" + } + }, + "user2": { + "name": "name expr 2", + "address": { + "street": "street request 2", + "city": "city request 2" + } + }, + "user3": { + "name": "name request 3", + "address": { + "street": "Street from the graphql response", + "city": "city request 3" + } + } + } + } +} diff --git a/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_client.snap b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_client.snap new file mode 100644 index 0000000000..6cc167698c --- /dev/null +++ b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_client.snap @@ -0,0 +1,25 @@ +--- +source: tests/core/spec.rs +expression: formatted +snapshot_kind: text +--- +type Address { + city: String + street: String +} + +type Query { + user1: User + user2: User + user3: User +} + +type User { + address: Address + id: Int + name: String +} + +schema { + query: Query +} diff --git a/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_error.snap b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_error.snap deleted file mode 100644 index 7e7969e515..0000000000 --- a/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_error.snap +++ /dev/null @@ -1,22 +0,0 @@ ---- -source: tests/core/spec.rs -expression: errors ---- -[ - { - "message": "Multiple resolvers detected [@http, @expr]", - "trace": [ - "Query", - "user1" - ], - "description": null - }, - { - "message": "Multiple resolvers detected [@http, @call]", - "trace": [ - "Query", - "user2" - ], - "description": null - } -] diff --git a/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_merged.snap b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_merged.snap new file mode 100644 index 0000000000..36b39ceaef --- /dev/null +++ b/tests/core/snapshots/test-multiple-resolvable-directives-on-field.md_merged.snap @@ -0,0 +1,27 @@ +--- +source: tests/core/spec.rs +expression: formatter +snapshot_kind: text +--- +schema @server @upstream { + query: Query +} + +type Address { + city: String + street: String +} + +type Query { + user1: User @expr(body: {name: "name expr 1"}) @http(url: "http://jsonplaceholder.typicode.com/users/1") + user2: User @http(url: "http://jsonplaceholder.typicode.com/users/2") @expr(body: {name: "name expr 2"}) + user3: User + @http(url: "http://jsonplaceholder.typicode.com/users/3") + @graphQL(args: [{key: "id", value: "3"}], url: "http://upstream/graphql", name: "user") +} + +type User { + address: Address + id: Int + name: String +} diff --git a/tests/execution/test-multiple-resolvable-directives-on-field-validation.md b/tests/execution/test-multiple-resolvable-directives-on-field-validation.md new file mode 100644 index 0000000000..951a6e2a96 --- /dev/null +++ b/tests/execution/test-multiple-resolvable-directives-on-field-validation.md @@ -0,0 +1,28 @@ +--- +error: true +--- + +# Test validation for multiple resolvable directives on field + +```graphql @config +schema @server { + query: Query +} + +type User { + name: String + id: Int + address: Address +} + +type Address { + city: String + street: String +} + +type Query { + user1: User @expr(body: {name: "{{.value.test}}"}) @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.id}}"}]) + user2: User @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "name", value: "{{.value.name}}"}]) @expr(body: {name: "{{.args.expr}}"}) + user3: User @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.address}}"}]) @graphQL(args: [{key: "id", value: "{{.args.id}}"}], url: "http://upstream/graphql", name: "user") +} +``` diff --git a/tests/execution/test-multiple-resolvable-directives-on-field.md b/tests/execution/test-multiple-resolvable-directives-on-field.md index 3be44ee54f..34fef6fc63 100644 --- a/tests/execution/test-multiple-resolvable-directives-on-field.md +++ b/tests/execution/test-multiple-resolvable-directives-on-field.md @@ -1,8 +1,4 @@ ---- -error: true ---- - -# test-multiple-resolvable-directives-on-field +# Multiple resolvable directives on field ```graphql @config schema @server { @@ -12,10 +8,78 @@ schema @server { type User { name: String id: Int + address: Address +} + +type Address { + city: String + street: String } type Query { - user1: User @expr(body: {name: "John"}) @http(url: "http://jsonplaceholder.typicode.com/users/1") - user2: User @http(url: "http://jsonplaceholder.typicode.com/users/2") @call(steps: [{query: "something"}]) + user1: User @expr(body: {name: "name expr 1"}) @http(url: "http://jsonplaceholder.typicode.com/users/1") + user2: User @http(url: "http://jsonplaceholder.typicode.com/users/2") @expr(body: {name: "name expr 2" }) + user3: User @http(url: "http://jsonplaceholder.typicode.com/users/3") @graphQL(args: [{key: "id", value: "3"}], url: "http://upstream/graphql", name: "user") } ``` + +```yml @mock +- request: + method: GET + url: http://jsonplaceholder.typicode.com/users/1 + response: + status: 200 + body: + address: + city: city request 1 + street: street request 1 + id: 1 + name: from request 1 + +- request: + method: GET + url: http://jsonplaceholder.typicode.com/users/2 + response: + status: 200 + body: + address: + city: city request 2 + street: street request 2 + id: 2 + name: from request 2 + +- request: + method: GET + url: http://jsonplaceholder.typicode.com/users/3 + response: + status: 200 + body: + address: + city: city request 3 + id: 3 + name: name request 3 + +- request: + method: POST + url: http://upstream/graphql + textBody: '{ "query": "query { user(id: 3) { name address { street city } } }" }' + response: + status: 200 + body: + data: + user: + address: + street: Street from the graphql response +``` + +```yml @test +- method: POST + url: http://localhost:8080/graphql + body: + query: | + query { + user1 { name address { street city } } + user2 { name address { street city } } + user3 { name address { street city } } + } +``` From 6e1fd8a6f3e485ccab0ba68f621309e525ae3581 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Sat, 23 Nov 2024 13:33:03 +0000 Subject: [PATCH 08/15] fix lint --- generated/.tailcallrc.schema.json | 236 +++++++----------- src/core/blueprint/mustache.rs | 6 +- src/core/blueprint/operators/grpc.rs | 2 +- src/core/config/config.rs | 7 +- src/core/config/resolver.rs | 22 +- src/core/config/transformer/subgraph.rs | 2 +- src/core/ir/eval.rs | 9 +- src/core/jit/transform/check_cache.rs | 12 +- ...solvable-directives-on-field-validation.md | 12 +- ...multiple-resolvable-directives-on-field.md | 6 +- 10 files changed, 126 insertions(+), 188 deletions(-) diff --git a/generated/.tailcallrc.schema.json b/generated/.tailcallrc.schema.json index e32e9b91f8..03e7b091d9 100644 --- a/generated/.tailcallrc.schema.json +++ b/generated/.tailcallrc.schema.json @@ -400,81 +400,13 @@ }, "Field": { "description": "A field definition containing all the metadata information about resolving a field.", - "type": "object", - "oneOf": [ - { - "type": "object", - "required": [ - "http" - ], - "properties": { - "http": { - "$ref": "#/definitions/Http" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "grpc" - ], - "properties": { - "grpc": { - "$ref": "#/definitions/Grpc" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "graphql" - ], - "properties": { - "graphql": { - "$ref": "#/definitions/GraphQL" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "call" - ], - "properties": { - "call": { - "$ref": "#/definitions/Call" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "js" - ], - "properties": { - "js": { - "$ref": "#/definitions/JS" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "expr" - ], - "properties": { - "expr": { - "$ref": "#/definitions/Expr" - } - }, - "additionalProperties": false - } + "type": [ + "object", + "array" ], + "items": { + "$ref": "#/definitions/Resolver" + }, "properties": { "args": { "description": "Map of argument name and its definition.", @@ -1008,6 +940,82 @@ } } }, + "Resolver": { + "oneOf": [ + { + "type": "object", + "required": [ + "http" + ], + "properties": { + "http": { + "$ref": "#/definitions/Http" + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "grpc" + ], + "properties": { + "grpc": { + "$ref": "#/definitions/Grpc" + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "graphql" + ], + "properties": { + "graphql": { + "$ref": "#/definitions/GraphQL" + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "call" + ], + "properties": { + "call": { + "$ref": "#/definitions/Call" + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "js" + ], + "properties": { + "js": { + "$ref": "#/definitions/JS" + } + }, + "additionalProperties": false + }, + { + "type": "object", + "required": [ + "expr" + ], + "properties": { + "expr": { + "$ref": "#/definitions/Expr" + } + }, + "additionalProperties": false + } + ] + }, "RootSchema": { "type": "object", "properties": { @@ -1323,81 +1331,13 @@ }, "Type": { "description": "Represents a GraphQL type. A type can be an object, interface, enum or scalar.", - "type": "object", - "oneOf": [ - { - "type": "object", - "required": [ - "http" - ], - "properties": { - "http": { - "$ref": "#/definitions/Http" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "grpc" - ], - "properties": { - "grpc": { - "$ref": "#/definitions/Grpc" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "graphql" - ], - "properties": { - "graphql": { - "$ref": "#/definitions/GraphQL" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "call" - ], - "properties": { - "call": { - "$ref": "#/definitions/Call" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "js" - ], - "properties": { - "js": { - "$ref": "#/definitions/JS" - } - }, - "additionalProperties": false - }, - { - "type": "object", - "required": [ - "expr" - ], - "properties": { - "expr": { - "$ref": "#/definitions/Expr" - } - }, - "additionalProperties": false - } + "type": [ + "object", + "array" ], + "items": { + "$ref": "#/definitions/Resolver" + }, "required": [ "fields" ], diff --git a/src/core/blueprint/mustache.rs b/src/core/blueprint/mustache.rs index b09e60f52b..e567eb877e 100644 --- a/src/core/blueprint/mustache.rs +++ b/src/core/blueprint/mustache.rs @@ -1,12 +1,10 @@ use tailcall_valid::{Valid, Validator}; use super::FieldDefinition; +use crate::core::config::{self, Config}; +use crate::core::directive::DirectiveCodec; use crate::core::ir::model::{IO, IR}; use crate::core::scalar; -use crate::core::{ - config::{self, Config}, - directive::DirectiveCodec, -}; struct MustachePartsValidator<'a> { type_of: &'a config::Type, diff --git a/src/core/blueprint/operators/grpc.rs b/src/core/blueprint/operators/grpc.rs index 108c8cbf84..6fae5add4e 100644 --- a/src/core/blueprint/operators/grpc.rs +++ b/src/core/blueprint/operators/grpc.rs @@ -9,10 +9,10 @@ use crate::core::config::group_by::GroupBy; use crate::core::config::{Config, ConfigModule, Field, GraphQLOperationType, Grpc}; use crate::core::grpc::protobuf::{ProtobufOperation, ProtobufSet}; use crate::core::grpc::request_template::RequestTemplate; +use crate::core::helpers; use crate::core::ir::model::{IO, IR}; use crate::core::json::JsonSchema; use crate::core::mustache::Mustache; -use crate::core::helpers; fn to_url(grpc: &Grpc, method: &GrpcMethod) -> Valid { Valid::succeed(grpc.url.as_str()).and_then(|base_url| { diff --git a/src/core/config/config.rs b/src/core/config/config.rs index 17e0255d31..600cf56f1d 100644 --- a/src/core/config/config.rs +++ b/src/core/config/config.rs @@ -14,10 +14,10 @@ use tailcall_typedefs_common::ServiceDocumentBuilder; use tailcall_valid::{Valid, Validator}; use super::directive::Directive; -use super::{from_document::from_document, Resolver, Resolvers}; +use super::from_document::from_document; use super::{ AddField, Alias, Cache, Call, Discriminate, Expr, GraphQL, Grpc, Http, Link, Modify, Omit, - Protected, Server, Telemetry, Upstream, JS, + Protected, Resolver, Resolvers, Server, Telemetry, Upstream, JS, }; use crate::core::config::npo::QueryPath; use crate::core::config::source::Source; @@ -694,7 +694,8 @@ mod tests { use pretty_assertions::assert_eq; use super::*; - use crate::core::{config::Resolver, directive::DirectiveCodec}; + use crate::core::config::Resolver; + use crate::core::directive::DirectiveCodec; #[test] fn test_field_has_or_not_batch_resolver() { diff --git a/src/core/config/resolver.rs b/src/core/config/resolver.rs index 611f520511..8834b7cd26 100644 --- a/src/core/config/resolver.rs +++ b/src/core/config/resolver.rs @@ -60,10 +60,11 @@ impl Resolver { #[derive(Default, Clone, Debug, PartialEq, Eq, schemars::JsonSchema)] pub struct Resolvers(pub Vec); -// Implement custom serializer to provide backward compatibility for JSON/YAML formats -// when converting config to config file. In case the only one resolver is defined -// serialize it as flatten structure instead of `resolvers: []` -// TODO: this is not required in case Tailcall drop defining type schema in json/yaml files +// Implement custom serializer to provide backward compatibility for JSON/YAML +// formats when converting config to config file. In case the only one resolver +// is defined serialize it as flatten structure instead of `resolvers: []` +// TODO: this is not required in case Tailcall drop defining type schema in +// json/yaml files impl Serialize for Resolvers { fn serialize(&self, serializer: S) -> Result where @@ -72,17 +73,18 @@ impl Serialize for Resolvers { let resolvers = &self.0; if resolvers.len() == 1 { - resolvers.get(0).unwrap().serialize(serializer) + resolvers.first().unwrap().serialize(serializer) } else { resolvers.serialize(serializer) } } } -// Implement custom deserializer to provide backward compatibility for JSON/YAML formats -// when parsing config files. In case the `resolvers` field is defined in config -// parse it as vec of [Resolver] and otherwise try to parse it as single [Resolver] -// TODO: this is not required in case Tailcall drop defining type schema in json/yaml files +// Implement custom deserializer to provide backward compatibility for JSON/YAML +// formats when parsing config files. In case the `resolvers` field is defined +// in config parse it as vec of [Resolver] and otherwise try to parse it as +// single [Resolver] TODO: this is not required in case Tailcall drop defining +// type schema in json/yaml files impl<'de> Deserialize<'de> for Resolvers { fn deserialize(deserializer: D) -> Result where @@ -94,7 +96,7 @@ impl<'de> Deserialize<'de> for Resolvers { let mut value = Value::deserialize(deserializer)?; if let Value::Object(obj) = &mut value { - if obj.len() == 0 { + if obj.is_empty() { return Ok(Resolvers::default()); } diff --git a/src/core/config/transformer/subgraph.rs b/src/core/config/transformer/subgraph.rs index aff870e981..24eed1f86c 100644 --- a/src/core/config/transformer/subgraph.rs +++ b/src/core/config/transformer/subgraph.rs @@ -50,7 +50,7 @@ impl Transform for Subgraph { ); } - if let Some(resolver) = ty.resolvers.get(0) { + if let Some(resolver) = ty.resolvers.first() { resolver_by_type.insert(type_name.clone(), resolver.clone()); KeysExtractor::validate(&config_types, resolver, type_name).and_then(|_| { diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index a3acd1ad58..e1eb21f212 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -175,13 +175,10 @@ mod tests { mod merge { use serde_json::json; - use crate::core::{ - blueprint::{Blueprint, DynamicValue}, - http::RequestContext, - ir::EmptyResolverContext, - }; - use super::*; + use crate::core::blueprint::{Blueprint, DynamicValue}; + use crate::core::http::RequestContext; + use crate::core::ir::EmptyResolverContext; #[tokio::test] async fn test_const_values() { diff --git a/src/core/jit/transform/check_cache.rs b/src/core/jit/transform/check_cache.rs index 8f704733c3..8928ad97bd 100644 --- a/src/core/jit/transform/check_cache.rs +++ b/src/core/jit/transform/check_cache.rs @@ -27,17 +27,9 @@ fn check_cache(ir: &IR) -> Option { (Some(age1), Some(age2)) => Some(age1.min(age2)), _ => None, }, - IR::Merge(vec) => vec - .iter() - .map(|ir| check_cache(ir)) - .min() - .unwrap_or_default(), + IR::Merge(vec) => vec.iter().map(check_cache).min().unwrap_or_default(), IR::Discriminate(_, ir) => check_cache(ir), - IR::Entity(hash_map) => hash_map - .values() - .map(|ir| check_cache(ir)) - .min() - .unwrap_or_default(), + IR::Entity(hash_map) => hash_map.values().map(check_cache).min().unwrap_or_default(), IR::Dynamic(_) | IR::ContextPath(_) | IR::Map(_) | IR::Service(_) => None, } } diff --git a/tests/execution/test-multiple-resolvable-directives-on-field-validation.md b/tests/execution/test-multiple-resolvable-directives-on-field-validation.md index 951a6e2a96..18f27eaa63 100644 --- a/tests/execution/test-multiple-resolvable-directives-on-field-validation.md +++ b/tests/execution/test-multiple-resolvable-directives-on-field-validation.md @@ -21,8 +21,14 @@ type Address { } type Query { - user1: User @expr(body: {name: "{{.value.test}}"}) @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.id}}"}]) - user2: User @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "name", value: "{{.value.name}}"}]) @expr(body: {name: "{{.args.expr}}"}) - user3: User @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.address}}"}]) @graphQL(args: [{key: "id", value: "{{.args.id}}"}], url: "http://upstream/graphql", name: "user") + user1: User + @expr(body: {name: "{{.value.test}}"}) + @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.id}}"}]) + user2: User + @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "name", value: "{{.value.name}}"}]) + @expr(body: {name: "{{.args.expr}}"}) + user3: User + @http(url: "http://jsonplaceholder.typicode.com/", query: [{key: "id", value: "{{.value.address}}"}]) + @graphQL(args: [{key: "id", value: "{{.args.id}}"}], url: "http://upstream/graphql", name: "user") } ``` diff --git a/tests/execution/test-multiple-resolvable-directives-on-field.md b/tests/execution/test-multiple-resolvable-directives-on-field.md index 34fef6fc63..c666e81b70 100644 --- a/tests/execution/test-multiple-resolvable-directives-on-field.md +++ b/tests/execution/test-multiple-resolvable-directives-on-field.md @@ -18,8 +18,10 @@ type Address { type Query { user1: User @expr(body: {name: "name expr 1"}) @http(url: "http://jsonplaceholder.typicode.com/users/1") - user2: User @http(url: "http://jsonplaceholder.typicode.com/users/2") @expr(body: {name: "name expr 2" }) - user3: User @http(url: "http://jsonplaceholder.typicode.com/users/3") @graphQL(args: [{key: "id", value: "3"}], url: "http://upstream/graphql", name: "user") + user2: User @http(url: "http://jsonplaceholder.typicode.com/users/2") @expr(body: {name: "name expr 2"}) + user3: User + @http(url: "http://jsonplaceholder.typicode.com/users/3") + @graphQL(args: [{key: "id", value: "3"}], url: "http://upstream/graphql", name: "user") } ``` From d43fca0472dca772c11692df9bad1f8aa939e22b Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Sat, 23 Nov 2024 13:44:40 +0000 Subject: [PATCH 09/15] fix merge --- src/core/jit/transform/auth_planer.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/jit/transform/auth_planer.rs b/src/core/jit/transform/auth_planer.rs index 585409d22f..04239a167b 100644 --- a/src/core/jit/transform/auth_planer.rs +++ b/src/core/jit/transform/auth_planer.rs @@ -79,6 +79,7 @@ pub fn drop_protect_ir(ir: &mut IR) -> bool { } IR::Map(_) => false, IR::Pipe(ir1, ir2) => drop_protect_ir(ir1) || drop_protect_ir(ir2), + IR::Merge(_) => false, IR::Discriminate(_, ir) => drop_protect_ir(ir), IR::Entity(_) => false, IR::Service(_) => false, From b269c133be1d2a649fe29ad6ca1cf32fc98def7b Mon Sep 17 00:00:00 2001 From: Panagiotis Date: Mon, 25 Nov 2024 12:04:17 +0000 Subject: [PATCH 10/15] fix: move tests closer to implementation --- src/core/ir/eval.rs | 69 ----------------------------------------- src/core/merge_right.rs | 65 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 69 deletions(-) diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index 1f0950f2bf..4496c2e0bc 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -166,72 +166,3 @@ impl IR { }) } } - -#[cfg(test)] -mod tests { - use super::*; - - mod merge { - use serde_json::json; - - use super::*; - use crate::core::blueprint::{Blueprint, DynamicValue}; - use crate::core::http::RequestContext; - use crate::core::ir::EmptyResolverContext; - - #[tokio::test] - async fn test_const_values() { - let a = DynamicValue::Value( - ConstValue::from_json(json!({ - "a": 1, - "c": { - "ca": false - } - })) - .unwrap(), - ); - - let b = DynamicValue::Value( - ConstValue::from_json(json!({ - "b": 2, - "c": { - "cb": 23 - } - })) - .unwrap(), - ); - - let c = DynamicValue::Value( - ConstValue::from_json(json!({ - "c" : { - "ca": true, - "cc": [1, 2] - }, - "d": "additional" - })) - .unwrap(), - ); - - let ir = IR::Merge([a, b, c].into_iter().map(IR::Dynamic).collect()); - let runtime = crate::cli::runtime::init(&Blueprint::default()); - let req_ctx = RequestContext::new(runtime); - let res_ctx = EmptyResolverContext {}; - let mut eval_ctx = EvalContext::new(&req_ctx, &res_ctx); - - let actual = ir.eval(&mut eval_ctx).await.unwrap(); - let expected = ConstValue::from_json(json!({ - "a": 1, - "b": 2, - "c": { - "ca": true, - "cb": 23, - "cc": [1, 2] - }, - "d": "additional" - })) - .unwrap(); - - assert_eq!(actual, expected); - } - } -} diff --git a/src/core/merge_right.rs b/src/core/merge_right.rs index 71b40061ea..2286d39e79 100644 --- a/src/core/merge_right.rs +++ b/src/core/merge_right.rs @@ -477,4 +477,69 @@ mod tests { assert_eq!(a.merge_right(b), expected); } + + mod eval { + use async_graphql_value::ConstValue; + use serde_json::json; + + use crate::core::blueprint::{Blueprint, DynamicValue}; + use crate::core::http::RequestContext; + use crate::core::ir::model::IR; + use crate::core::ir::{EmptyResolverContext, EvalContext}; + + #[tokio::test] + async fn test_const_values() { + let a = DynamicValue::Value( + ConstValue::from_json(json!({ + "a": 1, + "c": { + "ca": false + } + })) + .unwrap(), + ); + + let b = DynamicValue::Value( + ConstValue::from_json(json!({ + "b": 2, + "c": { + "cb": 23 + } + })) + .unwrap(), + ); + + let c = DynamicValue::Value( + ConstValue::from_json(json!({ + "c" : { + "ca": true, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(), + ); + + let ir = IR::Merge([a, b, c].into_iter().map(IR::Dynamic).collect()); + let runtime = crate::cli::runtime::init(&Blueprint::default()); + let req_ctx = RequestContext::new(runtime); + let res_ctx = EmptyResolverContext {}; + let mut eval_ctx = EvalContext::new(&req_ctx, &res_ctx); + + let actual = ir.eval(&mut eval_ctx).await.unwrap(); + let expected = ConstValue::from_json(json!({ + "a": 1, + "b": 2, + "c": { + "ca": true, + "cb": 23, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(); + + assert_eq!(actual, expected); + } + } } From ed8380fd3af00050edd7769ee331d99de0ec09ed Mon Sep 17 00:00:00 2001 From: Panagiotis Karatakis Date: Mon, 25 Nov 2024 12:05:37 +0000 Subject: [PATCH 11/15] refactor: Add TODO for future reference Co-authored-by: Tushar Mathur --- src/core/ir/eval.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index 4496c2e0bc..ffe9baff21 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -106,6 +106,7 @@ impl IR { .into_iter() .collect::>()?; + // TODO: This is a very opinionated merge. We should allow users to customize how they would like to merge the values. In future we should support more merging capabilities by adding an additional parameter to `Merge`. Ok(results .into_iter() .reduce(|acc, result| acc.merge_right(result)) From 66bd31a861fd2f3f46d599d7708d87b34bee8583 Mon Sep 17 00:00:00 2001 From: Panagiotis Date: Mon, 25 Nov 2024 13:11:07 +0000 Subject: [PATCH 12/15] feat: allow merging of configurations using @link --- src/core/config/resolver.rs | 16 +++++++--------- .../snapshots/test-merge-nested.md_merged.snap | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/core/config/resolver.rs b/src/core/config/resolver.rs index 8834b7cd26..0fc2f611f4 100644 --- a/src/core/config/resolver.rs +++ b/src/core/config/resolver.rs @@ -127,16 +127,14 @@ impl Deref for Resolvers { } } -// Custom implementation for MergeRight in order -// to provide compatibility with old tests where -// the same resolver could be defined in multiple configs -// leading to duplicated resolver in such tests impl MergeRight for Resolvers { - fn merge_right(self, other: Self) -> Self { - if other.is_empty() { - self - } else { - other + fn merge_right(mut self, other: Self) -> Self { + for resolver in other.0.into_iter() { + if !self.0.contains(&resolver) { + self.0.push(resolver); + } } + + self } } diff --git a/tests/core/snapshots/test-merge-nested.md_merged.snap b/tests/core/snapshots/test-merge-nested.md_merged.snap index 52a0dab842..53cfa26fe5 100644 --- a/tests/core/snapshots/test-merge-nested.md_merged.snap +++ b/tests/core/snapshots/test-merge-nested.md_merged.snap @@ -18,5 +18,5 @@ type Foo { } type Query { - hi: Foo @expr(body: {a: "world"}) + hi: Foo @expr(body: "world") @expr(body: {a: "world"}) } From 2474935b58a99cc2c0933705e2f03878e62e9a6f Mon Sep 17 00:00:00 2001 From: Panagiotis Date: Mon, 25 Nov 2024 13:25:12 +0000 Subject: [PATCH 13/15] fix: snapshot --- ..._config__config_module__merge__tests__federation_router.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap index 4a3cb46200..61c4627402 100644 --- a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap +++ b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap @@ -34,7 +34,7 @@ type Comment { } type Query { - addComment(postId: Int!, comment: CommentInput!): Boolean @http(url: "http://jsonplaceholder.typicode.com/add-comment", method: "POST") + addComment(postId: Int!, comment: CommentInput!): Boolean @http(url: "http://jsonplaceholder.typicode.com/add-comment") @http(url: "http://jsonplaceholder.typicode.com/add-comment", method: "POST") posts: [UserPost] @http(url: "http://jsonplaceholder.typicode.com/posts") searchComments(type: CommentSearch): [Comment] @http(url: "http://jsonplaceholder.typicode.com/comment") user(id: Int!): User @http(url: "http://jsonplaceholder.typicode.com/users/{{.args.id}}") From 7a53b7969490ba6787d7c9a4e6fc4845377d2dbd Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:48:14 +0000 Subject: [PATCH 14/15] Revert "fix: move tests closer to implementation" This reverts commit b269c133be1d2a649fe29ad6ca1cf32fc98def7b. --- src/core/ir/eval.rs | 69 +++++++++++++++++++++++++++++++++++++++++ src/core/merge_right.rs | 65 -------------------------------------- 2 files changed, 69 insertions(+), 65 deletions(-) diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index ffe9baff21..44469edd54 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -167,3 +167,72 @@ impl IR { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + mod merge { + use serde_json::json; + + use super::*; + use crate::core::blueprint::{Blueprint, DynamicValue}; + use crate::core::http::RequestContext; + use crate::core::ir::EmptyResolverContext; + + #[tokio::test] + async fn test_const_values() { + let a = DynamicValue::Value( + ConstValue::from_json(json!({ + "a": 1, + "c": { + "ca": false + } + })) + .unwrap(), + ); + + let b = DynamicValue::Value( + ConstValue::from_json(json!({ + "b": 2, + "c": { + "cb": 23 + } + })) + .unwrap(), + ); + + let c = DynamicValue::Value( + ConstValue::from_json(json!({ + "c" : { + "ca": true, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(), + ); + + let ir = IR::Merge([a, b, c].into_iter().map(IR::Dynamic).collect()); + let runtime = crate::cli::runtime::init(&Blueprint::default()); + let req_ctx = RequestContext::new(runtime); + let res_ctx = EmptyResolverContext {}; + let mut eval_ctx = EvalContext::new(&req_ctx, &res_ctx); + + let actual = ir.eval(&mut eval_ctx).await.unwrap(); + let expected = ConstValue::from_json(json!({ + "a": 1, + "b": 2, + "c": { + "ca": true, + "cb": 23, + "cc": [1, 2] + }, + "d": "additional" + })) + .unwrap(); + + assert_eq!(actual, expected); + } + } +} diff --git a/src/core/merge_right.rs b/src/core/merge_right.rs index 2286d39e79..71b40061ea 100644 --- a/src/core/merge_right.rs +++ b/src/core/merge_right.rs @@ -477,69 +477,4 @@ mod tests { assert_eq!(a.merge_right(b), expected); } - - mod eval { - use async_graphql_value::ConstValue; - use serde_json::json; - - use crate::core::blueprint::{Blueprint, DynamicValue}; - use crate::core::http::RequestContext; - use crate::core::ir::model::IR; - use crate::core::ir::{EmptyResolverContext, EvalContext}; - - #[tokio::test] - async fn test_const_values() { - let a = DynamicValue::Value( - ConstValue::from_json(json!({ - "a": 1, - "c": { - "ca": false - } - })) - .unwrap(), - ); - - let b = DynamicValue::Value( - ConstValue::from_json(json!({ - "b": 2, - "c": { - "cb": 23 - } - })) - .unwrap(), - ); - - let c = DynamicValue::Value( - ConstValue::from_json(json!({ - "c" : { - "ca": true, - "cc": [1, 2] - }, - "d": "additional" - })) - .unwrap(), - ); - - let ir = IR::Merge([a, b, c].into_iter().map(IR::Dynamic).collect()); - let runtime = crate::cli::runtime::init(&Blueprint::default()); - let req_ctx = RequestContext::new(runtime); - let res_ctx = EmptyResolverContext {}; - let mut eval_ctx = EvalContext::new(&req_ctx, &res_ctx); - - let actual = ir.eval(&mut eval_ctx).await.unwrap(); - let expected = ConstValue::from_json(json!({ - "a": 1, - "b": 2, - "c": { - "ca": true, - "cb": 23, - "cc": [1, 2] - }, - "d": "additional" - })) - .unwrap(); - - assert_eq!(actual, expected); - } - } } From fc47cd7609880280749c58050e6d7c4ee7d5a4b0 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:02:27 +0000 Subject: [PATCH 15/15] fix review comments --- src/core/config/config.rs | 6 +++--- .../fixtures/subgraph-users.graphql | 2 +- ..._module__merge__tests__federation_router.snap | 3 ++- src/core/config/resolver.rs | 16 ++++++++-------- src/core/ir/eval.rs | 4 +++- tailcall-macros/src/resolver.rs | 4 ++-- 6 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/core/config/config.rs b/src/core/config/config.rs index 600cf56f1d..9c1559c2a4 100644 --- a/src/core/config/config.rs +++ b/src/core/config/config.rs @@ -17,7 +17,7 @@ use super::directive::Directive; use super::from_document::from_document; use super::{ AddField, Alias, Cache, Call, Discriminate, Expr, GraphQL, Grpc, Http, Link, Modify, Omit, - Protected, Resolver, Resolvers, Server, Telemetry, Upstream, JS, + Protected, Resolver, ResolverSet, Server, Telemetry, Upstream, JS, }; use crate::core::config::npo::QueryPath; use crate::core::config::source::Source; @@ -117,7 +117,7 @@ pub struct Type { /// /// Apollo federation entity resolver. #[serde(flatten, default, skip_serializing_if = "is_default")] - pub resolvers: Resolvers, + pub resolvers: ResolverSet, /// /// Any additional directives #[serde(default, skip_serializing_if = "is_default")] @@ -226,7 +226,7 @@ pub struct Field { /// /// Resolver for the field #[serde(flatten, default, skip_serializing_if = "is_default")] - pub resolvers: Resolvers, + pub resolvers: ResolverSet, /// /// Any additional directives diff --git a/src/core/config/config_module/fixtures/subgraph-users.graphql b/src/core/config/config_module/fixtures/subgraph-users.graphql index d07194ec9e..22f0e6b63a 100644 --- a/src/core/config/config_module/fixtures/subgraph-users.graphql +++ b/src/core/config/config_module/fixtures/subgraph-users.graphql @@ -6,7 +6,7 @@ type Query { users: [User] @http(url: "http://jsonplaceholder.typicode.com/users") user(id: Int!): User @http(url: "http://jsonplaceholder.typicode.com/users/{{.args.id}}") addComment(postId: Int!, comment: CommentInput!): Boolean - @http(url: "http://jsonplaceholder.typicode.com/add-comment") + @http(url: "http://jsonplaceholder.typicode.com/add-comment", method: POST) } enum Role { diff --git a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap index 61c4627402..e2dc41f5da 100644 --- a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap +++ b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap @@ -1,6 +1,7 @@ --- source: src/core/config/config_module/merge.rs expression: merged.to_sdl() +snapshot_kind: text --- schema @server(port: 8000) @upstream(batch: {delay: 100, headers: []}, httpCache: 42) { query: Query @@ -34,7 +35,7 @@ type Comment { } type Query { - addComment(postId: Int!, comment: CommentInput!): Boolean @http(url: "http://jsonplaceholder.typicode.com/add-comment") @http(url: "http://jsonplaceholder.typicode.com/add-comment", method: "POST") + addComment(postId: Int!, comment: CommentInput!): Boolean @http(url: "http://jsonplaceholder.typicode.com/add-comment", method: "POST") posts: [UserPost] @http(url: "http://jsonplaceholder.typicode.com/posts") searchComments(type: CommentSearch): [Comment] @http(url: "http://jsonplaceholder.typicode.com/comment") user(id: Int!): User @http(url: "http://jsonplaceholder.typicode.com/users/{{.args.id}}") diff --git a/src/core/config/resolver.rs b/src/core/config/resolver.rs index 0fc2f611f4..48dbc994cc 100644 --- a/src/core/config/resolver.rs +++ b/src/core/config/resolver.rs @@ -58,14 +58,14 @@ impl Resolver { } #[derive(Default, Clone, Debug, PartialEq, Eq, schemars::JsonSchema)] -pub struct Resolvers(pub Vec); +pub struct ResolverSet(pub Vec); // Implement custom serializer to provide backward compatibility for JSON/YAML // formats when converting config to config file. In case the only one resolver // is defined serialize it as flatten structure instead of `resolvers: []` // TODO: this is not required in case Tailcall drop defining type schema in // json/yaml files -impl Serialize for Resolvers { +impl Serialize for ResolverSet { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, @@ -85,7 +85,7 @@ impl Serialize for Resolvers { // in config parse it as vec of [Resolver] and otherwise try to parse it as // single [Resolver] TODO: this is not required in case Tailcall drop defining // type schema in json/yaml files -impl<'de> Deserialize<'de> for Resolvers { +impl<'de> Deserialize<'de> for ResolverSet { fn deserialize(deserializer: D) -> Result where D: serde::Deserializer<'de>, @@ -97,7 +97,7 @@ impl<'de> Deserialize<'de> for Resolvers { if let Value::Object(obj) = &mut value { if obj.is_empty() { - return Ok(Resolvers::default()); + return Ok(ResolverSet::default()); } if let Some(value) = obj.remove("resolvers") { @@ -109,17 +109,17 @@ impl<'de> Deserialize<'de> for Resolvers { let resolver: Resolver = serde_json::from_value(value).map_err(Error::custom)?; - Ok(Resolvers::from(resolver)) + Ok(ResolverSet::from(resolver)) } } -impl From for Resolvers { +impl From for ResolverSet { fn from(value: Resolver) -> Self { Self(vec![value]) } } -impl Deref for Resolvers { +impl Deref for ResolverSet { type Target = Vec; fn deref(&self) -> &Self::Target { @@ -127,7 +127,7 @@ impl Deref for Resolvers { } } -impl MergeRight for Resolvers { +impl MergeRight for ResolverSet { fn merge_right(mut self, other: Self) -> Self { for resolver in other.0.into_iter() { if !self.0.contains(&resolver) { diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index 44469edd54..e7b0a8c179 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -106,7 +106,9 @@ impl IR { .into_iter() .collect::>()?; - // TODO: This is a very opinionated merge. We should allow users to customize how they would like to merge the values. In future we should support more merging capabilities by adding an additional parameter to `Merge`. + // TODO: This is a very opinionated merge. We should allow users to customize + // how they would like to merge the values. In future we should support more + // merging capabilities by adding an additional parameter to `Merge`. Ok(results .into_iter() .reduce(|acc, result| acc.merge_right(result)) diff --git a/tailcall-macros/src/resolver.rs b/tailcall-macros/src/resolver.rs index 2efc733b91..6d08f780d0 100644 --- a/tailcall-macros/src/resolver.rs +++ b/tailcall-macros/src/resolver.rs @@ -94,14 +94,14 @@ pub fn expand_resolver_derive(input: DeriveInput) -> syn::Result { impl #name { pub fn from_directives( directives: &[Positioned], - ) -> Valid { + ) -> Valid { Valid::from_iter(directives.iter(), |directive| { #(#variant_parsers)* Valid::succeed(None) }) .map(|resolvers| { - crate::core::config::Resolvers(resolvers.into_iter().flatten().collect()) + crate::core::config::ResolverSet(resolvers.into_iter().flatten().collect()) }) }