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(); + // 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) { + 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()) {