addressed review comments

apollographql · May 24, 2024 · 1ccd91a · 1ccd91a
1 parent 18cff54
commit 1ccd91a
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 19 deletions.
diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs
@@ -1301,9 +1301,11 @@ impl FetchDependencyGraphNode {
                 &operation_name,
             )?
         };
-        let fragments = fragments
-            .map(|rebased| rebased.for_subgraph(self.subgraph_name.clone(), subgraph_schema));
-        operation.optimize(fragments, Default::default())?;
+        if let Some(fragments) = fragments
+            .map(|rebased| rebased.for_subgraph(self.subgraph_name.clone(), subgraph_schema))
+        {
+            operation.optimize(fragments)?;
+        }
         let operation_document = operation.try_into()?;
 
         let node = super::PlanNode::Fetch(Box::new(super::FetchNode {

diff --git a/apollo-federation/src/query_plan/optimize.rs b/apollo-federation/src/query_plan/optimize.rs
@@ -1206,25 +1206,14 @@ impl SelectionSet {
 }
 
 impl Operation {
+    // PORT_NOTE: The JS version of `optimize` takes an optional `minUsagesToOptimize` argument.
+    //            However, it's only used in tests. So, it's removed in the Rust version.
     const DEFAULT_MIN_USAGES_TO_OPTIMIZE: u32 = 2;
 
-    pub(crate) fn optimize(
-        &mut self,
-        fragments: Option<&NamedFragments>,
-        min_usages_to_optimize: Option<u32>,
-    ) -> Result<(), FederationError> {
-        let min_usages_to_optimize =
-            min_usages_to_optimize.unwrap_or(Self::DEFAULT_MIN_USAGES_TO_OPTIMIZE);
-        let Some(fragments) = fragments else {
-            return Ok(());
-        };
+    pub(crate) fn optimize(&mut self, fragments: &NamedFragments) -> Result<(), FederationError> {
         if fragments.is_empty() {
             return Ok(());
         }
-        assert!(
-            min_usages_to_optimize >= 1,
-            "Expected 'min_usages_to_optimize' to be at least 1, but got {min_usages_to_optimize}"
-        );
 
         // Optimize the operation's selection set by re-using existing fragments.
         let optimized_selection = self.selection_set.optimize_at_root(fragments)?;
@@ -1234,8 +1223,8 @@ impl Operation {
 
         // Optimize the named fragment definitions by dropping low-usage ones.
         let mut final_fragments = fragments.clone();
-        let final_selection_set =
-            final_fragments.reduce_named_fragments(&optimized_selection, min_usages_to_optimize)?;
+        let final_selection_set = final_fragments
+            .reduce_named_fragments(&optimized_selection, Self::DEFAULT_MIN_USAGES_TO_OPTIMIZE)?;
 
         self.selection_set = final_selection_set;
         self.named_fragments = final_fragments;