From 534f5c2cd5110b8fa3602da51df95d326dd37e51 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Mon, 7 Aug 2023 12:02:05 +0200 Subject: [PATCH] Expands over-eager merging of field fix to handle `@defer` consistently The previously committed [#2713](https://github.com/apollographql/federation/pull/2713) fixed an issue introduced by [#2387](https://github.com/apollographql/federation/pull/2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [#2713](https://github.com/apollographql/federation/pull/2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [#2387](https://github.com/apollographql/federation/pull/2387). --- .changeset/sixty-walls-chew.md | 41 +++ internals-js/src/__tests__/operations.test.ts | 318 +++++++++++++++++- internals-js/src/operations.ts | 11 +- 3 files changed, 348 insertions(+), 22 deletions(-) create mode 100644 .changeset/sixty-walls-chew.md diff --git a/.changeset/sixty-walls-chew.md b/.changeset/sixty-walls-chew.md new file mode 100644 index 0000000000..d60b505e5f --- /dev/null +++ b/.changeset/sixty-walls-chew.md @@ -0,0 +1,41 @@ +--- +"@apollo/federation-internals": patch +--- + +Expands over-eager merging of field fix to handle `@defer` consistently + +The previously committed [#2713](https://github.com/apollographql/federation/pull/2713) fixed an issue introduced by +[#2387](https://github.com/apollographql/federation/pull/2387), ensuring that querying the same field with different +directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly +differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments +where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: +```graphq +query Test($skipField: Boolean!) { + x { + ... on X @defer { + a + } + ... on X @defer { + b + } + } +} +``` +the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following +[#2713](https://github.com/apollographql/federation/pull/2713), query: +```graphq +query Test($skipField: Boolean!) { + x @defer { + a + } + x @defer { + b + } +} +``` +_will_ merge both `x @defer`, resulting in a single deferred section. + +This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, +consistently with both 1) the case of fragments and 2) the behaviour prior to +[#2387](https://github.com/apollographql/federation/pull/2387). + \ 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 512860de8d..a6093dd771 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -2476,6 +2476,8 @@ describe('basic operations', () => { b1: Int b2: T } + + directive @customSkip(if: Boolean!, label: String!) on FIELD | INLINE_FRAGMENT `); const operation = parseOperation(schema, ` @@ -2521,29 +2523,309 @@ describe('basic operations', () => { ]); }) - test('fields are keyed on both name and directive applications', () => { - const operation = operationFromDocument(schema, gql` - query Test($skipIf: Boolean!) { - t { - v1 + describe('same field merging', () => { + test('do merge when same field and no directive', () => { + const operation = operationFromDocument(schema, gql` + query Test { + t { + v1 + } + t { + v2 + } } - t @skip(if: $skipIf) { - v2 + `); + + expect(operation.toString()).toMatchString(` + query Test { + t { + v1 + v2 + } } - } - `); + `); + }); - expect(operation.toString()).toMatchString(` - query Test($skipIf: Boolean!) { - t { - v1 + test('do merge when both have the _same_ directive', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t @skip(if: $skipIf) { + v1 + } + t @skip(if: $skipIf) { + v2 + } } - t @skip(if: $skipIf) { - v2 + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t @skip(if: $skipIf) { + v1 + v2 + } } - } - `); - }) + `); + }); + + test('do merge when both have the _same_ directive, even if argument order differs', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t @customSkip(if: $skipIf, label: "foo") { + v1 + } + t @customSkip(label: "foo", if: $skipIf) { + v2 + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t @customSkip(if: $skipIf, label: "foo") { + v1 + v2 + } + } + `); + }); + + test('do not merge when one has a directive and the other do not', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t { + v1 + } + t @skip(if: $skipIf) { + v2 + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t { + v1 + } + t @skip(if: $skipIf) { + v2 + } + } + `); + }); + + test('do not merge when both have _differing_ directives', () => { + const operation = operationFromDocument(schema, gql` + query Test($skip1: Boolean!, $skip2: Boolean!) { + t @skip(if: $skip1) { + v1 + } + t @skip(if: $skip2) { + v2 + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skip1: Boolean!, $skip2: Boolean!) { + t @skip(if: $skip1) { + v1 + } + t @skip(if: $skip2) { + v2 + } + } + `); + }); + + test('do not merge @defer directive, even if applied the same way', () => { + const operation = operationFromDocument(schema, gql` + query Test { + t @defer { + v1 + } + t @defer { + v2 + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test { + t @defer { + v1 + } + t @defer { + v2 + } + } + `); + }); + }); + + describe('same fragment merging', () => { + test('do merge when same fragment and no directive', () => { + const operation = operationFromDocument(schema, gql` + query Test { + t { + ... on T { + v1 + } + ... on T { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test { + t { + ... on T { + v1 + v2 + } + } + } + `); + }); + + test('do merge when both have the _same_ directive', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t { + ... on T @skip(if: $skipIf) { + v1 + } + ... on T @skip(if: $skipIf) { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t { + ... on T @skip(if: $skipIf) { + v1 + v2 + } + } + } + `); + }); + + test('do merge when both have the _same_ directive, even if argument order differs', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t { + ... on T @customSkip(if: $skipIf, label: "foo") { + v1 + } + ... on T @customSkip(label: "foo", if: $skipIf) { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t { + ... on T @customSkip(if: $skipIf, label: "foo") { + v1 + v2 + } + } + } + `); + }); + + test('do not merge when one has a directive and the other do not', () => { + const operation = operationFromDocument(schema, gql` + query Test($skipIf: Boolean!) { + t { + ... on T { + v1 + } + ... on T @skip(if: $skipIf) { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skipIf: Boolean!) { + t { + ... on T { + v1 + } + ... on T @skip(if: $skipIf) { + v2 + } + } + } + `); + }); + + test('do not merge when both have _differing_ directives', () => { + const operation = operationFromDocument(schema, gql` + query Test($skip1: Boolean!, $skip2: Boolean!) { + t { + ... on T @skip(if: $skip1) { + v1 + } + ... on T @skip(if: $skip2) { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test($skip1: Boolean!, $skip2: Boolean!) { + t { + ... on T @skip(if: $skip1) { + v1 + } + ... on T @skip(if: $skip2) { + v2 + } + } + } + `); + }); + + test('do not merge @defer directive, even if applied the same way', () => { + const operation = operationFromDocument(schema, gql` + query Test { + t { + ... on T @defer { + v1 + } + ... on T @defer { + v2 + } + } + } + `); + + expect(operation.toString()).toMatchString(` + query Test { + t { + ... on T @defer { + v1 + } + ... on T @defer { + v2 + } + } + } + `); + }); + }); }); describe('MutableSelectionSet', () => { diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 0103382d99..80c0ef4201 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -113,6 +113,10 @@ abstract class AbstractOperationElement> e } } } + + protected keyForDirectives(): string { + return this.appliedDirectives.map((d) => keyForDirective(d)).join(' '); + } } export class Field extends AbstractOperationElement> { @@ -146,7 +150,7 @@ export class Field ex } key(): string { - return this.responseName() + this.appliedDirectivesToString(); + return this.responseName() + this.keyForDirectives(); } asPathElement(): string { @@ -394,7 +398,7 @@ export class Field ex * 2. we sort the argument (by their name) before converting them to string, since argument order does not matter in graphQL. */ function keyForDirective( - directive: Directive, + directive: Directive>, directivesNeverEqualToThemselves: string[] = [ 'defer' ], ): string { if (directivesNeverEqualToThemselves.includes(directive.name)) { @@ -436,8 +440,7 @@ export class FragmentElement extends AbstractOperationElement { if (!this.computedKey) { // The key is such that 2 fragments with the same key within a selection set gets merged together. So the type-condition // is include, but so are the directives. - const keyForDirectives = this.appliedDirectives.map((d) => keyForDirective(d)).join(' '); - this.computedKey = '...' + (this.typeCondition ? ' on ' + this.typeCondition.name : '') + keyForDirectives; + this.computedKey = '...' + (this.typeCondition ? ' on ' + this.typeCondition.name : '') + this.keyForDirectives(); } return this.computedKey; }