From 339324051efc57700a020d7645c7ef3514875176 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 12 Jun 2022 22:10:27 +0300 Subject: [PATCH 1/3] refactor: subscribe: introduce new `buildPerEventExecutionContext` = introduces `buildPerEventExecutionContext` that creates an `ExecutionContext` for each subscribe event from the original `ExecutionContext` used to create the event stream = `subscribe` now directly builds the `ExecutionContext` instead of relying on `createSourceEventStream` = introduces `createSourceEventStreamImpl` and `executeImpl` functions that operate on the built `ExecutionContext` rather the `ExecutionArgs` = `subscribe` calls the `createSourceEventStreamImpl` function on the original context and eventuallys calls `executeImpl` on the per event context created by `buildEventExecutionContext`. Motivation: 1. remove unnecessary `buildExecutionContext` call, reducing duplicate work 2. paves the way for easily enhancing the `buildPerEventExecutionContext` to add a new `perEventContextFactory` argument to augment the context argument passed to resolvers as need per event. depends on #3638 --- src/execution/execute.ts | 45 +++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index bcebc879ff..bad4afacf1 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -174,6 +174,12 @@ export function execute(args: ExecutionArgs): PromiseOrValue { return { errors: exeContext }; } + return executeImpl(exeContext); +} + +function executeImpl( + exeContext: ExecutionContext, +): PromiseOrValue { // Return a Promise that will eventually resolve to the data described by // The "Response" section of the GraphQL specification. // @@ -319,6 +325,17 @@ export function buildExecutionContext( }; } +function buildPerEventExecutionContext( + exeContext: ExecutionContext, + payload: unknown, +): ExecutionContext { + return { + ...exeContext, + rootValue: payload, + errors: [], + }; +} + /** * Implements the "Executing operations" section of the spec. */ @@ -1017,20 +1034,29 @@ export function subscribe( ): PromiseOrValue< AsyncGenerator | ExecutionResult > { - const resultOrStream = createSourceEventStream(args); + // If a valid execution context cannot be created due to incorrect arguments, + // a "Response" with only errors is returned. + const exeContext = buildExecutionContext(args); + + // Return early errors if execution context failed. + if (!('schema' in exeContext)) { + return { errors: exeContext }; + } + + const resultOrStream = createSourceEventStreamImpl(exeContext); if (isPromise(resultOrStream)) { return resultOrStream.then((resolvedResultOrStream) => - mapSourceToResponse(resolvedResultOrStream, args), + mapSourceToResponse(exeContext, resolvedResultOrStream), ); } - return mapSourceToResponse(resultOrStream, args); + return mapSourceToResponse(exeContext, resultOrStream); } function mapSourceToResponse( + exeContext: ExecutionContext, resultOrStream: ExecutionResult | AsyncIterable, - args: ExecutionArgs, ): PromiseOrValue< AsyncGenerator | ExecutionResult > { @@ -1045,10 +1071,7 @@ function mapSourceToResponse( // "ExecuteSubscriptionEvent" algorithm, as it is nearly identical to the // "ExecuteQuery" algorithm, for which `execute` is also used. return mapAsyncIterator(resultOrStream, (payload: unknown) => - execute({ - ...args, - rootValue: payload, - }), + executeImpl(buildPerEventExecutionContext(exeContext, payload)), ); } @@ -1092,6 +1115,12 @@ export function createSourceEventStream( return { errors: exeContext }; } + return createSourceEventStreamImpl(exeContext); +} + +function createSourceEventStreamImpl( + exeContext: ExecutionContext, +): PromiseOrValue | ExecutionResult> { try { const eventStream = executeSubscription(exeContext); if (isPromise(eventStream)) { From 792fbbc17a91e63adaa5e39f22702ae98dac0fcd Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sun, 12 Jun 2022 22:27:30 +0300 Subject: [PATCH 2/3] improve code coverage ...now that subscribe and createSourceEventStream both call buildExecutionContext, build errors must be tested separately --- src/execution/__tests__/subscribe-test.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index 4852d86ad3..be3d3d1a9e 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -14,7 +14,7 @@ import { GraphQLList, GraphQLObjectType } from '../../type/definition'; import { GraphQLBoolean, GraphQLInt, GraphQLString } from '../../type/scalars'; import { GraphQLSchema } from '../../type/schema'; -import type { ExecutionResult } from '../execute'; +import type { ExecutionArgs, ExecutionResult } from '../execute'; import { createSourceEventStream, subscribe } from '../execute'; import { SimplePubSub } from './simplePubSub'; @@ -195,6 +195,15 @@ function subscribeWithBadFn( ); } +function subscribeWithBadArgs( + args: ExecutionArgs, +): PromiseOrValue> { + return expectEqualPromisesOrValues( + subscribe(args), + createSourceEventStream(args), + ); +} + /* eslint-disable @typescript-eslint/require-await */ // Check all error cases when initializing the subscription. describe('Subscription Initialization Phase', () => { @@ -394,7 +403,7 @@ describe('Subscription Initialization Phase', () => { const schema = new GraphQLSchema({ query: DummyQueryType }); const document = parse('subscription { unknownField }'); - const result = subscribe({ schema, document }); + const result = subscribeWithBadArgs({ schema, document }); expectJSON(result).toDeepEqual({ errors: [ { @@ -418,7 +427,7 @@ describe('Subscription Initialization Phase', () => { }); const document = parse('subscription { unknownField }'); - const result = subscribe({ schema, document }); + const result = subscribeWithBadArgs({ schema, document }); expectJSON(result).toDeepEqual({ errors: [ { @@ -441,7 +450,7 @@ describe('Subscription Initialization Phase', () => { }); // @ts-expect-error - expect(() => subscribe({ schema, document: {} })).to.throw(); + expect(() => subscribeWithBadArgs({ schema, document: {} })).to.throw(); }); it('throws an error if subscribe does not return an iterator', async () => { @@ -526,7 +535,7 @@ describe('Subscription Initialization Phase', () => { // If we receive variables that cannot be coerced correctly, subscribe() will // resolve to an ExecutionResult that contains an informative error description. - const result = subscribe({ schema, document, variableValues }); + const result = subscribeWithBadArgs({ schema, document, variableValues }); expectJSON(result).toDeepEqual({ errors: [ { From d51c393cc548d0cf4b31885a419f52b3ab98c936 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 21 Jun 2022 21:54:08 +0300 Subject: [PATCH 3/3] apply review feedback --- src/execution/__tests__/subscribe-test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts index be3d3d1a9e..5f256ca868 100644 --- a/src/execution/__tests__/subscribe-test.ts +++ b/src/execution/__tests__/subscribe-test.ts @@ -189,10 +189,7 @@ function subscribeWithBadFn( }); const document = parse('subscription { foo }'); - return expectEqualPromisesOrValues( - subscribe({ schema, document }), - createSourceEventStream({ schema, document }), - ); + return subscribeWithBadArgs({ schema, document }); } function subscribeWithBadArgs(