Preserve original coercion errors, improve error quality.

This is a fairly major refactoring of coerceValue which returns an Either so it can return a complete collection of errors. This allows originalError to be preserved for scalar coercion errors and ensures *all* errors are represented in the response.

This had a minor change to the logic in execute / subscribe to allow for buildExecutionContext to abrupt complete with multiple errors.

Author: leebyron

src/execution/__tests__/variables-test.js

@@ -11,7 +11,6 @@
 import { expect } from 'chai';
 import { describe, it } from 'mocha';
 import { execute } from '../execute';
-import { coerceValue } from '../values';
 import { parse } from '../../language';
 import {
   GraphQLSchema,
@@ -301,8 +300,8 @@ describe('Execute: Handles inputs', () => {
             {
               message:
                 'Variable "$input" got invalid value ' +
-                '{"a":"foo","b":"bar","c":null}.' +
-                '\nIn field "c": Expected "String!", found null.',
+                '{"a":"foo","b":"bar","c":null}; ' +
+                'Expected non-nullable type String! at value.c.',
               locations: [{ line: 2, column: 17 }],
               path: undefined,
             },
@@ -318,8 +317,8 @@ describe('Execute: Handles inputs', () => {
           errors: [
             {
               message:
-                'Variable "$input" got invalid value "foo bar".' +
-                '\nExpected "TestInputObject", found not an object.',
+                'Variable "$input" got invalid value "foo bar"; ' +
+                'Expected object type TestInputObject.',
               locations: [{ line: 2, column: 17 }],
               path: undefined,
             },
@@ -335,8 +334,8 @@ describe('Execute: Handles inputs', () => {
           errors: [
             {
               message:
-                'Variable "$input" got invalid value {"a":"foo","b":"bar"}.' +
-                '\nIn field "c": Expected "String!", found null.',
+                'Variable "$input" got invalid value {"a":"foo","b":"bar"}; ' +
+                'Field value.c of required type String! was not provided.',
               locations: [{ line: 2, column: 17 }],
               path: undefined,
             },
@@ -358,10 +357,15 @@ describe('Execute: Handles inputs', () => {
           errors: [
             {
               message:
-                'Variable "$input" got invalid value {"na":{"a":"foo"}}.' +
-                '\nIn field "na": In field "c": Expected "String!", ' +
-                'found null.' +
-                '\nIn field "nb": Expected "String!", found null.',
+                'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
+                'Field value.na.c of required type String! was not provided.',
+              locations: [{ line: 2, column: 19 }],
+              path: undefined,
+            },
+            {
+              message:
+                'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
+                'Field value.nb of required type String! was not provided.',
               locations: [{ line: 2, column: 19 }],
               path: undefined,
             },
@@ -380,8 +384,8 @@ describe('Execute: Handles inputs', () => {
             {
               message:
                 'Variable "$input" got invalid value ' +
-                '{"a":"foo","b":"bar","c":"baz","extra":"dog"}.' +
-                '\nIn field "extra": Unknown field.',
+                '{"a":"foo","b":"bar","c":"baz","extra":"dog"}; ' +
+                'Field "extra" is not defined by type TestInputObject.',
               locations: [{ line: 2, column: 17 }],
               path: undefined,
             },
@@ -535,8 +539,8 @@ describe('Execute: Handles inputs', () => {
         errors: [
           {
             message:
-              'Variable "$value" got invalid value null.\n' +
-              'Expected "String!", found null.',
+              'Variable "$value" got invalid value null; ' +
+              'Expected non-nullable type String!.',
             locations: [{ line: 2, column: 31 }],
             path: undefined,
           },
@@ -608,31 +612,21 @@ describe('Execute: Handles inputs', () => {
       const ast = parse(doc);
       const variables = { value: [1, 2, 3] };
 
-      expect(await execute(schema, ast, null, null, variables)).to.deep.equal({
+      const result = await execute(schema, ast, null, null, variables);
+
+      expect(result).to.deep.equal({
         errors: [
           {
             message:
-              'Variable "$value" got invalid value [1,2,3].\nExpected type ' +
-              '"String", found [1,2,3]; String cannot represent an array value: [1,2,3]',
+              'Variable "$value" got invalid value [1,2,3]; Expected type ' +
+              'String; String cannot represent an array value: [1,2,3]',
             locations: [{ line: 2, column: 31 }],
             path: undefined,
           },
         ],
       });
-    });
-
-    it('coercing an array to GraphQLString throws TypeError', async () => {
-      let caughtError;
-      try {
-        coerceValue(GraphQLString, [1, 2, 3]);
-      } catch (error) {
-        caughtError = error;
-      }
 
-      expect(caughtError instanceof TypeError).to.equal(true);
-      expect(caughtError && caughtError.message).to.equal(
-        'String cannot represent an array value: [1,2,3]',
-      );
+      expect(result.errors[0].originalError).not.to.equal(undefined);
     });
 
     it('serializing an array via GraphQLString throws TypeError', async () => {
@@ -744,8 +738,8 @@ describe('Execute: Handles inputs', () => {
         errors: [
           {
             message:
-              'Variable "$input" got invalid value null.\n' +
-              'Expected "[String]!", found null.',
+              'Variable "$input" got invalid value null; ' +
+              'Expected non-nullable type [String]!.',
             locations: [{ line: 2, column: 17 }],
             path: undefined,
           },
@@ -835,8 +829,8 @@ describe('Execute: Handles inputs', () => {
         errors: [
           {
             message:
-              'Variable "$input" got invalid value ["A",null,"B"].' +
-              '\nIn element #1: Expected "String!", found null.',
+              'Variable "$input" got invalid value ["A",null,"B"]; ' +
+              'Expected non-nullable type String! at value[1].',
             locations: [{ line: 2, column: 17 }],
             path: undefined,
           },
@@ -857,8 +851,8 @@ describe('Execute: Handles inputs', () => {
         errors: [
           {
             message:
-              'Variable "$input" got invalid value null.\n' +
-              'Expected "[String!]!", found null.',
+              'Variable "$input" got invalid value null; ' +
+              'Expected non-nullable type [String!]!.',
             locations: [{ line: 2, column: 17 }],
             path: undefined,
           },
@@ -897,8 +891,8 @@ describe('Execute: Handles inputs', () => {
         errors: [
           {
             message:
-              'Variable "$input" got invalid value ["A",null,"B"].' +
-              '\nIn element #1: Expected "String!", found null.',
+              'Variable "$input" got invalid value ["A",null,"B"]; ' +
+              'Expected non-nullable type String! at value[1].',
             locations: [{ line: 2, column: 17 }],
             path: undefined,
           },

src/execution/execute.js

@@ -187,19 +187,19 @@ function executeImpl(
 
   // If a valid context cannot be created due to incorrect arguments,
   // a "Response" with only errors is returned.
-  let context;
-  try {
-    context = buildExecutionContext(
-      schema,
-      document,
-      rootValue,
-      contextValue,
-      variableValues,
-      operationName,
-      fieldResolver,
-    );
-  } catch (error) {
-    return { errors: [error] };
+  const context = buildExecutionContext(
+    schema,
+    document,
+    rootValue,
+    contextValue,
+    variableValues,
+    operationName,
+    fieldResolver,
+  );
+
+  // Return early errors if execution context failed.
+  if (Array.isArray(context)) {
+    return { errors: context };
   }
 
   // Return a Promise that will eventually resolve to the data described by
@@ -291,20 +291,18 @@ export function buildExecutionContext(
   rawVariableValues: ?ObjMap<mixed>,
   operationName: ?string,
   fieldResolver: ?GraphQLFieldResolver<any, any>,
-): ExecutionContext {
+): $ReadOnlyArray<GraphQLError> | ExecutionContext {
   const errors: Array<GraphQLError> = [];
-  let operation: ?OperationDefinitionNode;
+  let operation: OperationDefinitionNode | void;
+  let hasMultipleAssumedOperations = false;
   const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
-  document.definitions.forEach(definition => {
+  for (let i = 0; i < document.definitions.length; i++) {
+    const definition = document.definitions[i];
     switch (definition.kind) {
       case Kind.OPERATION_DEFINITION:
         if (!operationName && operation) {
-          throw new GraphQLError(
-            'Must provide operation name if query contains ' +
-              'multiple operations.',
-          );
-        }
-        if (
+          hasMultipleAssumedOperations = true;
+        } else if (
           !operationName ||
           (definition.name && definition.name.value === operationName)
         ) {
@@ -315,19 +313,46 @@ export function buildExecutionContext(
         fragments[definition.name.value] = definition;
         break;
     }
-  });
+  }
+
   if (!operation) {
     if (operationName) {
-      throw new GraphQLError(`Unknown operation named "${operationName}".`);
+      errors.push(
+        new GraphQLError(`Unknown operation named "${operationName}".`),
+      );
     } else {
-      throw new GraphQLError('Must provide an operation.');
+      errors.push(new GraphQLError('Must provide an operation.'));
     }
+  } else if (hasMultipleAssumedOperations) {
+    errors.push(
+      new GraphQLError(
+        'Must provide operation name if query contains ' +
+          'multiple operations.',
+      ),
+    );
   }
-  const variableValues = getVariableValues(
-    schema,
-    operation.variableDefinitions || [],
-    rawVariableValues || {},
-  );
+
+  let variableValues;
+  if (operation) {
+    const coercedVariableValues = getVariableValues(
+      schema,
+      operation.variableDefinitions || [],
+      rawVariableValues || {},
+    );
+
+    if (coercedVariableValues.errors) {
+      errors.push(...coercedVariableValues.errors);
+    } else {
+      variableValues = coercedVariableValues.coerced;
+    }
+  }
+
+  if (errors.length !== 0) {
+    return errors;
+  }
+
+  invariant(operation, 'Has operation if no errors.');
+  invariant(variableValues, 'Has variabels if no errors.');
 
   return {
     schema,