From 02eab3ac4a0514bef8f9253a9e43418ba1c17843 Mon Sep 17 00:00:00 2001
From: Sylvain Lebresne <sylvain@apollographql.com>
Date: Mon, 3 Jul 2023 10:56:51 +0200
Subject: [PATCH] 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.
---
 .changeset/stale-birds-think.md               |   7 ++
 internals-js/src/__tests__/operations.test.ts | 116 ++++++++++++++++++
 internals-js/src/operations.ts                |  11 ++
 3 files changed, 134 insertions(+)
 create mode 100644 .changeset/stale-birds-think.md

diff --git a/.changeset/stale-birds-think.md b/.changeset/stale-birds-think.md
new file mode 100644
index 0000000000..cc02234cce
--- /dev/null
+++ b/.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.
+  
\ No newline at end of file
diff --git a/internals-js/src/__tests__/operations.test.ts b/internals-js/src/__tests__/operations.test.ts
index 9d4ac9be05..455e2f3a0c 100644
--- a/internals-js/src/__tests__/operations.test.ts
+++ b/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', () => {
diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts
index 2a6b4406f1..e80e689350 100644
--- a/internals-js/src/operations.ts
+++ b/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()) {