Address feedback (switch to dyn, and use named function for no-op che…

…ck cancellation)
apollographql · Mar 3, 2025 · 67fad15 · 67fad15
1 parent 44ccc10
commit 67fad15
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 38 deletions.
diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs
@@ -64,6 +64,8 @@ pub(crate) use contains::*;
 pub(crate) use directive_list::DirectiveList;
 pub(crate) use merging::*;
 pub(crate) use rebase::*;
+#[cfg(test)]
+pub(crate) use tests::never_cancel;
 
 pub(crate) const TYPENAME_FIELD: Name = name!("__typename");
 
@@ -1543,7 +1545,7 @@ impl SelectionSet {
 
     pub(crate) fn expand_all_fragments(
         &self,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<SelectionSet, FederationError> {
         let mut expanded_selections = vec![];
         SelectionSet::expand_selection_set(&mut expanded_selections, self, check_cancellation)?;
@@ -1560,7 +1562,7 @@ impl SelectionSet {
     fn expand_selection_set(
         destination: &mut Vec<Selection>,
         selection_set: &SelectionSet,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<(), FederationError> {
         for value in selection_set.selections.values() {
             check_cancellation()?;
@@ -2725,7 +2727,7 @@ impl InlineFragmentSelection {
     pub(crate) fn from_fragment_spread_selection(
         parent_type_position: CompositeTypeDefinitionPosition,
         fragment_spread_selection: &Arc<FragmentSpreadSelection>,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<InlineFragmentSelection, FederationError> {
         let schema = fragment_spread_selection.spread.schema.schema();
         for directive in fragment_spread_selection.spread.directives.iter() {
@@ -3759,7 +3761,7 @@ pub(crate) fn normalize_operation(
     named_fragments: NamedFragments,
     schema: &ValidFederationSchema,
     interface_types_with_interface_objects: &IndexSet<InterfaceTypeDefinitionPosition>,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<Operation, FederationError> {
     let mut normalized_selection_set =
         SelectionSet::from_selection_set(&operation.selection_set, &named_fragments, schema)?;

diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs
@@ -13,6 +13,7 @@ use super::SelectionKey;
 use super::SelectionSet;
 use super::normalize_operation;
 use crate::error::FederationError;
+use crate::error::SingleFederationError;
 use crate::query_graph::graph_path::OpPathElement;
 use crate::schema::ValidFederationSchema;
 use crate::schema::position::InterfaceTypeDefinitionPosition;
@@ -65,10 +66,16 @@ pub(super) fn parse_and_expand(
         fragments,
         schema,
         &Default::default(),
-        &|| Ok(()),
+        &never_cancel,
     )
 }
 
+/// The `normalize_operation()` function has a `check_cancellation` parameter that we'll want to
+/// configure to never cancel during tests. We create a convenience function here for that purpose.
+pub(crate) fn never_cancel() -> Result<(), SingleFederationError> {
+    Ok(())
+}
+
 #[test]
 fn expands_named_fragments() {
     let operation_with_named_fragment = r#"
@@ -106,7 +113,7 @@ type Foo {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         normalized_operation.named_fragments = Default::default();
@@ -161,7 +168,7 @@ type Foo {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         normalized_operation.named_fragments = Default::default();
@@ -204,7 +211,7 @@ type Query {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
 
@@ -240,7 +247,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -284,7 +291,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -331,7 +338,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -376,7 +383,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -423,7 +430,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skip1: Boolean!, $skip2: Boolean!) {
@@ -475,7 +482,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -542,7 +549,7 @@ type V {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -602,7 +609,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -649,7 +656,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -700,7 +707,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -749,7 +756,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skipIf: Boolean!) {
@@ -798,7 +805,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test($skip1: Boolean!, $skip2: Boolean!) {
@@ -851,7 +858,7 @@ type T {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -920,7 +927,7 @@ type V {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query Test {
@@ -967,7 +974,7 @@ type Foo {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query TestQuery {
@@ -1007,7 +1014,7 @@ type Foo {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query TestQuery {
@@ -1058,7 +1065,7 @@ scalar FieldSet
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &interface_objects,
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let expected = r#"query TestQuery {
@@ -1198,7 +1205,7 @@ mod make_selection_tests {
             Default::default(),
             &schema,
             &Default::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
 
@@ -1298,7 +1305,7 @@ mod lazy_map_tests {
             Default::default(),
             &schema,
             &Default::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
 
@@ -1357,7 +1364,7 @@ mod lazy_map_tests {
             Default::default(),
             &schema,
             &Default::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
 
@@ -1563,7 +1570,7 @@ fn test_expand_all_fragments1() {
             NamedFragments::new(&executable_document.fragments, &schema),
             &schema,
             &IndexSet::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         normalized_operation.named_fragments = Default::default();

diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs
@@ -2989,7 +2989,7 @@ impl OpGraphPath {
         context: &OpGraphPathContext,
         condition_resolver: &mut impl ConditionResolver,
         override_conditions: &EnabledOverrideConditions,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<(Option<Vec<SimultaneousPaths>>, Option<bool>), FederationError> {
         let span = debug_span!(
             "Trying to advance directly",
@@ -3784,7 +3784,7 @@ impl SimultaneousPaths {
     /// the options for the `SimultaneousPaths` as a whole.
     fn flat_cartesian_product(
         options_for_each_path: Vec<Vec<SimultaneousPaths>>,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<Vec<SimultaneousPaths>, FederationError> {
         // This can be written more tersely with a bunch of `reduce()`/`flat_map()`s and friends,
         // but when interfaces type-explode into many implementations, this can end up with fairly
@@ -3981,7 +3981,7 @@ impl SimultaneousPathsWithLazyIndirectPaths {
         operation_element: &OpPathElement,
         condition_resolver: &mut impl ConditionResolver,
         override_conditions: &EnabledOverrideConditions,
-        check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+        check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
     ) -> Result<Option<Vec<SimultaneousPathsWithLazyIndirectPaths>>, FederationError> {
         debug!(
             "Trying to advance paths for operation: path = {}, operation = {operation_element}",

diff --git a/apollo-federation/src/query_graph/path_tree.rs b/apollo-federation/src/query_graph/path_tree.rs
@@ -581,6 +581,7 @@ mod tests {
 
     use crate::error::FederationError;
     use crate::operation::Field;
+    use crate::operation::never_cancel;
     use crate::operation::normalize_operation;
     use crate::query_graph::QueryGraph;
     use crate::query_graph::QueryGraphEdgeTransition;
@@ -720,7 +721,7 @@ mod tests {
             Default::default(),
             &schema,
             &Default::default(),
-            &|| Ok(()),
+            &never_cancel,
         )
         .unwrap();
         let selection_set = Arc::new(normalized_operation.selection_set);

diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs
@@ -3488,7 +3488,7 @@ pub(crate) fn compute_nodes_for_tree(
     initial_node_path: FetchDependencyGraphNodePath,
     initial_defer_context: DeferContext,
     initial_conditions: &OpGraphPathContext,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<IndexSet<NodeIndex>, FederationError> {
     snapshot!("OpPathTree", initial_tree.to_string(), "path_tree");
     let mut stack = vec![ComputeNodesStackItem {
@@ -3600,7 +3600,7 @@ fn compute_nodes_for_key_resolution<'a>(
     edge_id: EdgeIndex,
     new_context: &'a OpGraphPathContext,
     created_nodes: &mut IndexSet<NodeIndex>,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<ComputeNodesStackItem<'a>, FederationError> {
     let edge = stack_item.tree.graph.edge_weight(edge_id)?;
     let Some(conditions) = &child.conditions else {
@@ -3855,7 +3855,7 @@ fn compute_nodes_for_op_path_element<'a>(
     child: &'a Arc<PathTreeChild<OpGraphPathTrigger, Option<EdgeIndex>>>,
     operation_element: &OpPathElement,
     created_nodes: &mut IndexSet<NodeIndex>,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<ComputeNodesStackItem<'a>, FederationError> {
     let Some(edge_id) = child.edge else {
         // A null edge means that the operation does nothing
@@ -4442,7 +4442,7 @@ fn handle_conditions_tree(
     query_graph_edge_id_if_typename_needed: Option<EdgeIndex>,
     defer_context: &DeferContext,
     created_nodes: &mut IndexSet<NodeIndex>,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<ConditionsNodeData, FederationError> {
     // In many cases, we can optimize conditions by merging the fields into previously existing
     // nodes. However, we only do this when the current node has only a single parent (it's hard to

diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs
@@ -617,7 +617,7 @@ pub(crate) fn compute_root_fetch_groups(
     dependency_graph: &mut FetchDependencyGraph,
     path: &OpPathTree,
     type_conditioned_fetching_enabled: bool,
-    check_cancellation: &impl Fn() -> Result<(), SingleFederationError>,
+    check_cancellation: &dyn Fn() -> Result<(), SingleFederationError>,
 ) -> Result<(), FederationError> {
     // The root of the pathTree is one of the "fake" root of the subgraphs graph,
     // which belongs to no subgraph but points to each ones.