diff --git a/apollo-federation/src/query_graph/build_query_graph.rs b/apollo-federation/src/query_graph/build_query_graph.rs index 929e4b8d0e..38b2232dcc 100644 --- a/apollo-federation/src/query_graph/build_query_graph.rs +++ b/apollo-federation/src/query_graph/build_query_graph.rs @@ -1902,7 +1902,8 @@ impl FederatedQueryGraphBuilder { } .into()); }; - if conditions.selections == followup_conditions.selections { + + if conditions == followup_conditions { continue; } } @@ -1926,7 +1927,7 @@ impl FederatedQueryGraphBuilder { // since we can do "start of query" -> C and that's always better. if matches!( followup_edge_weight.transition, - QueryGraphEdgeTransition::SubgraphEnteringTransition + QueryGraphEdgeTransition::RootTypeResolution { .. } ) { continue; } diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index d1360120c7..f6a90c070b 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -668,7 +668,9 @@ impl FetchDependencyGraph { // keeping nodes separated when they have a different path in their parent // allows to keep that "path in parent" more precisely, // which is important for some case of @requires). - for existing_id in self.children_of(parent.parent_node_id) { + for existing_id in + FetchDependencyGraph::sorted_nodes(self.children_of(parent.parent_node_id)) + { let existing = self.node_weight(existing_id)?; // we compare the subgraph names last because on average it improves performance if existing.merge_at.as_deref() == Some(merge_at) @@ -888,6 +890,27 @@ impl FetchDependencyGraph { }) } + /// By default, petgraph iterates over the nodes in the order of their node indices, but if + /// we retrieve node iterator based on the edges (e.g. children of/parents of), then resulting + /// iteration order is unspecified. In practice, it appears that edges are iterated in the + /// *reverse* iteration order. + /// + /// Since this behavior can affect the query plans, we can use this method to explicitly sort + /// the iterator to ensure we consistently follow the node index order. + /// + /// NOTE: In JS implementation, whenever we remove/merge nodes, we always shift left remaining + /// nodes so there are no gaps in the node IDs and the newly created nodes are always created + /// with the largest IDs. RS implementation has different behavior - whenever nodes are removed, + /// their IDs are later reused by petgraph so we no longer have guarantee that node with the + /// largest ID is the last node that was created. Due to the above, sorting by node IDs may still + /// result in different iteration order than the JS code, but in practice might be enough to + /// ensure correct plans. + fn sorted_nodes<'graph>( + nodes: impl Iterator + 'graph, + ) -> impl Iterator + 'graph { + nodes.sorted_by_key(|n| n.index()) + } + fn type_for_fetch_inputs( &self, type_name: &Name, @@ -898,7 +921,13 @@ impl FetchDependencyGraph { .try_into()?) } - /// Find redundant edges coming out of a node. See `remove_redundant_edges`. + /// Find redundant edges coming out of a node. See `remove_redundant_edges`. This method assumes + /// that the underlying graph does not have any cycles between nodes. + /// + /// PORT NOTE: JS implementation performs in-place removal of edges when finding the redundant + /// edges. In RS implementation we first collect the edges and then remove them. This has a side + /// effect that if we ever end up with a cycle in a graph (which is an invalid state), this method + /// may result in infinite loop. fn collect_redundant_edges(&self, node_index: NodeIndex, acc: &mut HashSet) { let mut stack = vec![]; for start_index in self.children_of(node_index) { @@ -907,7 +936,6 @@ impl FetchDependencyGraph { for edge in self.graph.edges_connecting(node_index, v) { acc.insert(edge.id()); } - stack.extend(self.children_of(v)); } } @@ -921,6 +949,9 @@ impl FetchDependencyGraph { let mut redundant_edges = HashSet::new(); self.collect_redundant_edges(node_index, &mut redundant_edges); + if !redundant_edges.is_empty() { + self.on_modification(); + } for edge in redundant_edges { self.graph.remove_edge(edge); } @@ -979,9 +1010,11 @@ impl FetchDependencyGraph { self.collect_redundant_edges(node_index, &mut redundant_edges); } - for edge in redundant_edges { - // PORT_NOTE: JS version calls `FetchGroup.removeChild`, which calls onModification. + // PORT_NOTE: JS version calls `FetchGroup.removeChild`, which calls onModification. + if !redundant_edges.is_empty() { self.on_modification(); + } + for edge in redundant_edges { self.graph.remove_edge(edge); } @@ -2123,18 +2156,17 @@ impl FetchDependencyGraph { let node = self.node_weight(node_id)?; let parent = self.node_weight(parent_relation.parent_node_id)?; let Some(parent_op_path) = &parent_relation.path_in_parent else { - return Err(FederationError::internal("Parent operation path is empty")); + return Ok(false); }; let type_at_path = self.type_at_path( &parent.selection_set.selection_set.type_position, &parent.selection_set.selection_set.schema, parent_op_path, )?; - let new_node_is_unneeded = parent_relation.path_in_parent.is_some() - && node - .selection_set - .selection_set - .can_rebase_on(&type_at_path, &parent.selection_set.selection_set.schema)?; + let new_node_is_unneeded = node + .selection_set + .selection_set + .can_rebase_on(&type_at_path, &parent.selection_set.selection_set.schema)?; Ok(new_node_is_unneeded) } diff --git a/apollo-federation/src/query_plan/generate.rs b/apollo-federation/src/query_plan/generate.rs index 0511deb498..4001b70f6f 100644 --- a/apollo-federation/src/query_plan/generate.rs +++ b/apollo-federation/src/query_plan/generate.rs @@ -19,7 +19,7 @@ struct Partial { // that implements all three methods. pub trait PlanBuilder { /// `add_to_plan`: how to obtain a new plan by taking some plan and adding a new element to it. - fn add_to_plan(&mut self, plan: &Plan, elem: Element) -> Plan; + fn add_to_plan(&mut self, plan: &Plan, elem: Element) -> Result; /// `compute_plan_cost`: how to compute the cost of a plan. fn compute_plan_cost(&mut self, plan: &mut Plan) -> Result; @@ -158,7 +158,7 @@ where let picked_index = pick_next(index, next_choices); let Extracted { extracted, is_last } = extract(picked_index, next_choices); - let mut new_partial_plan = plan_builder.add_to_plan(&partial_plan, extracted); + let mut new_partial_plan = plan_builder.add_to_plan(&partial_plan, extracted)?; let cost = plan_builder.compute_plan_cost(&mut new_partial_plan)?; if !is_last { @@ -252,7 +252,11 @@ mod tests { } impl<'a> PlanBuilder for TestPlanBuilder<'a> { - fn add_to_plan(&mut self, partial_plan: &Plan, new_element: Element) -> Plan { + fn add_to_plan( + &mut self, + partial_plan: &Plan, + new_element: Element, + ) -> Result { let new_plan: Plan = partial_plan .iter() .cloned() @@ -261,7 +265,7 @@ mod tests { if new_plan.len() == self.target_len { self.generated.push(new_plan.clone()) } - new_plan + Ok(new_plan) } fn compute_plan_cost(&mut self, plan: &mut Plan) -> Result { diff --git a/apollo-federation/src/query_plan/query_planning_traversal.rs b/apollo-federation/src/query_plan/query_planning_traversal.rs index edfdca8fa8..2dab41debf 100644 --- a/apollo-federation/src/query_plan/query_planning_traversal.rs +++ b/apollo-federation/src/query_plan/query_planning_traversal.rs @@ -1063,21 +1063,17 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { } impl<'a: 'b, 'b> PlanBuilder> for QueryPlanningTraversal<'a, 'b> { - fn add_to_plan(&mut self, plan_info: &PlanInfo, tree: Arc) -> PlanInfo { + fn add_to_plan( + &mut self, + plan_info: &PlanInfo, + tree: Arc, + ) -> Result { let mut updated_graph = plan_info.fetch_dependency_graph.clone(); - let result = self.updated_dependency_graph(&mut updated_graph, &tree); - if result.is_ok() { - PlanInfo { + self.updated_dependency_graph(&mut updated_graph, &tree) + .map(|_| PlanInfo { fetch_dependency_graph: updated_graph, path_tree: plan_info.path_tree.merge(&tree), - } - } else { - // Failed to update. Return the original plan. - PlanInfo { - fetch_dependency_graph: updated_graph, - path_tree: plan_info.path_tree.clone(), - } - } + }) } fn compute_plan_cost( diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs index 2b270770e1..d33ef66191 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/requires.rs @@ -1453,3 +1453,288 @@ fn it_require_of_multiple_field_when_one_is_also_a_key_to_reach_another() { "### ); } + +#[test] +fn it_handles_multiple_requires_with_multiple_fetches() { + let planner = planner!( + s1: r#" + type Query { + t: T + } + + interface I { + id: ID! + name: String! + } + + type T implements I @key(fields: "id") { + id: ID! + name: String! @shareable + x: X @shareable + v: V @shareable + } + + type U implements I @key(fields: "id") { + id: ID! + name: String! @external + } + + type V @key(fields: "id") @key(fields: "internalID") { + id: ID! + internalID: ID! + } + + type X @key(fields: "t { id }") { + t: T! + isX: Boolean! + } + "#, + s2: r#" + type V @key(fields: "id") { + id: ID! + internalID: ID! @shareable + y: Y! @shareable + zz: [Z!] @external + } + + type Z { + u: U! @external + } + + type Y @key(fields: "id") { + id: ID! + isY: Boolean! @external + } + + interface I { + id: ID! + name: String! + } + + type T implements I @key(fields: "id") { + id: ID! + name: String! @external + x: X @external + v: V @external + foo: [String!]! @requires(fields: "x { isX }\nv { y { isY } }") + bar: [I!]! @requires(fields: "x { isX }\nv { y { isY } zz { u { id } } }") + } + + type X { + isX: Boolean! @external + } + + type U implements I @key(fields: "id") { + id: ID! + name: String! @external + } + "#, + s3: r#" + type V @key(fields: "internalID") { + internalID: ID! + y: Y! @shareable + } + + type Y @key(fields: "id") { + id: ID! + isY: Boolean! + } + "#, + s4: r#" + type V @key(fields: "id") @key(fields: "internalID") { + id: ID! + internalID: ID! + zz: [Z!] @override(from: "s1") + } + + type Z { + free: Boolean + u: U! + v: V! + } + + interface I { + id: ID! + name: String! + } + + type T implements I @key(fields: "id") { + id: ID! + name: String! @shareable + x: X @shareable + v: V @shareable + } + + type X @key(fields: "t { id }", resolvable: false) { + t: T! @external + } + + type U implements I @key(fields: "id") { + id: ID! + name: String! @override(from: "s1") + } + "#, + ); + assert_plan!( + &planner, + r#" + { + t { + foo + bar { + name + } + } + } + "#, + + @r###" + QueryPlan { + Sequence { + Fetch(service: "s1") { + { + t { + __typename + id + x { + isX + } + v { + __typename + internalID + } + } + } + }, + Flatten(path: "t") { + Fetch(service: "s4") { + { + ... on T { + __typename + id + } + } => + { + ... on T { + v { + __typename + internalID + zz { + u { + id + } + } + } + } + } + }, + }, + Flatten(path: "t.v") { + Fetch(service: "s3") { + { + ... on V { + __typename + internalID + } + } => + { + ... on V { + y { + isY + } + } + } + }, + }, + Parallel { + Sequence { + Flatten(path: "t") { + Fetch(service: "s2") { + { + ... on T { + __typename + x { + isX + } + v { + y { + isY + } + zz { + u { + id + } + } + } + id + } + } => + { + ... on T { + bar { + __typename + ... on T { + __typename + id + } + ... on U { + __typename + id + } + } + } + } + }, + }, + Flatten(path: "t.bar.@") { + Fetch(service: "s4") { + { + ... on T { + __typename + id + } + ... on U { + __typename + id + } + } => + { + ... on T { + name + } + ... on U { + name + } + } + }, + }, + }, + Flatten(path: "t") { + Fetch(service: "s2") { + { + ... on T { + __typename + x { + isX + } + v { + y { + isY + } + } + id + } + } => + { + ... on T { + foo + } + } + }, + }, + }, + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/supergraphs/it_handles_multiple_requires_with_multiple_fetches.graphql b/apollo-federation/tests/query_plan/supergraphs/it_handles_multiple_requires_with_multiple_fetches.graphql new file mode 100644 index 0000000000..8e4d26a88e --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/it_handles_multiple_requires_with_multiple_fetches.graphql @@ -0,0 +1,130 @@ +# Composed from subgraphs with hash: 461e4a611a1faf2558d6ee6e3de4af24a043fc16 +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 + +interface I + @join__type(graph: S1) + @join__type(graph: S2) + @join__type(graph: S4) +{ + id: ID! + name: String! +} + +scalar join__FieldSet + +enum join__Graph { + S1 @join__graph(name: "s1", url: "none") + S2 @join__graph(name: "s2", url: "none") + S3 @join__graph(name: "s3", url: "none") + S4 @join__graph(name: "s4", 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: S1) + @join__type(graph: S2) + @join__type(graph: S3) + @join__type(graph: S4) +{ + t: T @join__field(graph: S1) +} + +type T implements I + @join__implements(graph: S1, interface: "I") + @join__implements(graph: S2, interface: "I") + @join__implements(graph: S4, interface: "I") + @join__type(graph: S1, key: "id") + @join__type(graph: S2, key: "id") + @join__type(graph: S4, key: "id") +{ + id: ID! + name: String! @join__field(graph: S1) @join__field(graph: S2, external: true) @join__field(graph: S4) + x: X @join__field(graph: S1) @join__field(graph: S2, external: true) @join__field(graph: S4) + v: V @join__field(graph: S1) @join__field(graph: S2, external: true) @join__field(graph: S4) + foo: [String!]! @join__field(graph: S2, requires: "x { isX }\nv { y { isY } }") + bar: [I!]! @join__field(graph: S2, requires: "x { isX }\nv { y { isY } zz { u { id } } }") +} + +type U implements I + @join__implements(graph: S1, interface: "I") + @join__implements(graph: S2, interface: "I") + @join__implements(graph: S4, interface: "I") + @join__type(graph: S1, key: "id") + @join__type(graph: S2, key: "id") + @join__type(graph: S4, key: "id") +{ + id: ID! + name: String! @join__field(graph: S1, external: true) @join__field(graph: S2, external: true) @join__field(graph: S4, override: "s1") +} + +type V + @join__type(graph: S1, key: "id") + @join__type(graph: S1, key: "internalID") + @join__type(graph: S2, key: "id") + @join__type(graph: S3, key: "internalID") + @join__type(graph: S4, key: "id") + @join__type(graph: S4, key: "internalID") +{ + id: ID! @join__field(graph: S1) @join__field(graph: S2) @join__field(graph: S4) + internalID: ID! + y: Y! @join__field(graph: S2) @join__field(graph: S3) + zz: [Z!] @join__field(graph: S2, external: true) @join__field(graph: S4, override: "s1") +} + +type X + @join__type(graph: S1, key: "t { id }") + @join__type(graph: S2) + @join__type(graph: S4, key: "t { id }", resolvable: false) +{ + t: T! @join__field(graph: S1) @join__field(graph: S4, external: true) + isX: Boolean! @join__field(graph: S1) @join__field(graph: S2, external: true) +} + +type Y + @join__type(graph: S2, key: "id") + @join__type(graph: S3, key: "id") +{ + id: ID! + isY: Boolean! @join__field(graph: S2, external: true) @join__field(graph: S3) +} + +type Z + @join__type(graph: S2) + @join__type(graph: S4) +{ + u: U! @join__field(graph: S2, external: true) @join__field(graph: S4) + free: Boolean @join__field(graph: S4) + v: V! @join__field(graph: S4) +}