From 9be9883fe05dc3007e6ada73a99ce9e75775b003 Mon Sep 17 00:00:00 2001 From: Chris Karcher Date: Thu, 18 Aug 2022 13:33:26 -0500 Subject: [PATCH] Fix crash in node when mixing sync/async resolvers (backport of #3706) --- src/execution/__tests__/executor-test.js | 50 ++++++++++++++++++++++++ src/execution/execute.js | 38 +++++++++++------- 2 files changed, 74 insertions(+), 14 deletions(-) diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js index aa9427ffc4..195b8cde60 100644 --- a/src/execution/__tests__/executor-test.js +++ b/src/execution/__tests__/executor-test.js @@ -3,6 +3,7 @@ import { describe, it } from 'mocha'; import inspect from '../../jsutils/inspect'; import invariant from '../../jsutils/invariant'; +import resolveOnNextTick from '../../__testUtils__/resolveOnNextTick'; import { Kind } from '../../language/kinds'; import { parse } from '../../language/parser'; @@ -646,6 +647,55 @@ describe('Execute: Handles basic execution tasks', () => { }); }); + it('handles sync errors combined with rejections', async () => { + let isAsyncResolverCalled = false; + + 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 result = await execute({ schema, document }); + + expect(isAsyncResolverCalled).to.equal(true); + expect(result).to.deep.equal({ + 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', () => { const A = new GraphQLObjectType({ name: 'A', diff --git a/src/execution/execute.js b/src/execution/execute.js index 35f440c720..370a308456 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -456,23 +456,33 @@ function executeFields( const results = Object.create(null); let containsPromise = false; - for (const responseName of Object.keys(fields)) { - const fieldNodes = fields[responseName]; - const fieldPath = addPath(path, responseName, parentType.name); - const result = resolveField( - exeContext, - parentType, - sourceValue, - fieldNodes, - fieldPath, - ); + try { + for (const responseName of Object.keys(fields)) { + const fieldNodes = fields[responseName]; + const fieldPath = addPath(path, responseName, parentType.name); + const result = resolveField( + 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 (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