Skip to content

Commit abddd09

Browse files
yaacovCRbenjie
yaacovCR
and
benjie
authored
Sibling errors should not be added after propagation (#4458)
Implementation of: graphql/graphql-spec#1184 @benjie --------- Co-authored-by: Benjie <benjie@jemjie.com>
1 parent d26810b commit abddd09

File tree

2 files changed

+193
-13
lines changed

2 files changed

+193
-13
lines changed

‎src/execution/__tests__/nonnull-test.ts‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ import { expect } from 'chai';
22
import { describe, it } from 'mocha';
33

44
import { expectJSON } from '../../__testUtils__/expectJSON';
5+
import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick';
6+
7+
import { invariant } from '../../jsutils/invariant';
58

69
import { parse } from '../../language/parser';
710

@@ -524,6 +527,143 @@ describe('Execute: handles non-nullable types', () => {
524527
});
525528
});
526529

530+
describe('Handles multiple errors for a single response position', () => {
531+
it('nullable and non-nullable root fields throw nested errors', async () => {
532+
const query = `
533+
{
534+
promiseNonNullNest {
535+
syncNonNull
536+
}
537+
promiseNest {
538+
syncNonNull
539+
}
540+
}
541+
`;
542+
const result = await executeQuery(query, throwingData);
543+
544+
expectJSON(result).toDeepEqual({
545+
data: null,
546+
errors: [
547+
{
548+
message: syncNonNullError.message,
549+
path: ['promiseNest', 'syncNonNull'],
550+
locations: [{ line: 7, column: 13 }],
551+
},
552+
{
553+
message: syncNonNullError.message,
554+
path: ['promiseNonNullNest', 'syncNonNull'],
555+
locations: [{ line: 4, column: 13 }],
556+
},
557+
],
558+
});
559+
});
560+
561+
it('a nullable root field throws a slower nested error after a non-nullable root field throws a nested error', async () => {
562+
const query = `
563+
{
564+
promiseNonNullNest {
565+
syncNonNull
566+
}
567+
promiseNest {
568+
promiseNonNull
569+
}
570+
}
571+
`;
572+
const result = await executeQuery(query, throwingData);
573+
574+
expectJSON(result).toDeepEqual({
575+
data: null,
576+
errors: [
577+
{
578+
message: syncNonNullError.message,
579+
path: ['promiseNonNullNest', 'syncNonNull'],
580+
locations: [{ line: 4, column: 13 }],
581+
},
582+
],
583+
});
584+
585+
// allow time for slower error to reject
586+
invariant(result.errors !== undefined);
587+
const initialErrors = [...result.errors];
588+
for (let i = 0; i < 5; i++) {
589+
// eslint-disable-next-line no-await-in-loop
590+
await resolveOnNextTick();
591+
}
592+
expectJSON(initialErrors).toDeepEqual(result.errors);
593+
});
594+
595+
it('nullable and non-nullable nested fields throw nested errors', async () => {
596+
const query = `
597+
{
598+
syncNest {
599+
promiseNonNullNest {
600+
syncNonNull
601+
}
602+
promiseNest {
603+
syncNonNull
604+
}
605+
}
606+
}
607+
`;
608+
const result = await executeQuery(query, throwingData);
609+
610+
expectJSON(result).toDeepEqual({
611+
data: { syncNest: null },
612+
errors: [
613+
{
614+
message: syncNonNullError.message,
615+
path: ['syncNest', 'promiseNest', 'syncNonNull'],
616+
locations: [{ line: 8, column: 15 }],
617+
},
618+
{
619+
message: syncNonNullError.message,
620+
path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'],
621+
locations: [{ line: 5, column: 15 }],
622+
},
623+
],
624+
});
625+
});
626+
627+
it('a nullable nested field throws a slower nested error after a non-nullable nested field throws a nested error', async () => {
628+
const query = `
629+
{
630+
syncNest {
631+
promiseNonNullNest {
632+
syncNonNull
633+
}
634+
promiseNest {
635+
promiseNest {
636+
promiseNest {
637+
promiseNonNull
638+
}
639+
}
640+
}
641+
}
642+
}
643+
`;
644+
const result = await executeQuery(query, throwingData);
645+
646+
expectJSON(result).toDeepEqual({
647+
data: { syncNest: null },
648+
errors: [
649+
{
650+
message: syncNonNullError.message,
651+
path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'],
652+
locations: [{ line: 5, column: 15 }],
653+
},
654+
],
655+
});
656+
657+
invariant(result.errors !== undefined);
658+
const initialErrors = [...result.errors];
659+
for (let i = 0; i < 20; i++) {
660+
// eslint-disable-next-line no-await-in-loop
661+
await resolveOnNextTick();
662+
}
663+
expectJSON(initialErrors).toDeepEqual(result.errors);
664+
});
665+
});
666+
527667
describe('Handles non-null argument', () => {
528668
const schemaWithNonNullArg = new GraphQLSchema({
529669
query: new GraphQLObjectType({

‎src/execution/execute.ts‎

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,46 @@ export interface ExecutionContext {
114114
fieldResolver: GraphQLFieldResolver<any, any>;
115115
typeResolver: GraphQLTypeResolver<any, any>;
116116
subscribeFieldResolver: GraphQLFieldResolver<any, any>;
117-
errors: Array<GraphQLError>;
117+
collectedErrors: CollectedErrors;
118+
}
119+
120+
/**
121+
* @internal
122+
*/
123+
class CollectedErrors {
124+
private _errorPositions: Set<Path | undefined>;
125+
private _errors: Array<GraphQLError>;
126+
constructor() {
127+
this._errorPositions = new Set<Path | undefined>();
128+
this._errors = [];
129+
}
130+
131+
get errors(): ReadonlyArray<GraphQLError> {
132+
return this._errors;
133+
}
134+
135+
add(error: GraphQLError, path: Path | undefined) {
136+
// Do not modify errors list if the execution position for this error or
137+
// any of its ancestors has already been nulled via error propagation.
138+
// This check should be unnecessary for implementations able to implement
139+
// actual cancellation.
140+
if (this._hasNulledPosition(path)) {
141+
return;
142+
}
143+
this._errorPositions.add(path);
144+
this._errors.push(error);
145+
}
146+
147+
private _hasNulledPosition(startPath: Path | undefined): boolean {
148+
let path = startPath;
149+
while (path !== undefined) {
150+
if (this._errorPositions.has(path)) {
151+
return true;
152+
}
153+
path = path.prev;
154+
}
155+
return this._errorPositions.has(undefined);
156+
}
118157
}
119158

120159
/**
@@ -206,17 +245,17 @@ export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
206245
const result = executeOperation(exeContext, operation, rootValue);
207246
if (isPromise(result)) {
208247
return result.then(
209-
(data) => buildResponse(data, exeContext.errors),
248+
(data) => buildResponse(data, exeContext.collectedErrors.errors),
210249
(error) => {
211-
exeContext.errors.push(error);
212-
return buildResponse(null, exeContext.errors);
250+
exeContext.collectedErrors.add(error, undefined);
251+
return buildResponse(null, exeContext.collectedErrors.errors);
213252
},
214253
);
215254
}
216-
return buildResponse(result, exeContext.errors);
255+
return buildResponse(result, exeContext.collectedErrors.errors);
217256
} catch (error) {
218-
exeContext.errors.push(error);
219-
return buildResponse(null, exeContext.errors);
257+
exeContext.collectedErrors.add(error, undefined);
258+
return buildResponse(null, exeContext.collectedErrors.errors);
220259
}
221260
}
222261

@@ -352,7 +391,7 @@ export function buildExecutionContext(
352391
fieldResolver: fieldResolver ?? defaultFieldResolver,
353392
typeResolver: typeResolver ?? defaultTypeResolver,
354393
subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver,
355-
errors: [],
394+
collectedErrors: new CollectedErrors(),
356395
};
357396
}
358397

@@ -558,13 +597,13 @@ function executeField(
558597
// to take a second callback for the error case.
559598
return completed.then(undefined, (rawError) => {
560599
const error = locatedError(rawError, fieldNodes, pathToArray(path));
561-
return handleFieldError(error, returnType, exeContext);
600+
return handleFieldError(error, returnType, path, exeContext);
562601
});
563602
}
564603
return completed;
565604
} catch (rawError) {
566605
const error = locatedError(rawError, fieldNodes, pathToArray(path));
567-
return handleFieldError(error, returnType, exeContext);
606+
return handleFieldError(error, returnType, path, exeContext);
568607
}
569608
}
570609

@@ -597,6 +636,7 @@ export function buildResolveInfo(
597636
function handleFieldError(
598637
error: GraphQLError,
599638
returnType: GraphQLOutputType,
639+
path: Path,
600640
exeContext: ExecutionContext,
601641
): null {
602642
// If the field type is non-nullable, then it is resolved without any
@@ -607,7 +647,7 @@ function handleFieldError(
607647

608648
// Otherwise, error protection is applied, logging the error and resolving
609649
// a null value for this field if one is encountered.
610-
exeContext.errors.push(error);
650+
exeContext.collectedErrors.add(error, path);
611651
return null;
612652
}
613653

@@ -779,13 +819,13 @@ function completeListValue(
779819
fieldNodes,
780820
pathToArray(itemPath),
781821
);
782-
return handleFieldError(error, itemType, exeContext);
822+
return handleFieldError(error, itemType, itemPath, exeContext);
783823
});
784824
}
785825
return completedItem;
786826
} catch (rawError) {
787827
const error = locatedError(rawError, fieldNodes, pathToArray(itemPath));
788-
return handleFieldError(error, itemType, exeContext);
828+
return handleFieldError(error, itemType, itemPath, exeContext);
789829
}
790830
});
791831

0 commit comments

Comments
 (0)