diff --git a/src/execution/IncrementalGraph.ts b/src/execution/IncrementalGraph.ts index 53b60d2b32..e71929e895 100644 --- a/src/execution/IncrementalGraph.ts +++ b/src/execution/IncrementalGraph.ts @@ -35,9 +35,22 @@ export class IncrementalGraph { } getNewRootNodes( + newDeferredFragmentRecords: + | ReadonlyArray + | undefined, incrementalDataRecords: ReadonlyArray, ): ReadonlyArray { const initialResultChildren = new Set(); + + if (newDeferredFragmentRecords !== undefined) { + for (const deferredFragmentRecord of newDeferredFragmentRecords) { + this._addDeferredFragment( + deferredFragmentRecord, + initialResultChildren, + ); + } + } + this._addIncrementalDataRecords( incrementalDataRecords, undefined, @@ -49,8 +62,17 @@ export class IncrementalGraph { addCompletedSuccessfulExecutionGroup( successfulExecutionGroup: SuccessfulExecutionGroup, ): void { - const { pendingExecutionGroup, incrementalDataRecords } = - successfulExecutionGroup; + const { + pendingExecutionGroup, + newDeferredFragmentRecords, + incrementalDataRecords, + } = successfulExecutionGroup; + + if (newDeferredFragmentRecords !== undefined) { + for (const deferredFragmentRecord of newDeferredFragmentRecords) { + this._addDeferredFragment(deferredFragmentRecord, undefined); + } + } const deferredFragmentRecords = pendingExecutionGroup.deferredFragmentRecords; @@ -135,11 +157,7 @@ export class IncrementalGraph { removeDeferredFragment( deferredFragmentRecord: DeferredFragmentRecord, ): boolean { - if (!this._rootNodes.has(deferredFragmentRecord)) { - return false; - } - this._rootNodes.delete(deferredFragmentRecord); - return true; + return this._rootNodes.delete(deferredFragmentRecord); } removeStream(streamRecord: StreamRecord): void { @@ -154,10 +172,6 @@ export class IncrementalGraph { for (const incrementalDataRecord of incrementalDataRecords) { if (isPendingExecutionGroup(incrementalDataRecord)) { for (const deferredFragmentRecord of incrementalDataRecord.deferredFragmentRecords) { - this._addDeferredFragment( - deferredFragmentRecord, - initialResultChildren, - ); deferredFragmentRecord.pendingExecutionGroups.add( incrementalDataRecord, ); @@ -170,7 +184,6 @@ export class IncrementalGraph { initialResultChildren.add(incrementalDataRecord); } else { for (const parent of parents) { - this._addDeferredFragment(parent, initialResultChildren); parent.children.add(incrementalDataRecord); } } @@ -219,9 +232,6 @@ export class IncrementalGraph { deferredFragmentRecord: DeferredFragmentRecord, initialResultChildren: Set | undefined, ): void { - if (this._rootNodes.has(deferredFragmentRecord)) { - return; - } const parent = deferredFragmentRecord.parent; if (parent === undefined) { invariant(initialResultChildren !== undefined); @@ -229,7 +239,6 @@ export class IncrementalGraph { return; } parent.children.add(deferredFragmentRecord); - this._addDeferredFragment(parent, initialResultChildren); } private _onExecutionGroup( @@ -251,6 +260,7 @@ export class IncrementalGraph { private async _onStreamItems(streamRecord: StreamRecord): Promise { let items: Array = []; let errors: Array = []; + let newDeferredFragmentRecords: Array = []; let incrementalDataRecords: Array = []; const streamItemQueue = streamRecord.streamItemQueue; let streamItemRecord: StreamItemRecord | undefined; @@ -268,10 +278,12 @@ export class IncrementalGraph { errors.length > 0 /* c8 ignore start */ ? { items, errors } /* c8 ignore stop */ : { items }, + newDeferredFragmentRecords, incrementalDataRecords, }); items = []; errors = []; + newDeferredFragmentRecords = []; incrementalDataRecords = []; } // eslint-disable-next-line no-await-in-loop @@ -286,6 +298,7 @@ export class IncrementalGraph { this._enqueue({ streamRecord, result: errors.length > 0 ? { items, errors } : { items }, + newDeferredFragmentRecords, incrementalDataRecords, }); } @@ -303,6 +316,9 @@ export class IncrementalGraph { if (result.errors !== undefined) { errors.push(...result.errors); } + if (result.newDeferredFragmentRecords !== undefined) { + newDeferredFragmentRecords.push(...result.newDeferredFragmentRecords); + } if (result.incrementalDataRecords !== undefined) { incrementalDataRecords.push(...result.incrementalDataRecords); } diff --git a/src/execution/IncrementalPublisher.ts b/src/execution/IncrementalPublisher.ts index b826a9b9c5..a2ea92f2f1 100644 --- a/src/execution/IncrementalPublisher.ts +++ b/src/execution/IncrementalPublisher.ts @@ -33,12 +33,14 @@ export function buildIncrementalResponse( context: IncrementalPublisherContext, result: ObjMap, errors: ReadonlyArray | undefined, + newDeferredFragmentRecords: ReadonlyArray | undefined, incrementalDataRecords: ReadonlyArray, ): ExperimentalIncrementalExecutionResults { const incrementalPublisher = new IncrementalPublisher(context); return incrementalPublisher.buildResponse( result, errors, + newDeferredFragmentRecords, incrementalDataRecords, ); } @@ -74,9 +76,13 @@ class IncrementalPublisher { buildResponse( data: ObjMap, errors: ReadonlyArray | undefined, + newDeferredFragmentRecords: + | ReadonlyArray + | undefined, incrementalDataRecords: ReadonlyArray, ): ExperimentalIncrementalExecutionResults { const newRootNodes = this._incrementalGraph.getNewRootNodes( + newDeferredFragmentRecords, incrementalDataRecords, ); @@ -228,18 +234,16 @@ class IncrementalPublisher { if (isFailedExecutionGroup(completedExecutionGroup)) { for (const deferredFragmentRecord of completedExecutionGroup .pendingExecutionGroup.deferredFragmentRecords) { - const id = deferredFragmentRecord.id; if ( - !this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord) + this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord) ) { - // This can occur if multiple deferred grouped field sets error for a fragment. - continue; + const id = deferredFragmentRecord.id; + invariant(id !== undefined); + context.completed.push({ + id, + errors: completedExecutionGroup.errors, + }); } - invariant(id !== undefined); - context.completed.push({ - id, - errors: completedExecutionGroup.errors, - }); } return; } @@ -316,9 +320,11 @@ class IncrementalPublisher { context.incremental.push(incrementalEntry); - const incrementalDataRecords = streamItemsResult.incrementalDataRecords; + const { newDeferredFragmentRecords, incrementalDataRecords } = + streamItemsResult; if (incrementalDataRecords !== undefined) { const newRootNodes = this._incrementalGraph.getNewRootNodes( + newDeferredFragmentRecords, incrementalDataRecords, ); context.pending.push(...this._toPendingResults(newRootNodes)); diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index 97dcfeceb6..8b2071a87a 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -99,6 +99,7 @@ const a = new GraphQLObjectType({ fields: { b: { type: b }, someField: { type: GraphQLString }, + nonNullErrorField: { type: new GraphQLNonNull(GraphQLString) }, }, name: 'a', }); @@ -1580,8 +1581,8 @@ describe('Execute: defer directive', () => { a: {}, }, pending: [ - { id: '0', path: [] }, - { id: '1', path: ['a'] }, + { id: '0', path: ['a'] }, + { id: '1', path: [] }, ], hasNext: true, }, @@ -1589,17 +1590,17 @@ describe('Execute: defer directive', () => { incremental: [ { data: { b: { c: {} } }, - id: '1', + id: '0', }, { data: { d: 'd' }, - id: '1', + id: '0', subPath: ['b', 'c'], }, ], completed: [ { - id: '0', + id: '1', errors: [ { message: @@ -1609,7 +1610,7 @@ describe('Execute: defer directive', () => { }, ], }, - { id: '1' }, + { id: '0' }, ], hasNext: false, }, @@ -1652,8 +1653,8 @@ describe('Execute: defer directive', () => { a: {}, }, pending: [ - { id: '0', path: [] }, - { id: '1', path: ['a'] }, + { id: '0', path: ['a'] }, + { id: '1', path: [] }, ], hasNext: true, }, @@ -1661,18 +1662,18 @@ describe('Execute: defer directive', () => { incremental: [ { data: { b: { c: {} } }, - id: '1', + id: '0', }, { data: { d: 'd' }, - id: '0', + id: '1', subPath: ['a', 'b', 'c'], }, ], completed: [ - { id: '0' }, + { id: '1' }, { - id: '1', + id: '0', errors: [ { message: @@ -1688,6 +1689,83 @@ describe('Execute: defer directive', () => { ]); }); + it('Nulls cross defer boundaries, failed fragment with slower shared child execution groups', async () => { + const document = parse(` + query { + ... @defer { + a { + someField + nonNullErrorField + b { + c { + d + } + } + } + } + a { + ... @defer { + someField + b { + e { + f + } + } + } + } + } + `); + const result = await complete(document, { + a: { + b: { c: { d: 'd' }, e: { f: 'f' } }, + someField: Promise.resolve('someField'), + }, + }); + expectJSON(result).toDeepEqual([ + { + data: { + a: {}, + }, + pending: [ + { id: '0', path: ['a'] }, + { id: '1', path: [] }, + ], + hasNext: true, + }, + { + completed: [ + { + id: '1', + errors: [ + { + message: + 'Cannot return null for non-nullable field a.nonNullErrorField.', + locations: [{ line: 6, column: 13 }], + path: ['a', 'nonNullErrorField'], + }, + ], + }, + ], + hasNext: true, + }, + { + incremental: [ + { + data: { b: {}, someField: 'someField' }, + id: '0', + }, + { + data: { e: { f: 'f' } }, + id: '0', + subPath: ['b'], + }, + ], + completed: [{ id: '0' }], + hasNext: false, + }, + ]); + }); + it('Handles multiple erroring deferred grouped field sets', async () => { const document = parse(` query { @@ -1882,8 +1960,8 @@ describe('Execute: defer directive', () => { a: {}, }, pending: [ - { id: '0', path: [] }, - { id: '1', path: ['a'] }, + { id: '0', path: ['a'] }, + { id: '1', path: [] }, ], hasNext: true, }, @@ -1891,21 +1969,21 @@ describe('Execute: defer directive', () => { incremental: [ { data: { b: { c: {} } }, - id: '1', + id: '0', }, { data: { d: 'd' }, - id: '1', + id: '0', subPath: ['b', 'c'], }, ], - completed: [{ id: '1' }], + completed: [{ id: '0' }], hasNext: true, }, { completed: [ { - id: '0', + id: '1', errors: [ { message: diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 6fd3fc5b4a..7e772120ef 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -206,6 +206,7 @@ export interface StreamUsage { interface GraphQLWrappedResult { rawResult: T; + newDeferredFragmentRecords: Array | undefined; incrementalDataRecords: Array | undefined; } @@ -403,7 +404,11 @@ function buildDataResponse( exeContext: ExecutionContext, graphqlWrappedResult: GraphQLWrappedResult>, ): ExecutionResult | ExperimentalIncrementalExecutionResults { - const { rawResult: data, incrementalDataRecords } = graphqlWrappedResult; + const { + rawResult: data, + newDeferredFragmentRecords, + incrementalDataRecords, + } = graphqlWrappedResult; const errors = exeContext.errors; if (incrementalDataRecords === undefined) { exeContext.abortSignalListener?.disconnect(); @@ -414,6 +419,7 @@ function buildDataResponse( exeContext, data, errors, + newDeferredFragmentRecords, incrementalDataRecords, ); } @@ -566,13 +572,18 @@ function executeRootExecutionPlan( undefined, ); } - const newDeferMap = getNewDeferMap(newDeferUsages, undefined, undefined); + + const { newDeferredFragmentRecords, newDeferMap } = getNewDeferMap( + newDeferUsages, + undefined, + undefined, + ); const { groupedFieldSet, newGroupedFieldSets } = buildExecutionPlan( originalGroupedFieldSet, ); - const graphqlWrappedResult = executeRootGroupedFieldSet( + let graphqlWrappedResult = executeRootGroupedFieldSet( exeContext, operation, rootType, @@ -581,6 +592,11 @@ function executeRootExecutionPlan( newDeferMap, ); + graphqlWrappedResult = withNewDeferredFragmentRecords( + graphqlWrappedResult, + newDeferredFragmentRecords, + ); + if (newGroupedFieldSets.size > 0) { const newPendingExecutionGroups = collectExecutionGroups( exeContext, @@ -600,6 +616,34 @@ function executeRootExecutionPlan( return graphqlWrappedResult; } +function withNewDeferredFragmentRecords( + result: PromiseOrValue>>, + newDeferredFragmentRecords: ReadonlyArray, +): PromiseOrValue>> { + if (isPromise(result)) { + return result.then((resolved) => { + addNewDeferredFragmentRecords(resolved, newDeferredFragmentRecords); + return resolved; + }); + } + addNewDeferredFragmentRecords(result, newDeferredFragmentRecords); + return result; +} + +function addNewDeferredFragmentRecords( + result: GraphQLWrappedResult, + newDeferredFragmentRecords: ReadonlyArray | undefined, +): void { + if (newDeferredFragmentRecords === undefined) { + return; + } + if (result.newDeferredFragmentRecords === undefined) { + result.newDeferredFragmentRecords = [...newDeferredFragmentRecords]; + } else { + result.newDeferredFragmentRecords.push(...newDeferredFragmentRecords); + } +} + function withNewExecutionGroups( result: PromiseOrValue>>, newPendingExecutionGroups: ReadonlyArray, @@ -706,6 +750,10 @@ function executeFieldsSerially( if (isPromise(result)) { return result.then((resolved) => { graphqlWrappedResult.rawResult[responseName] = resolved.rawResult; + addNewDeferredFragmentRecords( + graphqlWrappedResult, + resolved.newDeferredFragmentRecords, + ); addIncrementalDataRecords( graphqlWrappedResult, resolved.incrementalDataRecords, @@ -714,6 +762,10 @@ function executeFieldsSerially( }); } graphqlWrappedResult.rawResult[responseName] = result.rawResult; + addNewDeferredFragmentRecords( + graphqlWrappedResult, + result.newDeferredFragmentRecords, + ); addIncrementalDataRecords( graphqlWrappedResult, result.incrementalDataRecords, @@ -722,6 +774,7 @@ function executeFieldsSerially( }, { rawResult: Object.create(null), + newDeferredFragmentRecords: undefined, incrementalDataRecords: undefined, }, ); @@ -757,6 +810,7 @@ function executeFields( const results = Object.create(null); const graphqlWrappedResult: GraphQLWrappedResult> = { rawResult: results, + newDeferredFragmentRecords: undefined, incrementalDataRecords: undefined, }; let containsPromise = false; @@ -777,6 +831,10 @@ function executeFields( if (result !== undefined) { if (isPromise(result)) { results[responseName] = result.then((resolved) => { + addNewDeferredFragmentRecords( + graphqlWrappedResult, + resolved.newDeferredFragmentRecords, + ); addIncrementalDataRecords( graphqlWrappedResult, resolved.incrementalDataRecords, @@ -786,6 +844,10 @@ function executeFields( containsPromise = true; } else { results[responseName] = result.rawResult; + addNewDeferredFragmentRecords( + graphqlWrappedResult, + result.newDeferredFragmentRecords, + ); addIncrementalDataRecords( graphqlWrappedResult, result.incrementalDataRecords, @@ -815,6 +877,7 @@ function executeFields( // same map, but with any promises replaced with the values they resolved to. return promiseForObject(results, (resolved) => ({ rawResult: resolved, + newDeferredFragmentRecords: graphqlWrappedResult.newDeferredFragmentRecords, incrementalDataRecords: graphqlWrappedResult.incrementalDataRecords, })); } @@ -914,7 +977,11 @@ function executeField( path, incrementalContext, ); - return { rawResult: null, incrementalDataRecords: undefined }; + return { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; }); } return completed; @@ -927,7 +994,11 @@ function executeField( path, incrementalContext, ); - return { rawResult: null, incrementalDataRecords: undefined }; + return { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; } } @@ -1050,7 +1121,11 @@ function completeValue( // If result value is null or undefined then return null. if (result == null) { - return { rawResult: null, incrementalDataRecords: undefined }; + return { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; } // If field type is List, complete each item in the list with the inner type @@ -1072,6 +1147,7 @@ function completeValue( if (isLeafType(returnType)) { return { rawResult: completeLeafValue(returnType, result), + newDeferredFragmentRecords: undefined, incrementalDataRecords: undefined, }; } @@ -1149,7 +1225,11 @@ async function completePromisedValue( path, incrementalContext, ); - return { rawResult: null, incrementalDataRecords: undefined }; + return { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; } } @@ -1249,6 +1329,7 @@ async function completeAsyncIteratorValue( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = { rawResult: completedResults, + newDeferredFragmentRecords: undefined, incrementalDataRecords: undefined, }; let index = 0; @@ -1370,6 +1451,8 @@ async function completeAsyncIteratorValue( return containsPromise ? /* c8 ignore start */ Promise.all(completedResults).then((resolved) => ({ rawResult: resolved, + newDeferredFragmentRecords: + graphqlWrappedResult.newDeferredFragmentRecords, incrementalDataRecords: graphqlWrappedResult.incrementalDataRecords, })) : /* c8 ignore stop */ graphqlWrappedResult; @@ -1444,6 +1527,7 @@ function completeIterableValue( const completedResults: Array = []; const graphqlWrappedResult: GraphQLWrappedResult> = { rawResult: completedResults, + newDeferredFragmentRecords: undefined, incrementalDataRecords: undefined, }; let index = 0; @@ -1520,6 +1604,8 @@ function completeIterableValue( return containsPromise ? Promise.all(completedResults).then((resolved) => ({ rawResult: resolved, + newDeferredFragmentRecords: + graphqlWrappedResult.newDeferredFragmentRecords, incrementalDataRecords: graphqlWrappedResult.incrementalDataRecords, })) : graphqlWrappedResult; @@ -1560,6 +1646,10 @@ function completeListItemValue( completedResults.push( completedItem.then( (resolved) => { + addNewDeferredFragmentRecords( + parent, + resolved.newDeferredFragmentRecords, + ); addIncrementalDataRecords(parent, resolved.incrementalDataRecords); return resolved.rawResult; }, @@ -1580,6 +1670,10 @@ function completeListItemValue( } completedResults.push(completedItem.rawResult); + addNewDeferredFragmentRecords( + parent, + completedItem.newDeferredFragmentRecords, + ); addIncrementalDataRecords(parent, completedItem.incrementalDataRecords); } catch (rawError) { handleFieldError( @@ -1625,6 +1719,7 @@ async function completePromisedListItemValue( if (isPromise(completed)) { completed = await completed; } + addNewDeferredFragmentRecords(parent, completed.newDeferredFragmentRecords); addIncrementalDataRecords(parent, completed.incrementalDataRecords); return completed.rawResult; } catch (rawError) { @@ -1851,7 +1946,11 @@ function getNewDeferMap( newDeferUsages: ReadonlyArray, deferMap?: ReadonlyMap, path?: Path, -): ReadonlyMap { +): { + newDeferredFragmentRecords: Array; + newDeferMap: ReadonlyMap; +} { + const newDeferredFragmentRecords: Array = []; const newDeferMap = new Map(deferMap); // For each new deferUsage object: @@ -1870,11 +1969,17 @@ function getNewDeferMap( parent, ); + // Add the new record to the list of new records. + newDeferredFragmentRecords.push(deferredFragmentRecord); + // Update the map. newDeferMap.set(newDeferUsage, deferredFragmentRecord); } - return newDeferMap; + return { + newDeferredFragmentRecords, + newDeferMap, + }; } function deferredFragmentRecordFromDeferUsage( @@ -1946,14 +2051,18 @@ function executeSubExecutionPlan( ); } - const newDeferMap = getNewDeferMap(newDeferUsages, deferMap, path); + const { newDeferredFragmentRecords, newDeferMap } = getNewDeferMap( + newDeferUsages, + deferMap, + path, + ); const { groupedFieldSet, newGroupedFieldSets } = buildSubExecutionPlan( originalGroupedFieldSet, incrementalContext?.deferUsageSet, ); - const graphqlWrappedResult = executeFields( + let graphqlWrappedResult = executeFields( exeContext, returnType, sourceValue, @@ -1963,6 +2072,13 @@ function executeSubExecutionPlan( newDeferMap, ); + if (newDeferredFragmentRecords.length > 0) { + graphqlWrappedResult = withNewDeferredFragmentRecords( + graphqlWrappedResult, + newDeferredFragmentRecords, + ); + } + if (newGroupedFieldSets.size > 0) { const newPendingExecutionGroups = collectExecutionGroups( exeContext, @@ -2477,11 +2593,16 @@ function buildCompletedExecutionGroup( path: Path | undefined, result: GraphQLWrappedResult>, ): CompletedExecutionGroup { - const { rawResult: data, incrementalDataRecords } = result; + const { + rawResult: data, + newDeferredFragmentRecords, + incrementalDataRecords, + } = result; return { pendingExecutionGroup, path: pathToArray(path), result: errors === undefined ? { data } : { data, errors }, + newDeferredFragmentRecords, incrementalDataRecords, }; } @@ -2726,7 +2847,11 @@ function completeStreamItem( itemPath, incrementalContext, ); - result = { rawResult: null, incrementalDataRecords: undefined }; + result = { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; } } catch (error) { incrementalContext.completed = true; @@ -2746,7 +2871,11 @@ function completeStreamItem( itemPath, incrementalContext, ); - return { rawResult: null, incrementalDataRecords: undefined }; + return { + rawResult: null, + newDeferredFragmentRecords: undefined, + incrementalDataRecords: undefined, + }; }) .then( (resolvedItem) => { @@ -2770,10 +2899,15 @@ function buildStreamItemResult( errors: ReadonlyArray | undefined, result: GraphQLWrappedResult, ): StreamItemResult { - const { rawResult: item, incrementalDataRecords } = result; + const { + rawResult: item, + newDeferredFragmentRecords, + incrementalDataRecords, + } = result; return { item, errors, + newDeferredFragmentRecords, incrementalDataRecords, }; } diff --git a/src/execution/types.ts b/src/execution/types.ts index 87d47e3efc..50a3b8ad37 100644 --- a/src/execution/types.ts +++ b/src/execution/types.ts @@ -187,6 +187,7 @@ export interface SuccessfulExecutionGroup { pendingExecutionGroup: PendingExecutionGroup; path: Array; result: ExecutionGroupResult; + newDeferredFragmentRecords: ReadonlyArray | undefined; incrementalDataRecords: ReadonlyArray | undefined; errors?: never; } @@ -247,6 +248,9 @@ export function isDeferredFragmentRecord( export interface StreamItemResult { item?: unknown; + newDeferredFragmentRecords?: + | ReadonlyArray + | undefined; incrementalDataRecords?: ReadonlyArray | undefined; errors?: ReadonlyArray | undefined; } @@ -264,6 +268,9 @@ export interface StreamItemsResult { streamRecord: StreamRecord; errors?: ReadonlyArray; result?: StreamItemsRecordResult; + newDeferredFragmentRecords?: + | ReadonlyArray + | undefined; incrementalDataRecords?: ReadonlyArray | undefined; }