From 49d18827c8d4e4b4587fa8da4f974ff022d81905 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sat, 6 Apr 2024 22:10:08 +0300 Subject: [PATCH 1/5] perf: check if streamUsage is defined outside the loop introducing separate looping functions for when streamUsage is defined --- src/execution/execute.ts | 194 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 188 insertions(+), 6 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index cda6ab8254..2e6dce1c27 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1052,10 +1052,97 @@ async function completeAsyncIteratorValue( [], ]; let index = 0; - const streamUsage = getStreamUsage(exeContext, fieldGroup, path); // eslint-disable-next-line no-constant-condition while (true) { - if (streamUsage && index >= streamUsage.initialCount) { + const itemPath = addPath(path, index, undefined); + let iteration; + try { + // eslint-disable-next-line no-await-in-loop + iteration = await asyncIterator.next(); + } catch (rawError) { + throw locatedError(rawError, toNodes(fieldGroup), pathToArray(path)); + } + + // TODO: add test case for stream returning done before initialCount + /* c8 ignore next 3 */ + if (iteration.done) { + break; + } + + const item = iteration.value; + // TODO: add tests for stream backed by asyncIterator that returns a promise + /* c8 ignore start */ + if (isPromise(item)) { + completedResults.push( + completePromisedListItemValue( + item, + graphqlWrappedResult, + exeContext, + itemType, + fieldGroup, + info, + itemPath, + incrementalContext, + deferMap, + ), + ); + containsPromise = true; + } else if ( + /* c8 ignore stop */ + completeListItemValue( + item, + completedResults, + graphqlWrappedResult, + exeContext, + itemType, + fieldGroup, + info, + itemPath, + incrementalContext, + deferMap, + ) + // TODO: add tests for stream backed by asyncIterator that completes to a promise + /* c8 ignore start */ + ) { + containsPromise = true; + } + /* c8 ignore stop */ + index++; + } + + return containsPromise + ? /* c8 ignore start */ Promise.all(completedResults).then((resolved) => [ + resolved, + graphqlWrappedResult[1], + ]) + : /* c8 ignore stop */ graphqlWrappedResult; +} + +/** + * Complete a async iterator value by completing the result and calling + * recursively until all the results are completed. + */ +async function completeAsyncIteratorValueWithPossibleStream( + exeContext: ExecutionContext, + itemType: GraphQLOutputType, + fieldGroup: FieldGroup, + info: GraphQLResolveInfo, + path: Path, + asyncIterator: AsyncIterator, + streamUsage: StreamUsage, + incrementalContext: IncrementalContext | undefined, + deferMap: ReadonlyMap | undefined, +): Promise>> { + let containsPromise = false; + const completedResults: Array = []; + const graphqlWrappedResult: GraphQLWrappedResult> = [ + completedResults, + [], + ]; + let index = 0; + // eslint-disable-next-line no-constant-condition + while (true) { + if (index >= streamUsage.initialCount) { const returnFn = asyncIterator.return; let streamRecord: SubsequentResultRecord | CancellableStreamRecord; if (returnFn === undefined) { @@ -1166,17 +1253,32 @@ function completeListValue( deferMap: ReadonlyMap | undefined, ): PromiseOrValue>> { const itemType = returnType.ofType; + const streamUsage = getStreamUsage(exeContext, fieldGroup, path); if (isAsyncIterable(result)) { const asyncIterator = result[Symbol.asyncIterator](); - return completeAsyncIteratorValue( + if (streamUsage === undefined) { + return completeAsyncIteratorValue( + exeContext, + itemType, + fieldGroup, + info, + path, + asyncIterator, + incrementalContext, + deferMap, + ); + } + + return completeAsyncIteratorValueWithPossibleStream( exeContext, itemType, fieldGroup, info, path, asyncIterator, + streamUsage, incrementalContext, deferMap, ); @@ -1188,13 +1290,27 @@ function completeListValue( ); } - return completeIterableValue( + if (streamUsage === undefined) { + return completeIterableValue( + exeContext, + itemType, + fieldGroup, + info, + path, + result, + incrementalContext, + deferMap, + ); + } + + return completeIterableValueWithPossibleStream( exeContext, itemType, fieldGroup, info, path, result, + streamUsage, incrementalContext, deferMap, ); @@ -1219,13 +1335,79 @@ function completeIterableValue( [], ]; let index = 0; - const streamUsage = getStreamUsage(exeContext, fieldGroup, path); + for (const item of items) { + // No need to modify the info object containing the path, + // since from here on it is not ever accessed by resolver functions. + const itemPath = addPath(path, index, undefined); + + if (isPromise(item)) { + completedResults.push( + completePromisedListItemValue( + item, + graphqlWrappedResult, + exeContext, + itemType, + fieldGroup, + info, + itemPath, + incrementalContext, + deferMap, + ), + ); + containsPromise = true; + } else if ( + completeListItemValue( + item, + completedResults, + graphqlWrappedResult, + exeContext, + itemType, + fieldGroup, + info, + itemPath, + incrementalContext, + deferMap, + ) + ) { + containsPromise = true; + } + index++; + } + + return containsPromise + ? Promise.all(completedResults).then((resolved) => [ + resolved, + graphqlWrappedResult[1], + ]) + : graphqlWrappedResult; +} + +function completeIterableValueWithPossibleStream( + exeContext: ExecutionContext, + itemType: GraphQLOutputType, + fieldGroup: FieldGroup, + info: GraphQLResolveInfo, + path: Path, + items: Iterable, + streamUsage: StreamUsage, + incrementalContext: IncrementalContext | undefined, + deferMap: ReadonlyMap | undefined, +): PromiseOrValue>> { + // This is specified as a simple map, however we're optimizing the path + // where the list contains no Promises by avoiding creating another Promise. + let containsPromise = false; + const completedResults: Array = []; + const graphqlWrappedResult: GraphQLWrappedResult> = [ + completedResults, + [], + ]; + let index = 0; const iterator = items[Symbol.iterator](); let iteration = iterator.next(); while (!iteration.done) { const item = iteration.value; - if (streamUsage && index >= streamUsage.initialCount) { + if (index >= streamUsage.initialCount) { const streamRecord: SubsequentResultRecord = { label: streamUsage.label, path, From 300af34b8033c0ea273248f957fc35e719ccf6a8 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Thu, 4 Apr 2024 17:04:59 +0300 Subject: [PATCH 2/5] use undefined for empty --- src/execution/IncrementalPublisher.ts | 31 ++-- src/execution/execute.ts | 200 +++++++++++++++++--------- 2 files changed, 152 insertions(+), 79 deletions(-) diff --git a/src/execution/IncrementalPublisher.ts b/src/execution/IncrementalPublisher.ts index b5f66b6322..0722da1ed1 100644 --- a/src/execution/IncrementalPublisher.ts +++ b/src/execution/IncrementalPublisher.ts @@ -1,3 +1,4 @@ +import { invariant } from '../jsutils/invariant.js'; import { isPromise } from '../jsutils/isPromise.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; import type { Path } from '../jsutils/Path.js'; @@ -172,7 +173,7 @@ export interface FormattedCompletedResult { export function buildIncrementalResponse( context: IncrementalPublisherContext, result: ObjMap, - errors: ReadonlyArray, + errors: ReadonlyArray | undefined, incrementalDataRecords: ReadonlyArray, ): ExperimentalIncrementalExecutionResults { const incrementalPublisher = new IncrementalPublisher(context); @@ -184,7 +185,7 @@ export function buildIncrementalResponse( } interface IncrementalPublisherContext { - cancellableStreams: Set; + cancellableStreams: Set | undefined; } /** @@ -218,7 +219,7 @@ class IncrementalPublisher { buildResponse( data: ObjMap, - errors: ReadonlyArray, + errors: ReadonlyArray | undefined, incrementalDataRecords: ReadonlyArray, ): ExperimentalIncrementalExecutionResults { this._addIncrementalDataRecords(incrementalDataRecords); @@ -227,7 +228,7 @@ class IncrementalPublisher { const pending = this._pendingSourcesToResults(); const initialResult: InitialIncrementalExecutionResult = - errors.length === 0 + errors === undefined ? { data, pending, hasNext: true } : { errors, data, pending, hasNext: true }; @@ -444,8 +445,12 @@ class IncrementalPublisher { }; const returnStreamIterators = async (): Promise => { + const cancellableStreams = this._context.cancellableStreams; + if (cancellableStreams === undefined) { + return; + } const promises: Array> = []; - for (const streamRecord of this._context.cancellableStreams) { + for (const streamRecord of cancellableStreams) { if (streamRecord.earlyReturn !== undefined) { promises.push(streamRecord.earlyReturn()); } @@ -519,9 +524,11 @@ class IncrementalPublisher { ); } - this._addIncrementalDataRecords( - deferredGroupedFieldSetResult.incrementalDataRecords, - ); + const incrementalDataRecords = + deferredGroupedFieldSetResult.incrementalDataRecords; + if (incrementalDataRecords !== undefined) { + this._addIncrementalDataRecords(incrementalDataRecords); + } for (const deferredFragmentRecord of deferredGroupedFieldSetResult.deferredFragmentRecords) { const id = deferredFragmentRecord.id; @@ -587,6 +594,7 @@ class IncrementalPublisher { }); this._pending.delete(streamRecord); if (isCancellableStreamRecord(streamRecord)) { + invariant(this._context.cancellableStreams !== undefined); this._context.cancellableStreams.delete(streamRecord); streamRecord.earlyReturn().catch(() => { /* c8 ignore next 1 */ @@ -597,6 +605,7 @@ class IncrementalPublisher { this._completed.push({ id }); this._pending.delete(streamRecord); if (isCancellableStreamRecord(streamRecord)) { + invariant(this._context.cancellableStreams !== undefined); this._context.cancellableStreams.delete(streamRecord); } } else { @@ -607,7 +616,7 @@ class IncrementalPublisher { this._incremental.push(incrementalEntry); - if (streamItemsResult.incrementalDataRecords.length > 0) { + if (streamItemsResult.incrementalDataRecords !== undefined) { this._addIncrementalDataRecords( streamItemsResult.incrementalDataRecords, ); @@ -675,7 +684,7 @@ interface ReconcilableDeferredGroupedFieldSetResult { deferredFragmentRecords: ReadonlyArray; path: Array; result: BareDeferredGroupedFieldSetResult; - incrementalDataRecords: ReadonlyArray; + incrementalDataRecords: ReadonlyArray | undefined; sent?: true | undefined; errors?: never; } @@ -743,7 +752,7 @@ function isCancellableStreamRecord( interface ReconcilableStreamItemsResult { streamRecord: SubsequentResultRecord; result: BareStreamItemsResult; - incrementalDataRecords: ReadonlyArray; + incrementalDataRecords: ReadonlyArray | undefined; errors?: never; } diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 2e6dce1c27..88d5a0749d 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -142,12 +142,12 @@ export interface ExecutionContext { fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; - errors: Array; - cancellableStreams: Set; + errors: Array | undefined; + cancellableStreams: Set | undefined; } interface IncrementalContext { - errors: Array; + errors: Array | undefined; deferUsageSet?: DeferUsageSet | undefined; } @@ -169,7 +169,7 @@ export interface StreamUsage { fieldGroup: FieldGroup; } -type GraphQLWrappedResult = [T, Array]; +type GraphQLWrappedResult = [T, Array | undefined]; const UNEXPECTED_EXPERIMENTAL_DIRECTIVES = 'The provided schema unexpectedly contains experimental directives (@defer or @stream). These directives may only be utilized if experimental execution features are explicitly enabled.'; @@ -353,30 +353,44 @@ function withNewDeferredGroupedFieldSets( ): PromiseOrValue>> { if (isPromise(result)) { return result.then((resolved) => { - resolved[1].push(...newDeferredGroupedFieldSetRecords); + addIncrementalDataRecords(resolved, newDeferredGroupedFieldSetRecords); return resolved; }); } - result[1].push(...newDeferredGroupedFieldSetRecords); + addIncrementalDataRecords(result, newDeferredGroupedFieldSetRecords); return result; } +function addIncrementalDataRecords( + graphqlWrappedResult: GraphQLWrappedResult, + incrementalDataRecords: ReadonlyArray | undefined, +): void { + if (incrementalDataRecords === undefined) { + return; + } + if (graphqlWrappedResult[1] === undefined) { + graphqlWrappedResult[1] = [...incrementalDataRecords]; + } else { + graphqlWrappedResult[1].push(...incrementalDataRecords); + } +} + function withError( - errors: Array, + errors: Array | undefined, error: GraphQLError, ): ReadonlyArray { - return errors.length === 0 ? [error] : [...errors, error]; + return errors === undefined ? [error] : [...errors, error]; } function buildDataResponse( exeContext: ExecutionContext, data: ObjMap, - incrementalDataRecords: ReadonlyArray, + incrementalDataRecords: ReadonlyArray | undefined, ): ExecutionResult | ExperimentalIncrementalExecutionResults { const errors = exeContext.errors; - if (incrementalDataRecords.length === 0) { - return errors.length > 0 ? { errors, data } : { data }; + if (incrementalDataRecords === undefined) { + return errors !== undefined ? { errors, data } : { data }; } return buildIncrementalResponse( @@ -488,8 +502,8 @@ export function buildExecutionContext( fieldResolver: fieldResolver ?? defaultFieldResolver, typeResolver: typeResolver ?? defaultTypeResolver, subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, - errors: [], - cancellableStreams: new Set(), + errors: undefined, + cancellableStreams: undefined, }; } @@ -499,8 +513,8 @@ function buildPerEventExecutionContext( ): ExecutionContext { return { ...exeContext, - errors: [], rootValue: payload, + errors: undefined, }; } @@ -580,15 +594,15 @@ function executeFieldsSerially( if (isPromise(result)) { return result.then((resolved) => { graphqlWrappedResult[0][responseName] = resolved[0]; - graphqlWrappedResult[1].push(...resolved[1]); + addIncrementalDataRecords(graphqlWrappedResult, resolved[1]); return graphqlWrappedResult; }); } graphqlWrappedResult[0][responseName] = result[0]; - graphqlWrappedResult[1].push(...result[1]); + addIncrementalDataRecords(graphqlWrappedResult, result[1]); return graphqlWrappedResult; }, - [Object.create(null), []] as GraphQLWrappedResult>, + [Object.create(null), undefined] as GraphQLWrappedResult>, ); } @@ -608,7 +622,7 @@ function executeFields( const results = Object.create(null); const graphqlWrappedResult: GraphQLWrappedResult> = [ results, - [], + undefined, ]; let containsPromise = false; @@ -628,13 +642,13 @@ function executeFields( if (result !== undefined) { if (isPromise(result)) { results[responseName] = result.then((resolved) => { - graphqlWrappedResult[1].push(...resolved[1]); + addIncrementalDataRecords(graphqlWrappedResult, resolved[1]); return resolved[0]; }); containsPromise = true; } else { results[responseName] = result[0]; - graphqlWrappedResult[1].push(...result[1]); + addIncrementalDataRecords(graphqlWrappedResult, result[1]); } } } @@ -746,16 +760,28 @@ function executeField( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. return completed.then(undefined, (rawError) => { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, returnType, fieldGroup, path, errors); - return [null, []]; + handleFieldError( + rawError, + exeContext, + returnType, + fieldGroup, + path, + incrementalContext, + ); + return [null, undefined]; }); } return completed; } catch (rawError) { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, returnType, fieldGroup, path, errors); - return [null, []]; + handleFieldError( + rawError, + exeContext, + returnType, + fieldGroup, + path, + incrementalContext, + ); + return [null, undefined]; } } @@ -788,10 +814,11 @@ export function buildResolveInfo( function handleFieldError( rawError: unknown, + exeContext: ExecutionContext, returnType: GraphQLOutputType, fieldGroup: FieldGroup, path: Path, - errors: Array, + incrementalContext: IncrementalContext | undefined, ): void { const error = locatedError(rawError, toNodes(fieldGroup), pathToArray(path)); @@ -803,6 +830,12 @@ function handleFieldError( // Otherwise, error protection is applied, logging the error and resolving // a null value for this field if one is encountered. + const context = incrementalContext ?? exeContext; + let errors = context.errors; + if (errors === undefined) { + errors = []; + context.errors = errors; + } errors.push(error); } @@ -865,7 +898,7 @@ function completeValue( // If result value is null or undefined then return null. if (result == null) { - return [null, []]; + return [null, undefined]; } // If field type is List, complete each item in the list with the inner type @@ -885,7 +918,7 @@ function completeValue( // If field type is a leaf type, Scalar or Enum, serialize to a valid value, // returning null if serialization is not possible. if (isLeafType(returnType)) { - return [completeLeafValue(returnType, result), []]; + return [completeLeafValue(returnType, result), undefined]; } // If field type is an abstract type, Interface or Union, determine the @@ -952,9 +985,15 @@ async function completePromisedValue( } return completed; } catch (rawError) { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, returnType, fieldGroup, path, errors); - return [null, []]; + handleFieldError( + rawError, + exeContext, + returnType, + fieldGroup, + path, + incrementalContext, + ); + return [null, undefined]; } } @@ -1049,7 +1088,7 @@ async function completeAsyncIteratorValue( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = [ completedResults, - [], + undefined, ]; let index = 0; // eslint-disable-next-line no-constant-condition @@ -1137,7 +1176,7 @@ async function completeAsyncIteratorValueWithPossibleStream( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = [ completedResults, - [], + undefined, ]; let index = 0; // eslint-disable-next-line no-constant-condition @@ -1156,6 +1195,9 @@ async function completeAsyncIteratorValueWithPossibleStream( path, earlyReturn: returnFn.bind(asyncIterator), }; + if (exeContext.cancellableStreams === undefined) { + exeContext.cancellableStreams = new Set(); + } exeContext.cancellableStreams.add(streamRecord); } @@ -1170,7 +1212,7 @@ async function completeAsyncIteratorValueWithPossibleStream( itemType, ); - graphqlWrappedResult[1].push(firstStreamItems); + addIncrementalDataRecords(graphqlWrappedResult, [firstStreamItems]); break; } @@ -1332,7 +1374,7 @@ function completeIterableValue( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = [ completedResults, - [], + undefined, ]; let index = 0; for (const item of items) { @@ -1399,7 +1441,7 @@ function completeIterableValueWithPossibleStream( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = [ completedResults, - [], + undefined, ]; let index = 0; const iterator = items[Symbol.iterator](); @@ -1424,7 +1466,7 @@ function completeIterableValueWithPossibleStream( itemType, ); - graphqlWrappedResult[1].push(firstStreamItems); + addIncrementalDataRecords(graphqlWrappedResult, [firstStreamItems]); break; } @@ -1511,12 +1553,18 @@ function completeListItemValue( completedResults.push( completedItem.then( (resolved) => { - parent[1].push(...resolved[1]); + addIncrementalDataRecords(parent, resolved[1]); return resolved[0]; }, (rawError) => { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, itemType, fieldGroup, itemPath, errors); + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); return null; }, ), @@ -1525,10 +1573,16 @@ function completeListItemValue( } completedResults.push(completedItem[0]); - parent[1].push(...completedItem[1]); + addIncrementalDataRecords(parent, completedItem[1]); } catch (rawError) { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, itemType, fieldGroup, itemPath, errors); + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); completedResults.push(null); } return false; @@ -1560,11 +1614,17 @@ async function completePromisedListItemValue( if (isPromise(completed)) { completed = await completed; } - parent[1].push(...completed[1]); + addIncrementalDataRecords(parent, completed[1]); return completed[0]; } catch (rawError) { - const errors = (incrementalContext ?? exeContext).errors; - handleFieldError(rawError, itemType, fieldGroup, itemPath, errors); + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); return null; } } @@ -2222,7 +2282,7 @@ function executeDeferredGroupedFieldSets( path, groupedFieldSet, { - errors: [], + errors: undefined, deferUsageSet, }, deferMap, @@ -2312,7 +2372,7 @@ function executeDeferredGroupedFieldSet( } function buildDeferredGroupedFieldSetResult( - errors: ReadonlyArray, + errors: ReadonlyArray | undefined, deferredFragmentRecords: ReadonlyArray, path: Path | undefined, result: GraphQLWrappedResult>, @@ -2321,7 +2381,7 @@ function buildDeferredGroupedFieldSetResult( deferredFragmentRecords, path: pathToArray(path), result: - errors.length === 0 ? { data: result[0] } : { data: result[0], errors }, + errors === undefined ? { data: result[0] } : { data: result[0], errors }, incrementalDataRecords: result[1], }; } @@ -2356,7 +2416,7 @@ function firstSyncStreamItems( initialPath, initialItem, exeContext, - { errors: [] }, + { errors: undefined }, fieldGroup, info, itemType, @@ -2378,7 +2438,7 @@ function firstSyncStreamItems( currentPath, item, exeContext, - { errors: [] }, + { errors: undefined }, fieldGroup, info, itemType, @@ -2426,15 +2486,17 @@ function prependNextResolvedStreamItems( result: StreamItemsResult, nextStreamItems: StreamItemsRecord, ): StreamItemsResult { - return isReconcilableStreamItemsResult(result) - ? { - ...result, - incrementalDataRecords: [ - nextStreamItems, - ...result.incrementalDataRecords, - ], - } - : result; + if (!isReconcilableStreamItemsResult(result)) { + return result; + } + const incrementalDataRecords = result.incrementalDataRecords; + return { + ...result, + incrementalDataRecords: + incrementalDataRecords === undefined + ? [nextStreamItems] + : [nextStreamItems, ...incrementalDataRecords], + }; } function firstAsyncStreamItems( @@ -2494,7 +2556,7 @@ async function getNextAsyncStreamItemsResult( itemPath, iteration.value, exeContext, - { errors: [] }, + { errors: undefined }, fieldGroup, info, itemType, @@ -2567,12 +2629,13 @@ function completeStreamItems( } catch (rawError) { handleFieldError( rawError, + exeContext, itemType, fieldGroup, itemPath, - incrementalContext.errors, + incrementalContext, ); - result = [null, []]; + result = [null, undefined]; } } catch (error) { return { @@ -2586,12 +2649,13 @@ function completeStreamItems( .then(undefined, (rawError) => { handleFieldError( rawError, + exeContext, itemType, fieldGroup, itemPath, - incrementalContext.errors, + incrementalContext, ); - return [null, []] as GraphQLWrappedResult; + return [null, undefined] as GraphQLWrappedResult; }) .then( (resolvedItem) => @@ -2615,14 +2679,14 @@ function completeStreamItems( } function buildStreamItemsResult( - errors: ReadonlyArray, + errors: ReadonlyArray | undefined, streamRecord: SubsequentResultRecord, result: GraphQLWrappedResult, ): StreamItemsResult { return { streamRecord, result: - errors.length === 0 + errors === undefined ? { items: [result[0]] } : { items: [result[0]], From ab728247479c88176ffd3874857a4d6340d2a5c8 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 5 Apr 2024 14:01:02 +0300 Subject: [PATCH 3/5] add filtering again --- src/execution/IncrementalPublisher.ts | 3 +- src/execution/execute.ts | 452 ++++++++++++++------------ src/jsutils/promiseForObject.ts | 7 +- 3 files changed, 243 insertions(+), 219 deletions(-) diff --git a/src/execution/IncrementalPublisher.ts b/src/execution/IncrementalPublisher.ts index 0722da1ed1..3ffcf55b2d 100644 --- a/src/execution/IncrementalPublisher.ts +++ b/src/execution/IncrementalPublisher.ts @@ -664,7 +664,7 @@ function isDeferredFragmentRecord( return 'parent' in subsequentResultRecord; } -function isDeferredGroupedFieldSetRecord( +export function isDeferredGroupedFieldSetRecord( incrementalDataRecord: IncrementalDataRecord, ): incrementalDataRecord is DeferredGroupedFieldSetRecord { return 'deferredFragmentRecords' in incrementalDataRecord; @@ -703,6 +703,7 @@ function isNonReconcilableDeferredGroupedFieldSetResult( } export interface DeferredGroupedFieldSetRecord { + path: Path | undefined; deferredFragmentRecords: ReadonlyArray; result: PromiseOrValue; } diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 88d5a0749d..0a938260c0 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -72,6 +72,7 @@ import type { import { buildIncrementalResponse, DeferredFragmentRecord, + isDeferredGroupedFieldSetRecord, isReconcilableStreamItemsResult, } from './IncrementalPublisher.js'; import { mapAsyncIterable } from './mapAsyncIterable.js'; @@ -142,13 +143,15 @@ export interface ExecutionContext { fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; subscribeFieldResolver: GraphQLFieldResolver; - errors: Array | undefined; + errors: Map | undefined; cancellableStreams: Set | undefined; + incrementalDataRecords: Array | undefined; } interface IncrementalContext { - errors: Array | undefined; + errors: Map | undefined; deferUsageSet?: DeferUsageSet | undefined; + incrementalDataRecords: Array | undefined; } export interface ExecutionArgs { @@ -169,8 +172,6 @@ export interface StreamUsage { fieldGroup: FieldGroup; } -type GraphQLWrappedResult = [T, Array | undefined]; - const UNEXPECTED_EXPERIMENTAL_DIRECTIVES = 'The provided schema unexpectedly contains experimental directives (@defer or @stream). These directives may only be utilized if experimental execution features are explicitly enabled.'; @@ -283,11 +284,9 @@ function executeOperation( ); let groupedFieldSet = collectedFields.groupedFieldSet; const newDeferUsages = collectedFields.newDeferUsages; - let graphqlWrappedResult: PromiseOrValue< - GraphQLWrappedResult> - >; + let data: PromiseOrValue>; if (newDeferUsages.length === 0) { - graphqlWrappedResult = executeRootGroupedFieldSet( + data = executeRootGroupedFieldSet( exeContext, operation.operation, rootType, @@ -301,7 +300,7 @@ function executeOperation( const newGroupedFieldSets = fieldPLan.newGroupedFieldSets; const newDeferMap = addNewDeferredFragments(newDeferUsages, new Map()); - graphqlWrappedResult = executeRootGroupedFieldSet( + data = executeRootGroupedFieldSet( exeContext, operation.operation, rootType, @@ -322,85 +321,132 @@ function executeOperation( newDeferMap, ); - graphqlWrappedResult = withNewDeferredGroupedFieldSets( - graphqlWrappedResult, + addIncrementalDataRecords( + exeContext, newDeferredGroupedFieldSetRecords, ); } } - if (isPromise(graphqlWrappedResult)) { - return graphqlWrappedResult.then( - (resolved) => buildDataResponse(exeContext, resolved[0], resolved[1]), + if (isPromise(data)) { + return data.then( + (resolved) => buildDataResponse(exeContext, resolved), (error) => ({ data: null, errors: withError(exeContext.errors, error), }), ); } - return buildDataResponse( - exeContext, - graphqlWrappedResult[0], - graphqlWrappedResult[1], - ); + return buildDataResponse(exeContext, data); } catch (error) { return { data: null, errors: withError(exeContext.errors, error) }; } } -function withNewDeferredGroupedFieldSets( - result: PromiseOrValue>>, - newDeferredGroupedFieldSetRecords: ReadonlyArray, -): PromiseOrValue>> { - if (isPromise(result)) { - return result.then((resolved) => { - addIncrementalDataRecords(resolved, newDeferredGroupedFieldSetRecords); - return resolved; - }); - } - - addIncrementalDataRecords(result, newDeferredGroupedFieldSetRecords); - return result; -} - function addIncrementalDataRecords( - graphqlWrappedResult: GraphQLWrappedResult, - incrementalDataRecords: ReadonlyArray | undefined, + context: ExecutionContext | IncrementalContext, + newIncrementalDataRecords: ReadonlyArray, ): void { + const incrementalDataRecords = context.incrementalDataRecords; if (incrementalDataRecords === undefined) { + context.incrementalDataRecords = [...newIncrementalDataRecords]; return; } - if (graphqlWrappedResult[1] === undefined) { - graphqlWrappedResult[1] = [...incrementalDataRecords]; - } else { - graphqlWrappedResult[1].push(...incrementalDataRecords); - } + incrementalDataRecords.push(...newIncrementalDataRecords); } function withError( - errors: Array | undefined, + errors: ReadonlyMap | undefined, error: GraphQLError, ): ReadonlyArray { - return errors === undefined ? [error] : [...errors, error]; + return errors === undefined ? [error] : [...errors.values(), error]; } function buildDataResponse( exeContext: ExecutionContext, data: ObjMap, - incrementalDataRecords: ReadonlyArray | undefined, ): ExecutionResult | ExperimentalIncrementalExecutionResults { - const errors = exeContext.errors; + const { errors, incrementalDataRecords } = exeContext; if (incrementalDataRecords === undefined) { - return errors !== undefined ? { errors, data } : { data }; + return buildSingleResult(data, errors); + } + + if (errors === undefined) { + return buildIncrementalResponse( + exeContext, + data, + undefined, + incrementalDataRecords, + ); + } + + const filteredIncrementalDataRecords = filterIncrementalDataRecords( + undefined, + errors, + incrementalDataRecords, + ); + + if (filteredIncrementalDataRecords.length === 0) { + return buildSingleResult(data, errors); } return buildIncrementalResponse( exeContext, data, - errors, - incrementalDataRecords, + Array.from(errors.values()), + filteredIncrementalDataRecords, ); } +function buildSingleResult( + data: ObjMap, + errors: ReadonlyMap | undefined, +): ExecutionResult { + return errors !== undefined + ? { errors: Array.from(errors.values()), data } + : { data }; +} + +function filterIncrementalDataRecords( + initialPath: Path | undefined, + errors: ReadonlyMap, + incrementalDataRecords: ReadonlyArray, +): ReadonlyArray { + const filteredIncrementalDataRecords: Array = []; + for (const incrementalDataRecord of incrementalDataRecords) { + let currentPath: Path | undefined = isDeferredGroupedFieldSetRecord( + incrementalDataRecord, + ) + ? incrementalDataRecord.path + : incrementalDataRecord.streamRecord.path; + + if (errors.has(currentPath)) { + continue; + } + + const paths: Array = [currentPath]; + let filtered = false; + while (currentPath !== initialPath) { + // Because currentPath leads to initialPath or is undefined, and the + // loop will exit if initialPath is undefined, currentPath must be + // defined. + // TODO: Consider, however, adding an invariant. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + currentPath = currentPath!.prev; + if (errors.has(currentPath)) { + filtered = true; + break; + } + paths.push(currentPath); + } + + if (!filtered) { + filteredIncrementalDataRecords.push(incrementalDataRecord); + } + } + + return filteredIncrementalDataRecords; +} + /** * Also implements the "Executing requests" section of the GraphQL specification. * However, it guarantees to complete synchronously (or throw an error) assuming @@ -504,6 +550,7 @@ export function buildExecutionContext( subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, errors: undefined, cancellableStreams: undefined, + incrementalDataRecords: undefined, }; } @@ -525,7 +572,7 @@ function executeRootGroupedFieldSet( rootValue: unknown, groupedFieldSet: GroupedFieldSet, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { switch (operation) { case OperationTypeNode.QUERY: return executeFields( @@ -574,10 +621,10 @@ function executeFieldsSerially( groupedFieldSet: GroupedFieldSet, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { return promiseReduce( groupedFieldSet, - (graphqlWrappedResult, [responseName, fieldGroup]) => { + (results, [responseName, fieldGroup]) => { const fieldPath = addPath(path, responseName, parentType.name); const result = executeField( exeContext, @@ -589,20 +636,18 @@ function executeFieldsSerially( deferMap, ); if (result === undefined) { - return graphqlWrappedResult; + return results; } if (isPromise(result)) { return result.then((resolved) => { - graphqlWrappedResult[0][responseName] = resolved[0]; - addIncrementalDataRecords(graphqlWrappedResult, resolved[1]); - return graphqlWrappedResult; + results[responseName] = resolved; + return results; }); } - graphqlWrappedResult[0][responseName] = result[0]; - addIncrementalDataRecords(graphqlWrappedResult, result[1]); - return graphqlWrappedResult; + results[responseName] = result; + return results; }, - [Object.create(null), undefined] as GraphQLWrappedResult>, + Object.create(null), ); } @@ -618,12 +663,8 @@ function executeFields( groupedFieldSet: GroupedFieldSet, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { const results = Object.create(null); - const graphqlWrappedResult: GraphQLWrappedResult> = [ - results, - undefined, - ]; let containsPromise = false; try { @@ -640,24 +681,16 @@ function executeFields( ); if (result !== undefined) { + results[responseName] = result; if (isPromise(result)) { - results[responseName] = result.then((resolved) => { - addIncrementalDataRecords(graphqlWrappedResult, resolved[1]); - return resolved[0]; - }); containsPromise = true; - } else { - results[responseName] = result[0]; - addIncrementalDataRecords(graphqlWrappedResult, result[1]); } } } } catch (error) { if (containsPromise) { // Ensure that any promises returned by other fields are handled, as they may also reject. - return promiseForObject(results, () => { - /* noop */ - }).finally(() => { + return promiseForObject(results).finally(() => { throw error; }) as never; } @@ -666,16 +699,13 @@ function executeFields( // If there are no promises, we can just return the object and any incrementalDataRecords if (!containsPromise) { - return graphqlWrappedResult; + return results; } // Otherwise, results is a map from field name to the result of resolving that // field, which is possibly a promise. Return a promise that will return this // same map, but with any promises replaced with the values they resolved to. - return promiseForObject(results, (resolved) => [ - resolved, - graphqlWrappedResult[1], - ]); + return promiseForObject(results); } function toNodes(fieldGroup: FieldGroup): ReadonlyArray { @@ -696,7 +726,7 @@ function executeField( path: Path, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue> | undefined { +): PromiseOrValue { const fieldName = fieldGroup[0].node.name.value; const fieldDef = exeContext.schema.getField(parentType, fieldName); if (!fieldDef) { @@ -768,7 +798,7 @@ function executeField( path, incrementalContext, ); - return [null, undefined]; + return null; }); } return completed; @@ -781,7 +811,7 @@ function executeField( path, incrementalContext, ); - return [null, undefined]; + return null; } } @@ -833,10 +863,10 @@ function handleFieldError( const context = incrementalContext ?? exeContext; let errors = context.errors; if (errors === undefined) { - errors = []; + errors = new Map(); context.errors = errors; } - errors.push(error); + errors.set(path, error); } /** @@ -869,7 +899,7 @@ function completeValue( result: unknown, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue> { +): PromiseOrValue { // If result is an Error, throw a located error. if (result instanceof Error) { throw result; @@ -888,7 +918,7 @@ function completeValue( incrementalContext, deferMap, ); - if ((completed as GraphQLWrappedResult)[0] === null) { + if (completed === null) { throw new Error( `Cannot return null for non-nullable field ${info.parentType.name}.${info.fieldName}.`, ); @@ -898,7 +928,7 @@ function completeValue( // If result value is null or undefined then return null. if (result == null) { - return [null, undefined]; + return null; } // If field type is List, complete each item in the list with the inner type @@ -918,7 +948,7 @@ function completeValue( // If field type is a leaf type, Scalar or Enum, serialize to a valid value, // returning null if serialization is not possible. if (isLeafType(returnType)) { - return [completeLeafValue(returnType, result), undefined]; + return completeLeafValue(returnType, result); } // If field type is an abstract type, Interface or Union, determine the @@ -966,7 +996,7 @@ async function completePromisedValue( result: Promise, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): Promise> { +): Promise { try { const resolved = await result; let completed = completeValue( @@ -993,7 +1023,7 @@ async function completePromisedValue( path, incrementalContext, ); - return [null, undefined]; + return null; } } @@ -1083,13 +1113,9 @@ async function completeAsyncIteratorValue( asyncIterator: AsyncIterator, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): Promise>> { +): Promise> { let containsPromise = false; const completedResults: Array = []; - const graphqlWrappedResult: GraphQLWrappedResult> = [ - completedResults, - undefined, - ]; let index = 0; // eslint-disable-next-line no-constant-condition while (true) { @@ -1115,7 +1141,6 @@ async function completeAsyncIteratorValue( completedResults.push( completePromisedListItemValue( item, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1131,7 +1156,6 @@ async function completeAsyncIteratorValue( completeListItemValue( item, completedResults, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1145,16 +1169,11 @@ async function completeAsyncIteratorValue( ) { containsPromise = true; } - /* c8 ignore stop */ + index++; } - return containsPromise - ? /* c8 ignore start */ Promise.all(completedResults).then((resolved) => [ - resolved, - graphqlWrappedResult[1], - ]) - : /* c8 ignore stop */ graphqlWrappedResult; + return containsPromise ? Promise.all(completedResults) : completedResults; } /** @@ -1171,13 +1190,9 @@ async function completeAsyncIteratorValueWithPossibleStream( streamUsage: StreamUsage, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): Promise>> { +): Promise> { let containsPromise = false; const completedResults: Array = []; - const graphqlWrappedResult: GraphQLWrappedResult> = [ - completedResults, - undefined, - ]; let index = 0; // eslint-disable-next-line no-constant-condition while (true) { @@ -1212,7 +1227,8 @@ async function completeAsyncIteratorValueWithPossibleStream( itemType, ); - addIncrementalDataRecords(graphqlWrappedResult, [firstStreamItems]); + const context = incrementalContext ?? exeContext; + addIncrementalDataRecords(context, [firstStreamItems]); break; } @@ -1238,7 +1254,6 @@ async function completeAsyncIteratorValueWithPossibleStream( completedResults.push( completePromisedListItemValue( item, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1254,7 +1269,6 @@ async function completeAsyncIteratorValueWithPossibleStream( completeListItemValue( item, completedResults, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1273,11 +1287,8 @@ async function completeAsyncIteratorValueWithPossibleStream( } return containsPromise - ? /* c8 ignore start */ Promise.all(completedResults).then((resolved) => [ - resolved, - graphqlWrappedResult[1], - ]) - : /* c8 ignore stop */ graphqlWrappedResult; + ? /* c8 ignore start */ Promise.all(completedResults) + : /* c8 ignore stop */ completedResults; } /** @@ -1293,7 +1304,7 @@ function completeListValue( result: unknown, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { const itemType = returnType.ofType; const streamUsage = getStreamUsage(exeContext, fieldGroup, path); @@ -1367,15 +1378,11 @@ function completeIterableValue( items: Iterable, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { // This is specified as a simple map, however we're optimizing the path // where the list contains no Promises by avoiding creating another Promise. let containsPromise = false; const completedResults: Array = []; - const graphqlWrappedResult: GraphQLWrappedResult> = [ - completedResults, - undefined, - ]; let index = 0; for (const item of items) { // No need to modify the info object containing the path, @@ -1386,7 +1393,6 @@ function completeIterableValue( completedResults.push( completePromisedListItemValue( item, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1401,7 +1407,6 @@ function completeIterableValue( completeListItemValue( item, completedResults, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1413,15 +1418,11 @@ function completeIterableValue( ) { containsPromise = true; } + index++; } - return containsPromise - ? Promise.all(completedResults).then((resolved) => [ - resolved, - graphqlWrappedResult[1], - ]) - : graphqlWrappedResult; + return containsPromise ? Promise.all(completedResults) : completedResults; } function completeIterableValueWithPossibleStream( @@ -1434,15 +1435,11 @@ function completeIterableValueWithPossibleStream( streamUsage: StreamUsage, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { // This is specified as a simple map, however we're optimizing the path // where the list contains no Promises by avoiding creating another Promise. let containsPromise = false; const completedResults: Array = []; - const graphqlWrappedResult: GraphQLWrappedResult> = [ - completedResults, - undefined, - ]; let index = 0; const iterator = items[Symbol.iterator](); let iteration = iterator.next(); @@ -1466,7 +1463,8 @@ function completeIterableValueWithPossibleStream( itemType, ); - addIncrementalDataRecords(graphqlWrappedResult, [firstStreamItems]); + const context = incrementalContext ?? exeContext; + addIncrementalDataRecords(context, [firstStreamItems]); break; } @@ -1478,7 +1476,6 @@ function completeIterableValueWithPossibleStream( completedResults.push( completePromisedListItemValue( item, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1493,7 +1490,6 @@ function completeIterableValueWithPossibleStream( completeListItemValue( item, completedResults, - graphqlWrappedResult, exeContext, itemType, fieldGroup, @@ -1510,12 +1506,7 @@ function completeIterableValueWithPossibleStream( iteration = iterator.next(); } - return containsPromise - ? Promise.all(completedResults).then((resolved) => [ - resolved, - graphqlWrappedResult[1], - ]) - : graphqlWrappedResult; + return containsPromise ? Promise.all(completedResults) : completedResults; } /** @@ -1526,7 +1517,6 @@ function completeIterableValueWithPossibleStream( function completeListItemValue( item: unknown, completedResults: Array, - parent: GraphQLWrappedResult>, exeContext: ExecutionContext, itemType: GraphQLOutputType, fieldGroup: FieldGroup, @@ -1551,29 +1541,22 @@ function completeListItemValue( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. completedResults.push( - completedItem.then( - (resolved) => { - addIncrementalDataRecords(parent, resolved[1]); - return resolved[0]; - }, - (rawError) => { - handleFieldError( - rawError, - exeContext, - itemType, - fieldGroup, - itemPath, - incrementalContext, - ); - return null; - }, - ), + completedItem.then(undefined, (rawError) => { + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); + return null; + }), ); return true; } - completedResults.push(completedItem[0]); - addIncrementalDataRecords(parent, completedItem[1]); + completedResults.push(completedItem); } catch (rawError) { handleFieldError( rawError, @@ -1590,7 +1573,6 @@ function completeListItemValue( async function completePromisedListItemValue( item: unknown, - parent: GraphQLWrappedResult>, exeContext: ExecutionContext, itemType: GraphQLOutputType, fieldGroup: FieldGroup, @@ -1614,8 +1596,7 @@ async function completePromisedListItemValue( if (isPromise(completed)) { completed = await completed; } - addIncrementalDataRecords(parent, completed[1]); - return completed[0]; + return completed; } catch (rawError) { handleFieldError( rawError, @@ -1660,7 +1641,7 @@ function completeAbstractValue( result: unknown, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { const resolveTypeFn = returnType.resolveType ?? exeContext.typeResolver; const contextValue = exeContext.contextValue; const runtimeType = resolveTypeFn(result, contextValue, info, returnType); @@ -1773,7 +1754,7 @@ function completeObjectValue( result: unknown, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { // 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. @@ -1885,7 +1866,7 @@ function collectAndExecuteSubfields( result: unknown, incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, -): PromiseOrValue>> { +): PromiseOrValue> { // Collect sub-fields to execute to complete this value. const collectedSubfields = collectSubfields( exeContext, @@ -1939,10 +1920,8 @@ function collectAndExecuteSubfields( newDeferMap, ); - return withNewDeferredGroupedFieldSets( - subFields, - newDeferredGroupedFieldSetRecords, - ); + const context = incrementalContext ?? exeContext; + addIncrementalDataRecords(context, newDeferredGroupedFieldSetRecords); } return subFields; } @@ -2284,11 +2263,13 @@ function executeDeferredGroupedFieldSets( { errors: undefined, deferUsageSet, + incrementalDataRecords: undefined, }, deferMap, ); const deferredGroupedFieldSetRecord: DeferredGroupedFieldSetRecord = { + path, deferredFragmentRecords, result: shouldDefer(parentDeferUsages, deferUsageSet) ? Promise.resolve().then(executor) @@ -2327,9 +2308,9 @@ function executeDeferredGroupedFieldSet( incrementalContext: IncrementalContext, deferMap: ReadonlyMap, ): PromiseOrValue { - let result; + let data; try { - result = executeFields( + data = executeFields( exeContext, parentType, sourceValue, @@ -2346,11 +2327,11 @@ function executeDeferredGroupedFieldSet( }; } - if (isPromise(result)) { - return result.then( + if (isPromise(data)) { + return data.then( (resolved) => buildDeferredGroupedFieldSetResult( - incrementalContext.errors, + incrementalContext, deferredFragmentRecords, path, resolved, @@ -2364,25 +2345,50 @@ function executeDeferredGroupedFieldSet( } return buildDeferredGroupedFieldSetResult( - incrementalContext.errors, + incrementalContext, deferredFragmentRecords, path, - result, + data, ); } function buildDeferredGroupedFieldSetResult( - errors: ReadonlyArray | undefined, + incrementalContext: IncrementalContext, deferredFragmentRecords: ReadonlyArray, path: Path | undefined, - result: GraphQLWrappedResult>, + data: ObjMap, ): DeferredGroupedFieldSetResult { + const { errors, incrementalDataRecords } = incrementalContext; + if (incrementalDataRecords === undefined) { + return { + deferredFragmentRecords, + path: pathToArray(path), + result: + errors === undefined + ? { data } + : { data, errors: [...errors.values()] }, + incrementalDataRecords, + }; + } + + if (errors === undefined) { + return { + deferredFragmentRecords, + path: pathToArray(path), + result: { data }, + incrementalDataRecords, + }; + } + return { deferredFragmentRecords, path: pathToArray(path), - result: - errors === undefined ? { data: result[0] } : { data: result[0], errors }, - incrementalDataRecords: result[1], + result: { data, errors: [...errors.values()] }, + incrementalDataRecords: filterIncrementalDataRecords( + path, + errors, + incrementalDataRecords, + ), }; } @@ -2416,7 +2422,7 @@ function firstSyncStreamItems( initialPath, initialItem, exeContext, - { errors: undefined }, + { errors: undefined, incrementalDataRecords: undefined }, fieldGroup, info, itemType, @@ -2438,7 +2444,7 @@ function firstSyncStreamItems( currentPath, item, exeContext, - { errors: undefined }, + { errors: undefined, incrementalDataRecords: undefined }, fieldGroup, info, itemType, @@ -2556,7 +2562,7 @@ async function getNextAsyncStreamItemsResult( itemPath, iteration.value, exeContext, - { errors: undefined }, + { errors: undefined, incrementalDataRecords: undefined }, fieldGroup, info, itemType, @@ -2601,11 +2607,7 @@ function completeStreamItems( new Map(), ).then( (resolvedItem) => - buildStreamItemsResult( - incrementalContext.errors, - streamRecord, - resolvedItem, - ), + buildStreamItemsResult(incrementalContext, streamRecord, resolvedItem), (error) => ({ streamRecord, errors: withError(incrementalContext.errors, error), @@ -2613,10 +2615,10 @@ function completeStreamItems( ); } - let result: PromiseOrValue>; + let completedItem; try { try { - result = completeValue( + completedItem = completeValue( exeContext, itemType, fieldGroup, @@ -2635,7 +2637,7 @@ function completeStreamItems( itemPath, incrementalContext, ); - result = [null, undefined]; + completedItem = null; } } catch (error) { return { @@ -2644,8 +2646,8 @@ function completeStreamItems( }; } - if (isPromise(result)) { - return result + if (isPromise(completedItem)) { + return completedItem .then(undefined, (rawError) => { handleFieldError( rawError, @@ -2655,12 +2657,12 @@ function completeStreamItems( itemPath, incrementalContext, ); - return [null, undefined] as GraphQLWrappedResult; + return null; }) .then( (resolvedItem) => buildStreamItemsResult( - incrementalContext.errors, + incrementalContext, streamRecord, resolvedItem, ), @@ -2672,26 +2674,48 @@ function completeStreamItems( } return buildStreamItemsResult( - incrementalContext.errors, + incrementalContext, streamRecord, - result, + completedItem, ); } function buildStreamItemsResult( - errors: ReadonlyArray | undefined, + incrementalContext: IncrementalContext, streamRecord: SubsequentResultRecord, - result: GraphQLWrappedResult, + completedItem: unknown, ): StreamItemsResult { + const { errors, incrementalDataRecords } = incrementalContext; + if (incrementalDataRecords === undefined) { + return { + streamRecord, + result: + errors === undefined + ? { items: [completedItem] } + : { items: [completedItem], errors: [...errors.values()] }, + incrementalDataRecords, + }; + } + + if (errors === undefined) { + return { + streamRecord, + result: { items: [completedItem] }, + incrementalDataRecords, + }; + } + + const path = streamRecord.path; return { streamRecord, - result: - errors === undefined - ? { items: [result[0]] } - : { - items: [result[0]], - errors: [...errors], - }, - incrementalDataRecords: result[1], + result: { + items: [completedItem], + errors: [...errors.values()], + }, + incrementalDataRecords: filterIncrementalDataRecords( + path, + errors, + incrementalDataRecords, + ), }; } diff --git a/src/jsutils/promiseForObject.ts b/src/jsutils/promiseForObject.ts index 25b3413923..ff48d9f218 100644 --- a/src/jsutils/promiseForObject.ts +++ b/src/jsutils/promiseForObject.ts @@ -7,10 +7,9 @@ import type { ObjMap } from './ObjMap.js'; * This is akin to bluebird's `Promise.props`, but implemented only using * `Promise.all` so it will work with any implementation of ES6 promises. */ -export async function promiseForObject( +export async function promiseForObject( object: ObjMap>, - callback: (object: ObjMap) => U, -): Promise { +): Promise> { const keys = Object.keys(object); const values = Object.values(object); @@ -19,5 +18,5 @@ export async function promiseForObject( for (let i = 0; i < keys.length; ++i) { resolvedObject[keys[i]] = resolvedValues[i]; } - return callback(resolvedObject); + return resolvedObject; } From 37c914853816f493b4063f5a71bd0908486d6dba Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 7 Apr 2024 15:19:44 +0300 Subject: [PATCH 4/5] remove completePromisedListItemValue --- src/execution/execute.ts | 128 +++++++-------------------------------- 1 file changed, 21 insertions(+), 107 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 0a938260c0..75fa22c66d 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1134,27 +1134,9 @@ async function completeAsyncIteratorValue( break; } - const item = iteration.value; - // TODO: add tests for stream backed by asyncIterator that returns a promise - /* c8 ignore start */ - if (isPromise(item)) { - completedResults.push( - completePromisedListItemValue( - item, - exeContext, - itemType, - fieldGroup, - info, - itemPath, - incrementalContext, - deferMap, - ), - ); - containsPromise = true; - } else if ( - /* c8 ignore stop */ + if ( completeListItemValue( - item, + iteration.value, completedResults, exeContext, itemType, @@ -1248,24 +1230,7 @@ async function completeAsyncIteratorValueWithPossibleStream( } const item = iteration.value; - // TODO: add tests for stream backed by asyncIterator that returns a promise - /* c8 ignore start */ - if (isPromise(item)) { - completedResults.push( - completePromisedListItemValue( - item, - exeContext, - itemType, - fieldGroup, - info, - itemPath, - incrementalContext, - deferMap, - ), - ); - containsPromise = true; - } else if ( - /* c8 ignore stop */ + if ( completeListItemValue( item, completedResults, @@ -1389,21 +1354,7 @@ function completeIterableValue( // since from here on it is not ever accessed by resolver functions. const itemPath = addPath(path, index, undefined); - if (isPromise(item)) { - completedResults.push( - completePromisedListItemValue( - item, - exeContext, - itemType, - fieldGroup, - info, - itemPath, - incrementalContext, - deferMap, - ), - ); - containsPromise = true; - } else if ( + if ( completeListItemValue( item, completedResults, @@ -1472,21 +1423,7 @@ function completeIterableValueWithPossibleStream( // since from here on it is not ever accessed by resolver functions. const itemPath = addPath(path, index, undefined); - if (isPromise(item)) { - completedResults.push( - completePromisedListItemValue( - item, - exeContext, - itemType, - fieldGroup, - info, - itemPath, - incrementalContext, - deferMap, - ), - ); - containsPromise = true; - } else if ( + if ( completeListItemValue( item, completedResults, @@ -1525,6 +1462,22 @@ function completeListItemValue( incrementalContext: IncrementalContext | undefined, deferMap: ReadonlyMap | undefined, ): boolean { + if (isPromise(item)) { + completedResults.push( + completePromisedValue( + exeContext, + itemType, + fieldGroup, + info, + itemPath, + item, + incrementalContext, + deferMap, + ), + ); + return true; + } + try { const completedItem = completeValue( exeContext, @@ -1571,45 +1524,6 @@ function completeListItemValue( return false; } -async function completePromisedListItemValue( - item: unknown, - exeContext: ExecutionContext, - itemType: GraphQLOutputType, - fieldGroup: FieldGroup, - info: GraphQLResolveInfo, - itemPath: Path, - incrementalContext: IncrementalContext | undefined, - deferMap: ReadonlyMap | undefined, -): Promise { - try { - const resolved = await item; - let completed = completeValue( - exeContext, - itemType, - fieldGroup, - info, - itemPath, - resolved, - incrementalContext, - deferMap, - ); - if (isPromise(completed)) { - completed = await completed; - } - return completed; - } catch (rawError) { - handleFieldError( - rawError, - exeContext, - itemType, - fieldGroup, - itemPath, - incrementalContext, - ); - return null; - } -} - /** * Complete a Scalar or Enum by serializing to a valid value, returning * null if serialization is not possible. From e5c9a71892c8b2a2f2aa830b5815598ff11e2606 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 7 Apr 2024 15:22:31 +0300 Subject: [PATCH 5/5] remove completePromisedValue --- src/execution/__tests__/nonnull-test.ts | 30 ++--- src/execution/execute.ts | 165 +++++++++++++----------- 2 files changed, 104 insertions(+), 91 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index 12b223a622..d8a8c4e592 100644 --- a/src/execution/__tests__/nonnull-test.ts +++ b/src/execution/__tests__/nonnull-test.ts @@ -259,16 +259,6 @@ describe('Execute: handles non-nullable types', () => { path: ['syncNest', 'syncNest', 'sync'], locations: [{ line: 6, column: 22 }], }, - { - message: promiseError.message, - path: ['syncNest', 'promise'], - locations: [{ line: 5, column: 11 }], - }, - { - message: promiseError.message, - path: ['syncNest', 'syncNest', 'promise'], - locations: [{ line: 6, column: 27 }], - }, { message: syncError.message, path: ['syncNest', 'promiseNest', 'sync'], @@ -284,6 +274,21 @@ describe('Execute: handles non-nullable types', () => { path: ['promiseNest', 'syncNest', 'sync'], locations: [{ line: 12, column: 22 }], }, + { + message: promiseError.message, + path: ['syncNest', 'promise'], + locations: [{ line: 5, column: 11 }], + }, + { + message: promiseError.message, + path: ['syncNest', 'syncNest', 'promise'], + locations: [{ line: 6, column: 27 }], + }, + { + message: syncError.message, + path: ['promiseNest', 'promiseNest', 'sync'], + locations: [{ line: 13, column: 25 }], + }, { message: promiseError.message, path: ['syncNest', 'promiseNest', 'promise'], @@ -299,11 +304,6 @@ describe('Execute: handles non-nullable types', () => { path: ['promiseNest', 'syncNest', 'promise'], locations: [{ line: 12, column: 27 }], }, - { - message: syncError.message, - path: ['promiseNest', 'promiseNest', 'sync'], - locations: [{ line: 13, column: 25 }], - }, { message: promiseError.message, path: ['promiseNest', 'promiseNest', 'promise'], diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 75fa22c66d..831cf38487 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -763,15 +763,33 @@ function executeField( const result = resolveFn(source, args, contextValue, info); if (isPromise(result)) { - return completePromisedValue( - exeContext, - returnType, - fieldGroup, - info, - path, - result, - incrementalContext, - deferMap, + return ( + result + .then((resolved) => + completeValue( + exeContext, + returnType, + fieldGroup, + info, + path, + resolved, + incrementalContext, + deferMap, + ), + ) + // Note: we don't rely on a `catch` method, but we do expect "thenable" + // to take a second callback for the error case. + .then(undefined, (rawError) => { + handleFieldError( + rawError, + exeContext, + returnType, + fieldGroup, + path, + incrementalContext, + ); + return null; + }) ); } @@ -987,46 +1005,6 @@ function completeValue( ); } -async function completePromisedValue( - exeContext: ExecutionContext, - returnType: GraphQLOutputType, - fieldGroup: FieldGroup, - info: GraphQLResolveInfo, - path: Path, - result: Promise, - incrementalContext: IncrementalContext | undefined, - deferMap: ReadonlyMap | undefined, -): Promise { - try { - const resolved = await result; - let completed = completeValue( - exeContext, - returnType, - fieldGroup, - info, - path, - resolved, - incrementalContext, - deferMap, - ); - - if (isPromise(completed)) { - completed = await completed; - } - return completed; - } catch (rawError) { - handleFieldError( - rawError, - exeContext, - returnType, - fieldGroup, - path, - incrementalContext, - ); - return null; - } -} - /** * Returns an object containing info for streaming if a field should be * streamed based on the experimental flag, stream directive present and @@ -1464,16 +1442,32 @@ function completeListItemValue( ): boolean { if (isPromise(item)) { completedResults.push( - completePromisedValue( - exeContext, - itemType, - fieldGroup, - info, - itemPath, - item, - incrementalContext, - deferMap, - ), + item + .then((resolved) => + completeValue( + exeContext, + itemType, + fieldGroup, + info, + itemPath, + resolved, + incrementalContext, + deferMap, + ), + ) + // Note: we don't rely on a `catch` method, but we do expect "thenable" + // to take a second callback for the error case. + .then(undefined, (rawError) => { + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); + return null; + }), ); return true; } @@ -2510,23 +2504,42 @@ function completeStreamItems( itemType: GraphQLOutputType, ): PromiseOrValue { if (isPromise(item)) { - return completePromisedValue( - exeContext, - itemType, - fieldGroup, - info, - itemPath, - item, - incrementalContext, - new Map(), - ).then( - (resolvedItem) => - buildStreamItemsResult(incrementalContext, streamRecord, resolvedItem), - (error) => ({ - streamRecord, - errors: withError(incrementalContext.errors, error), - }), - ); + return item + .then((resolved) => + completeValue( + exeContext, + itemType, + fieldGroup, + info, + itemPath, + resolved, + incrementalContext, + new Map(), + ), + ) + .then(undefined, (rawError) => { + handleFieldError( + rawError, + exeContext, + itemType, + fieldGroup, + itemPath, + incrementalContext, + ); + return null; + }) + .then( + (resolvedItem) => + buildStreamItemsResult( + incrementalContext, + streamRecord, + resolvedItem, + ), + (error) => ({ + streamRecord, + errors: withError(incrementalContext.errors, error), + }), + ); } let completedItem;