From af18110e90f62f0092046eb576f49752c8fdbcbd Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Thu, 23 Nov 2023 00:03:38 +0200 Subject: [PATCH 1/2] do not emit pending for empty non-published subsequent results The publish method checks to see if a subsequent result is empty; this same logic should be employed to suppress pending notices for empty records. This has already been achieved for subsequent results that are children of the initial result, as we generated the pending notices from the list of initially published records. For subsequent results that are children of other subsequent results, we previously generated the pending notice prior to actually publishing. This change integrates the logic: the publishing method itself returns a pending notice as required. This results in a bug-fix for subsequent records of other subsequent records as well as a reduction of code for subsequent results to the initial result. --- src/execution/IncrementalPublisher.ts | 44 ++++++++++++---------- src/execution/__tests__/defer-test.ts | 54 +++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 20 deletions(-) diff --git a/src/execution/IncrementalPublisher.ts b/src/execution/IncrementalPublisher.ts index 760043c78b..0caf8bf674 100644 --- a/src/execution/IncrementalPublisher.ts +++ b/src/execution/IncrementalPublisher.ts @@ -301,25 +301,20 @@ export class IncrementalPublisher { initialResultRecord: InitialResultRecord, data: ObjMap | null, ): ExecutionResult | ExperimentalIncrementalExecutionResults { + const pendingSources = new Set(); for (const child of initialResultRecord.children) { if (child.filtered) { continue; } - this._publish(child); + const maybePendingSource = this._publish(child); + if (maybePendingSource) { + pendingSources.add(maybePendingSource); + } } const errors = initialResultRecord.errors; const initialResult = errors.length === 0 ? { data } : { errors, data }; - const pending = this._pending; - if (pending.size > 0) { - const pendingSources = new Set(); - for (const subsequentResultRecord of pending) { - const pendingSource = isStreamItemsRecord(subsequentResultRecord) - ? subsequentResultRecord.streamRecord - : subsequentResultRecord; - pendingSources.add(pendingSource); - } - + if (pendingSources.size > 0) { return { initialResult: { ...initialResult, @@ -542,13 +537,10 @@ export class IncrementalPublisher { if (child.filtered) { continue; } - const pendingSource = isStreamItemsRecord(child) - ? child.streamRecord - : child; - if (!pendingSource.pendingSent) { - newPendingSources.add(pendingSource); + const maybePendingSource = this._publish(child); + if (maybePendingSource) { + newPendingSources.add(maybePendingSource); } - this._publish(child); } if (isStreamItemsRecord(subsequentResultRecord)) { if (subsequentResultRecord.isFinalRecord) { @@ -655,14 +647,20 @@ export class IncrementalPublisher { return result; } - private _publish(subsequentResultRecord: SubsequentResultRecord): void { + private _publish( + subsequentResultRecord: SubsequentResultRecord, + ): DeferredFragmentRecord | StreamRecord | undefined { if (isStreamItemsRecord(subsequentResultRecord)) { if (subsequentResultRecord.isCompleted) { this._push(subsequentResultRecord); - return; + } else { + this._introduce(subsequentResultRecord); } - this._introduce(subsequentResultRecord); + const stream = subsequentResultRecord.streamRecord; + if (!stream.pendingSent) { + return stream; + } return; } @@ -673,6 +671,12 @@ export class IncrementalPublisher { subsequentResultRecord.children.size > 0 ) { this._push(subsequentResultRecord); + } else { + return; + } + + if (!subsequentResultRecord.pendingSent) { + return subsequentResultRecord; } } diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index 261db67df9..fc44cdb228 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -64,6 +64,7 @@ const anotherNestedObject = new GraphQLObjectType({ const hero = { name: 'Luke', + lastName: 'SkyWalker', id: 1, friends, nestedObject, @@ -112,6 +113,7 @@ const heroType = new GraphQLObjectType({ fields: { id: { type: GraphQLID }, name: { type: GraphQLString }, + lastName: { type: GraphQLString }, nonNullName: { type: new GraphQLNonNull(GraphQLString) }, friends: { type: new GraphQLList(friendType), @@ -566,6 +568,58 @@ describe('Execute: defer directive', () => { ]); }); + it('Separately emits defer fragments with different labels with varying subfields with superimposed masked defer', async () => { + const document = parse(` + query HeroNameQuery { + ... @defer(label: "DeferID") { + hero { + id + } + } + ... @defer(label: "DeferName") { + hero { + name + lastName + ... @defer { + lastName + } + } + } + } + `); + const result = await complete(document); + expectJSON(result).toDeepEqual([ + { + data: {}, + pending: [ + { id: '0', path: [], label: 'DeferID' }, + { id: '1', path: [], label: 'DeferName' }, + ], + hasNext: true, + }, + { + incremental: [ + { + data: { hero: {} }, + id: '0', + }, + { + data: { id: '1' }, + id: '0', + subPath: ['hero'], + }, + { + data: { name: 'Luke', lastName: 'SkyWalker' }, + id: '1', + subPath: ['hero'], + }, + ], + completed: [{ id: '0' }, { id: '1' }], + hasNext: false, + }, + ]); + }); + it('Separately emits defer fragments with different labels with varying subfields that return promises', async () => { const document = parse(` query HeroNameQuery { From 4203dda6db584fa7474415c3ee6cb2ce8515b777 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 21 Nov 2023 15:49:40 +0200 Subject: [PATCH 2/2] add checks for actual deferral --- src/execution/__tests__/defer-test.ts | 188 ++++++++++++++++++++++++-- 1 file changed, 179 insertions(+), 9 deletions(-) diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts index fc44cdb228..60f59d4391 100644 --- a/src/execution/__tests__/defer-test.ts +++ b/src/execution/__tests__/defer-test.ts @@ -156,6 +156,65 @@ async function complete(document: DocumentNode, rootValue: unknown = { hero }) { return result; } +function getCountingHero() { + let stopped = false; + let count = 0; + const counts = new Map(); + function increment() { + if (stopped) { + return; + } + // eslint-disable-next-line @typescript-eslint/no-floating-promises + Promise.resolve().then(() => { + count++; + increment(); + }); + } + increment(); + const countingHero = { + stop: () => { + stopped = true; + }, + counts, + hero: () => { + counts.set('hero', count); + return { + id: () => { + counts.set('id', count); + return hero.id; + }, + name: () => { + counts.set('name', count); + return hero.name; + }, + lastName: () => { + counts.set('lastName', count); + return hero.lastName; + }, + nestedObject: () => { + counts.set('nestedObject', count); + return { + deeperObject: () => { + counts.set('deeperObject', count); + return { + foo: () => { + counts.set('foo', count); + return 'foo'; + }, + bar: () => { + counts.set('bar', count); + return 'bar'; + }, + }; + }, + }; + }, + }; + }, + }; + return countingHero; +} + describe('Execute: defer directive', () => { it('Can defer fragments containing scalar types', async () => { const document = parse(` @@ -169,7 +228,8 @@ describe('Execute: defer directive', () => { name } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { @@ -194,6 +254,11 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(0); + expect(countingHero.counts.get('id')).to.equal(0); + expect(countingHero.counts.get('name')).to.equal(1); }); it('Can disable defer using if argument', async () => { const document = parse(` @@ -487,7 +552,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: { @@ -518,6 +584,11 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(0); + expect(countingHero.counts.get('id')).to.equal(1); + expect(countingHero.counts.get('name')).to.equal(1); }); it('Separately emits defer fragments with different labels with varying subfields', async () => { @@ -535,7 +606,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: {}, @@ -566,6 +638,11 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(1); + expect(countingHero.counts.get('id')).to.equal(1); + expect(countingHero.counts.get('name')).to.equal(1); }); it('Separately emits defer fragments with different labels with varying subfields with superimposed masked defer', async () => { @@ -587,7 +664,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: {}, @@ -618,6 +696,12 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(1); + expect(countingHero.counts.get('id')).to.equal(1); + expect(countingHero.counts.get('name')).to.equal(1); + expect(countingHero.counts.get('lastName')).to.equal(1); }); it('Separately emits defer fragments with different labels with varying subfields that return promises', async () => { @@ -688,7 +772,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: { @@ -720,6 +805,11 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(0); + expect(countingHero.counts.get('id')).to.equal(1); + expect(countingHero.counts.get('name')).to.equal(1); }); it('Separately emits nested defer fragments with varying subfields of same priorities but different level of defers', async () => { @@ -735,7 +825,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: {}, @@ -770,6 +861,11 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(1); + expect(countingHero.counts.get('name')).to.equal(1); + expect(countingHero.counts.get('id')).to.equal(2); }); it('Can deduplicate multiple defers on the same object', async () => { @@ -908,9 +1004,8 @@ describe('Execute: defer directive', () => { } } `); - const result = await complete(document, { - hero: { nestedObject: { deeperObject: { foo: 'foo', bar: 'bar' } } }, - }); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); expectJSON(result).toDeepEqual([ { data: { @@ -947,6 +1042,81 @@ describe('Execute: defer directive', () => { hasNext: false, }, ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(0); + expect(countingHero.counts.get('nestedObject')).to.equal(1); + expect(countingHero.counts.get('deeperObject')).to.equal(1); + expect(countingHero.counts.get('foo')).to.equal(1); + expect(countingHero.counts.get('bar')).to.equal(2); + }); + + it('Deduplicates subfields present in a parent defer payload', async () => { + const document = parse(` + query { + hero { + ... @defer { + nestedObject { + deeperObject { + foo + } + ... @defer { + deeperObject { + foo + bar + } + } + } + } + } + } + `); + const countingHero = getCountingHero(); + const result = await complete(document, { hero: countingHero.hero }); + expectJSON(result).toDeepEqual([ + { + data: { + hero: {}, + }, + pending: [{ id: '0', path: ['hero'] }], + hasNext: true, + }, + { + pending: [{ id: '1', path: ['hero', 'nestedObject'] }], + incremental: [ + { + data: { + nestedObject: { + deeperObject: { foo: 'foo' }, + }, + }, + id: '0', + }, + ], + completed: [{ id: '0' }], + hasNext: true, + }, + { + incremental: [ + { + data: { + bar: 'bar', + }, + id: '1', + subPath: ['deeperObject'], + }, + ], + completed: [{ id: '1' }], + hasNext: false, + }, + ]); + + countingHero.stop(); + expect(countingHero.counts.get('hero')).to.equal(0); + expect(countingHero.counts.get('nestedObject')).to.equal(1); + expect(countingHero.counts.get('deeperObject')).to.equal(1); + expect(countingHero.counts.get('foo')).to.equal(1); + expect(countingHero.counts.get('bar')).to.equal(2); }); it('Deduplicates fields with deferred fragments at multiple levels', async () => {