Fix crash in node when mixing sync/async resolvers (backport of graphql#3529)

Author: chrskrchr

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();
+}

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',

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