From c70b006f48193d4e22fe9bc14f02c34643f8429e Mon Sep 17 00:00:00 2001 From: Maksim Karelov Date: Thu, 30 Jun 2022 15:31:52 +0300 Subject: [PATCH 1/9] Fix execution of `flatten` when `fetch` in `sequence` returns null --- apollo-router/src/query_planner/mod.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index ffe9ec7168..55b2ac27d4 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -167,6 +167,22 @@ impl PlanNode { let mut value; let mut errors; + if !current_dir.is_empty() { + // Check if parent data has field requested by Flatten + match parent_value.pointer(¤t_dir.to_string()) { + Some(matched_value) => { + // If it's not we don't need to run underlying fetch + if matched_value.is_null() { + return ( + Value::default(), + Vec::new(), + ) + } + } + _ => {} + } + } + match self { PlanNode::Sequence { nodes } => { value = parent_value.clone(); From c228ce96ea18e6c05310bda2f073c302c7952d98 Mon Sep 17 00:00:00 2001 From: Maksim Karelov Date: Fri, 1 Jul 2022 01:32:47 +0300 Subject: [PATCH 2/9] Move `current_dir` check from `execute_recursively` to `fetch_node` --- apollo-router/src/query_planner/mod.rs | 33 +++++++++++++------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 55b2ac27d4..79ccc35953 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -167,22 +167,6 @@ impl PlanNode { let mut value; let mut errors; - if !current_dir.is_empty() { - // Check if parent data has field requested by Flatten - match parent_value.pointer(¤t_dir.to_string()) { - Some(matched_value) => { - // If it's not we don't need to run underlying fetch - if matched_value.is_null() { - return ( - Value::default(), - Vec::new(), - ) - } - } - _ => {} - } - } - match self { PlanNode::Sequence { nodes } => { value = parent_value.clone(); @@ -517,7 +501,22 @@ pub(crate) mod fetch { ) .await { - Some(variables) => variables, + Some(variables) => { + if variables.variables.is_empty() && !current_dir.is_empty() { + match data.pointer(¤t_dir.to_string()) { + Some(matched_value) => { + if matched_value.is_null() { + return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); + } + }, + None => { + return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); + } + } + } + + variables + }, None => { return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); } From 4d7756051edb567edc81cd76ebf0c03bec74e549 Mon Sep 17 00:00:00 2001 From: Maksim Karelov Date: Mon, 4 Jul 2022 14:10:15 +0300 Subject: [PATCH 3/9] Move `current_dir` check from `fetch_node` to `Variables::new` --- apollo-router/src/query_planner/mod.rs | 30 ++++++++++++-------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 79ccc35953..49b2ab71e7 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -454,6 +454,19 @@ pub(crate) mod fetch { Some(Variables { variables, paths }) } else { + if !current_dir.is_empty() { + match data.pointer(current_dir.to_string().as_str()) { + Some(matched_value) => { + if matched_value.is_null() { + return None + } + } + None => { + return None + } + } + } + Some(Variables { variables: variable_usages .iter() @@ -501,22 +514,7 @@ pub(crate) mod fetch { ) .await { - Some(variables) => { - if variables.variables.is_empty() && !current_dir.is_empty() { - match data.pointer(¤t_dir.to_string()) { - Some(matched_value) => { - if matched_value.is_null() { - return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); - } - }, - None => { - return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); - } - } - } - - variables - }, + Some(variables) => variables, None => { return Ok((Value::from_path(current_dir, Value::Null), Vec::new())); } From 3e20450f07820ddf1900161eb6710ae75d4792e5 Mon Sep 17 00:00:00 2001 From: Maksim Karelov Date: Wed, 6 Jul 2022 12:20:53 +0300 Subject: [PATCH 4/9] Add schema and query_plan for nested mutations test --- .../testdata/query_plan_nested_mutations.json | 26 +++++++ .../testdata/schema_nested_mutations.graphql | 75 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json create mode 100644 apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql diff --git a/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json b/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json new file mode 100644 index 0000000000..9118dc48fe --- /dev/null +++ b/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json @@ -0,0 +1,26 @@ +{ + "kind": "Sequence", + "nodes": [ + { + "kind": "Fetch", + "serviceName": "outer", + "variableUsages": [], + "operation": "mutation{outerMutation{mutation{__typename}}}", + "operationKind": "mutation" + }, + { + "kind": "Flatten", + "path": [ + "outerMutation", + "mutation" + ], + "node": { + "kind": "Fetch", + "serviceName": "inner", + "variableUsages": [], + "operation": "mutation{...on Mutation{innerMutation}}", + "operationKind": "mutation" + } + } + ] +} \ No newline at end of file diff --git a/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql b/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql new file mode 100644 index 0000000000..a2faeb8521 --- /dev/null +++ b/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql @@ -0,0 +1,75 @@ +schema +@link(url: "https://specs.apollo.dev/link/v1.0") +@link(url: "https://specs.apollo.dev/join/v0.2", for: EXECUTION) +{ + query: Query + mutation: Mutation +} + +directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +scalar join__FieldSet + +enum join__Graph { + INNER @join__graph(name: "inner", url: "http://inner") + OUTER @join__graph(name: "outer", url: "http://outer") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Mutation +@join__type(graph: INNER) +@join__type(graph: OUTER) +{ + version: Version + innerMutation: Boolean @join__field(graph: INNER) + outerMutation: OuterMutationResult @join__field(graph: OUTER) +} + +type OuterMutationError +@join__type(graph: OUTER) +{ + message: String! +} + +type OuterMutationResult +@join__type(graph: OUTER) +{ + error: OuterMutationError + mutation: Mutation +} + +type Query +@join__type(graph: INNER) +@join__type(graph: OUTER) +{ + version: Version +} + +type Version +@join__type(graph: INNER) +@join__type(graph: OUTER) +{ + inner: String! @join__field(graph: INNER) + outer: String! @join__field(graph: OUTER) +} \ No newline at end of file From d8b242fab73096fcf5d0e412a816a1f38468f2f6 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 25 Jul 2022 15:50:04 +0200 Subject: [PATCH 5/9] add a test --- apollo-router/src/query_planner/mod.rs | 123 ++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 4 deletions(-) diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index b7a01ba9cc..2dfe1c1ba0 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -425,12 +425,10 @@ pub(crate) mod fetch { match data.pointer(current_dir.to_string().as_str()) { Some(matched_value) => { if matched_value.is_null() { - return None + return None; } } - None => { - return None - } + None => return None, } } @@ -883,4 +881,121 @@ mod tests { "subgraph requests must be http post" ); } + + #[tokio::test] + async fn dependent_mutations() { + let schema = r#"schema + @core(feature: "https://specs.apollo.dev/core/v0.1"), + @core(feature: "https://specs.apollo.dev/join/v0.1") + { + query: Query + mutation: Mutation + } + + directive @core(feature: String!) repeatable on SCHEMA + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION + directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE + directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE + directive @join__graph(name: String!, url: String!) on ENUM_VALUE + scalar join__FieldSet + + enum join__Graph { + A @join__graph(name: "A" url: "http://localhost:4001") + B @join__graph(name: "B" url: "http://localhost:4004") + } + + type Mutation { + mutationA: Mutation @join__field(graph: A) + mutationB: Boolean @join__field(graph: B) + } + + type Query { + query: Boolean @join__field(graph: A) + }"#; + + let query_plan: QueryPlan = QueryPlan { + root: serde_json::from_str( + r#"{ + "kind": "Sequence", + "nodes": [ + { + "kind": "Fetch", + "serviceName": "A", + "variableUsages": [], + "operation": "mutation{mutationA{__typename}}", + "operationKind": "mutation" + }, + { + "kind": "Flatten", + "path": [ + "mutationA" + ], + "node": { + "kind": "Fetch", + "serviceName": "B", + "variableUsages": [], + "operation": "mutation{...on Mutation{mutationB}}", + "operationKind": "mutation" + } + } + ] + }"#, + ) + .unwrap(), + usage_reporting: UsageReporting { + stats_report_key: "this is a test report key".to_string(), + referenced_fields_by_type: Default::default(), + }, + options: QueryPlanOptions::default(), + }; + + let succeeded: Arc = Default::default(); + + let mut mock_a_service = plugin::test::MockSubgraphService::new(); + mock_a_service + .expect_call() + .times(1) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + + let mut mock_b_service = plugin::test::MockSubgraphService::new(); + mock_b_service + .expect_call() + .times(1) + .returning(|_| Ok(SubgraphResponse::fake_builder().build())); //.never(); + + let sf = Arc::new(MockSubgraphFactory { + subgraphs: HashMap::from([ + ( + "A".into(), + ServiceBuilder::new() + .buffer(1) + .service(mock_a_service.build().boxed()), + ), + ( + "B".into(), + ServiceBuilder::new() + .buffer(1) + .service(mock_b_service.build().boxed()), + ), + ]), + plugins: Default::default(), + }); + + let (sender, _) = futures::channel::mpsc::channel(10); + let _response = query_plan + .execute( + &Context::new(), + &sf, + &Arc::new( + http_ext::Request::fake_builder() + .headers(Default::default()) + .body(Default::default()) + .build() + .expect("fake builds should always work; qed"), + ), + &Schema::from_str(schema).unwrap(), + sender, + ) + .await; + } } From ccb57b75f8318c54dc2b4395d62ee88d111cb1ab Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 25 Jul 2022 16:32:46 +0200 Subject: [PATCH 6/9] add a get method to JsonExt so we won't need to convert the path to string then reparse it --- apollo-router/src/json_ext.rs | 20 ++++++++++++++-- apollo-router/src/query_planner/mod.rs | 32 +++++++++++++++----------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/apollo-router/src/json_ext.rs b/apollo-router/src/json_ext.rs index 6bbce9d0d5..c381ce81e2 100644 --- a/apollo-router/src/json_ext.rs +++ b/apollo-router/src/json_ext.rs @@ -64,10 +64,14 @@ pub trait ValueExt { #[track_caller] fn from_path(path: &Path, value: Value) -> Value; - /// Insert a `value` at a `Path` + /// Insert a `Value` at a `Path` #[track_caller] fn insert(&mut self, path: &Path, value: Value) -> Result<(), FetchError>; + /// Get a `Value` from a `Path` + #[track_caller] + fn get_path<'a>(&'a self, path: &'a Path) -> Result<&'a Value, FetchError>; + /// Select all values matching a `Path`. /// /// the function passed as argument will be called with the values found and their Path @@ -241,7 +245,7 @@ impl ValueExt for Value { res_value } - /// Insert a `value` at a `Path` + /// Insert a `Value` at a `Path` #[track_caller] fn insert(&mut self, path: &Path, value: Value) -> Result<(), FetchError> { let mut current_node = self; @@ -318,6 +322,18 @@ impl ValueExt for Value { Ok(()) } + /// Get a `Value` from a `Path` + #[track_caller] + fn get_path<'a>(&'a self, path: &'a Path) -> Result<&'a Value, FetchError> { + let mut res = Err(FetchError::ExecutionPathNotFound { + reason: "value not found".to_string(), + }); + iterate_path(&mut Path::default(), &path.0, self, &mut |_path, value| { + res = Ok(value); + }); + res + } + #[track_caller] fn select_values_and_paths<'a, F>(&'a self, path: &'a Path, mut f: F) where diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 2dfe1c1ba0..79e50cd4b3 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -421,14 +421,18 @@ pub(crate) mod fetch { Some(Variables { variables, paths }) } else { + // with nested operations (Query or Mutation has an operation returning a Query or Mutation), + // when the first fetch fails, the query plan wwill still execute up until the second fetch, + // where `requires` is empty (not a federated fetch), the current dir is not emmpty (child of + // the previous operation field) and the data is null. In that case, we recognize that we + // should not perform the next fetch if !current_dir.is_empty() { - match data.pointer(current_dir.to_string().as_str()) { - Some(matched_value) => { - if matched_value.is_null() { - return None; - } - } - None => return None, + if data + .get_path(¤t_dir) + .map(|value| value.is_null()) + .unwrap_or(true) + { + return None; } } @@ -914,6 +918,12 @@ mod tests { }"#; let query_plan: QueryPlan = QueryPlan { + // generated from: + // mutation { + // mutationA { + // mutationB + // } + // } root: serde_json::from_str( r#"{ "kind": "Sequence", @@ -949,19 +959,15 @@ mod tests { options: QueryPlanOptions::default(), }; - let succeeded: Arc = Default::default(); - let mut mock_a_service = plugin::test::MockSubgraphService::new(); mock_a_service .expect_call() .times(1) .returning(|_| Ok(SubgraphResponse::fake_builder().build())); + // the first fetch returned null, so there should never be a call to B let mut mock_b_service = plugin::test::MockSubgraphService::new(); - mock_b_service - .expect_call() - .times(1) - .returning(|_| Ok(SubgraphResponse::fake_builder().build())); //.never(); + mock_b_service.expect_call().never(); let sf = Arc::new(MockSubgraphFactory { subgraphs: HashMap::from([ From 50d581eb67c7846d11e26e1e6f199ede4b33e130 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 25 Jul 2022 16:35:37 +0200 Subject: [PATCH 7/9] remove test files they are small enough to get inlined --- .../testdata/query_plan_nested_mutations.json | 26 ------- .../testdata/schema_nested_mutations.graphql | 75 ------------------- 2 files changed, 101 deletions(-) delete mode 100644 apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json delete mode 100644 apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql diff --git a/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json b/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json deleted file mode 100644 index 9118dc48fe..0000000000 --- a/apollo-router/src/query_planner/testdata/query_plan_nested_mutations.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "kind": "Sequence", - "nodes": [ - { - "kind": "Fetch", - "serviceName": "outer", - "variableUsages": [], - "operation": "mutation{outerMutation{mutation{__typename}}}", - "operationKind": "mutation" - }, - { - "kind": "Flatten", - "path": [ - "outerMutation", - "mutation" - ], - "node": { - "kind": "Fetch", - "serviceName": "inner", - "variableUsages": [], - "operation": "mutation{...on Mutation{innerMutation}}", - "operationKind": "mutation" - } - } - ] -} \ No newline at end of file diff --git a/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql b/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql deleted file mode 100644 index a2faeb8521..0000000000 --- a/apollo-router/src/query_planner/testdata/schema_nested_mutations.graphql +++ /dev/null @@ -1,75 +0,0 @@ -schema -@link(url: "https://specs.apollo.dev/link/v1.0") -@link(url: "https://specs.apollo.dev/join/v0.2", for: EXECUTION) -{ - query: Query - mutation: Mutation -} - -directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION - -directive @join__graph(name: String!, url: String!) on ENUM_VALUE - -directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE - -directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR - -directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA - -scalar join__FieldSet - -enum join__Graph { - INNER @join__graph(name: "inner", url: "http://inner") - OUTER @join__graph(name: "outer", url: "http://outer") -} - -scalar link__Import - -enum link__Purpose { - """ - `SECURITY` features provide metadata necessary to securely resolve fields. - """ - SECURITY - - """ - `EXECUTION` features provide metadata necessary for operation execution. - """ - EXECUTION -} - -type Mutation -@join__type(graph: INNER) -@join__type(graph: OUTER) -{ - version: Version - innerMutation: Boolean @join__field(graph: INNER) - outerMutation: OuterMutationResult @join__field(graph: OUTER) -} - -type OuterMutationError -@join__type(graph: OUTER) -{ - message: String! -} - -type OuterMutationResult -@join__type(graph: OUTER) -{ - error: OuterMutationError - mutation: Mutation -} - -type Query -@join__type(graph: INNER) -@join__type(graph: OUTER) -{ - version: Version -} - -type Version -@join__type(graph: INNER) -@join__type(graph: OUTER) -{ - inner: String! @join__field(graph: INNER) - outer: String! @join__field(graph: OUTER) -} \ No newline at end of file From c6e98546ad850764e0e8bfe4196edee732ad063c Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 25 Jul 2022 16:38:10 +0200 Subject: [PATCH 8/9] changelog --- NEXT_CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 55fb978fac..34190286f2 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -64,6 +64,21 @@ The benchmarks show that this PR gives a 23% gain in requests per second compare By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1423 +### do not perform nested fetches if the parent one returned null ([PR #1332](https://github.com/apollographql/router/pull/1332) + +In a query of the form: +```graphql +mutation { + mutationA { + mutationB + } +} +``` + +If `mutationA` returned null, we should not execute `mutationB`. + +By [@Ty3uK](https://github.com/Ty3uK) in https://github.com/apollographql/router/pull/1332 + ## 🛠 Maintenance ## 📚 Documentation From 5140250a682daba08afcb6e7ae258c03a68205f7 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Mon, 25 Jul 2022 16:53:22 +0200 Subject: [PATCH 9/9] lint --- apollo-router/src/query_planner/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/apollo-router/src/query_planner/mod.rs b/apollo-router/src/query_planner/mod.rs index 79e50cd4b3..2bee7b3457 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -426,14 +426,13 @@ pub(crate) mod fetch { // where `requires` is empty (not a federated fetch), the current dir is not emmpty (child of // the previous operation field) and the data is null. In that case, we recognize that we // should not perform the next fetch - if !current_dir.is_empty() { - if data - .get_path(¤t_dir) + if !current_dir.is_empty() + && data + .get_path(current_dir) .map(|value| value.is_null()) .unwrap_or(true) - { - return None; - } + { + return None; } Some(Variables {