fix(federation): misc fixes

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;
                         }

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<Item = NodeIndex> + 'graph,
+    ) -> impl Iterator<Item = NodeIndex> + '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<EdgeIndex>) {
         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)
     }
 

apollo-federation/src/query_plan/generate.rs

@@ -19,7 +19,7 @@ struct Partial<Plan, Element> {
 // that implements all three methods.
 pub trait PlanBuilder<Plan, Element> {
     /// `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<Plan, FederationError>;
 
     /// `compute_plan_cost`: how to compute the cost of a plan.
     fn compute_plan_cost(&mut self, plan: &mut Plan) -> Result<QueryPlanCost, FederationError>;
@@ -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<Plan, Element> 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<Plan, FederationError> {
             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<QueryPlanCost, FederationError> {

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<PlanInfo, Arc<OpPathTree>> for QueryPlanningTraversal<'a, 'b> {
-    fn add_to_plan(&mut self, plan_info: &PlanInfo, tree: Arc<OpPathTree>) -> PlanInfo {
+    fn add_to_plan(
+        &mut self,
+        plan_info: &PlanInfo,
+        tree: Arc<OpPathTree>,
+    ) -> Result<PlanInfo, FederationError> {
         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(