Avoid leaving unused fragments (#2628)

Some interaction of the code that re-expand named fragments that are
used only once and the normalization we code post expansion could leave
some fragment unused, yet included in the list of fragment, which
ultimately led to an invalid query. This commit make sure we don't
include such unused fragments.

Author: Sylvain Lebresne

.changeset/fresh-ants-add.md

@@ -0,0 +1,6 @@
+---
+"@apollo/federation-internals": patch
+---
+
+Fix issue where subgraph fetches may have unused fragments (and are thus invalid).
+  

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

@@ -1401,6 +1401,97 @@ describe('fragments optimization', () => {
       `);
     });
   });
+
+  test('does not leave unused fragments', () => {
+    const schema = parseSchema(`
+      type Query {
+        t1: T1
+      }
+
+      union U1 = T1 | T2 | T3
+      union U2 =      T2 | T3
+
+      type T1 {
+        x: Int
+      }
+
+      type T2 {
+        y: Int
+      }
+
+      type T3 {
+        z: Int
+      }
+    `);
+    const gqlSchema = schema.toGraphQLJSSchema();
+
+    const operation = parseOperation(schema, `
+      query {
+        t1 {
+          ...Outer
+        }
+      }
+
+      fragment Outer on U1 {
+        ... on T1 {
+          x
+        }
+        ... on T2 {
+          ... Inner
+        }
+        ... on T3 {
+          ... Inner
+        }
+      }
+
+      fragment Inner on U2 {
+        ... on T2 {
+          y
+        }
+      }
+    `);
+    expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]);
+
+    const withoutFragments = operation.expandAllFragments();
+    expect(withoutFragments.toString()).toMatchString(`
+      {
+        t1 {
+          x
+        }
+      }
+    `);
+
+    // This is a bit of contrived example, but the reusing code will be able
+    // to figure out that the `Outer` fragment can be reused and will initially
+    // do so, but it's only use once, so it will expand it, which yields:
+    // {
+    //   t1 {
+    //     ... on T1 {
+    //       x
+    //     }
+    //     ... on T2 {
+    //       ... Inner
+    //     }
+    //     ... on T3 {
+    //       ... Inner
+    //     }
+    //   }
+    // }
+    // and so `Inner` will not be expanded (it's used twice). Except that
+    // the `normalize` code is apply then and will _remove_ both instances
+    // of `.... Inner`. Which is ok, but we must make sure the fragment
+    // itself is removed since it is not used now, which this test ensures.
+    const optimized = withoutFragments.optimize(operation.fragments!, 2);
+    expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]);
+
+    expect(optimized.toString()).toMatchString(`
+      {
+        t1 {
+          x
+        }
+      }
+    `);
+  });
 });
 
 describe('validations', () => {

internals-js/src/operations.ts

@@ -910,7 +910,7 @@ export class Operation {
       return this;
     }
 
-    const finalFragments = computeFragmentsToKeep(optimizedSelection, fragments, minUsagesToOptimize);
+    let finalFragments = computeFragmentsToKeep(optimizedSelection, fragments, minUsagesToOptimize);
 
     // If there is fragment usages and we're not keeping all fragments, we need to expand fragments.
     if (finalFragments !== null && finalFragments?.size !== fragments.size) {
@@ -923,6 +923,21 @@ export class Operation {
       // Expanding fragments could create some "inefficiencies" that we wouldn't have if we hadn't re-optimized
       // the fragments to de-optimize it later, so we do a final "normalize" pass to remove those.
       optimizedSelection = optimizedSelection.normalize({ parentType: optimizedSelection.parentType });
+
+      // And if we've expanded some fragments but kept others, then it's not 100% impossible that some
+      // fragment was used multiple times in some expanded fragment(s), but that post-expansion all of
+      // it's usages are "dead" branches that are removed by the final `normalize`. In that case though,
+      // we need to ensure we don't include the now-unused fragment in the final list of fragments.
+      // TODO: remark that the same reasoning could leave a single instance of a fragment usage, so if
+      // we really really want to never have less than `minUsagesToOptimize`, we could do some loop of
+      // `expand then normalize` unless all fragments are provably used enough. We don't bother, because
+      // leaving this is not a huge deal and it's not worth the complexity, but it could be that we can
+      // refactor all this later to avoid this case without additional complexity.
+      if (finalFragments) {
+        const usages = new Map<string, number>();
+        optimizedSelection.collectUsedFragmentNames(usages);
+        finalFragments = finalFragments.filter((f) => (usages.get(f.name) ?? 0) > 0);
+      }
     }
 
     return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined);