From 9884c4b52d464b21eb318f7b6b40f68c7693d5cd Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 22 Aug 2024 16:35:58 -0700 Subject: [PATCH] fix(federation): fixed a bug where cached node costs were not reset after modification (#5870) --- .../src/query_plan/fetch_dependency_graph.rs | 6 +- .../query_plan/build_query_plan_tests.rs | 125 ++++++++++++++++++ ...merging_fetches_reset_cached_costs.graphql | 82 ++++++++++++ 3 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_reset_cached_costs.graphql diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index dfc862e23b..ae51fc7224 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -2051,7 +2051,7 @@ impl FetchDependencyGraph { if path.is_empty() { mutable_node - .selection_set + .selection_set_mut() .add_selections(&merged.selection_set.selection_set)?; } else { // The merged nodes might have some @include/@skip at top-level that are already part of the path. If so, @@ -2061,7 +2061,7 @@ impl FetchDependencyGraph { &path.conditional_directives(), )?; mutable_node - .selection_set + .selection_set_mut() .add_at_path(path, Some(&Arc::new(merged_selection_set)))?; } @@ -2361,9 +2361,9 @@ impl FetchDependencyGraphNode { } fn remove_inputs_from_selection(&mut self) -> Result<(), FederationError> { - let fetch_selection_set = &mut self.selection_set; if let Some(inputs) = &mut self.inputs { self.cached_cost = None; + let fetch_selection_set = &mut self.selection_set; for (_, selection) in &inputs.selection_sets_per_parent_type { fetch_selection_set.selection_set = Arc::new(fetch_selection_set.selection_set.minus(selection)?); diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests.rs b/apollo-federation/tests/query_plan/build_query_plan_tests.rs index 4062311f5b..8f256843e2 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -954,3 +954,128 @@ fn redundant_typename_for_inline_fragments_without_type_condition() { "### ); } + +#[test] +fn test_merging_fetches_reset_cached_costs() { + // This is a test for ROUTER-553. + let planner = planner!( + A: r#" + type Query { + start: S @shareable + } + + type S @key(fields: "id") { + id: ID! + u: U @shareable + } + + type U @key(fields: "id") { + id: ID! + } + "#, + B: r#" + type Query { + start: S @shareable + } + + type S @key(fields: "id") { + id: ID! + } + "#, + C: r#" + type S @key(fields: "id") { + id: ID! + x: X + a: String! + } + + type X { + t: T + } + + type T { + u: U @shareable + } + + type U @key(fields: "id") { + id: ID! + b: String + } + "#, + ); + assert_plan!( + &planner, + r#"{ + start { + u { + b + } + a + x { + t { + u { + id + } + } + } + } + }"#, + @r###" + QueryPlan { + Sequence { + Fetch(service: "A") { + { + start { + __typename + u { + __typename + id + } + id + } + } + }, + Parallel { + Flatten(path: "start") { + Fetch(service: "C") { + { + ... on S { + __typename + id + } + } => + { + ... on S { + a + x { + t { + u { + id + } + } + } + } + } + }, + }, + Flatten(path: "start.u") { + Fetch(service: "C") { + { + ... on U { + __typename + id + } + } => + { + ... on U { + b + } + } + }, + }, + }, + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_reset_cached_costs.graphql b/apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_reset_cached_costs.graphql new file mode 100644 index 0000000000..c513bf6ad9 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/test_merging_fetches_reset_cached_costs.graphql @@ -0,0 +1,82 @@ +# Composed from subgraphs with hash: 30529f3ff76bc917d12a637ede6e78ce39e7bca8 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) +{ + query: Query +} + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +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, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +scalar join__FieldSet + +enum join__Graph { + A @join__graph(name: "A", url: "none") + B @join__graph(name: "B", url: "none") + C @join__graph(name: "C", url: "none") +} + +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 Query + @join__type(graph: A) + @join__type(graph: B) + @join__type(graph: C) +{ + start: S @join__field(graph: A) @join__field(graph: B) +} + +type S + @join__type(graph: A, key: "id") + @join__type(graph: B, key: "id") + @join__type(graph: C, key: "id") +{ + id: ID! + u: U @join__field(graph: A) + x: X @join__field(graph: C) + a: String! @join__field(graph: C) +} + +type T + @join__type(graph: C) +{ + u: U +} + +type U + @join__type(graph: A, key: "id") + @join__type(graph: C, key: "id") +{ + id: ID! + b: String @join__field(graph: C) +} + +type X + @join__type(graph: C) +{ + t: T +}