From 6bdb8b8cea23552de0d4675be453f06628250adf Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 28 Oct 2024 16:44:11 +0200 Subject: [PATCH] stop resolvers after execution ends addresses: #3792 --- src/execution/__tests__/nonnull-test.ts | 63 ++++++++++++++++++++ src/execution/execute.ts | 77 ++++++++++++++++++------- 2 files changed, 118 insertions(+), 22 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index ff08aafd73..6a321931b9 100644 --- a/src/execution/__tests__/nonnull-test.ts +++ b/src/execution/__tests__/nonnull-test.ts @@ -2,6 +2,7 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON.js'; +import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js'; import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; @@ -526,6 +527,68 @@ describe('Execute: handles non-nullable types', () => { }); }); + describe('cancellation with null bubbling', () => { + function nestedPromise(n: number): string { + return n > 0 ? `promiseNest { ${nestedPromise(n - 1)} }` : 'promise'; + } + + it('returns an single error without cancellation', async () => { + const query = ` + { + promiseNonNull, + ${nestedPromise(4)} + } + `; + + const result = await executeQuery(query, throwingData); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + // does not include syncNullError because result returns prior to it being added + { + message: 'promiseNonNull', + path: ['promiseNonNull'], + locations: [{ line: 3, column: 11 }], + }, + ], + }); + }); + + it('stops running despite error', async () => { + const query = ` + { + promiseNonNull, + ${nestedPromise(10)} + } + `; + + let counter = 0; + const rootValue = { + ...throwingData, + promiseNest() { + return new Promise((resolve) => { + counter++; + resolve(rootValue); + }); + }, + }; + const result = await executeQuery(query, rootValue); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: 'promiseNonNull', + path: ['promiseNonNull'], + locations: [{ line: 3, column: 11 }], + }, + ], + }); + const counterAtExecutionEnd = counter; + await resolveOnNextTick(); + expect(counter).to.equal(counterAtExecutionEnd); + }); + }); + describe('Handles non-null argument', () => { const schemaWithNonNullArg = new GraphQLSchema({ query: new GraphQLObjectType({ diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 5cd377489a..b749b7717b 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -165,11 +165,13 @@ export interface ExecutionContext { validatedExecutionArgs: ValidatedExecutionArgs; errors: Array | undefined; canceller: Canceller; + completed: boolean; cancellableStreams: Set | undefined; } interface IncrementalContext { errors: Array | undefined; + completed: boolean; deferUsageSet?: DeferUsageSet | undefined; } @@ -316,6 +318,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( validatedExecutionArgs, errors: undefined, canceller: new Canceller(validatedExecutionArgs.abortSignal), + completed: false, cancellableStreams: undefined, }; try { @@ -366,8 +369,12 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( if (isPromise(graphqlWrappedResult)) { return graphqlWrappedResult.then( - (resolved) => buildDataResponse(exeContext, resolved), + (resolved) => { + exeContext.completed = true; + return buildDataResponse(exeContext, resolved); + }, (error: unknown) => { + exeContext.completed = true; exeContext.canceller.unsubscribe(); return { data: null, @@ -376,8 +383,10 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( }, ); } + exeContext.completed = true; return buildDataResponse(exeContext, graphqlWrappedResult); } catch (error) { + exeContext.completed = true; exeContext.canceller.unsubscribe(); return { data: null, errors: withError(exeContext.errors, error) }; } @@ -1754,6 +1763,10 @@ function completeObjectValue( incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, ): PromiseOrValue>> { + if ((incrementalContext ?? exeContext).completed) { + throw new Error('Completed, aborting.'); + } + // If there is an isTypeOf predicate function, call it with the // current result. If isTypeOf returns false, then raise an error rather // than continuing execution. @@ -2271,6 +2284,7 @@ function collectExecutionGroups( groupedFieldSet, { errors: undefined, + completed: false, deferUsageSet, }, deferMap, @@ -2330,6 +2344,7 @@ function executeExecutionGroup( deferMap, ); } catch (error) { + incrementalContext.completed = true; return { pendingExecutionGroup, path: pathToArray(path), @@ -2339,21 +2354,27 @@ function executeExecutionGroup( if (isPromise(result)) { return result.then( - (resolved) => - buildCompletedExecutionGroup( + (resolved) => { + incrementalContext.completed = true; + return buildCompletedExecutionGroup( incrementalContext.errors, pendingExecutionGroup, path, resolved, - ), - (error: unknown) => ({ - pendingExecutionGroup, - path: pathToArray(path), - errors: withError(incrementalContext.errors, error as GraphQLError), - }), + ); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + pendingExecutionGroup, + path: pathToArray(path), + errors: withError(incrementalContext.errors, error as GraphQLError), + }; + }, ); } + incrementalContext.completed = true; return buildCompletedExecutionGroup( incrementalContext.errors, pendingExecutionGroup, @@ -2408,7 +2429,7 @@ function buildSyncStreamItemQueue( initialPath, initialItem, exeContext, - { errors: undefined }, + { errors: undefined, completed: false }, fieldDetailsList, info, itemType, @@ -2439,7 +2460,7 @@ function buildSyncStreamItemQueue( itemPath, value, exeContext, - { errors: undefined }, + { errors: undefined, completed: false }, fieldDetailsList, info, itemType, @@ -2531,7 +2552,7 @@ async function getNextAsyncStreamItemResult( itemPath, iteration.value, exeContext, - { errors: undefined }, + { errors: undefined, completed: false }, fieldDetailsList, info, itemType, @@ -2578,11 +2599,16 @@ function completeStreamItem( incrementalContext, new Map(), ).then( - (resolvedItem) => - buildStreamItemResult(incrementalContext.errors, resolvedItem), - (error: unknown) => ({ - errors: withError(incrementalContext.errors, error as GraphQLError), - }), + (resolvedItem) => { + incrementalContext.completed = true; + return buildStreamItemResult(incrementalContext.errors, resolvedItem); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + errors: withError(incrementalContext.errors, error as GraphQLError), + }; + }, ); } @@ -2611,6 +2637,7 @@ function completeStreamItem( result = { rawResult: null, incrementalDataRecords: undefined }; } } catch (error) { + incrementalContext.completed = true; return { errors: withError(incrementalContext.errors, error), }; @@ -2630,14 +2657,20 @@ function completeStreamItem( return { rawResult: null, incrementalDataRecords: undefined }; }) .then( - (resolvedItem) => - buildStreamItemResult(incrementalContext.errors, resolvedItem), - (error: unknown) => ({ - errors: withError(incrementalContext.errors, error as GraphQLError), - }), + (resolvedItem) => { + incrementalContext.completed = true; + return buildStreamItemResult(incrementalContext.errors, resolvedItem); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + errors: withError(incrementalContext.errors, error as GraphQLError), + }; + }, ); } + incrementalContext.completed = true; return buildStreamItemResult(incrementalContext.errors, result); }