From 21b49657e449c53b210c6a2126335006ffd601e9 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 19 Jun 2024 22:50:29 +0300 Subject: [PATCH 1/2] fix(incremental): properly initiate nested deferred grouped field sets when early execution is disabled, deferred grouped field sets should start immediately if and only if one of their deferred fragments is released as pending see: graphql/defer-stream-wg#84 --- src/execution/IncrementalGraph.ts | 21 ++-- src/execution/__tests__/defer-test.ts | 132 +++++++++++++++++++++++++- src/execution/execute.ts | 29 ++++-- src/execution/types.ts | 21 ++++ 4 files changed, 180 insertions(+), 23 deletions(-) diff --git a/src/execution/IncrementalGraph.ts b/src/execution/IncrementalGraph.ts index 7469a36fa5..6480c4a89b 100644 --- a/src/execution/IncrementalGraph.ts +++ b/src/execution/IncrementalGraph.ts @@ -8,6 +8,7 @@ import type { GraphQLError } from '../error/GraphQLError.js'; import type { DeferredFragmentRecord, DeferredGroupedFieldSetRecord, + DeferredGroupedFieldSetResult, IncrementalDataRecord, IncrementalDataRecordResult, ReconcilableDeferredGroupedFieldSetResult, @@ -113,12 +114,10 @@ export class IncrementalGraph { reconcilableResults: ReadonlyArray; } | undefined { - // TODO: add test case? - /* c8 ignore next 3 */ - if (!this._rootNodes.has(deferredFragmentRecord)) { - return; - } - if (deferredFragmentRecord.deferredGroupedFieldSetRecords.size > 0) { + if ( + !this._rootNodes.has(deferredFragmentRecord) || + deferredFragmentRecord.deferredGroupedFieldSetRecords.size > 0 + ) { return; } const reconcilableResults = Array.from( @@ -202,6 +201,7 @@ export class IncrementalGraph { for (const node of maybeEmptyNewRootNodes) { if (isDeferredFragmentRecord(node)) { if (node.deferredGroupedFieldSetRecords.size > 0) { + node.setAsPending(); for (const deferredGroupedFieldSetRecord of node.deferredGroupedFieldSetRecords) { if (!this._completesRootNode(deferredGroupedFieldSetRecord)) { this._onDeferredGroupedFieldSet(deferredGroupedFieldSetRecord); @@ -253,12 +253,9 @@ export class IncrementalGraph { private _onDeferredGroupedFieldSet( deferredGroupedFieldSetRecord: DeferredGroupedFieldSetRecord, ): void { - const deferredGroupedFieldSetResult = deferredGroupedFieldSetRecord.result; - const result = - deferredGroupedFieldSetResult instanceof BoxedPromiseOrValue - ? deferredGroupedFieldSetResult.value - : deferredGroupedFieldSetResult().value; - + const result = ( + deferredGroupedFieldSetRecord.result as BoxedPromiseOrValue + ).value; if (isPromise(result)) { // eslint-disable-next-line @typescript-eslint/no-floating-promises result.then((resolved) => this._enqueue(resolved)); diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index e74aebb9ae..2853415bd0 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -1,10 +1,12 @@ -import { expect } from 'chai'; +import { assert, expect } from 'chai'; import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON.js'; import { expectPromise } from '../../__testUtils__/expectPromise.js'; import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js'; +import { promiseWithResolvers } from '../../jsutils/promiseWithResolvers.js'; + import type { DocumentNode } from '../../language/ast.js'; import { parse } from '../../language/parser.js'; @@ -856,6 +858,134 @@ describe('Execute: defer directive', () => { ]); }); + it('Initiates all deferred grouped field sets immediately if and only if they have been released as pending', async () => { + const document = parse(` + query { + ... @defer { + a { + ... @defer { + b { + c { d } + } + } + } + } + ... @defer { + a { + someField + ... @defer { + b { + e { f } + } + } + } + } + } + `); + + const { promise: slowFieldPromise, resolve: resolveSlowField } = + promiseWithResolvers(); + let cResolverCalled = false; + let eResolverCalled = false; + const executeResult = experimentalExecuteIncrementally({ + schema, + document, + rootValue: { + a: { + someField: slowFieldPromise, + b: { + c: () => { + cResolverCalled = true; + return { d: 'd' }; + }, + e: () => { + eResolverCalled = true; + return { f: 'f' }; + }, + }, + }, + }, + enableEarlyExecution: false, + }); + + assert('initialResult' in executeResult); + + const result1 = executeResult.initialResult; + expectJSON(result1).toDeepEqual({ + data: {}, + pending: [ + { id: '0', path: [] }, + { id: '1', path: [] }, + ], + hasNext: true, + }); + + const iterator = executeResult.subsequentResults[Symbol.asyncIterator](); + + expect(cResolverCalled).to.equal(false); + expect(eResolverCalled).to.equal(false); + + const result2 = await iterator.next(); + expectJSON(result2).toDeepEqual({ + value: { + pending: [{ id: '2', path: ['a'] }], + incremental: [ + { + data: { a: {} }, + id: '0', + }, + { + data: { b: {} }, + id: '2', + }, + { + data: { c: { d: 'd' } }, + id: '2', + subPath: ['b'], + }, + ], + completed: [{ id: '0' }, { id: '2' }], + hasNext: true, + }, + done: false, + }); + + expect(cResolverCalled).to.equal(true); + expect(eResolverCalled).to.equal(false); + + resolveSlowField('someField'); + + const result3 = await iterator.next(); + expectJSON(result3).toDeepEqual({ + value: { + pending: [{ id: '3', path: ['a'] }], + incremental: [ + { + data: { someField: 'someField' }, + id: '1', + subPath: ['a'], + }, + { + data: { e: { f: 'f' } }, + id: '3', + subPath: ['b'], + }, + ], + completed: [{ id: '1' }, { id: '3' }], + hasNext: false, + }, + done: false, + }); + + expect(eResolverCalled).to.equal(true); + + const result4 = await iterator.next(); + expectJSON(result4).toDeepEqual({ + value: undefined, + done: true, + }); + }); + it('Can deduplicate multiple defers on the same object', async () => { const document = parse(` query { diff --git a/src/execution/execute.ts b/src/execution/execute.ts index f32036f645..92d6977ed1 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -2114,16 +2114,25 @@ function executeDeferredGroupedFieldSets( deferMap, ); - const shouldDeferThisDeferUsageSet = shouldDefer( - parentDeferUsages, - deferUsageSet, - ); - - deferredGroupedFieldSetRecord.result = shouldDeferThisDeferUsageSet - ? exeContext.enableEarlyExecution - ? new BoxedPromiseOrValue(Promise.resolve().then(executor)) - : () => new BoxedPromiseOrValue(executor()) - : new BoxedPromiseOrValue(executor()); + if (exeContext.enableEarlyExecution) { + deferredGroupedFieldSetRecord.result = new BoxedPromiseOrValue( + shouldDefer(parentDeferUsages, deferUsageSet) + ? Promise.resolve().then(executor) + : executor(), + ); + } else { + deferredGroupedFieldSetRecord.result = () => + new BoxedPromiseOrValue(executor()); + const resolveThunk = () => { + const maybeThunk = deferredGroupedFieldSetRecord.result; + if (!(maybeThunk instanceof BoxedPromiseOrValue)) { + deferredGroupedFieldSetRecord.result = maybeThunk(); + } + }; + for (const deferredFragmentRecord of deferredFragmentRecords) { + deferredFragmentRecord.onPending(resolveThunk); + } + } newDeferredGroupedFieldSetRecords.push(deferredGroupedFieldSetRecord); } diff --git a/src/execution/types.ts b/src/execution/types.ts index c88ae9986e..76205ccaec 100644 --- a/src/execution/types.ts +++ b/src/execution/types.ts @@ -224,6 +224,9 @@ export class DeferredFragmentRecord { reconcilableResults: Set; children: Set; + private pending: boolean; + private fns: Array<() => void>; + constructor( path: Path | undefined, label: string | undefined, @@ -235,6 +238,24 @@ export class DeferredFragmentRecord { this.deferredGroupedFieldSetRecords = new Set(); this.reconcilableResults = new Set(); this.children = new Set(); + this.pending = false; + this.fns = []; + } + + onPending(fn: () => void): void { + if (this.pending) { + fn(); + } else { + this.fns.push(fn); + } + } + + setAsPending(): void { + this.pending = true; + let fn; + while ((fn = this.fns.shift()) !== undefined) { + fn(); + } } } From a5e24f081c6721204219a9bf92e8515d2a7cb262 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 3 Jul 2024 22:25:13 +0300 Subject: [PATCH 2/2] add additional test --- src/execution/__tests__/defer-test.ts | 121 +++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index 2853415bd0..04f3399ffb 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -858,7 +858,7 @@ describe('Execute: defer directive', () => { ]); }); - it('Initiates all deferred grouped field sets immediately if and only if they have been released as pending', async () => { + it('Initiates deferred grouped field sets only if they have been released as pending', async () => { const document = parse(` query { ... @defer { @@ -986,6 +986,125 @@ describe('Execute: defer directive', () => { }); }); + it('Initiates all deferred grouped field sets immediately once they have been released as pending', async () => { + const document = parse(` + query { + ... @defer { + a { + ... @defer { + b { + c { d } + } + } + } + } + ... @defer { + a { + ... @defer { + b { + c { d } + e { f } + } + } + } + } + } + `); + + const { promise: cPromise, resolve: resolveC } = + // eslint-disable-next-line @typescript-eslint/no-invalid-void-type + promiseWithResolvers(); + let cResolverCalled = false; + let eResolverCalled = false; + const executeResult = experimentalExecuteIncrementally({ + schema, + document, + rootValue: { + a: { + b: { + c: async () => { + cResolverCalled = true; + await cPromise; + return { d: 'd' }; + }, + e: () => { + eResolverCalled = true; + return { f: 'f' }; + }, + }, + }, + }, + enableEarlyExecution: false, + }); + + assert('initialResult' in executeResult); + + const result1 = executeResult.initialResult; + expectJSON(result1).toDeepEqual({ + data: {}, + pending: [ + { id: '0', path: [] }, + { id: '1', path: [] }, + ], + hasNext: true, + }); + + const iterator = executeResult.subsequentResults[Symbol.asyncIterator](); + + expect(cResolverCalled).to.equal(false); + expect(eResolverCalled).to.equal(false); + + const result2 = await iterator.next(); + expectJSON(result2).toDeepEqual({ + value: { + pending: [ + { id: '2', path: ['a'] }, + { id: '3', path: ['a'] }, + ], + incremental: [ + { + data: { a: {} }, + id: '0', + }, + ], + completed: [{ id: '0' }, { id: '1' }], + hasNext: true, + }, + done: false, + }); + + resolveC(); + + expect(cResolverCalled).to.equal(true); + expect(eResolverCalled).to.equal(true); + + const result3 = await iterator.next(); + expectJSON(result3).toDeepEqual({ + value: { + incremental: [ + { + data: { b: { c: { d: 'd' } } }, + id: '2', + }, + { + data: { e: { f: 'f' } }, + id: '3', + subPath: ['b'], + }, + ], + completed: [{ id: '2' }, { id: '3' }], + hasNext: false, + }, + done: false, + }); + + const result4 = await iterator.next(); + expectJSON(result4).toDeepEqual({ + value: undefined, + done: true, + }); + }); + it('Can deduplicate multiple defers on the same object', async () => { const document = parse(` query {