Fix normalization of fragment spreads (#2659)

The code that "normalize" selection sets to remove unecessary fragments
is also used to remove impossibe (and thus invalid) branches when some
reused fragments gets expanded away due to only be used once.

However, while the code for inline fragments was correctly removing
impossible branches, the similar code for named spreads was not, which
in some rare cases could result in an invalid subgraph fetch.

.changeset/rotten-ants-clean.md

@@ -0,0 +1,10 @@
+---
+"@apollo/federation-internals": patch
+---
+
+Fix issue in the code to reuse fragments that, in some rare circumstances, could led to invalid queries where a named
+spread was use in an invalid position. If triggered, this resulted in an subgraph fetch whereby a named spread was
+used inside a sub-selection even though the spread condition did not intersect the parent type (the exact error message
+would depend on the client library used to handle subgraph fetches, but with GraphQL-js, the error message had the
+form "Fragment <F> cannot be spread here as objects of type <X> can never be of type <Y>").
+

internals-js/src/__tests__/operations.test.ts

@@ -1812,6 +1812,98 @@ describe('fragments optimization', () => {
         }
       `);
     });
+
+    test('when a spread inside an expanded fragment should be "normalized away"', () => {
+      const schema = parseSchema(`
+        type Query {
+          t1: T1
+          i: I
+        }
+
+        interface I {
+          id: ID!
+        }
+
+        type T1 implements I {
+          id: ID!
+          a: Int
+        }
+
+        type T2 implements I {
+          id: ID!
+          b: Int
+          c: Int
+        }
+      `);
+      const gqlSchema = schema.toGraphQLJSSchema();
+
+      const operation = parseOperation(schema, `
+        {
+          t1 {
+            ...GetAll
+          }
+          i {
+            ...GetT2
+          }
+        }
+
+        fragment GetAll on I {
+           ... on T1 {
+             a
+           }
+           ...GetT2
+           ... on T2 {
+             c
+           }
+        }
+
+        fragment GetT2 on T2 {
+           b
+        }
+      `);
+      expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]);
+
+      const withoutFragments = operation.expandAllFragments();
+      expect(withoutFragments.toString()).toMatchString(`
+        {
+          t1 {
+            a
+          }
+          i {
+            ... on T2 {
+              b
+            }
+          }
+        }
+      `);
+
+      // As we re-optimize, we will initially generated the initial query. But
+      // as we ask to only optimize fragments used more than once, the `GetAll`
+      // fragment will be re-expanded (`GetT2` will not because the code will say
+      // that it is used both in the expanded `GetAll` but also inside `i`).
+      // But because `GetAll` is within `t1: T1`, that expansion should actually
+      // get rid of anything `T2`-related.
+      // This test exists because a previous version of the code was not correctly
+      // "getting rid" of the `...GetT2` spread, keeping in the query, which is
+      // invalid (we cannot have `...GetT2` inside `t1`).
+      const optimized = withoutFragments.optimize(operation.fragments!, 2);
+      expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]);
+
+      expect(optimized.toString()).toMatchString(`
+        fragment GetT2 on T2 {
+          b
+        }
+
+        {
+          t1 {
+            a
+          }
+          i {
+            ...GetT2
+          }
+        }
+      `);
+    });
   });
 
   test('does not leave unused fragments', () => {

internals-js/src/operations.ts

@@ -3137,6 +3137,26 @@ export abstract class FragmentSelection extends AbstractSelection<FragmentElemen
   abstract equals(that: Selection): boolean;
 
   abstract contains(that: Selection, options?: { ignoreMissingTypename?: boolean }): ContainsResult;
+
+  normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined {
+    const thisCondition = this.element.typeCondition;
+
+    // This method assumes by contract that `parentType` runtimes intersects `this.parentType`'s, but `parentType`
+    // runtimes may be a subset. So first check if the selection should not be discarded on that account (that
+    // is, we should not keep the selection if its condition runtimes don't intersect at all with those of
+    // `parentType` as that would ultimately make an invalid selection set).
+    if (thisCondition && parentType !== this.parentType) {
+      const conditionRuntimes = possibleRuntimeTypes(thisCondition);
+      const typeRuntimes = possibleRuntimeTypes(parentType);
+      if (!conditionRuntimes.some((t) => typeRuntimes.includes(t))) {
+        return undefined;
+      }
+    }
+
+    return this.normalizeKnowingItIntersects({ parentType, recursive });
+  }
+
+  protected abstract normalizeKnowingItIntersects({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined;
 }
 
 class InlineFragmentSelection extends FragmentSelection {
@@ -3317,21 +3337,9 @@ class InlineFragmentSelection extends FragmentSelection {
       : this.withUpdatedComponents(newElement, newSelection);
   }
 
-  normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined {
+  protected normalizeKnowingItIntersects({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined {
     const thisCondition = this.element.typeCondition;
 
-    // This method assumes by contract that `parentType` runtimes intersects `this.parentType`'s, but `parentType`
-    // runtimes may be a subset. So first check if the selection should not be discarded on that account (that
-    // is, we should not keep the selection if its condition runtimes don't intersect at all with those of
-    // `parentType` as that would ultimately make an invalid selection set).
-    if (thisCondition && parentType !== this.parentType) {
-      const conditionRuntimes = possibleRuntimeTypes(thisCondition);
-      const typeRuntimes = possibleRuntimeTypes(parentType);
-      if (!conditionRuntimes.some((t) => typeRuntimes.includes(t))) {
-        return undefined;
-      }
-    }
-
     // We know the condition is "valid", but it may not be useful. That said, if the condition has directives,
     // we preserve the fragment no matter what.
     if (this.element.appliedDirectives.length === 0) {
@@ -3472,7 +3480,7 @@ class FragmentSpreadSelection extends FragmentSelection {
     assert(false, `Unsupported`);
   }
 
-  normalize({ parentType }: { parentType: CompositeType }): FragmentSelection  {
+  normalizeKnowingItIntersects({ parentType }: { parentType: CompositeType }): FragmentSelection {
     // We must update the spread parent type if necessary since we're not going deeper,
     // or we'll be fundamentally losing context.
     assert(parentType.schema() === this.parentType.schema(), 'Should not try to normalize using a type from another schema');