From 75624cfebeb1165ad7cb771ac04be479d79e650c 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 | 85 ++++++++++++++++++------- 2 files changed, 124 insertions(+), 24 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index ced32ae35d..d88761a57d 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 e1b1a68bc9..fee0bc4ac9 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -163,10 +163,12 @@ export interface ValidatedExecutionArgs { export interface ExecutionContext { validatedExecutionArgs: ValidatedExecutionArgs; + completed: boolean; cancellableStreams: Set | undefined; } interface IncrementalContext { + completed: boolean; deferUsageSet?: DeferUsageSet | undefined; } @@ -312,6 +314,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( ): PromiseOrValue { const exeContext: ExecutionContext = { validatedExecutionArgs, + completed: false, cancellableStreams: undefined, }; try { @@ -362,15 +365,23 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent( if (isPromise(graphqlWrappedResult)) { return graphqlWrappedResult.then( - (resolved) => buildDataResponse(exeContext, resolved), - (error: unknown) => ({ - data: null, - errors: [error as GraphQLError], - }), + (resolved) => { + exeContext.completed = true; + return buildDataResponse(exeContext, resolved); + }, + (error: unknown) => { + exeContext.completed = true; + return { + data: null, + errors: [error as GraphQLError], + }; + }, ); } + exeContext.completed = true; return buildDataResponse(exeContext, graphqlWrappedResult); } catch (error) { + exeContext.completed = true; return { data: null, errors: [error] }; } } @@ -818,7 +829,7 @@ function executeField( validatedExecutionArgs; const fieldName = fieldDetailsList[0].node.name.value; const fieldDef = schema.getField(parentType, fieldName); - if (!fieldDef) { + if (!fieldDef || (incrementalContext ?? exeContext).completed) { return; } @@ -2279,6 +2290,7 @@ function collectExecutionGroups( path, groupedFieldSet, { + completed: false, deferUsageSet, }, deferMap, @@ -2338,6 +2350,7 @@ function executeExecutionGroup( deferMap, ); } catch (error) { + incrementalContext.completed = true; return { pendingExecutionGroup, path: pathToArray(path), @@ -2347,16 +2360,26 @@ function executeExecutionGroup( if (isPromise(result)) { return result.then( - (resolved) => - buildCompletedExecutionGroup(pendingExecutionGroup, path, resolved), - (error: unknown) => ({ - pendingExecutionGroup, - path: pathToArray(path), - errors: [error as GraphQLError], - }), + (resolved) => { + incrementalContext.completed = true; + return buildCompletedExecutionGroup( + pendingExecutionGroup, + path, + resolved, + ); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + pendingExecutionGroup, + path: pathToArray(path), + errors: [error as GraphQLError], + }; + }, ); } + incrementalContext.completed = true; return buildCompletedExecutionGroup(pendingExecutionGroup, path, result); } @@ -2405,7 +2428,7 @@ function buildSyncStreamItemQueue( initialPath, initialItem, exeContext, - {}, + { completed: false }, fieldDetailsList, info, itemType, @@ -2436,7 +2459,7 @@ function buildSyncStreamItemQueue( itemPath, value, exeContext, - {}, + { completed: false }, fieldDetailsList, info, itemType, @@ -2528,7 +2551,7 @@ async function getNextAsyncStreamItemResult( itemPath, iteration.value, exeContext, - {}, + { completed: false }, fieldDetailsList, info, itemType, @@ -2575,10 +2598,16 @@ function completeStreamItem( incrementalContext, new Map(), ).then( - (resolvedItem) => buildStreamItemResult(resolvedItem), - (error: unknown) => ({ - errors: [error as GraphQLError], - }), + (resolvedItem) => { + incrementalContext.completed = true; + return buildStreamItemResult(resolvedItem); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + errors: [error as GraphQLError], + }; + }, ); } @@ -2605,6 +2634,7 @@ function completeStreamItem( }; } } catch (error) { + incrementalContext.completed = true; return { errors: [error], }; @@ -2620,13 +2650,20 @@ function completeStreamItem( ], })) .then( - (resolvedItem) => buildStreamItemResult(resolvedItem), - (error: unknown) => ({ - errors: [error as GraphQLError], - }), + (resolvedItem) => { + incrementalContext.completed = true; + return buildStreamItemResult(resolvedItem); + }, + (error: unknown) => { + incrementalContext.completed = true; + return { + errors: [error as GraphQLError], + }; + }, ); } + incrementalContext.completed = true; return buildStreamItemResult(result); }