Fix incorrect removal of fragments only used by other fragments (#2648)

The patch from #2628, while it did fix the issue of leaving unused
fragments, mistakenly introduced a regression in that fragments
that are used by other fragments but are not used in the query
selection are now removed, which is obviously undesirable.

This was simply because the patch of #2628 only counted the usages
in the main selection, not the ones other fragments as it should
have. This commit fixes that.

Author: Sylvain Lebresne

.changeset/stale-birds-think.md

@@ -0,0 +1,7 @@
+---
+"@apollo/federation-internals": patch
+---
+
+Fix regression in named fragment reuse introduced by 2.4.8 that caused fragments that were only used by other fragments
+to not be reused, even if they are making the overall query smaller and thus should be reused.
+  

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

@@ -1904,6 +1904,122 @@ describe('fragments optimization', () => {
       }
     `);
   });
+
+  test('keeps fragments only used by other fragments (if they are used enough times)', () => {
+    const schema = parseSchema(`
+      type Query {
+        t1: T
+        t2: T
+      }
+
+      type T {
+        a1: Int
+        a2: Int
+        b1: B
+        b2: B
+      }
+
+      type B {
+        x: Int
+        y: Int
+      }
+    `);
+    const gqlSchema = schema.toGraphQLJSSchema();
+
+    const operation = parseOperation(schema, `
+      query {
+        t1 {
+          ...TFields
+        }
+        t2 {
+          ...TFields
+        }
+      }
+
+      fragment TFields on T {
+        ...DirectFieldsOfT
+        b1 {
+          ...BFields
+        }
+        b2 {
+          ...BFields
+        }
+      }
+
+      fragment DirectFieldsOfT on T {
+        a1
+        a2
+      }
+
+      fragment BFields on B {
+        x
+        y
+      }
+    `);
+    expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]);
+
+    const withoutFragments = operation.expandAllFragments();
+    expect(withoutFragments.toString()).toMatchString(`
+      {
+        t1 {
+          a1
+          a2
+          b1 {
+            x
+            y
+          }
+          b2 {
+            x
+            y
+          }
+        }
+        t2 {
+          a1
+          a2
+          b1 {
+            x
+            y
+          }
+          b2 {
+            x
+            y
+          }
+        }
+      }
+    `);
+
+    const optimized = withoutFragments.optimize(operation.fragments!, 2);
+    expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]);
+
+    // The `DirectFieldsOfT` fragments should not be kept as it is used only once within `TFields`,
+    // but the `BFields` one should be kept.
+    expect(optimized.toString()).toMatchString(`
+      fragment BFields on B {
+        x
+        y
+      }
+ 
+      fragment TFields on T {
+        a1
+        a2
+        b1 {
+          ...BFields
+        }
+        b2 {
+          ...BFields
+        }
+      }
+
+      {
+        t1 {
+          ...TFields
+        }
+        t2 {
+          ...TFields
+        }
+      }
+    `);
+  });
 });
 
 describe('validations', () => {

internals-js/src/operations.ts

@@ -946,7 +946,9 @@ export class Operation {
       // refactor all this later to avoid this case without additional complexity.
       if (finalFragments) {
         const usages = new Map<string, number>();
+        // Collecting all usages, both in the selection and within other fragments.
         optimizedSelection.collectUsedFragmentNames(usages);
+        finalFragments.collectUsedFragmentNames(usages);
         finalFragments = finalFragments.filter((f) => (usages.get(f.name) ?? 0) > 0);
       }
     }
@@ -1284,6 +1286,15 @@ export class NamedFragments {
     return this.fragments.values();
   }
 
+  /**
+   * Collect the usages of fragments that are used within the selection of other fragments.
+   */
+  collectUsedFragmentNames(collector: Map<string, number>) {
+    for (const fragment of this.definitions()) {
+      fragment.collectUsedFragmentNames(collector);
+    }
+  }
+
   map(mapper: (def: NamedFragmentDefinition) => NamedFragmentDefinition): NamedFragments {
     const mapped = new NamedFragments();
     for (const def of this.fragments.values()) {