From 651fc3f3eec7d1f92423f2e36e8f93c219cb4f03 Mon Sep 17 00:00:00 2001 From: Chris Karcher Date: Wed, 11 May 2022 10:19:35 -0500 Subject: [PATCH 1/3] Fix crash in node when mixing sync/async resolvers (backport of #3529) --- src/__testUtils__/expectJSON.js | 51 ++++++++++++++++++++++++ src/execution/__tests__/executor-test.js | 51 ++++++++++++++++++++++++ src/execution/execute.js | 38 +++++++++++------- 3 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 src/__testUtils__/expectJSON.js diff --git a/src/__testUtils__/expectJSON.js b/src/__testUtils__/expectJSON.js new file mode 100644 index 0000000000..9e09af0e85 --- /dev/null +++ b/src/__testUtils__/expectJSON.js @@ -0,0 +1,51 @@ +import { expect } from 'chai'; + +import isObjectLike from '../jsutils/isObjectLike'; +import mapValue from '../jsutils/mapValue'; + +/** + * Deeply transforms an arbitrary value to a JSON-safe value by calling toJSON + * on any nested value which defines it. + */ +function toJSONDeep(value) { + if (!isObjectLike(value)) { + return value; + } + + if (typeof value.toJSON === 'function') { + return value.toJSON(); + } + + if (Array.isArray(value)) { + return value.map(toJSONDeep); + } + + return mapValue(value, toJSONDeep); +} + +export default function expectJSON(actual) { + const actualJSON = toJSONDeep(actual); + + return { + toDeepEqual(expected) { + const expectedJSON = toJSONDeep(expected); + expect(actualJSON).to.deep.equal(expectedJSON); + }, + toDeepNestedProperty(path, expected) { + const expectedJSON = toJSONDeep(expected); + expect(actualJSON).to.deep.nested.property(path, expectedJSON); + }, + }; +} + +export function expectToThrowJSON(fn) { + function mapException() { + try { + return fn(); + } catch (error) { + throw toJSONDeep(error); + } + } + + return expect(mapException).to.throw(); +} \ No newline at end of file diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js index aa9427ffc4..cebbbcc950 100644 --- a/src/execution/__tests__/executor-test.js +++ b/src/execution/__tests__/executor-test.js @@ -3,6 +3,8 @@ import { describe, it } from 'mocha'; import inspect from '../../jsutils/inspect'; import invariant from '../../jsutils/invariant'; +import expectJSON from '../../__testUtils__/expectJSON'; +import resolveOnNextTick from '../../__testUtils__/resolveOnNextTick'; import { Kind } from '../../language/kinds'; import { parse } from '../../language/parser'; @@ -646,6 +648,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); + 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', () => { const A = new GraphQLObjectType({ name: 'A', diff --git a/src/execution/execute.js b/src/execution/execute.js index 35f440c720..457ac34658 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 From b9dc10cfc407de71e590650782b651bede061475 Mon Sep 17 00:00:00 2001 From: Chris Karcher Date: Wed, 11 May 2022 10:58:27 -0500 Subject: [PATCH 2/3] fix formatting and flow typing issues --- src/__testUtils__/expectJSON.js | 41 +++--------------------- src/execution/__tests__/executor-test.js | 2 +- src/execution/execute.js | 10 +++--- 3 files changed, 10 insertions(+), 43 deletions(-) diff --git a/src/__testUtils__/expectJSON.js b/src/__testUtils__/expectJSON.js index 9e09af0e85..e676155347 100644 --- a/src/__testUtils__/expectJSON.js +++ b/src/__testUtils__/expectJSON.js @@ -1,51 +1,18 @@ import { expect } from 'chai'; -import isObjectLike from '../jsutils/isObjectLike'; -import mapValue from '../jsutils/mapValue'; +import toJSONDeep from '../language/__tests__/toJSONDeep'; -/** - * Deeply transforms an arbitrary value to a JSON-safe value by calling toJSON - * on any nested value which defines it. - */ -function toJSONDeep(value) { - if (!isObjectLike(value)) { - return value; - } - - if (typeof value.toJSON === 'function') { - return value.toJSON(); - } - - if (Array.isArray(value)) { - return value.map(toJSONDeep); - } - - return mapValue(value, toJSONDeep); -} - -export default function expectJSON(actual) { +export default function expectJSON(actual: mixed) { const actualJSON = toJSONDeep(actual); return { - toDeepEqual(expected) { + toDeepEqual(expected: mixed) { const expectedJSON = toJSONDeep(expected); expect(actualJSON).to.deep.equal(expectedJSON); }, - toDeepNestedProperty(path, expected) { + toDeepNestedProperty(path: string, expected: mixed) { const expectedJSON = toJSONDeep(expected); expect(actualJSON).to.deep.nested.property(path, expectedJSON); }, }; } - -export function expectToThrowJSON(fn) { - function mapException() { - try { - return fn(); - } catch (error) { - throw toJSONDeep(error); - } - } - - return expect(mapException).to.throw(); -} \ No newline at end of file diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js index cebbbcc950..c4d5252214 100644 --- a/src/execution/__tests__/executor-test.js +++ b/src/execution/__tests__/executor-test.js @@ -689,7 +689,7 @@ describe('Execute: Handles basic execution tasks', () => { errors: [ { message: - 'Cannot return null for non-nullable field Query.syncNullError.', + 'Cannot return null for non-nullable field Query.syncNullError.', locations: [{ line: 4, column: 9 }], path: ['syncNullError'], }, diff --git a/src/execution/execute.js b/src/execution/execute.js index 457ac34658..370a308456 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -461,11 +461,11 @@ function executeFields( const fieldNodes = fields[responseName]; const fieldPath = addPath(path, responseName, parentType.name); const result = resolveField( - exeContext, - parentType, - sourceValue, - fieldNodes, - fieldPath, + exeContext, + parentType, + sourceValue, + fieldNodes, + fieldPath, ); if (result !== undefined) { From cd9b7b5e771e763a70a078112ad3c0bb853515d3 Mon Sep 17 00:00:00 2001 From: Chris Karcher Date: Wed, 11 May 2022 12:54:48 -0500 Subject: [PATCH 3/3] replace `expectJSON` usage with `.to.deep.equal` --- src/__testUtils__/expectJSON.js | 18 ------------------ src/execution/__tests__/executor-test.js | 3 +-- 2 files changed, 1 insertion(+), 20 deletions(-) delete mode 100644 src/__testUtils__/expectJSON.js diff --git a/src/__testUtils__/expectJSON.js b/src/__testUtils__/expectJSON.js deleted file mode 100644 index e676155347..0000000000 --- a/src/__testUtils__/expectJSON.js +++ /dev/null @@ -1,18 +0,0 @@ -import { expect } from 'chai'; - -import toJSONDeep from '../language/__tests__/toJSONDeep'; - -export default function expectJSON(actual: mixed) { - const actualJSON = toJSONDeep(actual); - - return { - toDeepEqual(expected: mixed) { - const expectedJSON = toJSONDeep(expected); - expect(actualJSON).to.deep.equal(expectedJSON); - }, - toDeepNestedProperty(path: string, expected: mixed) { - const expectedJSON = toJSONDeep(expected); - expect(actualJSON).to.deep.nested.property(path, expectedJSON); - }, - }; -} diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js index c4d5252214..195b8cde60 100644 --- a/src/execution/__tests__/executor-test.js +++ b/src/execution/__tests__/executor-test.js @@ -3,7 +3,6 @@ import { describe, it } from 'mocha'; import inspect from '../../jsutils/inspect'; import invariant from '../../jsutils/invariant'; -import expectJSON from '../../__testUtils__/expectJSON'; import resolveOnNextTick from '../../__testUtils__/resolveOnNextTick'; import { Kind } from '../../language/kinds'; @@ -684,7 +683,7 @@ describe('Execute: Handles basic execution tasks', () => { const result = await execute({ schema, document }); expect(isAsyncResolverCalled).to.equal(true); - expectJSON(result).toDeepEqual({ + expect(result).to.deep.equal({ data: null, errors: [ {