Merge branch 'better-error-messages-for-inputs' of https://github.com/freiksenet/graphql-js into freiksenet-better-error-messages-for-inputs

Author: leebyron

src/execution/__tests__/variables.js

@@ -56,6 +56,14 @@ var TestInputObject = new GraphQLInputObjectType({
   }
 });
 
+var TestNestedInputObject = new GraphQLInputObjectType({
+  name: 'TestNestedInputObject',
+  fields: {
+    na: { type: new GraphQLNonNull(TestInputObject) },
+    nb: { type: new GraphQLNonNull(GraphQLString) },
+  },
+});
+
 var TestType = new GraphQLObjectType({
   name: 'TestType',
   fields: {
@@ -79,6 +87,15 @@ var TestType = new GraphQLObjectType({
       args: { input: { type: GraphQLString, defaultValue: 'Hello World' } },
       resolve: (_, { input }) => input && JSON.stringify(input)
     },
+    fieldWithNestedInputObject: {
+      type: GraphQLString,
+      args: {
+        input: {
+          type: TestNestedInputObject, defaultValue: 'Hello World'
+        }
+      },
+      resolve: (_, { input }) => input && JSON.stringify(input)
+    },
     list: {
       type: GraphQLString,
       args: { input: { type: new GraphQLList(GraphQLString) } },
@@ -231,8 +248,9 @@ describe('Execute: Handles inputs', () => {
         expect(caughtError).to.containSubset({
           locations: [ { line: 2, column: 17 } ],
           message:
-            'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: {"a":"foo","b":"bar","c":null}.'
+            'Variable "$input" got invalid value ' +
+            '{"a":"foo","b":"bar","c":null}.' +
+            '\nIn field "c": Expected "String!", found null.'
         });
       });
 
@@ -249,8 +267,8 @@ describe('Execute: Handles inputs', () => {
         expect(caughtError).to.containSubset({
           locations: [ { line: 2, column: 17 } ],
           message:
-            'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: "foo bar".'
+            'Variable "$input" got invalid value "foo bar".' +
+            '\nExpected "TestInputObject", found not an object.'
         });
       });
 
@@ -267,9 +285,35 @@ describe('Execute: Handles inputs', () => {
         expect(caughtError).to.containSubset({
           locations: [ { line: 2, column: 17 } ],
           message:
-            'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: {"a":"foo","b":"bar"}.'
+            'Variable "$input" got invalid value {"a":"foo","b":"bar"}.' +
+            '\nIn field "c": Expected "String!", found null.'
+        });
+      });
+
+      it('errors on deep nested errors and with many errors', async () => {
+        var nestedDoc = `
+          query q($input: TestNestedInputObject) {
+            fieldWithNestedObjectInput(input: $input)
+          }
+        `;
+        var nestedAst = parse(nestedDoc);
+        var params = { input: { na: { a: 'foo' } } };
+
+        var caughtError;
+        try {
+          execute(schema, nestedAst, null, params);
+        } catch (error) {
+          caughtError = error;
+        }
+
+        expect(caughtError).to.containSubset({
+          locations: [ { line: 2, column: 19 } ],
+          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.'
         });
+
       });
 
       it('errors on addition of unknown input field', async () => {
@@ -285,8 +329,10 @@ describe('Execute: Handles inputs', () => {
         expect(caughtError).to.containSubset({
           locations: [ { line: 2, column: 17 } ],
           message:
-            'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: {"a":"foo","b":"bar","c":"baz","d":"dog"}.'
+            'Variable "$input" got invalid value ' +
+             '{"a":"foo","b":"bar","c":"baz","d":"dog"}.' +
+             '\nIn field "d": Expected type "ComplexScalar", found "dog".'
+
         });
       });
 
@@ -642,8 +688,8 @@ describe('Execute: Handles inputs', () => {
       expect(caughtError).to.containSubset({
         locations: [ { line: 2, column: 17 } ],
         message:
-          'Variable "$input" expected value of type "[String!]" but got: ' +
-          '["A",null,"B"].'
+          'Variable "$input" got invalid value ["A",null,"B"].' +
+          '\nIn element #1: Expected "String!", found null.'
       });
     });
 
@@ -705,8 +751,8 @@ describe('Execute: Handles inputs', () => {
       expect(caughtError).to.containSubset({
         locations: [ { line: 2, column: 17 } ],
         message:
-          'Variable "$input" expected value of type "[String!]!" but got: ' +
-          '["A",null,"B"].'
+          'Variable "$input" got invalid value ["A",null,"B"].' +
+          '\nIn element #1: Expected "String!", found null.'
       });
     });
 

src/execution/values.js

@@ -94,7 +94,8 @@ function getVariableValue(
     );
   }
   var inputType: GraphQLInputType = (type: any);
-  if (isValidJSValue(input, inputType)) {
+  var errors = isValidJSValue(input, inputType);
+  if (!errors.length) {
     if (isNullish(input)) {
       var defaultValue = definitionAST.defaultValue;
       if (defaultValue) {
@@ -110,9 +111,10 @@ function getVariableValue(
       [ definitionAST ]
     );
   }
+  var message = errors ? '\n' + errors.join('\n') : '';
   throw new GraphQLError(
-    `Variable "$${variable.name.value}" expected value of type ` +
-    `"${print(definitionAST.type)}" but got: ${JSON.stringify(input)}.`,
+    `Variable "$${variable.name.value}" got invalid value ` +
+    `${JSON.stringify(input)}.${message}`,
     [ definitionAST ]
   );
 }

src/type/__tests__/enumType.js

@@ -122,8 +122,10 @@ describe('Type System: Enum Values', () => {
       await graphql(schema, '{ colorEnum(fromEnum: "GREEN") }')
     ).to.deep.equal({
       errors: [
-        { message:
-            'Argument "fromEnum" expected type "Color" but got: "GREEN".' }
+        {
+          message: 'Argument "fromEnum" has invalid value "GREEN".' +
+            '\nExpected type \"Color\", found "GREEN".'
+        }
       ]
     });
   });
@@ -143,7 +145,8 @@ describe('Type System: Enum Values', () => {
       await graphql(schema, '{ colorEnum(fromEnum: 1) }')
     ).to.deep.equal({
       errors: [
-        { message: 'Argument "fromEnum" expected type "Color" but got: 1.' }
+        { message: 'Argument "fromEnum" has invalid value 1.' +
+            '\nExpected type "Color", found 1.' }
       ]
     });
   });
@@ -153,7 +156,8 @@ describe('Type System: Enum Values', () => {
       await graphql(schema, '{ colorEnum(fromInt: GREEN) }')
     ).to.deep.equal({
       errors: [
-        { message: 'Argument "fromInt" expected type "Int" but got: GREEN.' }
+        { message: 'Argument "fromInt" has invalid value GREEN.' +
+            '\nExpected type "Int", found GREEN.' }
       ]
     });
   });
@@ -213,8 +217,11 @@ describe('Type System: Enum Values', () => {
       )
     ).to.deep.equal({
       errors: [
-        { message:
-            'Variable "$color" expected value of type "Color!" but got: 2.' }
+        {
+          message:
+            'Variable "\$color" got invalid value 2.' +
+            '\nExpected type "Color", found 2.'
+        }
       ]
     });
   });

src/utilities/isValidJSValue.js

@@ -25,45 +25,61 @@ import type { GraphQLInputType } from '../type/definition';
  * accepted for that type. This is primarily useful for validating the
  * runtime values of query variables.
  */
-export function isValidJSValue(value: any, type: GraphQLInputType): boolean {
+export function isValidJSValue(value: any, type: GraphQLInputType): [ string ] {
   // A value must be provided if the type is non-null.
   if (type instanceof GraphQLNonNull) {
+    var ofType: GraphQLInputType = (type.ofType: any);
     if (isNullish(value)) {
-      return false;
+      if (ofType.name) {
+        return [ `Expected "${ofType.name}!", found null.` ];
+      }
+      return [ 'Expected non-null value, found null.' ];
     }
-    var nullableType: GraphQLInputType = (type.ofType: any);
-    return isValidJSValue(value, nullableType);
+    return isValidJSValue(value, ofType);
   }
 
   if (isNullish(value)) {
-    return true;
+    return [];
   }
 
   // Lists accept a non-list value as a list of one.
   if (type instanceof GraphQLList) {
     var itemType: GraphQLInputType = (type.ofType: any);
     if (Array.isArray(value)) {
-      return value.every(item => isValidJSValue(item, itemType));
+      return value.reduce((acc, item, index) => {
+        var errors = isValidJSValue(item, itemType);
+        return acc.concat(errors.map(error =>
+          `In element #${index}: ${error}`
+        ));
+      }, []);
     }
     return isValidJSValue(value, itemType);
   }
 
   // Input objects check each defined field.
   if (type instanceof GraphQLInputObjectType) {
     if (typeof value !== 'object') {
-      return false;
+      return [ `Expected "${type.name}", found not an object.` ];
     }
     var fields = type.getFields();
 
+    var errors = [];
+
     // Ensure every provided field is defined.
-    if (Object.keys(value).some(fieldName => !fields[fieldName])) {
-      return false;
+    for (var providedField of Object.keys(value)) {
+      if (!fields[providedField]) {
+        errors.push('In field "${providedField}": Unknown field.');
+      }
     }
 
     // Ensure every defined field is valid.
-    return Object.keys(fields).every(
-      fieldName => isValidJSValue(value[fieldName], fields[fieldName].type)
-    );
+    for (var fieldName of Object.keys(fields)) {
+      var newErrors = isValidJSValue(value[fieldName], fields[fieldName].type);
+      errors.push(...(newErrors.map(error =>
+        `In field "${fieldName}": ${error}`
+      )));
+    }
+    return errors;
   }
 
   invariant(
@@ -73,5 +89,12 @@ export function isValidJSValue(value: any, type: GraphQLInputType): boolean {
 
   // Scalar/Enum input checks to ensure the type can parse the value to
   // a non-null value.
-  return !isNullish(type.parseValue(value));
+  var parseResult = type.parseValue(value);
+  if (isNullish(parseResult)) {
+    return [
+      `Expected type "${type.name}", found ${JSON.stringify(value)}.`
+    ];
+  }
+
+  return [];
 }

src/utilities/isValidLiteralValue.js

@@ -8,6 +8,7 @@
  *  of patent rights can be found in the PATENTS file in the same directory.
  */
 
+import { print } from '../language/printer';
 import type { Value, ListValue, ObjectValue } from '../language/ast';
 import {
   VARIABLE,
@@ -37,56 +38,75 @@ import isNullish from '../jsutils/isNullish';
 export function isValidLiteralValue(
   type: GraphQLInputType,
   valueAST: Value
-): boolean {
+): [ string ] {
   // A value must be provided if the type is non-null.
   if (type instanceof GraphQLNonNull) {
+    var ofType: GraphQLInputType = (type.ofType: any);
     if (!valueAST) {
-      return false;
+      if (ofType.name) {
+        return [ `Expected "${ofType.name}!", found null.` ];
+      }
+      return [ 'Expected non-null value, found null.' ];
     }
-    var ofType: GraphQLInputType = (type.ofType: any);
     return isValidLiteralValue(ofType, valueAST);
   }
 
   if (!valueAST) {
-    return true;
+    return [];
   }
 
   // This function only tests literals, and assumes variables will provide
   // values of the correct type.
   if (valueAST.kind === VARIABLE) {
-    return true;
+    return [];
   }
 
   // Lists accept a non-list value as a list of one.
   if (type instanceof GraphQLList) {
     var itemType: GraphQLInputType = (type.ofType: any);
     if (valueAST.kind === LIST) {
-      return (valueAST: ListValue).values.every(
-        itemAST => isValidLiteralValue(itemType, itemAST)
-      );
+      return (valueAST: ListValue).values.reduce((acc, itemAST, index) => {
+        var errors = isValidLiteralValue(itemType, itemAST);
+        return acc.concat(errors.map(error =>
+          `In element #${index}: ${error}`
+        ));
+      }, []);
     }
     return isValidLiteralValue(itemType, valueAST);
   }
 
   // Input objects check each defined field and look for undefined fields.
   if (type instanceof GraphQLInputObjectType) {
     if (valueAST.kind !== OBJECT) {
-      return false;
+      return [ `Expected "${type.name}", found not an object.` ];
     }
     var fields = type.getFields();
 
+    var errors = [];
+
     // Ensure every provided field is defined.
     var fieldASTs = (valueAST: ObjectValue).fields;
-    if (fieldASTs.some(fieldAST => !fields[fieldAST.name.value])) {
-      return false;
+    for (var providedFieldAST of fieldASTs) {
+      if (!fields[providedFieldAST.name.value]) {
+        errors.push(
+          `In field "${providedFieldAST.name.value}": Unknown field.`
+        );
+      }
     }
 
     // Ensure every defined field is valid.
     var fieldASTMap = keyMap(fieldASTs, fieldAST => fieldAST.name.value);
-    return Object.keys(fields).every(fieldName => isValidLiteralValue(
-      fields[fieldName].type,
-      fieldASTMap[fieldName] && fieldASTMap[fieldName].value
-    ));
+    for (var fieldName of Object.keys(fields)) {
+      var result = isValidLiteralValue(
+        fields[fieldName].type,
+        fieldASTMap[fieldName] && fieldASTMap[fieldName].value
+      );
+      errors.push(...(result.map(error =>
+        `In field "${fieldName}": ${error}`
+      )));
+    }
+
+    return errors;
   }
 
   invariant(
@@ -96,5 +116,10 @@ export function isValidLiteralValue(
 
   // Scalar/Enum input checks to ensure the type can parse the value to
   // a non-null value.
-  return !isNullish(type.parseLiteral(valueAST));
+  var parseResult = type.parseLiteral(valueAST);
+  if (isNullish(parseResult)) {
+    return [ `Expected type "${type.name}", found ${print(valueAST)}.` ];
+  }
+
+  return [];
 }