Validate literals in a single rule with finer precision

This generalizes the "arguments of correct type" and "default values of correct type" to a single rule "values of correct type" which has been re-written to rely on a traversal rather than the utility function `isValidLiteralValue`. To reduce breaking scope, this does not remove that utility even though it's no longer used directly within the library. Since the default values rule included another validation rule that rule was renamed to a more apt "variable default value allowed". This also includes the original errors from custom scalars in the validation error output, solving the remainder of #821. Closes #821
graphql · Dec 14, 2017 · 9655a46 · 9655a46
1 parent 1d90ad2
commit 9655a46
Show file tree

Hide file tree

Showing 20 changed files with 775 additions and 644 deletions.
diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js
@@ -208,9 +208,9 @@ describe('Execute: Handles inputs', () => {
           errors: [
             {
               message:
-                'Argument "input" got invalid value ["foo", "bar", "baz"].\n' +
-                'Expected "TestInputObject", found not an object.',
+                'Argument "input" has invalid value ["foo", "bar", "baz"].',
               path: ['fieldWithObjectInput'],
+              locations: [{ line: 3, column: 39 }],
             },
           ],
         });
@@ -981,9 +981,7 @@ describe('Execute: Handles inputs', () => {
         },
         errors: [
           {
-            message:
-              'Argument "input" got invalid value WRONG_TYPE.\n' +
-              'Expected type "String", found WRONG_TYPE.',
+            message: 'Argument "input" has invalid value WRONG_TYPE.',
             locations: [{ line: 2, column: 46 }],
             path: ['fieldWithDefaultArgumentValue'],
           },

diff --git a/src/execution/values.js b/src/execution/values.js
@@ -14,7 +14,6 @@ import keyMap from '../jsutils/keyMap';
 import { coerceValue } from '../utilities/coerceValue';
 import { typeFromAST } from '../utilities/typeFromAST';
 import { valueFromAST } from '../utilities/valueFromAST';
-import { isValidLiteralValue } from '../utilities/isValidLiteralValue';
 import * as Kind from '../language/kinds';
 import { print } from '../language/printer';
 import { isInputType, isNonNullType } from '../type/definition';
@@ -164,10 +163,11 @@ export function getArgumentValues(
       const valueNode = argumentNode.value;
       const coercedValue = valueFromAST(valueNode, argType, variableValues);
       if (isInvalid(coercedValue)) {
-        const errors = isValidLiteralValue(argType, valueNode);
-        const message = errors ? '\n' + errors.join('\n') : '';
+        // Note: ValuesOfCorrectType validation should catch this before
+        // execution. This is a runtime check to ensure execution does not
+        // continue with an invalid argument value.
         throw new GraphQLError(
-          `Argument "${name}" got invalid value ${print(valueNode)}.${message}`,
+          `Argument "${name}" has invalid value ${print(valueNode)}.`,
           [argumentNode.value],
         );
       }

diff --git a/src/index.js b/src/index.js
@@ -261,8 +261,6 @@ export {
   // All validation rules in the GraphQL Specification.
   specifiedRules,
   // Individual validation rules.
-  ArgumentsOfCorrectTypeRule,
-  DefaultValuesOfCorrectTypeRule,
   FieldsOnCorrectTypeRule,
   FragmentsOnCompositeTypesRule,
   KnownArgumentNamesRule,
@@ -285,7 +283,9 @@ export {
   UniqueInputFieldNamesRule,
   UniqueOperationNamesRule,
   UniqueVariableNamesRule,
+  ValuesOfCorrectTypeRule,
   VariablesAreInputTypesRule,
+  VariablesDefaultValueAllowedRule,
   VariablesInAllowedPositionRule,
 } from './validation';
 

diff --git a/src/jsutils/orList.js b/src/jsutils/orList.js
@@ -0,0 +1,24 @@
+/**
+ * Copyright (c) 2015-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+const MAX_LENGTH = 5;
+
+/**
+ * Given [ A, B, C ] return 'A, B, or C'.
+ */
+export default function orList(items: $ReadOnlyArray<string>): string {
+  const selected = items.slice(0, MAX_LENGTH);
+  return selected.reduce(
+    (list, quoted, index) =>
+      list +
+      (selected.length > 2 ? ', ' : ' ') +
+      (index === selected.length - 1 ? 'or ' : '') +
+      quoted,
+  );
+}
diff --git a/src/jsutils/quotedOrList.js b/src/jsutils/quotedOrList.js
@@ -7,20 +7,11 @@
  * @flow
  */
 
-const MAX_LENGTH = 5;
+import orList from './orList';
 
 /**
  * Given [ A, B, C ] return '"A", "B", or "C"'.
  */
 export default function quotedOrList(items: $ReadOnlyArray<string>): string {
-  const selected = items.slice(0, MAX_LENGTH);
-  return selected
-    .map(item => `"${item}"`)
-    .reduce(
-      (list, quoted, index) =>
-        list +
-        (selected.length > 2 ? ', ' : ' ') +
-        (index === selected.length - 1 ? 'or ' : '') +
-        quoted,
-    );
+  return orList(items.map(item => `"${item}"`));
 }
diff --git a/src/type/__tests__/enumType-test.js b/src/type/__tests__/enumType-test.js
@@ -161,8 +161,21 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Argument "fromEnum" has invalid value "GREEN".' +
-            '\nExpected type "Color", found "GREEN".',
+            'Expected type Color, found "GREEN"; Did you mean the enum value: GREEN?',
+          locations: [{ line: 1, column: 23 }],
+        },
+      ],
+    });
+  });
+
+  it('does not accept values not in the enum', async () => {
+    expect(
+      await graphql(schema, '{ colorEnum(fromEnum: GREENISH) }'),
+    ).to.jsonEqual({
+      errors: [
+        {
+          message:
+            'Expected type Color, found GREENISH; Did you mean the enum value: GREEN?',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -180,6 +193,7 @@ describe('Type System: Enum Values', () => {
         {
           message: 'Expected a value of type "Color" but received: GREEN',
           locations: [{ line: 1, column: 3 }],
+          path: ['colorEnum'],
         },
       ],
     });
@@ -189,9 +203,7 @@ describe('Type System: Enum Values', () => {
     expect(await graphql(schema, '{ colorEnum(fromEnum: 1) }')).to.jsonEqual({
       errors: [
         {
-          message:
-            'Argument "fromEnum" has invalid value 1.' +
-            '\nExpected type "Color", found 1.',
+          message: 'Expected type Color, found 1.',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -203,9 +215,7 @@ describe('Type System: Enum Values', () => {
       {
         errors: [
           {
-            message:
-              'Argument "fromInt" has invalid value GREEN.' +
-              '\nExpected type "Int", found GREEN.',
+            message: 'Expected type Int, found GREEN.',
             locations: [{ line: 1, column: 22 }],
           },
         ],

diff --git a/src/utilities/TypeInfo.js b/src/utilities/TypeInfo.js
@@ -62,6 +62,9 @@ export class TypeInfo {
     // to support non-spec-compliant codebases. You should never need to use it.
     // It may disappear in the future.
     getFieldDefFn?: typeof getFieldDef,
+    // Initial type may be provided in rare cases to facilitate traversals
+    // beginning somewhere other than documents.
+    initialType?: GraphQLType,
   ): void {
     this._schema = schema;
     this._typeStack = [];
@@ -72,6 +75,17 @@ export class TypeInfo {
     this._argument = null;
     this._enumValue = null;
     this._getFieldDef = getFieldDefFn || getFieldDef;
+    if (initialType) {
+      if (isInputType(initialType)) {
+        this._inputTypeStack.push(initialType);
+      }
+      if (isCompositeType(initialType)) {
+        this._parentTypeStack.push(initialType);
+      }
+      if (isOutputType(initialType)) {
+        this._typeStack.push(initialType);
+      }
+    }
   }
 
   getType(): ?GraphQLOutputType {
@@ -92,6 +106,12 @@ export class TypeInfo {
     }
   }
 
+  getParentInputType(): ?GraphQLInputType {
+    if (this._inputTypeStack.length > 1) {
+      return this._inputTypeStack[this._inputTypeStack.length - 2];
+    }
+  }
+
   getFieldDef(): ?GraphQLField<*, *> {
     if (this._fieldDefStack.length > 0) {
       return this._fieldDefStack[this._fieldDefStack.length - 1];
@@ -183,10 +203,9 @@ export class TypeInfo {
         break;
       case Kind.LIST:
         const listType: mixed = getNullableType(this.getInputType());
-        let itemType: mixed;
-        if (isListType(listType)) {
-          itemType = listType.ofType;
-        }
+        const itemType: mixed = isListType(listType)
+          ? listType.ofType
+          : listType;
         this._inputTypeStack.push(isInputType(itemType) ? itemType : undefined);
         break;
       case Kind.OBJECT_FIELD:

diff --git a/src/utilities/isValidLiteralValue.js b/src/utilities/isValidLiteralValue.js
@@ -7,123 +7,30 @@
  * @flow
  */
 
-import { print } from '../language/printer';
-import type {
-  ValueNode,
-  ListValueNode,
-  ObjectValueNode,
-} from '../language/ast';
-import * as Kind from '../language/kinds';
-import {
-  isScalarType,
-  isEnumType,
-  isInputObjectType,
-  isListType,
-  isNonNullType,
-} from '../type/definition';
+import { TypeInfo } from './TypeInfo';
+import type { GraphQLError } from '../error/GraphQLError';
+import type { ValueNode } from '../language/ast';
+import { DOCUMENT } from '../language/kinds';
+import { visit, visitWithTypeInfo } from '../language/visitor';
 import type { GraphQLInputType } from '../type/definition';
-import isInvalid from '../jsutils/isInvalid';
-import keyMap from '../jsutils/keyMap';
+import { GraphQLSchema } from '../type/schema';
+import { ValuesOfCorrectType } from '../validation/rules/ValuesOfCorrectType';
+import { ValidationContext } from '../validation/validate';
 
 /**
- * Utility for validators which determines if a value literal node is valid
- * given an input type.
+ * Utility which determines if a value literal node is valid for an input type.
  *
- * Note that this only validates literal values, variables are assumed to
- * provide values of the correct type.
+ * Deprecated. Rely on validation for documents containing literal values.
  */
 export function isValidLiteralValue(
   type: GraphQLInputType,
   valueNode: ValueNode,
-): Array<string> {
-  // A value must be provided if the type is non-null.
-  if (isNonNullType(type)) {
-    if (!valueNode || valueNode.kind === Kind.NULL) {
-      return [`Expected "${String(type)}", found null.`];
-    }
-    return isValidLiteralValue(type.ofType, valueNode);
-  }
-
-  if (!valueNode || valueNode.kind === Kind.NULL) {
-    return [];
-  }
-
-  // This function only tests literals, and assumes variables will provide
-  // values of the correct type.
-  if (valueNode.kind === Kind.VARIABLE) {
-    return [];
-  }
-
-  // Lists accept a non-list value as a list of one.
-  if (isListType(type)) {
-    const itemType = type.ofType;
-    if (valueNode.kind === Kind.LIST) {
-      return (valueNode: ListValueNode).values.reduce((acc, item, index) => {
-        const errors = isValidLiteralValue(itemType, item);
-        return acc.concat(
-          errors.map(error => `In element #${index}: ${error}`),
-        );
-      }, []);
-    }
-    return isValidLiteralValue(itemType, valueNode);
-  }
-
-  // Input objects check each defined field and look for undefined fields.
-  if (isInputObjectType(type)) {
-    if (valueNode.kind !== Kind.OBJECT) {
-      return [`Expected "${type.name}", found not an object.`];
-    }
-    const fields = type.getFields();
-
-    const errors = [];
-
-    // Ensure every provided field is defined.
-    const fieldNodes = (valueNode: ObjectValueNode).fields;
-    fieldNodes.forEach(providedFieldNode => {
-      if (!fields[providedFieldNode.name.value]) {
-        errors.push(
-          `In field "${providedFieldNode.name.value}": Unknown field.`,
-        );
-      }
-    });
-
-    // Ensure every defined field is valid.
-    const fieldNodeMap = keyMap(fieldNodes, fieldNode => fieldNode.name.value);
-    Object.keys(fields).forEach(fieldName => {
-      const result = isValidLiteralValue(
-        fields[fieldName].type,
-        fieldNodeMap[fieldName] && fieldNodeMap[fieldName].value,
-      );
-      errors.push(...result.map(error => `In field "${fieldName}": ${error}`));
-    });
-
-    return errors;
-  }
-
-  if (isEnumType(type)) {
-    if (valueNode.kind !== Kind.ENUM || !type.getValue(valueNode.value)) {
-      return [`Expected type "${type.name}", found ${print(valueNode)}.`];
-    }
-
-    return [];
-  }
-
-  if (isScalarType(type)) {
-    // Scalars determine if a literal value is valid via parseLiteral().
-    try {
-      const parseResult = type.parseLiteral(valueNode, null);
-      if (isInvalid(parseResult)) {
-        return [`Expected type "${type.name}", found ${print(valueNode)}.`];
-      }
-    } catch (error) {
-      const printed = print(valueNode);
-      const message = error.message;
-      return [`Expected type "${type.name}", found ${printed}; ${message}`];
-    }
-
-    return [];
-  }
-
-  /* istanbul ignore next */
-  throw new Error(`Unknown type: ${(type: empty)}.`);
+): $ReadOnlyArray<GraphQLError> {
+  const emptySchema = new GraphQLSchema({});
+  const emptyDoc = { kind: DOCUMENT, definitions: [] };
+  const typeInfo = new TypeInfo(emptySchema, undefined, type);
+  const context = new ValidationContext(emptySchema, emptyDoc, typeInfo);
+  const visitor = ValuesOfCorrectType(context);
+  visit(valueNode, visitWithTypeInfo(typeInfo), visitor);
+  return context.getErrors();
 }