From 13d5464ec985919e9b2bc0a591fca7838312689e Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 15 Jul 2025 16:25:31 +0300 Subject: [PATCH 1/2] 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 db63cc53f11ea66cdfed2ed5b4fb6d9524f9d84e Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 15 Jul 2025 16:54:34 +0300 Subject: [PATCH 2/2] 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); } });