Make input objects and lists return better errors

freiksenet · freiksenet · commit 2ada5c69ecf3 · 2015-11-10T15:59:42.000+02:00
Add index of invalid element or field of invalid field and make it recurse
into children until faulty place is found.
diff --git a/src/execution/__tests__/variables.js b/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) } },
@@ -232,7 +249,8 @@ describe('Execute: Handles inputs', () => {
           locations: [ { line: 2, column: 17 } ],
           message:
             'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: {"a":"foo","b":"bar","c":null}.'
+            'got: {"a":"foo","b":"bar","c":null}. ' +
+            'In field c: Expected non-null value.'
         });
       });
 
@@ -250,7 +268,7 @@ describe('Execute: Handles inputs', () => {
           locations: [ { line: 2, column: 17 } ],
           message:
             'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: "foo bar".'
+            'got: "foo bar". Not an object.'
         });
       });
 
@@ -268,10 +286,37 @@ describe('Execute: Handles inputs', () => {
           locations: [ { line: 2, column: 17 } ],
           message:
             'Variable "$input" expected value of type "TestInputObject" but ' +
-            'got: {"a":"foo","b":"bar"}.'
+            'got: {"a":"foo","b":"bar"}. In field c: Expected non-null value.'
         });
       });
 
+      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" expected value of type "TestNestedInputObject" but ' +
+            'got: {"na":{"a":"foo"}}.' +
+            ' In field na: In field c: Expected non-null value.' +
+            ' In field nb: Expected non-null value.'
+        });
+
+      });
+
       it('errors on addition of unknown input field', async () => {
         var params = { input: { a: 'foo', b: 'bar', c: 'baz', d: 'dog' } };
 
@@ -643,7 +688,7 @@ describe('Execute: Handles inputs', () => {
         locations: [ { line: 2, column: 17 } ],
         message:
           'Variable "$input" expected value of type "[String!]" but got: ' +
-          '["A",null,"B"].'
+          '["A",null,"B"]. In element #1: Expected non-null value.'
       });
     });
 
@@ -706,7 +751,7 @@ describe('Execute: Handles inputs', () => {
         locations: [ { line: 2, column: 17 } ],
         message:
           'Variable "$input" expected value of type "[String!]!" but got: ' +
-          '["A",null,"B"].'
+          '["A",null,"B"]. In element #1: Expected non-null value.'
       });
     });
 
diff --git a/src/execution/values.js b/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,11 @@ 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)}.`,
+    `"${print(definitionAST.type)}" but got: ${JSON.stringify(input)}.` +
+    `${message}`,
     [ definitionAST ]
   );
 }
diff --git a/src/utilities/isValidJSValue.js b/src/utilities/isValidJSValue.js
@@ -25,45 +25,58 @@ 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) {
     if (isNullish(value)) {
-      return false;
+      return [ 'Expected non-null value.' ];
     }
     var nullableType: GraphQLInputType = (type.ofType: any);
     return isValidJSValue(value, nullableType);
   }
 
   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 [ '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('Unknown field "${providedField}".');
+      }
     }
 
     // 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 +86,10 @@ 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.strigify(value)}".` ];
+  }
+
+  return [];
 }
diff --git a/src/utilities/isValidLiteralValue.js b/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,70 @@ 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) {
     if (!valueAST) {
-      return false;
+      return [ 'Expected non-null value.' ];
     }
     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 [ '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(`Unknown field "${providedFieldAST.name.value}".`);
+      }
     }
 
     // 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 +111,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 [];
 }
diff --git a/src/validation/__tests__/ArgumentsOfCorrectType.js b/src/validation/__tests__/ArgumentsOfCorrectType.js
@@ -15,9 +15,9 @@ import {
 } from '../rules/ArgumentsOfCorrectType';
 
 
-function badValue(argName, typeName, value, line, column) {
+function badValue(argName, typeName, value, line, column, errors) {
   return {
-    message: badValueMessage(argName, typeName, value),
+    message: badValueMessage(argName, typeName, value, errors),
     locations: [ { line, column } ],
   };
 }
@@ -725,7 +725,9 @@ describe('Validate: Argument values of correct type', () => {
           }
         }
       `, [
-        badValue('complexArg', 'ComplexInput', '{intField: 4}', 4, 41),
+        badValue('complexArg', 'ComplexInput', '{intField: 4}', 4, 41, [
+          'In field requiredField: Expected non-null value.'
+        ]),
       ]);
     });
 
@@ -766,7 +768,8 @@ describe('Validate: Argument values of correct type', () => {
           'ComplexInput',
           '{requiredField: true, unknownField: "value"}',
           4,
-          41
+          41,
+          [ 'Unknown field unknownField.' ]
         ),
       ]);
     });
diff --git a/src/validation/__tests__/DefaultValuesOfCorrectType.js b/src/validation/__tests__/DefaultValuesOfCorrectType.js
@@ -23,9 +23,9 @@ function defaultForNonNullArg(varName, typeName, guessTypeName, line, column) {
   };
 }
 
-function badValue(varName, typeName, val, line, column) {
+function badValue(varName, typeName, val, line, column, errors) {
   return {
-    message: badValueForDefaultArgMessage(varName, typeName, val),
+    message: badValueForDefaultArgMessage(varName, typeName, val, errors),
     locations: [ { line, column } ],
   };
 }
@@ -83,7 +83,9 @@ describe('Validate: Variable default values of correct type', () => {
     `, [
       badValue('a', 'Int', '"one"', 3, 19),
       badValue('b', 'String', '4', 4, 22),
-      badValue('c', 'ComplexInput', '"notverycomplex"', 5, 28)
+      badValue('c', 'ComplexInput', '"notverycomplex"', 5, 28, [
+        'Not an object.'
+      ])
     ]);
   });
 
@@ -93,7 +95,9 @@ describe('Validate: Variable default values of correct type', () => {
         dog { name }
       }
     `, [
-      badValue('a', 'ComplexInput', '{intField: 3}', 2, 53)
+      badValue('a', 'ComplexInput', '{intField: 3}', 2, 53, [
+        'In field requiredField: Expected non-null value.'
+      ])
     ]);
   });
 
diff --git a/src/validation/rules/ArgumentsOfCorrectType.js b/src/validation/rules/ArgumentsOfCorrectType.js
diff --git a/src/validation/rules/DefaultValuesOfCorrectType.js b/src/validation/rules/DefaultValuesOfCorrectType.js
