From 21b49657e449c53b210c6a2126335006ffd601e9 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 19 Jun 2024 22:50:29 +0300 Subject: [PATCH 1/3] 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/3] 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 { From aac351951b1da7f8134b367d461167e4dd468841 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 3 Jul 2024 21:28:55 +0300 Subject: [PATCH 3/3] alternative solution deferred grouped field sets are only initiated when the WrappedGraphQLResult in which they are contained are processed --- src/execution/IncrementalGraph.ts | 16 ++++++++-------- src/execution/__tests__/defer-test.ts | 4 ++-- src/execution/execute.ts | 9 --------- src/execution/types.ts | 21 --------------------- 4 files changed, 10 insertions(+), 40 deletions(-) diff --git a/src/execution/IncrementalGraph.ts b/src/execution/IncrementalGraph.ts index 6480c4a89b..a64dfb7f5e 100644 --- a/src/execution/IncrementalGraph.ts +++ b/src/execution/IncrementalGraph.ts @@ -8,7 +8,6 @@ import type { GraphQLError } from '../error/GraphQLError.js'; import type { DeferredFragmentRecord, DeferredGroupedFieldSetRecord, - DeferredGroupedFieldSetResult, IncrementalDataRecord, IncrementalDataRecordResult, ReconcilableDeferredGroupedFieldSetResult, @@ -201,7 +200,6 @@ 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,14 +251,16 @@ export class IncrementalGraph { private _onDeferredGroupedFieldSet( deferredGroupedFieldSetRecord: DeferredGroupedFieldSetRecord, ): void { - const result = ( - deferredGroupedFieldSetRecord.result as BoxedPromiseOrValue - ).value; - if (isPromise(result)) { + let deferredGroupedFieldSetResult = deferredGroupedFieldSetRecord.result; + if (!(deferredGroupedFieldSetResult instanceof BoxedPromiseOrValue)) { + deferredGroupedFieldSetResult = deferredGroupedFieldSetResult(); + } + const value = deferredGroupedFieldSetResult.value; + if (isPromise(value)) { // eslint-disable-next-line @typescript-eslint/no-floating-promises - result.then((resolved) => this._enqueue(resolved)); + value.then((resolved) => this._enqueue(resolved)); } else { - this._enqueue(result); + this._enqueue(value); } } diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index 04f3399ffb..97dcfeceb6 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -986,7 +986,7 @@ describe('Execute: defer directive', () => { }); }); - it('Initiates all deferred grouped field sets immediately once they have been released as pending', async () => { + it('Initiates unique deferred grouped field sets after those that are common to sibling defers', async () => { const document = parse(` query { ... @defer { @@ -1076,7 +1076,7 @@ describe('Execute: defer directive', () => { resolveC(); expect(cResolverCalled).to.equal(true); - expect(eResolverCalled).to.equal(true); + expect(eResolverCalled).to.equal(false); const result3 = await iterator.next(); expectJSON(result3).toDeepEqual({ diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 92d6977ed1..ed6a3452ab 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -2123,15 +2123,6 @@ function executeDeferredGroupedFieldSets( } 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 76205ccaec..c88ae9986e 100644 --- a/src/execution/types.ts +++ b/src/execution/types.ts @@ -224,9 +224,6 @@ export class DeferredFragmentRecord { reconcilableResults: Set; children: Set; - private pending: boolean; - private fns: Array<() => void>; - constructor( path: Path | undefined, label: string | undefined, @@ -238,24 +235,6 @@ 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(); - } } }