[FIX] Include locations for errors related to failed value completion. (#445)

Error locations were previously only being added due to async value resolution and not during value completion. This meant if an error was raised due to an incorrect response from a resolver function that it would not have gotten wrapped in a location-aware GraphQLError object.

This includes a new intermediate phase which catches and assigns these locations before further propogating the error.

This also assigns non-enumable property for `originalError` so it is not included during JSON.stringify.

Author: leebyron

src/error/GraphQLError.js

@@ -17,19 +17,21 @@ export class GraphQLError extends Error {
   message: string;
   stack: string;
   nodes: ?Array<Node>;
-  source: Source;
-  positions: Array<number>;
-  locations: any;
-  path: Array<string | number>;
+  source: ?Source;
+  positions: ?Array<number>;
+  locations: ?Array<{ line: number, column: number }>;
+  path: ?Array<string | number>;
   originalError: ?Error;
 
   constructor(
     message: string,
     // A flow bug keeps us from declaring nodes as an array of Node
     nodes?: Array<any/* Node */>,
     stack?: ?string,
-    source?: Source,
-    positions?: Array<number>
+    source?: ?Source,
+    positions?: ?Array<number>,
+    path?: ?Array<string|number>,
+    originalError?: ?Error
   ) {
     super(message);
 
@@ -94,5 +96,17 @@ export class GraphQLError extends Error {
       // service adheres to the spec.
       enumerable: true,
     }: any));
+
+    Object.defineProperty(this, 'path', {
+      value: path,
+      // By being enumerable, JSON.stringify will include `path` in the
+      // resulting output. This ensures that the simplist possible GraphQL
+      // service adheres to the spec.
+      enumerable: true
+    });
+
+    Object.defineProperty(this, 'originalError', {
+      value: originalError
+    });
   }
 }

src/error/locatedError.js

@@ -21,12 +21,23 @@ export function locatedError(
   nodes: Array<any>,
   path: Array<string | number>
 ): GraphQLError {
+  // Note: this uses a brand-check to support GraphQL errors originating from
+  // other contexts.
+  if (originalError && originalError.hasOwnProperty('locations')) {
+    return (originalError: any);
+  }
+
   const message = originalError ?
     originalError.message || String(originalError) :
     'An unknown error occurred.';
   const stack = originalError ? originalError.stack : null;
-  const error = new GraphQLError(message, nodes, stack);
-  error.path = path;
-  error.originalError = originalError;
-  return error;
+  return new GraphQLError(
+    message,
+    nodes,
+    stack,
+    null,
+    null,
+    path,
+    originalError
+  );
 }

src/execution/__tests__/executor-test.js

@@ -363,8 +363,6 @@ describe('Execute: Handles basic execution tasks', () => {
         locations: [ { line: 6, column: 7 } ] },
       { message: 'Error getting syncReturnErrorList3',
         locations: [ { line: 6, column: 7 } ] },
-      { message: 'Error getting asyncReturnError',
-        locations: [ { line: 13, column: 7 } ] },
       { message: 'Error getting asyncReject',
         locations: [ { line: 8, column: 7 } ] },
       { message: 'Error getting asyncRawReject',
@@ -375,6 +373,8 @@ describe('Execute: Handles basic execution tasks', () => {
         locations: [ { line: 11, column: 7 } ] },
       { message: 'Error getting asyncRawError',
         locations: [ { line: 12, column: 7 } ] },
+      { message: 'Error getting asyncReturnError',
+        locations: [ { line: 13, column: 7 } ] },
     ]);
   });
 

src/execution/execute.js

@@ -620,9 +620,9 @@ function completeValueCatchingError(
   result: mixed
 ): mixed {
   // If the field type is non-nullable, then it is resolved without any
-  // protection from errors.
+  // protection from errors, however it still properly locates the error.
   if (returnType instanceof GraphQLNonNull) {
-    return completeValue(
+    return completeValueWithLocatedError(
       exeContext,
       returnType,
       fieldASTs,
@@ -635,7 +635,7 @@ function completeValueCatchingError(
   // Otherwise, error protection is applied, logging the error and resolving
   // a null value for this field if one is encountered.
   try {
-    const completed = completeValue(
+    const completed = completeValueWithLocatedError(
       exeContext,
       returnType,
       fieldASTs,
@@ -644,8 +644,8 @@ function completeValueCatchingError(
       result
     );
     if (isThenable(completed)) {
-      // If `completeValue` returned a rejected promise, log the rejection
-      // error and resolve to null.
+      // If `completeValueWithLocatedError` returned a rejected promise, log
+      // the rejection error and resolve to null.
       // Note: we don't rely on a `catch` method, but we do expect "thenable"
       // to take a second callback for the error case.
       return ((completed: any): Promise<*>).then(undefined, error => {
@@ -655,13 +655,43 @@ function completeValueCatchingError(
     }
     return completed;
   } catch (error) {
-    // If `completeValue` returned abruptly (threw an error), log the error
-    // and return null.
+    // If `completeValueWithLocatedError` returned abruptly (threw an error),
+    // log the error and return null.
     exeContext.errors.push(error);
     return null;
   }
 }
 
+// This is a small wrapper around completeValue which annotates errors with
+// location information.
+function completeValueWithLocatedError(
+  exeContext: ExecutionContext,
+  returnType: GraphQLType,
+  fieldASTs: Array<Field>,
+  info: GraphQLResolveInfo,
+  path: Array<string | number>,
+  result: mixed
+): mixed {
+  try {
+    const completed = completeValue(
+      exeContext,
+      returnType,
+      fieldASTs,
+      info,
+      path,
+      result
+    );
+    if (isThenable(completed)) {
+      return ((completed: any): Promise<*>).catch(
+        error => Promise.reject(locatedError(error, fieldASTs, path))
+      );
+    }
+    return completed;
+  } catch (error) {
+    throw locatedError(error, fieldASTs, path);
+  }
+}
+
 /**
  * Implements the instructions for completeValue as defined in the
  * "Field entries" section of the spec.
@@ -694,23 +724,20 @@ function completeValue(
   // If result is a Promise, apply-lift over completeValue.
   if (isThenable(result)) {
     return ((result: any): Promise<*>).then(
-      // Once resolved to a value, complete that value.
       resolved => completeValue(
         exeContext,
         returnType,
         fieldASTs,
         info,
         path,
         resolved
-      ),
-      // If rejected, create a located error, and continue to reject.
-      error => Promise.reject(locatedError(error, fieldASTs, path))
+      )
     );
   }
 
   // If result is an Error, throw a located error.
   if (result instanceof Error) {
-    throw locatedError(result, fieldASTs, path);
+    throw result;
   }
 
   // If field type is NonNull, complete for inner type, and throw field error
@@ -725,10 +752,9 @@ function completeValue(
       result
     );
     if (completed === null) {
-      throw new GraphQLError(
+      throw new Error(
         `Cannot return null for non-nullable field ${
-          info.parentType.name}.${info.fieldName}.`,
-        fieldASTs
+          info.parentType.name}.${info.fieldName}.`
       );
     }
     return completed;
@@ -785,8 +811,7 @@ function completeValue(
   }
 
   // Not reachable. All possible output types have been considered.
-  invariant(
-    false,
+  throw new Error(
     `Cannot complete value of unexpected type "${String(returnType)}".`
   );
 }
@@ -845,7 +870,13 @@ function completeLeafValue(
 ): mixed {
   invariant(returnType.serialize, 'Missing serialize method on type');
   const serializedResult = returnType.serialize(result);
-  return isNullish(serializedResult) ? null : serializedResult;
+  if (isNullish(serializedResult)) {
+    throw new Error(
+      `Expected a value of type "${String(returnType)}" but ` +
+      `received: ${String(result)}`
+    );
+  }
+  return serializedResult;
 }
 
 /**

src/type/__tests__/enumType-test.js

@@ -174,10 +174,14 @@ describe('Type System: Enum Values', () => {
   it('does not accept incorrect internal value', async () => {
     expect(
       await graphql(schema, '{ colorEnum(fromString: "GREEN") }')
-    ).to.jsonEqual({
+    ).to.containSubset({
       data: {
         colorEnum: null
-      }
+      },
+      errors: [
+        { message: 'Expected a value of type "Color" but received: GREEN',
+          locations: [ { line: 1, column: 3 } ] }
+      ]
     });
   });
 
@@ -367,13 +371,18 @@ describe('Type System: Enum Values', () => {
         good: complexEnum(provideGoodValue: true)
         bad: complexEnum(provideBadValue: true)
       }`)
-    ).to.jsonEqual({
+    ).to.containSubset({
       data: {
         first: 'ONE',
         second: 'TWO',
         good: 'TWO',
         bad: null
-      }
+      },
+      errors: [ {
+        message:
+          'Expected a value of type "Complex" but received: [object Object]',
+        locations: [ { line: 5, column: 9 } ]
+      } ]
     });
   });