diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index b26a23f1e1..de01f76fd5 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -117,6 +117,21 @@ The benchmarks show that this change brings a 23% gain in requests per second co 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 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 90b150c391..8b9ae8f35a 100644 --- a/apollo-router/src/query_planner/mod.rs +++ b/apollo-router/src/query_planner/mod.rs @@ -889,6 +889,20 @@ 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() + && data + .get_path(current_dir) + .map(|value| value.is_null()) + .unwrap_or(true) + { + return None; + } + Some(Variables { variables: variable_usages .iter() @@ -1560,4 +1574,123 @@ mod tests { r#"{"data":{"t":{"y":"Y","__typename":"T","id":1234,"x":"X"}},"path":["t"]}"# ); } + + #[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 { + // generated from: + // mutation { + // mutationA { + // mutationB + // } + // } + 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 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().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; + } }