From 4a82557ae6d3b3c6cd72bcd528254296ecf7e9e8 Mon Sep 17 00:00:00 2001 From: Chris Karcher Date: Mon, 17 Oct 2022 12:07:43 -0500 Subject: [PATCH] Fix crash in node when mixing sync/async resolvers (backport of #3706) (#3707) Co-authored-by: Ivan Goncharov --- src/execution/__tests__/executor-test.ts | 51 ++++++++++++++++++++++++ src/execution/execute.ts | 36 +++++++++++------ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index 116334aded..c758d3e426 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'; @@ -625,6 +626,56 @@ describe('Execute: Handles basic execution tasks', () => { }); }); + it('handles sync errors combined with rejections', async () => { + let isAsyncResolverFinished = 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(); + isAsyncResolverFinished = true; + return 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 = execute({ schema, document }); + + expect(isAsyncResolverFinished).to.equal(false); + expectJSON(await result).toDeepEqual({ + data: null, + errors: [ + { + message: + 'Cannot return null for non-nullable field Query.syncNullError.', + locations: [{ line: 4, column: 9 }], + path: ['syncNullError'], + }, + ], + }); + expect(isAsyncResolverFinished).to.equal(true); + }); + 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 4b8cf3a6f7..55c22ea9de 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -445,22 +445,32 @@ function executeFields( const results = Object.create(null); let containsPromise = false; - for (const [responseName, fieldNodes] of fields.entries()) { - const fieldPath = addPath(path, responseName, parentType.name); - const result = executeField( - exeContext, - parentType, - sourceValue, - fieldNodes, - fieldPath, - ); + try { + for (const [responseName, fieldNodes] of fields.entries()) { + const fieldPath = addPath(path, responseName, parentType.name); + 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 (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