Address feedback

src/query_graph/condition_resolver.rs

@@ -18,14 +18,15 @@ pub(crate) trait ConditionResolver {
     ) -> Result<ConditionResolution, FederationError>;
 }
 
-// TODO: This could probably be refactored into an enum.
 #[derive(Debug, Clone)]
-pub(crate) struct ConditionResolution {
-    pub(crate) satisfied: bool,
-    pub(crate) cost: QueryPlanCost,
-    pub(crate) path_tree: Option<Arc<OpPathTree>>,
-    // Note that this is not guaranteed to be set even if satisfied is false.
-    pub(crate) unsatisfied_condition_reason: Option<UnsatisfiedConditionReason>,
+pub(crate) enum ConditionResolution {
+    Satisfied {
+        cost: QueryPlanCost,
+        path_tree: Option<Arc<OpPathTree>>,
+    },
+    Unsatisfied {
+        reason: Option<UnsatisfiedConditionReason>,
+    },
 }
 
 #[derive(Debug, Clone)]
@@ -35,21 +36,14 @@ pub(crate) enum UnsatisfiedConditionReason {
 
 impl ConditionResolution {
     pub(crate) fn no_conditions() -> Self {
-        Self {
-            satisfied: true,
+        Self::Satisfied {
             cost: 0,
             path_tree: None,
-            unsatisfied_condition_reason: None,
         }
     }
 
     pub(crate) fn unsatisfied_conditions() -> Self {
-        Self {
-            satisfied: false,
-            cost: -1,
-            path_tree: None,
-            unsatisfied_condition_reason: None,
-        }
+        Self::Unsatisfied { reason: None }
     }
 }
 

src/query_graph/graph_path.rs

@@ -129,24 +129,12 @@ where
     defer_on_tail: Option<DeferDirectiveArguments>,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, derive_more::From)]
 pub(crate) enum GraphPathTrigger {
     Op(Arc<OpGraphPathTrigger>),
     Transition(Arc<QueryGraphEdgeTransition>),
 }
 
-impl From<Arc<OpGraphPathTrigger>> for GraphPathTrigger {
-    fn from(value: Arc<OpGraphPathTrigger>) -> Self {
-        GraphPathTrigger::Op(value)
-    }
-}
-
-impl From<Arc<QueryGraphEdgeTransition>> for GraphPathTrigger {
-    fn from(value: Arc<QueryGraphEdgeTransition>) -> Self {
-        GraphPathTrigger::Transition(value)
-    }
-}
-
 #[derive(Debug, Clone)]
 pub(crate) struct SubgraphEnteringEdgeInfo {
     /// The index within the `edges` array.
@@ -157,6 +145,9 @@ pub(crate) struct SubgraphEnteringEdgeInfo {
 
 /// Wrapper for an override ID, which indicates a relationship between a group of `OpGraphPath`s
 /// where one "overrides" the others in the group.
+///
+/// Note that we shouldn't add `derive(Serialize, Deserialize)` to this without changing the types
+/// to be something like UUIDs.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
 pub(crate) struct OverrideId(usize);
 
@@ -474,11 +465,15 @@ where
         condition_resolution: ConditionResolution,
         defer: Option<DeferDirectiveArguments>,
     ) -> Result<Self, FederationError> {
-        if !condition_resolution.satisfied {
+        let ConditionResolution::Satisfied {
+            path_tree: condition_path_tree,
+            cost: condition_cost,
+        } = condition_resolution
+        else {
             return Err(FederationError::internal(
                 "Cannot add an edge to a path if its conditions cannot be satisfied",
             ));
-        }
+        };
 
         let mut edges = self.edges.clone();
         let mut edge_triggers = self.edge_triggers.clone();
@@ -487,7 +482,7 @@ where
         let Some(new_edge) = edge.into() else {
             edges.push(edge);
             edge_triggers.push(Arc::new(trigger));
-            edge_conditions.push(condition_resolution.path_tree);
+            edge_conditions.push(condition_path_tree);
             return Ok(GraphPath {
                 graph: self.graph.clone(),
                 head: self.head,
@@ -523,7 +518,7 @@ where
                 edge_weight, tail_weight
             )));
         }
-        if let Some(path_tree) = &condition_resolution.path_tree {
+        if let Some(path_tree) = &condition_path_tree {
             if edge_weight.conditions.is_none() {
                 return Err(FederationError::internal(format!(
                     "Unexpectedly got conditions paths {} for edge {} without conditions",
@@ -621,7 +616,7 @@ where
                                     edge_conditions.pop();
                                     edges.push(new_edge.into());
                                     edge_triggers.push(Arc::new(trigger));
-                                    edge_conditions.push(condition_resolution.path_tree);
+                                    edge_conditions.push(condition_path_tree);
                                     return Ok(GraphPath {
                                         graph: self.graph.clone(),
                                         head: self.head,
@@ -677,7 +672,7 @@ where
                 edge_conditions.pop();
                 edges.push(edge);
                 edge_triggers.push(Arc::new(trigger));
-                edge_conditions.push(condition_resolution.path_tree);
+                edge_conditions.push(condition_path_tree);
                 return Ok(GraphPath {
                     graph: self.graph.clone(),
                     head: self.head,
@@ -695,7 +690,7 @@ where
                     {
                         Some(SubgraphEnteringEdgeInfo {
                             index: self.edges.len() - 1,
-                            conditions_cost: condition_resolution.cost,
+                            conditions_cost: condition_cost,
                         })
                     } else {
                         None
@@ -715,7 +710,7 @@ where
 
         edges.push(edge);
         edge_triggers.push(Arc::new(trigger));
-        edge_conditions.push(condition_resolution.path_tree);
+        edge_conditions.push(condition_path_tree);
         Ok(GraphPath {
             graph: self.graph.clone(),
             head: self.head,
@@ -730,7 +725,7 @@ where
             {
                 Some(SubgraphEnteringEdgeInfo {
                     index: self.edges.len(),
-                    conditions_cost: condition_resolution.cost,
+                    conditions_cost: condition_cost,
                 })
             } else {
                 None
@@ -885,19 +880,20 @@ where
                     last_edge_weight.transition,
                     QueryGraphEdgeTransition::KeyResolution
                 ) {
-                    let in_same_subgraph = if let Some(path_tree) = &resolution.path_tree {
+                    let in_same_subgraph = if let ConditionResolution::Satisfied {
+                        path_tree: Some(path_tree),
+                        ..
+                    } = &resolution
+                    {
                         path_tree.is_all_in_same_subgraph()?
                     } else {
                         true
                     };
                     if in_same_subgraph {
                         let (edge_head, _) = self.graph.edge_endpoints(edge)?;
                         if self.graph.get_locally_satisfiable_key(edge_head)?.is_none() {
-                            return Ok(ConditionResolution {
-                                unsatisfied_condition_reason: Some(
-                                    UnsatisfiedConditionReason::NoPostRequireKey,
-                                ),
-                                ..ConditionResolution::unsatisfied_conditions()
+                            return Ok(ConditionResolution::Unsatisfied {
+                                reason: Some(UnsatisfiedConditionReason::NoPostRequireKey),
                             });
                         };
                         // We're in a case where we have an `@requires` application (we have
@@ -951,7 +947,7 @@ impl OpGraphPath {
             &Default::default(),
             &Default::default(),
         )?;
-        if condition_resolution.satisfied {
+        if matches!(condition_resolution, ConditionResolution::Satisfied { .. }) {
             self.add(
                 operation_field.into(),
                 edge.into(),
@@ -1877,7 +1873,10 @@ impl OpGraphPath {
                                     &Default::default(),
                                     &Default::default(),
                                 )?;
-                                if !condition_resolution.satisfied {
+                                if matches!(
+                                    condition_resolution,
+                                    ConditionResolution::Unsatisfied { .. }
+                                ) {
                                     return Ok((None, None));
                                 }
                                 let fragment_path = self.add(

src/query_graph/mod.rs

@@ -427,10 +427,7 @@ impl QueryGraph {
         node: NodeIndex,
         field: &NormalizedField,
     ) -> Option<EdgeIndex> {
-        // PORT_NOTE: The JS codebase found all matching candidates and asserted there was only one.
-        // We consider it unlikely for there to be more than one candidate given all the code paths
-        // that create edges, so we've removed the assert here and return on the first match.
-        self.out_edges(node).find_map(|edge_ref| {
+        let mut candidates = self.out_edges(node).filter_map(|edge_ref| {
             let edge_weight = edge_ref.weight();
             let QueryGraphEdgeTransition::FieldCollection {
                 field_definition_position,
@@ -446,7 +443,19 @@ impl QueryGraph {
             } else {
                 None
             }
-        })
+        });
+        if let Some(candidate) = candidates.next() {
+            // PORT_NOTE: The JS codebase used an assertion rather than a debug assertion here. We
+            // consider it unlikely for there to be more than one candidate given all the code paths
+            // that create edges, so we've downgraded this to a debug assertion.
+            debug_assert!(
+                candidates.next().is_none(),
+                "Unexpectedly found multiple candidates",
+            );
+            Some(candidate)
+        } else {
+            None
+        }
     }
 
     pub(crate) fn edge_for_inline_fragment(
@@ -458,10 +467,7 @@ impl QueryGraph {
             // No type condition means the type hasn't changed, meaning there is no edge to take.
             return None;
         };
-        // PORT_NOTE: The JS codebase found all matching candidates and asserted there was only one.
-        // We consider it unlikely for there to be more than one candidate given all the code paths
-        // that create edges, so we've removed the assert here and return on the first match.
-        self.out_edges(node).find_map(|edge_ref| {
+        let mut candidates = self.out_edges(node).filter_map(|edge_ref| {
             let edge_weight = edge_ref.weight();
             let QueryGraphEdgeTransition::Downcast {
                 to_type_position, ..
@@ -477,7 +483,19 @@ impl QueryGraph {
             } else {
                 None
             }
-        })
+        });
+        if let Some(candidate) = candidates.next() {
+            // PORT_NOTE: The JS codebase used an assertion rather than a debug assertion here. We
+            // consider it unlikely for there to be more than one candidate given all the code paths
+            // that create edges, so we've downgraded this to a debug assertion.
+            debug_assert!(
+                candidates.next().is_none(),
+                "Unexpectedly found multiple candidates",
+            );
+            Some(candidate)
+        } else {
+            None
+        }
     }
 
     /// Given the possible runtime types at the head of the given edge, returns the possible runtime

src/query_plan/operation.rs

@@ -35,7 +35,10 @@ const TYPENAME_FIELD: Name = name!("__typename");
 // Global storage for the counter used to uniquely identify selections
 static NEXT_ID: atomic::AtomicUsize = atomic::AtomicUsize::new(1);
 
-// opaque wrapper of the unique selection ID type
+/// Opaque wrapper of the unique selection ID type.
+///
+/// Note that we shouldn't add `derive(Serialize, Deserialize)` to this without changing the types
+/// to be something like UUIDs.
 #[derive(Clone, Debug, Eq, PartialEq, Hash)]
 pub(crate) struct SelectionId(usize);
 
@@ -676,7 +679,6 @@ pub(crate) mod normalized_inline_fragment_selection {
     use crate::schema::position::CompositeTypeDefinitionPosition;
     use crate::schema::ValidFederationSchema;
     use apollo_compiler::ast::DirectiveList;
-    use apollo_compiler::name;
     use std::sync::Arc;
 
     /// An analogue of the apollo-compiler type `InlineFragment` with these changes:
@@ -739,7 +741,7 @@ pub(crate) mod normalized_inline_fragment_selection {
         pub(crate) fn defer_directive_arguments(
             &self,
         ) -> Result<Option<DeferDirectiveArguments>, FederationError> {
-            if let Some(directive) = self.directives.get(&name!("defer")) {
+            if let Some(directive) = self.directives.get("defer") {
                 Ok(Some(defer_directive_arguments(directive)?))
             } else {
                 Ok(None)