From bc7049bcb29caf6a88bf4b6b16f7294e3b2314ce Mon Sep 17 00:00:00 2001 From: Lee Houghton Date: Tue, 5 Apr 2022 18:15:59 +0100 Subject: [PATCH 1/2] Fix crash in node when mixing sync/async resolvers Fixes #3528 --- src/execution/__tests__/executor-test.ts | 54 ++++++++++++++++++++++++ src/execution/execute.ts | 45 ++++++++++++++------ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index 116334aded..49ad3fd616 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -625,6 +625,60 @@ describe('Execute: Handles basic execution tasks', () => { }); }); + it('handles sync errors combined with rejections', async () => { + const catchUnhandledRejections = (reason: any) => { + throw new Error('Unhandled rejection: ' + reason.message); + }; + + process.on('unhandledRejection', catchUnhandledRejections); + + try { + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Type', + fields: { + syncNullError: { + type: new GraphQLNonNull(GraphQLString), + resolve: () => null, + }, + asyncNullError: { + type: new GraphQLNonNull(GraphQLString), + resolve: () => Promise.resolve(null), + }, + }, + }), + }); + + // Order is important here, as the promise has to be created before the synchronous error is thrown + const document = parse(` + { + asyncNullError + syncNullError + } + `); + + const rootValue = {}; + + const result = await execute({ schema, document, rootValue }); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: + 'Cannot return null for non-nullable field Type.asyncNullError.', + locations: [{ line: 3, column: 11 }], + path: ['asyncNullError'], + }, + ], + }); + + // Allow time for the asyncReject field to be rejected + await new Promise((resolve) => process.nextTick(resolve)); + } finally { + process.off('unhandledRejection', catchUnhandledRejections); + } + }); + it('Full response path is included for non-nullable fields', () => { const A: GraphQLObjectType = new GraphQLObjectType({ name: 'A', diff --git a/src/execution/execute.ts b/src/execution/execute.ts index d3c21385e8..c35c4f267f 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -444,34 +444,53 @@ function executeFields( ): PromiseOrValue> { const results = Object.create(null); let containsPromise = false; + let error: Error | undefined; for (const [responseName, fieldNodes] of fields.entries()) { const fieldPath = addPath(path, responseName, parentType.name); - const result = executeField( - exeContext, - parentType, - sourceValue, - fieldNodes, - fieldPath, - ); + try { + const result = executeField( + exeContext, + parentType, + sourceValue, + fieldNodes, + fieldPath, + ); - if (result !== undefined) { - results[responseName] = result; - if (isPromise(result)) { - containsPromise = true; + if (result !== undefined) { + results[responseName] = result; + if (isPromise(result)) { + containsPromise = true; + } } + } catch (err) { + error = err; + } + + if (error) { + break; } } // If there are no promises, we can just return the object if (!containsPromise) { - return results; + if (error) { + throw error; + } else { + return results; + } } // Otherwise, results is a map from field name to the result of resolving that // field, which is possibly a promise. Return a promise that will return this // same map, but with any promises replaced with the values they resolved to. - return promiseForObject(results); + const promise = promiseForObject(results); + if (error) { + // Ensure that any promises returned by other fields are handled, as they may also reject. + return promise.then(() => Promise.reject(error)); + } + + return promise; } /** From 5bfb9d6788b4d8adce451210b4522b056348355b Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 19 Apr 2022 23:16:13 +0300 Subject: [PATCH 2/2] review changes --- src/execution/__tests__/executor-test.ts | 88 +++++++++++------------- src/execution/execute.ts | 33 ++++----- 2 files changed, 54 insertions(+), 67 deletions(-) diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index 49ad3fd616..9520b06748 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -2,6 +2,7 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON'; +import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick'; import { inspect } from '../../jsutils/inspect'; import { invariant } from '../../jsutils/invariant'; @@ -626,57 +627,52 @@ describe('Execute: Handles basic execution tasks', () => { }); it('handles sync errors combined with rejections', async () => { - const catchUnhandledRejections = (reason: any) => { - throw new Error('Unhandled rejection: ' + reason.message); - }; - - process.on('unhandledRejection', catchUnhandledRejections); + let isAsyncResolverCalled = false; - try { - const schema = new GraphQLSchema({ - query: new GraphQLObjectType({ - name: 'Type', - fields: { - syncNullError: { - type: new GraphQLNonNull(GraphQLString), - resolve: () => null, - }, - asyncNullError: { - type: new GraphQLNonNull(GraphQLString), - resolve: () => Promise.resolve(null), + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + syncNullError: { + type: new GraphQLNonNull(GraphQLString), + resolve: () => null, + }, + asyncNullError: { + type: new GraphQLNonNull(GraphQLString), + async resolve() { + await resolveOnNextTick(); + await resolveOnNextTick(); + await resolveOnNextTick(); + isAsyncResolverCalled = true; + return Promise.resolve(null); }, }, - }), - }); + }, + }), + }); - // Order is important here, as the promise has to be created before the synchronous error is thrown - const document = parse(` - { - asyncNullError - syncNullError - } - `); - - const rootValue = {}; - - const result = await execute({ schema, document, rootValue }); - expectJSON(result).toDeepEqual({ - data: null, - errors: [ - { - message: - 'Cannot return null for non-nullable field Type.asyncNullError.', - locations: [{ line: 3, column: 11 }], - path: ['asyncNullError'], - }, - ], - }); + // Order is important here, as the promise has to be created before the synchronous error is thrown + const document = parse(` + { + asyncNullError + syncNullError + } + `); - // Allow time for the asyncReject field to be rejected - await new Promise((resolve) => process.nextTick(resolve)); - } finally { - process.off('unhandledRejection', catchUnhandledRejections); - } + const result = await execute({ schema, document }); + + expect(isAsyncResolverCalled).to.equal(true); + expectJSON(result).toDeepEqual({ + data: null, + errors: [ + { + message: + 'Cannot return null for non-nullable field Query.syncNullError.', + locations: [{ line: 4, column: 9 }], + path: ['syncNullError'], + }, + ], + }); }); it('Full response path is included for non-nullable fields', () => { diff --git a/src/execution/execute.ts b/src/execution/execute.ts index c35c4f267f..281d730058 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -444,11 +444,10 @@ function executeFields( ): PromiseOrValue> { const results = Object.create(null); let containsPromise = false; - let error: Error | undefined; - for (const [responseName, fieldNodes] of fields.entries()) { - const fieldPath = addPath(path, responseName, parentType.name); - try { + try { + for (const [responseName, fieldNodes] of fields.entries()) { + const fieldPath = addPath(path, responseName, parentType.name); const result = executeField( exeContext, parentType, @@ -463,34 +462,26 @@ function executeFields( containsPromise = true; } } - } catch (err) { - error = err; } - - if (error) { - break; + } catch (error) { + if (containsPromise) { + // Ensure that any promises returned by other fields are handled, as they may also reject. + return promiseForObject(results).finally(() => { + throw error; + }); } + throw error; } // If there are no promises, we can just return the object if (!containsPromise) { - if (error) { - throw error; - } else { - return results; - } + return results; } // Otherwise, results is a map from field name to the result of resolving that // field, which is possibly a promise. Return a promise that will return this // same map, but with any promises replaced with the values they resolved to. - const promise = promiseForObject(results); - if (error) { - // Ensure that any promises returned by other fields are handled, as they may also reject. - return promise.then(() => Promise.reject(error)); - } - - return promise; + return promiseForObject(results); } /**