From b24cbcde325d6e53788debae9818dd8cddebf9e1 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Mon, 13 Aug 2018 12:52:56 -0400 Subject: [PATCH 1/3] Make error handling consistent in createSourceEventStream --- src/subscription/subscribe.js | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/subscription/subscribe.js b/src/subscription/subscribe.js index ebd52f8086..5f9ad16709 100644 --- a/src/subscription/subscribe.js +++ b/src/subscription/subscribe.js @@ -35,8 +35,9 @@ import { getOperationRootType } from '../utilities/getOperationRootType'; * Implements the "Subscribe" algorithm described in the GraphQL specification. * * Returns a Promise which resolves to either an AsyncIterator (if successful) - * or an ExecutionResult (client error). The promise will be rejected if a - * server error occurs. + * or an ExecutionResult (error). The promise will be rejected if the schema or + * other arguments to this function are invalid, or if the resolved event stream + * is not an async iterable. * * If the client-provided arguments to this function do not result in a * compliant subscription, a GraphQL Response (ExecutionResult) with @@ -171,7 +172,6 @@ function subscribeImpl( reportGraphQLError, ) : ((resultOrStream: any): ExecutionResult), - reportGraphQLError, ); } @@ -179,11 +179,21 @@ function subscribeImpl( * Implements the "CreateSourceEventStream" algorithm described in the * GraphQL specification, resolving the subscription source event stream. * - * Returns a Promise. + * Returns a Promise which resolves to either an AsyncIterable (if successful) + * or an ExecutionResult (error). The promise will be rejected if the schema or + * other arguments to this function are invalid, or if the resolved event stream + * is not an async iterable. * - * If the client-provided invalid arguments, the source stream could not be - * created, or the resolver did not return an AsyncIterable, this function will - * will throw an error, which should be caught and handled by the caller. + * If the client-provided arguments to this function do not result in a + * compliant subscription, a GraphQL Response (ExecutionResult) with + * descriptive errors and no data will be returned. + * + * If the the source stream could not be created due to faulty subscription + * resolver logic or underlying systems, the promise will resolve to a single + * ExecutionResult containing `errors` and no `data`. + * + * If the operation succeeded, the promise resolves to the AsyncIterable for the + * event stream returned by the resolver. * * A Source Event Stream represents a sequence of events, each of which triggers * a GraphQL execution for that event. @@ -270,7 +280,11 @@ export function createSourceEventStream( return Promise.resolve(result).then(eventStream => { // If eventStream is an Error, rethrow a located error. if (eventStream instanceof Error) { - throw locatedError(eventStream, fieldNodes, responsePathAsArray(path)); + return { + errors: [ + locatedError(eventStream, fieldNodes, responsePathAsArray(path)), + ], + }; } // Assert field returned an event stream, otherwise yield an error. @@ -284,6 +298,8 @@ export function createSourceEventStream( ); }); } catch (error) { - return Promise.reject(error); + return error instanceof GraphQLError + ? Promise.resolve({ errors: [error] }) + : Promise.reject(error); } } From 889344d8756eea1e48c99dba086e9d1ab7e5424f Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Fri, 17 Aug 2018 23:03:38 -0400 Subject: [PATCH 2/3] Address PR feedback --- src/subscription/__tests__/subscribe-test.js | 120 ++++++++++++++++++- src/subscription/subscribe.js | 3 + 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/src/subscription/__tests__/subscribe-test.js b/src/subscription/__tests__/subscribe-test.js index 99acc04f05..0638d1f779 100644 --- a/src/subscription/__tests__/subscribe-test.js +++ b/src/subscription/__tests__/subscribe-test.js @@ -9,7 +9,7 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import EventEmitter from 'events'; import eventEmitterAsyncIterator from './eventEmitterAsyncIterator'; -import { subscribe } from '../subscribe'; +import { createSourceEventStream, subscribe } from '../subscribe'; import { parse } from '../../language'; import { GraphQLSchema, @@ -429,6 +429,58 @@ describe('Subscription Initialization Phase', () => { } }); + it('resolves to an error for source event stream resolver errors', async () => { + // Returning an error + const subscriptionReturningErrorSchema = emailSchemaWithResolvers(() => { + return new Error('test error'); + }); + await testReportsError(subscriptionReturningErrorSchema); + + // Throwing an error + const subscriptionThrowingErrorSchema = emailSchemaWithResolvers(() => { + throw new Error('test error'); + }); + await testReportsError(subscriptionThrowingErrorSchema); + + // Resolving to an error + const subscriptionResolvingErrorSchema = emailSchemaWithResolvers( + async () => { + return new Error('test error'); + }, + ); + await testReportsError(subscriptionResolvingErrorSchema); + + // Rejecting with an error + const subscriptionRejectingErrorSchema = emailSchemaWithResolvers( + async () => { + throw new Error('test error'); + }, + ); + await testReportsError(subscriptionRejectingErrorSchema); + + async function testReportsError(schema) { + // Promise | ExecutionResult> + const result = await createSourceEventStream( + schema, + parse(` + subscription { + importantEmail + } + `), + ); + + expect(result).to.deep.equal({ + errors: [ + { + message: 'test error', + locations: [{ line: 3, column: 13 }], + path: ['importantEmail'], + }, + ], + }); + } + }); + it('resolves to an error if variables were wrong type', async () => { // If we receive variables that cannot be coerced correctly, subscribe() // will resolve to an ExecutionResult that contains an informative error @@ -933,4 +985,70 @@ describe('Subscription Publish Phase', () => { value: undefined, }); }); + + it('should pass through error resolved from source event stream', async () => { + const erroringEmailSchema = emailSchemaWithResolvers( + async function*() { + yield { email: { subject: 'Hello' } }; + yield new Error('test error'); + }, + email => { + if (email instanceof Error) { + throw email; + } + + return email; + }, + ); + + const subscription = await subscribe( + erroringEmailSchema, + parse(` + subscription { + importantEmail { + email { + subject + } + } + } + `), + ); + + const payload1 = await subscription.next(); + expect(payload1).to.deep.equal({ + done: false, + value: { + data: { + importantEmail: { + email: { + subject: 'Hello', + }, + }, + }, + }, + }); + + const payload2 = await subscription.next(); + expect(payload2).to.deep.equal({ + done: false, + value: { + data: { + importantEmail: null, + }, + errors: [ + { + message: 'test error', + locations: [{ line: 3, column: 11 }], + path: ['importantEmail'], + }, + ], + }, + }); + + const payload3 = await subscription.next(); + expect(payload3).to.deep.equal({ + done: true, + value: undefined, + }); + }); }); diff --git a/src/subscription/subscribe.js b/src/subscription/subscribe.js index 5f9ad16709..1ab3ad6e64 100644 --- a/src/subscription/subscribe.js +++ b/src/subscription/subscribe.js @@ -298,6 +298,9 @@ export function createSourceEventStream( ); }); } catch (error) { + // As with reportGraphQLError above, if the error is a GraphQLError, report + // it as an ExecutionResult; otherwise treat it as a system-class error and + // re-throw it. return error instanceof GraphQLError ? Promise.resolve({ errors: [error] }) : Promise.reject(error); From c7b3344e5d22d92e32dddef3bc5e793c98970af9 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Fri, 17 Aug 2018 23:13:26 -0400 Subject: [PATCH 3/3] Actually fix coverage --- src/subscription/__tests__/subscribe-test.js | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/subscription/__tests__/subscribe-test.js b/src/subscription/__tests__/subscribe-test.js index 0638d1f779..4ee9f5128a 100644 --- a/src/subscription/__tests__/subscribe-test.js +++ b/src/subscription/__tests__/subscribe-test.js @@ -10,6 +10,7 @@ import { describe, it } from 'mocha'; import EventEmitter from 'events'; import eventEmitterAsyncIterator from './eventEmitterAsyncIterator'; import { createSourceEventStream, subscribe } from '../subscribe'; +import { GraphQLError } from '../../error'; import { parse } from '../../language'; import { GraphQLSchema, @@ -986,19 +987,13 @@ describe('Subscription Publish Phase', () => { }); }); - it('should pass through error resolved from source event stream', async () => { + it('should resolve GraphQL error from source event stream', async () => { const erroringEmailSchema = emailSchemaWithResolvers( async function*() { yield { email: { subject: 'Hello' } }; - yield new Error('test error'); - }, - email => { - if (email instanceof Error) { - throw email; - } - - return email; + throw new GraphQLError('test error'); }, + email => email, ); const subscription = await subscribe( @@ -1032,14 +1027,9 @@ describe('Subscription Publish Phase', () => { expect(payload2).to.deep.equal({ done: false, value: { - data: { - importantEmail: null, - }, errors: [ { message: 'test error', - locations: [{ line: 3, column: 11 }], - path: ['importantEmail'], }, ], },