From b51420d5863f5a4895c5e4bc1cdfba1c00e8a1c4 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 15 Jul 2025 16:25:31 +0300 Subject: [PATCH 1/5] add test documenting current behavior --- src/execution/__tests__/nonnull-test.ts | 169 ++++++++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index 427f2a64d6..12bbe45910 100644 --- a/src/execution/__tests__/nonnull-test.ts +++ b/src/execution/__tests__/nonnull-test.ts @@ -13,6 +13,8 @@ import { buildSchema } from '../../utilities/buildASTSchema'; import type { ExecutionResult } from '../execute'; import { execute, executeSync } from '../execute'; +import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick'; +import { invariant } from '../../jsutils/invariant'; const syncError = new Error('sync'); const syncNonNullError = new Error('syncNonNull'); @@ -524,6 +526,173 @@ describe('Execute: handles non-nullable types', () => { }); }); + describe('Handles multiple errors for a single response position', () => { + it('nullable and non-nullable root fields throw nested errors', async () => { + const query = ` + { + promiseNonNullNest { + syncNonNull + } + promiseNest { + syncNonNull + } + } + `; + const result = await executeQuery(query, throwingData); + + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: syncNonNullError.message, + path: ['promiseNest', 'syncNonNull'], + locations: [{ line: 7, column: 13 }], + }, + { + message: syncNonNullError.message, + path: ['promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 4, column: 13 }], + }, + ], + }); + }); + + it('a nullable root field throws a slower nested error after a non-nullable root field throws a nested error', async () => { + const query = ` + { + promiseNonNullNest { + syncNonNull + } + promiseNest { + promiseNonNull + } + } + `; + const result = await executeQuery(query, throwingData); + + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: syncNonNullError.message, + path: ['promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 4, column: 13 }], + }, + ], + }); + + // allow time for slower error to reject + await resolveOnNextTick(); + await resolveOnNextTick(); + + // result silently modified after operation completes! + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: syncNonNullError.message, + path: ['promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 4, column: 13 }], + }, + { + message: promiseNonNullError.message, + path: ['promiseNest', 'promiseNonNull'], + locations: [{ line: 7, column: 13 }], + }, + ], + }); + }); + + it('nullable and non-nullable nested fields throw nested errors', async () => { + const query = ` + { + syncNest { + promiseNonNullNest { + syncNonNull + } + promiseNest { + syncNonNull + } + } + } + `; + const result = await executeQuery(query, throwingData); + + expectJSON(result).toDeepEqual({ + data: { syncNest: null }, + errors: [ + { + message: syncNonNullError.message, + path: ['syncNest', 'promiseNest', 'syncNonNull'], + locations: [{ line: 8, column: 15 }], + }, + { + message: syncNonNullError.message, + path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 5, column: 15 }], + }, + ], + }); + }); + + it('a nullable nested field throws a slower nested error after a non-nullable nested field throws a nested error', async () => { + const query = ` + { + syncNest { + promiseNonNullNest { + syncNonNull + } + promiseNest { + promiseNest { + promiseNest { + promiseNonNull + } + } + } + } + } + `; + const result = await executeQuery(query, throwingData); + + expectJSON(result).toDeepEqual({ + data: { syncNest: null }, + errors: [ + { + message: syncNonNullError.message, + path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 5, column: 15 }], + }, + ], + }); + + // allow time for slower error to reject + await resolveOnNextTick(); + + // result silently modified after operation completes! + expectJSON(result).toDeepEqual({ + data: { syncNest: null }, + errors: [ + { + message: syncNonNullError.message, + path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'], + locations: [{ line: 5, column: 15 }], + }, + { + message: promiseNonNullError.message, + path: [ + 'syncNest', + 'promiseNest', + 'promiseNest', + 'promiseNest', + 'promiseNonNull', + ], + locations: [{ line: 10, column: 19 }], + }, + ], + }); + }); + }); + describe('Handles non-null argument', () => { const schemaWithNonNullArg = new GraphQLSchema({ query: new GraphQLObjectType({ From 5c2864c615510aa210704a7d5013fea74fb7fefa Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 15 Jul 2025 16:54:34 +0300 Subject: [PATCH 2/5] exclude errors with already nulled positions --- src/execution/__tests__/nonnull-test.ts | 63 +++++++------------------ src/execution/execute.ts | 43 +++++++++++++---- 2 files changed, 52 insertions(+), 54 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index 12bbe45910..bb0de995a6 100644 --- a/src/execution/__tests__/nonnull-test.ts +++ b/src/execution/__tests__/nonnull-test.ts @@ -2,6 +2,9 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON'; +import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick'; + +import { invariant } from '../../jsutils/invariant'; import { parse } from '../../language/parser'; @@ -13,8 +16,6 @@ import { buildSchema } from '../../utilities/buildASTSchema'; import type { ExecutionResult } from '../execute'; import { execute, executeSync } from '../execute'; -import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick'; -import { invariant } from '../../jsutils/invariant'; const syncError = new Error('sync'); const syncNonNullError = new Error('syncNonNull'); @@ -582,25 +583,13 @@ describe('Execute: handles non-nullable types', () => { }); // allow time for slower error to reject - await resolveOnNextTick(); - await resolveOnNextTick(); - - // result silently modified after operation completes! - expectJSON(result).toDeepEqual({ - data: null, - errors: [ - { - message: syncNonNullError.message, - path: ['promiseNonNullNest', 'syncNonNull'], - locations: [{ line: 4, column: 13 }], - }, - { - message: promiseNonNullError.message, - path: ['promiseNest', 'promiseNonNull'], - locations: [{ line: 7, column: 13 }], - }, - ], - }); + invariant(result.errors !== undefined); + const initialErrors = [...result.errors]; + for (let i = 0; i < 5; i++) { + // eslint-disable-next-line no-await-in-loop + await resolveOnNextTick(); + } + expectJSON(initialErrors).toDeepEqual(result.errors); }); it('nullable and non-nullable nested fields throw nested errors', async () => { @@ -665,31 +654,13 @@ describe('Execute: handles non-nullable types', () => { ], }); - // allow time for slower error to reject - await resolveOnNextTick(); - - // result silently modified after operation completes! - expectJSON(result).toDeepEqual({ - data: { syncNest: null }, - errors: [ - { - message: syncNonNullError.message, - path: ['syncNest', 'promiseNonNullNest', 'syncNonNull'], - locations: [{ line: 5, column: 15 }], - }, - { - message: promiseNonNullError.message, - path: [ - 'syncNest', - 'promiseNest', - 'promiseNest', - 'promiseNest', - 'promiseNonNull', - ], - locations: [{ line: 10, column: 19 }], - }, - ], - }); + invariant(result.errors !== undefined); + const initialErrors = [...result.errors]; + for (let i = 0; i < 20; i++) { + // eslint-disable-next-line no-await-in-loop + await resolveOnNextTick(); + } + expectJSON(initialErrors).toDeepEqual(result.errors); }); }); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 3021354e28..bed2114c63 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -114,6 +114,7 @@ export interface ExecutionContext { fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; + errorPositions: Set; errors: Array; } @@ -208,15 +209,19 @@ export function execute(args: ExecutionArgs): PromiseOrValue { return result.then( (data) => buildResponse(data, exeContext.errors), (error) => { - exeContext.errors.push(error); - return buildResponse(null, exeContext.errors); + const { errorPositions, errors } = exeContext; + errorPositions.add(undefined); + errors.push(error); + return buildResponse(null, errors); }, ); } return buildResponse(result, exeContext.errors); } catch (error) { - exeContext.errors.push(error); - return buildResponse(null, exeContext.errors); + const { errorPositions, errors } = exeContext; + errorPositions.add(undefined); + errors.push(error); + return buildResponse(null, errors); } } @@ -352,6 +357,7 @@ export function buildExecutionContext( fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, + errorPositions: new Set(), errors: [], }; } @@ -558,13 +564,13 @@ function executeField( // to take a second callback for the error case. return completed.then(undefined, (rawError) => { const error = locatedError(rawError, fieldNodes, pathToArray(path)); - return handleFieldError(error, returnType, exeContext); + return handleFieldError(error, returnType, path, exeContext); }); } return completed; } catch (rawError) { const error = locatedError(rawError, fieldNodes, pathToArray(path)); - return handleFieldError(error, returnType, exeContext); + return handleFieldError(error, returnType, path, exeContext); } } @@ -597,6 +603,7 @@ export function buildResolveInfo( function handleFieldError( error: GraphQLError, returnType: GraphQLOutputType, + path: Path, exeContext: ExecutionContext, ): null { // If the field type is non-nullable, then it is resolved without any @@ -605,12 +612,32 @@ function handleFieldError( throw error; } + // Do not modify errors list if a response position for this error has already been nulled. + const errorPositions = exeContext.errorPositions; + if (hasNulledPosition(errorPositions, path)) { + return null; + } + errorPositions.add(path); + // Otherwise, error protection is applied, logging the error and resolving // a null value for this field if one is encountered. exeContext.errors.push(error); return null; } +function hasNulledPosition( + errorPositions: Set, + path: Path | undefined, +): boolean { + if (errorPositions.has(path)) { + return true; + } + if (path === undefined) { + return false; + } + return hasNulledPosition(errorPositions, path.prev); +} + /** * Implements the instructions for completeValue as defined in the * "Value Completion" section of the spec. @@ -779,13 +806,13 @@ function completeListValue( fieldNodes, pathToArray(itemPath), ); - return handleFieldError(error, itemType, exeContext); + return handleFieldError(error, itemType, itemPath, exeContext); }); } return completedItem; } catch (rawError) { const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - return handleFieldError(error, itemType, exeContext); + return handleFieldError(error, itemType, itemPath, exeContext); } }); From 8893d1571048f9beefad437c8ae39d61a0b1952e Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 23 Dec 2025 13:14:54 +0200 Subject: [PATCH 3/5] review feedback to avoid recursion Co-authored-by: Benjie --- src/execution/execute.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index bed2114c63..c14abf0c3f 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -627,15 +627,16 @@ function handleFieldError( function hasNulledPosition( errorPositions: Set, - path: Path | undefined, + startPath: Path | undefined, ): boolean { - if (errorPositions.has(path)) { - return true; - } - if (path === undefined) { - return false; + let path = startPath; + while (path !== undefined) { + if (errorPositions.has(path)) { + return true; + } + path = path.prev; } - return hasNulledPosition(errorPositions, path.prev); + return errorPositions.has(undefined); } /** From 76f9a8d6e058fbe37d68fa31fc7038a3eed71522 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 23 Dec 2025 14:32:13 +0200 Subject: [PATCH 4/5] add CollectedErrors utility class --- src/execution/execute.ts | 79 ++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index c14abf0c3f..d23d306d85 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -114,8 +114,41 @@ export interface ExecutionContext { fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; - errorPositions: Set; - errors: Array; + collectedErrors: CollectedErrors; +} + +/** + * @internal + */ +class CollectedErrors { + private _errorPositions: Set; + private _errors: Array; + constructor() { + this._errorPositions = new Set(); + this._errors = []; + } + get errors(): ReadonlyArray { + return this._errors; + } + add(error: GraphQLError, path: Path | undefined) { + // Do not modify errors list if a response position for this error has already been nulled. + // This check is unnecessary for implementations able to implement proper cancellation. + if (this._hasNulledPosition(path)) { + return; + } + this._errorPositions.add(path); + this._errors.push(error); + } + private _hasNulledPosition(startPath: Path | undefined): boolean { + let path = startPath; + while (path !== undefined) { + if (this._errorPositions.has(path)) { + return true; + } + path = path.prev; + } + return this._errorPositions.has(undefined); + } } /** @@ -207,21 +240,17 @@ export function execute(args: ExecutionArgs): PromiseOrValue { const result = executeOperation(exeContext, operation, rootValue); if (isPromise(result)) { return result.then( - (data) => buildResponse(data, exeContext.errors), + (data) => buildResponse(data, exeContext.collectedErrors.errors), (error) => { - const { errorPositions, errors } = exeContext; - errorPositions.add(undefined); - errors.push(error); - return buildResponse(null, errors); + exeContext.collectedErrors.add(error, undefined); + return buildResponse(null, exeContext.collectedErrors.errors); }, ); } - return buildResponse(result, exeContext.errors); + return buildResponse(result, exeContext.collectedErrors.errors); } catch (error) { - const { errorPositions, errors } = exeContext; - errorPositions.add(undefined); - errors.push(error); - return buildResponse(null, errors); + exeContext.collectedErrors.add(error, undefined); + return buildResponse(null, exeContext.collectedErrors.errors); } } @@ -357,8 +386,7 @@ export function buildExecutionContext( fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, - errorPositions: new Set(), - errors: [], + collectedErrors: new CollectedErrors(), }; } @@ -612,33 +640,12 @@ function handleFieldError( throw error; } - // Do not modify errors list if a response position for this error has already been nulled. - const errorPositions = exeContext.errorPositions; - if (hasNulledPosition(errorPositions, path)) { - return null; - } - errorPositions.add(path); - // Otherwise, error protection is applied, logging the error and resolving // a null value for this field if one is encountered. - exeContext.errors.push(error); + exeContext.collectedErrors.add(error, path); return null; } -function hasNulledPosition( - errorPositions: Set, - startPath: Path | undefined, -): boolean { - let path = startPath; - while (path !== undefined) { - if (errorPositions.has(path)) { - return true; - } - path = path.prev; - } - return errorPositions.has(undefined); -} - /** * Implements the instructions for completeValue as defined in the * "Value Completion" section of the spec. From 00fc696da7732fdf77865841218e9600b731e80a Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 23 Dec 2025 16:12:25 +0200 Subject: [PATCH 5/5] lint --- src/execution/execute.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index d23d306d85..83165fc266 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -127,9 +127,11 @@ class CollectedErrors { this._errorPositions = new Set(); this._errors = []; } + get errors(): ReadonlyArray { return this._errors; } + add(error: GraphQLError, path: Path | undefined) { // Do not modify errors list if a response position for this error has already been nulled. // This check is unnecessary for implementations able to implement proper cancellation. @@ -139,6 +141,7 @@ class CollectedErrors { this._errorPositions.add(path); this._errors.push(error); } + private _hasNulledPosition(startPath: Path | undefined): boolean { let path = startPath; while (path !== undefined) {