From ceefed92c1cea76b139d6088c1ef7fd60f3bbd59 Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Thu, 28 Nov 2024 11:59:34 +0100 Subject: [PATCH] fix(2892): consider entity resolvers in n + 1 check (#2978) --- Cargo.lock | 8 +- Cargo.toml | 2 +- src/core/config/config.rs | 18 +- .../npo/fixtures/entity-resolver.graphql | 47 ++++++ .../fixtures/multiple-deeply-nested.graphql | 23 +++ .../npo/fixtures/multiple-type-usage.graphql | 29 ++++ src/core/config/npo/fixtures/nested.graphql | 21 +++ ...tracker__tests__cycles_with_resolvers.snap | 1 - ..._npo__tracker__tests__entity_resolver.snap | 15 ++ ...racker__tests__multiple_deeply_nested.snap | 10 ++ ...g__npo__tracker__tests__multiple_keys.snap | 1 + ...__tracker__tests__multiple_type_usage.snap | 8 + ...g__npo__tracker__tests__nested_config.snap | 7 + src/core/config/npo/tracker.rs | 158 +++++++++++++----- src/core/config/resolver.rs | 14 ++ 15 files changed, 304 insertions(+), 58 deletions(-) create mode 100644 src/core/config/npo/fixtures/entity-resolver.graphql create mode 100644 src/core/config/npo/fixtures/multiple-deeply-nested.graphql create mode 100644 src/core/config/npo/fixtures/multiple-type-usage.graphql create mode 100644 src/core/config/npo/fixtures/nested.graphql create mode 100644 src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__entity_resolver.snap create mode 100644 src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_deeply_nested.snap create mode 100644 src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_type_usage.snap create mode 100644 src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__nested_config.snap diff --git a/Cargo.lock b/Cargo.lock index cee593aec7..21d0ed4fab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -643,7 +643,7 @@ dependencies = [ "bitflags 2.6.0", "cexpr", "clang-sys", - "itertools 0.10.5", + "itertools 0.12.1", "lazy_static", "lazycell", "log", @@ -2849,7 +2849,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4979f22fdb869068da03c9f7528f8297c6fd2606bc3a4affe42e6a823fdb8da4" dependencies = [ "cfg-if", - "windows-targets 0.48.5", + "windows-targets 0.52.6", ] [[package]] @@ -5570,9 +5570,9 @@ dependencies = [ [[package]] name = "tailcall-chunk" -version = "0.2.5" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d684c265789778d9b76a268b079bca63dd8801d695e3c1d7f41df78e7a7adb4f" +checksum = "5d244d8876e9677c9699d5254b72366c9249760d73a8b7295d1fb3eb6333f682" [[package]] name = "tailcall-cloudflare" diff --git a/Cargo.toml b/Cargo.toml index 65906bbfc5..f03aef860f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -175,7 +175,7 @@ strum = "0.26.2" tailcall-valid = { workspace = true } dashmap = "6.1.0" urlencoding = "2.1.3" -tailcall-chunk = "0.2.5" +tailcall-chunk = "0.3.0" # to build rquickjs bindings on systems without builtin bindings [target.'cfg(all(target_os = "windows", target_arch = "x86"))'.dependencies] diff --git a/src/core/config/config.rs b/src/core/config/config.rs index 9c1559c2a4..95f11893f7 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, ResolverSet, Server, Telemetry, Upstream, JS, + Protected, ResolverSet, Server, Telemetry, Upstream, JS, }; use crate::core::config::npo::QueryPath; use crate::core::config::source::Source; @@ -136,6 +136,14 @@ impl Display for Type { } impl Type { + pub fn has_resolver(&self) -> bool { + self.resolvers.has_resolver() + } + + pub fn has_batched_resolver(&self) -> bool { + self.resolvers.is_batched() + } + pub fn fields(mut self, fields: Vec<(&str, Field)>) -> Self { let mut graphql_fields = BTreeMap::new(); for (name, field) in fields { @@ -243,15 +251,11 @@ impl MergeRight for Field { impl Field { pub fn has_resolver(&self) -> bool { - !self.resolvers.is_empty() + self.resolvers.has_resolver() } pub fn has_batched_resolver(&self) -> bool { - if self.resolvers.is_empty() { - false - } else { - self.resolvers.iter().all(Resolver::is_batched) - } + self.resolvers.is_batched() } pub fn int() -> Self { diff --git a/src/core/config/npo/fixtures/entity-resolver.graphql b/src/core/config/npo/fixtures/entity-resolver.graphql new file mode 100644 index 0000000000..4cd04e44a7 --- /dev/null +++ b/src/core/config/npo/fixtures/entity-resolver.graphql @@ -0,0 +1,47 @@ +schema @server @upstream { + query: Query +} + +type T1 @http(url: "") { + t1: Int +} + +type T2 @http(url: "") { + t2: [N] @http(url: "") +} + +type T3 @http(url: "", batchKey: ["id"]) { + t3: [N] @http(url: "") +} + +type T4 @http(url: "", batchKey: ["id"]) { + t4: [N] @http(url: "", batchKey: ["id"]) +} + +type T5 @http(url: "", batchKey: ["id"]) { + t5: [String] @http(url: "", batchKey: ["id"]) +} + +type T6 @http(url: "", batchKey: ["id"]) { + t6: [Int] +} + +type N { + n: [Int] @http(url: "") +} + +type Query { + x: String + t1: T1 @http(url: "") + t2: T2 @http(url: "") + t3: T3 @http(url: "") + t4: T4 @http(url: "") + t5: T5 @http(url: "") + t6: T6 @http(url: "") + t1_ls: [T1] @http(url: "") + t2_ls: [T2] @http(url: "") + t3_ls: [T3] @http(url: "") + t4_ls: [T4] @http(url: "") + t5_ls: [T5] @http(url: "") + t6_ls: [T6] @http(url: "") +} diff --git a/src/core/config/npo/fixtures/multiple-deeply-nested.graphql b/src/core/config/npo/fixtures/multiple-deeply-nested.graphql new file mode 100644 index 0000000000..fe46113f8a --- /dev/null +++ b/src/core/config/npo/fixtures/multiple-deeply-nested.graphql @@ -0,0 +1,23 @@ +schema @server @upstream { + query: Query +} + +type Root { + nested: Nested1 + nested_list: [Nested1] +} + +type Nested1 { + a: DeepNested @http(url: "") + b: DeepNested @http(url: "", batchKey: ["id"]) + c: DeepNested @http(url: "") + d: [DeepNested] @http(url: "") +} + +type DeepNested { + test: [String] @http(url: "") +} + +type Query { + root: Root @http(url: "") +} diff --git a/src/core/config/npo/fixtures/multiple-type-usage.graphql b/src/core/config/npo/fixtures/multiple-type-usage.graphql new file mode 100644 index 0000000000..fe19d28962 --- /dev/null +++ b/src/core/config/npo/fixtures/multiple-type-usage.graphql @@ -0,0 +1,29 @@ +schema @server @upstream { + query: Query +} + +type T1 { + t1: Int +} + +type T2 { + t2: [N] @http(url: "") +} + +type T3 { + t3: [N] @http(url: "", batchKey: ["id"]) +} + +type N { + n: Int @http(url: "") +} + +type Query { + x: String + t1: T1 @http(url: "") + t2: T2 @http(url: "") + t3: T3 @http(url: "") + t1_ls: [T1] @http(url: "") + t2_ls: [T2] @http(url: "") + t3_ls: [T3] @http(url: "") +} diff --git a/src/core/config/npo/fixtures/nested.graphql b/src/core/config/npo/fixtures/nested.graphql new file mode 100644 index 0000000000..20b1f5fb20 --- /dev/null +++ b/src/core/config/npo/fixtures/nested.graphql @@ -0,0 +1,21 @@ +schema @server(port: 8030) { + query: Query +} + +type Query { + version: Int! @expr(body: 1) + foo: [Foo!]! @expr(body: [{fizz: "buzz"}]) + bar: [Foo!]! @expr(body: [{fizz: "buzz"}]) + buzz: [Foo!]! @expr(body: [{fizz: "buzz"}]) +} + +type Foo { + fizz: String! + c: Int +} + +type Foo { + fizz: String! + c: Int + spam: [String!]! @http(url: "https://example.com/spam", query: [{key: "id", value: "{{.value.fizz}}"}]) +} diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__cycles_with_resolvers.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__cycles_with_resolvers.snap index 3ec61ef370..9829be427a 100644 --- a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__cycles_with_resolvers.snap +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__cycles_with_resolvers.snap @@ -2,5 +2,4 @@ source: src/core/config/npo/tracker.rs expression: actual --- -query { f1 { f1 { f2 } } } query { f1 { f2 } } diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__entity_resolver.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__entity_resolver.snap new file mode 100644 index 0000000000..22e7d62088 --- /dev/null +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__entity_resolver.snap @@ -0,0 +1,15 @@ +--- +source: src/core/config/npo/tracker.rs +expression: actual +snapshot_kind: text +--- +query { t2 { t2 { n } } } +query { t2_ls { t2 } } +query { t3 { t3 { n } } } +query { t3_ls { t3 } } +query { t4 { t4 { n } } } +query { t4_ls { t4 { n } } } +query { __entities(representations: [{ __typename: "T1"}]) } +query { __entities(representations: [{ __typename: "T2"}]) } +query { __entities(representations: [{ __typename: "T3"}]) { t3 } } +query { __entities(representations: [{ __typename: "T4"}]) { t4 { n } } } diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_deeply_nested.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_deeply_nested.snap new file mode 100644 index 0000000000..2415c7d408 --- /dev/null +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_deeply_nested.snap @@ -0,0 +1,10 @@ +--- +source: src/core/config/npo/tracker.rs +expression: actual +snapshot_kind: text +--- +query { root { nested { d { test } } } } +query { root { nested_list { a } } } +query { root { nested_list { b { test } } } } +query { root { nested_list { c } } } +query { root { nested_list { d } } } diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_keys.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_keys.snap index 46866fe99c..a373ad037c 100644 --- a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_keys.snap +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_keys.snap @@ -1,6 +1,7 @@ --- source: src/core/config/npo/tracker.rs expression: actual +snapshot_kind: text --- query { f1 { f2 } } query { f1 { f3 } } diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_type_usage.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_type_usage.snap new file mode 100644 index 0000000000..8cbff01fc8 --- /dev/null +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__multiple_type_usage.snap @@ -0,0 +1,8 @@ +--- +source: src/core/config/npo/tracker.rs +expression: actual +--- +query { t2 { t2 { n } } } +query { t2_ls { t2 } } +query { t3 { t3 { n } } } +query { t3_ls { t3 { n } } } diff --git a/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__nested_config.snap b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__nested_config.snap new file mode 100644 index 0000000000..0722c0f3ee --- /dev/null +++ b/src/core/config/npo/snapshots/tailcall__core__config__npo__tracker__tests__nested_config.snap @@ -0,0 +1,7 @@ +--- +source: src/core/config/npo/tracker.rs +expression: actual +--- +query { bar { spam } } +query { buzz { spam } } +query { foo { spam } } diff --git a/src/core/config/npo/tracker.rs b/src/core/config/npo/tracker.rs index 01a672f52d..53d55378a7 100644 --- a/src/core/config/npo/tracker.rs +++ b/src/core/config/npo/tracker.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt::{Display, Formatter}; use tailcall_chunk::Chunk; @@ -8,16 +8,16 @@ use crate::core::config::Config; /// /// Represents a list of query paths that can issue a N + 1 query #[derive(Default, Debug, PartialEq)] -pub struct QueryPath<'a>(Vec>); +pub struct QueryPath(Vec>); -impl QueryPath<'_> { +impl QueryPath { pub fn size(&self) -> usize { self.0.len() } } -impl<'a> From>>> for QueryPath<'a> { - fn from(chunk: Chunk>>) -> Self { +impl<'a> From>>> for QueryPath { + fn from(chunk: Chunk>>) -> Self { QueryPath( chunk .as_vec() @@ -26,7 +26,7 @@ impl<'a> From>>> for QueryPath<'a> { chunk .as_vec() .iter() - .map(|field_name| field_name.as_str()) + .map(|chunk_name| chunk_name.to_string()) .collect() }) .collect(), @@ -34,7 +34,7 @@ impl<'a> From>>> for QueryPath<'a> { } } -impl Display for QueryPath<'_> { +impl Display for QueryPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let query_data: Vec = self .0 @@ -58,11 +58,11 @@ impl Display for QueryPath<'_> { }) .collect(); - let val = query_data.iter().rfold("".to_string(), |s, query| { + let val = query_data.iter().fold("".to_string(), |s, query| { if s.is_empty() { query.to_string() } else { - format!("{}\n{}", query, s) + format!("{}\n{}", s, query) } }); @@ -92,9 +92,6 @@ impl<'a> FieldName<'a> { fn new(name: &'a str) -> Self { Self(name) } - fn as_str(self) -> &'a str { - self.0 - } } impl Display for FieldName<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -102,11 +99,32 @@ impl Display for FieldName<'_> { } } +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +enum Name<'a> { + Field(FieldName<'a>), + Entity(TypeName<'a>), +} + +impl Display for Name<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Name::Field(field_name) => write!(f, "{}", field_name), + Name::Entity(type_name) => write!( + f, + "__entities(representations: [{{ __typename: \"{}\"}}])", + type_name + ), + } + } +} + /// A module that tracks the query paths that can issue a N + 1 calls to /// upstream. pub struct PathTracker<'a> { config: &'a Config, - cache: HashMap<(TypeName<'a>, bool), Chunk>>>, + // Caches resolved chunks for the specific type + // with is_list info since the result is different depending on this flag + cache: HashMap<(TypeName<'a>, bool), Chunk>>>, } impl<'a> PathTracker<'a> { @@ -114,58 +132,81 @@ impl<'a> PathTracker<'a> { PathTracker { config, cache: Default::default() } } - #[allow(clippy::too_many_arguments)] fn iter( &mut self, - path: Chunk>, + parent_name: Option>, type_name: TypeName<'a>, is_list: bool, - visited: HashSet<(TypeName<'a>, FieldName<'a>)>, - ) -> Chunk>> { - if let Some(chunks) = self.cache.get(&(type_name, is_list)) { - return chunks.clone(); - } + ) -> Chunk>> { + let chunks = if let Some(chunks) = self.cache.get(&(type_name, is_list)) { + chunks.clone() + } else { + // set empty value in the cache to prevent infinity recursion + self.cache.insert((type_name, is_list), Chunk::default()); + + let mut chunks = Chunk::default(); + if let Some(type_of) = self.config.find_type(type_name.as_str()) { + for (name, field) in type_of.fields.iter() { + let field_name = Name::Field(FieldName::new(name)); - let mut chunks = Chunk::default(); - if let Some(type_of) = self.config.find_type(type_name.as_str()) { - for (name, field) in type_of.fields.iter() { - let field_name = FieldName::new(name); - let path = path.clone().append(field_name); - if !visited.contains(&(type_name, field_name)) { if is_list && field.has_resolver() && !field.has_batched_resolver() { - chunks = chunks.append(path.clone()); + chunks = chunks.append(Chunk::new(field_name)); } else { - let mut visited = visited.clone(); - visited.insert((type_name, field_name)); let is_list = is_list | field.type_of.is_list(); chunks = chunks.concat(self.iter( - path, + Some(field_name), TypeName::new(field.type_of.name()), is_list, - visited, )) } } } - } - self.cache.insert((type_name, is_list), chunks.clone()); - chunks + self.cache.insert((type_name, is_list), chunks.clone()); + + chunks + }; + + // chunks contains only paths from the current type. + // Prepend every subpath with parent path + if let Some(path) = parent_name { + let vec = chunks.as_vec(); + + Chunk::from_iter(vec.into_iter().map(|chunk| chunk.prepend(path))) + } else { + chunks + } } - fn find_chunks(&mut self) -> Chunk>> { - match &self.config.schema.query { + fn find_chunks(&mut self) -> Chunk>> { + let mut chunks = match &self.config.schema.query { None => Chunk::default(), - Some(query) => self.iter( - Chunk::default(), - TypeName::new(query.as_str()), - false, - HashSet::new(), - ), + Some(query) => self.iter(None, TypeName::new(query.as_str()), false), + }; + + for (type_name, type_of) in &self.config.types { + if type_of.has_resolver() { + let parent_path = Name::Entity(TypeName(type_name.as_str())); + // entity resolver are used to fetch multiple instances at once + // and therefore the resolver itself should be batched to avoid n + 1 + if type_of.has_batched_resolver() { + // if batched resolver is present traverse inner fields + chunks = chunks.concat(self.iter( + Some(parent_path), + TypeName::new(type_name.as_str()), + // entities are basically returning list of data + true, + )); + } else { + chunks = chunks.append(Chunk::new(parent_path)); + } + } } + + chunks } - pub fn find(mut self) -> QueryPath<'a> { + pub fn find(mut self) -> QueryPath { QueryPath::from(self.find_chunks()) } } @@ -241,8 +282,35 @@ mod tests { #[test] fn test_multiple_keys() { let config = include_config!("fixtures/multiple-keys.graphql").unwrap(); - let actual = config.n_plus_one(); - insta::assert_snapshot!(actual); + assert_n_plus_one!(config); + } + + #[test] + fn test_multiple_type_usage() { + let config = include_config!("fixtures/multiple-type-usage.graphql").unwrap(); + + assert_n_plus_one!(config); + } + + #[test] + fn test_entity_resolver() { + let config = include_config!("fixtures/entity-resolver.graphql").unwrap(); + + assert_n_plus_one!(config); + } + + #[test] + fn test_nested_config() { + let config = include_config!("fixtures/nested.graphql").unwrap(); + + assert_n_plus_one!(config); + } + + #[test] + fn test_multiple_deeply_nested() { + let config = include_config!("fixtures/multiple-deeply-nested.graphql").unwrap(); + + assert_n_plus_one!(config); } } diff --git a/src/core/config/resolver.rs b/src/core/config/resolver.rs index 48dbc994cc..686419a4d6 100644 --- a/src/core/config/resolver.rs +++ b/src/core/config/resolver.rs @@ -60,6 +60,20 @@ impl Resolver { #[derive(Default, Clone, Debug, PartialEq, Eq, schemars::JsonSchema)] pub struct ResolverSet(pub Vec); +impl ResolverSet { + pub fn has_resolver(&self) -> bool { + !self.0.is_empty() + } + + pub fn is_batched(&self) -> bool { + if self.0.is_empty() { + false + } else { + self.0.iter().all(Resolver::is_batched) + } + } +} + // 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: []`