Skip to content

incremental: always only emit single completion error for deferred fragment #4092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/execution/IncrementalPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,13 +508,16 @@ class IncrementalPublisher {
) {
for (const deferredFragmentRecord of deferredGroupedFieldSetResult.deferredFragmentRecords) {
const id = deferredFragmentRecord.id;
if (id !== undefined) {
this._completed.push({
id,
errors: deferredGroupedFieldSetResult.errors,
});
this._pending.delete(deferredFragmentRecord);
// This can occur if multiple deferred grouped field sets error for a fragment.
if (!this._pending.has(deferredFragmentRecord)) {
continue;
}
invariant(id !== undefined);
this._completed.push({
id,
errors: deferredGroupedFieldSetResult.errors,
});
this._pending.delete(deferredFragmentRecord);
}
return;
}
Expand All @@ -535,8 +538,12 @@ class IncrementalPublisher {
// TODO: add test case for this.
// Presumably, this can occur if an error causes a fragment to be completed early,
// while an asynchronous deferred grouped field set result is enqueued.
// Presumably, this can also occur if multiple deferred fragments are executed
// early and share a deferred grouped field set. This could be worked around
// another way, such as by checking whether the completedResultQueue already
// contains the completedResult prior to pushing.
/* c8 ignore next 3 */
if (id === undefined) {
if (!this._pending.has(deferredFragmentRecord)) {
continue;
}
const reconcilableResults = deferredFragmentRecord.reconcilableResults;
Expand All @@ -546,6 +553,7 @@ class IncrementalPublisher {
) {
continue;
}
invariant(id !== undefined);
for (const reconcilableResult of reconcilableResults) {
if (reconcilableResult.sent) {
continue;
Expand Down
146 changes: 146 additions & 0 deletions src/execution/__tests__/defer-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,152 @@ describe('Execute: defer directive', () => {
]);
});

it('Handles multiple erroring deferred grouped field sets', async () => {
const document = parse(`
query {
... @defer {
a {
b {
c {
someError: nonNullErrorField
}
}
}
}
... @defer {
a {
b {
c {
anotherError: nonNullErrorField
}
}
}
}
}
`);
const result = await complete(document, {
a: {
b: { c: { nonNullErrorField: null } },
},
});
expectJSON(result).toDeepEqual([
{
data: {},
pending: [
{ id: '0', path: [] },
{ id: '1', path: [] },
],
hasNext: true,
},
{
completed: [
{
id: '0',
errors: [
{
message:
'Cannot return null for non-nullable field c.nonNullErrorField.',
locations: [{ line: 7, column: 17 }],
path: ['a', 'b', 'c', 'someError'],
},
],
},
{
id: '1',
errors: [
{
message:
'Cannot return null for non-nullable field c.nonNullErrorField.',
locations: [{ line: 16, column: 17 }],
path: ['a', 'b', 'c', 'anotherError'],
},
],
},
],
hasNext: false,
},
]);
});

it('Handles multiple erroring deferred grouped field sets for the same fragment', async () => {
const document = parse(`
query {
... @defer {
a {
b {
someC: c {
d: d
}
anotherC: c {
d: d
}
}
}
}
... @defer {
a {
b {
someC: c {
someError: nonNullErrorField
}
anotherC: c {
anotherError: nonNullErrorField
}
}
}
}
}
`);
const result = await complete(document, {
a: {
b: { c: { d: 'd', nonNullErrorField: null } },
},
});
expectJSON(result).toDeepEqual([
{
data: {},
pending: [
{ id: '0', path: [] },
{ id: '1', path: [] },
],
hasNext: true,
},
{
incremental: [
{
data: { a: { b: { someC: {}, anotherC: {} } } },
id: '0',
},
{
data: { d: 'd' },
id: '0',
subPath: ['a', 'b', 'someC'],
},
{
data: { d: 'd' },
id: '0',
subPath: ['a', 'b', 'anotherC'],
},
],
completed: [
{
id: '1',
errors: [
{
message:
'Cannot return null for non-nullable field c.nonNullErrorField.',
locations: [{ line: 19, column: 17 }],
path: ['a', 'b', 'someC', 'someError'],
},
],
},
{ id: '0' },
],
hasNext: false,
},
]);
});

it('filters a payload with a null that cannot be merged', async () => {
const document = parse(`
query {
Expand Down
Loading