Fix potential rogue expansion of fragments when deoptimizing

internals-js/src/operations.ts

@@ -951,7 +951,6 @@ export class SelectionSet extends Freezable<SelectionSet> {
     if (names && names.length === 0) {
       return this;
     }
-
     const newFragments = updateSelectionSetFragments
       ? (names ? this.fragments?.without(names) : undefined)
       : this.fragments;
@@ -1605,6 +1604,8 @@ export abstract class FragmentSelection extends Freezable<FragmentSelection> {
 
   abstract withNormalizedDefer(normalizer: DeferNormalizer): FragmentSelection | SelectionSet;
 
+  abstract updateForAddingTo(selectionSet: SelectionSet): FragmentSelection;
+
   protected us(): FragmentSelection {
     return this;
   }
@@ -1624,30 +1625,6 @@ export abstract class FragmentSelection extends Freezable<FragmentSelection> {
     return mergeVariables(this.element().variables(), this.selectionSet.usedVariables());
   }
 
-  updateForAddingTo(selectionSet: SelectionSet): FragmentSelection {
-    const updatedFragment = this.element().updateForAddingTo(selectionSet);
-    if (this.element() === updatedFragment) {
-      return this.cloneIfFrozen();
-    }
-
-    // Like for fields, we create a new selection that not only uses the updated fragment, but also ensures
-    // the underlying selection set uses the updated type as parent type.
-    const updatedCastedType = updatedFragment.castedType();
-    let updatedSelectionSet : SelectionSet | undefined;
-    if (this.selectionSet.parentType !== updatedCastedType) {
-      updatedSelectionSet = new SelectionSet(updatedCastedType);
-      // Note that re-adding every selection ensures that anything frozen will be cloned as needed, on top of handling any knock-down
-      // effect of the type change.
-      for (const selection of this.selectionSet.selections()) {
-        updatedSelectionSet.add(selection);
-      }
-    } else {
-      updatedSelectionSet = this.selectionSet?.cloneIfFrozen();
-    }
-
-    return new InlineFragmentSelection(updatedFragment, updatedSelectionSet);
-  }
-
   filter(predicate: (selection: Selection) => boolean): FragmentSelection | undefined {
     if (!predicate(this)) {
       return undefined;
@@ -1713,6 +1690,31 @@ class InlineFragmentSelection extends FragmentSelection {
     this.selectionSet.validate();
   }
 
+  updateForAddingTo(selectionSet: SelectionSet): FragmentSelection {
+    const updatedFragment = this.element().updateForAddingTo(selectionSet);
+    if (this.element() === updatedFragment) {
+      return this.cloneIfFrozen();
+    }
+
+    // Like for fields, we create a new selection that not only uses the updated fragment, but also ensures
+    // the underlying selection set uses the updated type as parent type.
+    const updatedCastedType = updatedFragment.castedType();
+    let updatedSelectionSet : SelectionSet | undefined;
+    if (this.selectionSet.parentType !== updatedCastedType) {
+      updatedSelectionSet = new SelectionSet(updatedCastedType);
+      // Note that re-adding every selection ensures that anything frozen will be cloned as needed, on top of handling any knock-down
+      // effect of the type change.
+      for (const selection of this.selectionSet.selections()) {
+        updatedSelectionSet.add(selection);
+      }
+    } else {
+      updatedSelectionSet = this.selectionSet?.cloneIfFrozen();
+    }
+
+    return new InlineFragmentSelection(updatedFragment, updatedSelectionSet);
+  }
+
+
   get selectionSet(): SelectionSet {
     return this._selectionSet;
   }
@@ -1785,7 +1787,6 @@ class InlineFragmentSelection extends FragmentSelection {
     if (updatedSubSelections === this.selectionSet && !hasDeferToRemove) {
       return this;
     }
-
     const newFragment = hasDeferToRemove ? this.fragmentElement.withoutDefer() : this.fragmentElement;
     if (!newFragment) {
       return updatedSubSelections;
@@ -1877,6 +1878,16 @@ class FragmentSpreadSelection extends FragmentSelection {
     return this;
   }
 
+  updateForAddingTo(_selectionSet: SelectionSet): FragmentSelection {
+    // This is a little bit iffy, because the fragment could link to a schema (typically the supergraph API one)
+    // that is different from the one of `_selectionSet` (say, a subgraph fetch selection in which we're trying to
+    // reuse a user fragment). But in practice, we expand all fragments when we do query planning and only re-add
+    // fragments back at the very end, so this should be fine. Importantly, we don't want this method to mistakenly
+    // expand the spread, as that would compromise the code that optimize subgraph fetches to re-use named
+    // fragments.
+    return this;
+  }
+
   expandFragments(names?: string[], updateSelectionSetFragments: boolean = true): FragmentSelection | readonly Selection[] {
     if (names && !names.includes(this.namedFragment.name)) {
       return this;

query-planner-js/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The Federation v0.x equivalent for this package can be found [here](https://github.com/apollographql/federation/blob/version-0.x/query-planner-js/CHANGELOG.md) on the `version-0.x` branch of this repo.
 
+
+- Fix issue with fragment reusing code something mistakenly re-expanding fragments [PR #2098](https://github.com/apollographql/federation/pull/2098).
+
 ## 2.1.0-alpha.4
 
 - Update peer dependency `graphql` to `^16.5.0` to use `GraphQLErrorOptions` [PR #2060](https://github.com/apollographql/federation/pull/2060)

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -3723,4 +3723,70 @@ describe('Named fragments preservation', () => {
 
     expect(plan).toMatchInlineSnapshot(reuseQueryFragments ? withReuse : withoutReuse);
   });
+
+  it('works with nested fragments when only the nested fragment gets preserved', () => {
+    const subgraph1 = {
+      name: 'Subgraph1',
+      typeDefs: gql`
+        type Query {
+          t: T
+        }
+
+        type T @key(fields : "id") {
+          id: ID!
+          a: V
+          b: V
+        }
+
+        type V {
+          v: Int
+        }
+
+      `
+    }
+
+    const [api, queryPlanner] = composeAndCreatePlanner(subgraph1);
+    let operation = operationFromDocument(api, gql`
+      {
+        t {
+          ...OnT
+        }
+      }
+
+      fragment OnT on T {
+        a {
+          ...OnV
+        }
+        b {
+          ...OnV
+        }
+      }
+
+      fragment OnV on V {
+        v
+      }
+    `);
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "Subgraph1") {
+          {
+            t {
+              a {
+                ...OnV
+              }
+              b {
+                ...OnV
+              }
+            }
+          }
+
+          fragment OnV on V {
+            v
+          }
+        },
+      }
+    `);
+  });
 });