Fix potential rogue expansion of fragments when deoptimizing (apollog…

…raphql#2098)

The code from apollographql#1911 ensures that named fragments from the user query are
reused in subgraph query "when appropriate", but there were some code
path where a fragment that was reused (correctly) could end up be
"re-expanded" (due to an existing method not handling spreads properly).

The resulting subgraph query ended up with the fragment definition
existing but never being used, which is invalid in graphQL.

This patch ensures that a reused fragment is not mistakenly re-expanded
by the code (and thus avoids a query with unused fragments).

Fixes apollographql#2092

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
+          }
+        },
+      }
+    `);
+  });
 });