Skip to content

Commit

Permalink
Expands over-eager merging of field fix to handle @defer consistent…
Browse files Browse the repository at this point in the history 
…ly (#2720)

The previously committed [#2713](#2713) fixed an issue introduced by
[#2387](#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](#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](#2387).
  • Loading branch information
Sylvain Lebresne authored Aug 15, 2023
1 parent 93988db commit 1add932
Show file tree
Hide file tree
Showing 3 changed files with 348 additions and 22 deletions.
41 changes: 41 additions & 0 deletions .changeset/sixty-walls-chew.md
View file
Original file line number Diff line number Diff line change
@@ -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).

318 changes: 300 additions & 18 deletions internals-js/src/__tests__/operations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, `
Expand Down Expand Up @@ -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', () => {
Expand Down
Loading

0 comments on commit 1add932

Please sign in to comment.