Fix unhandled error when parsing custom scalar literals. (#1126)

Fixes #910

This factors out the enum value validation from scalar value validation and ensures the same try/catch is used in isValidLiteralValue as isValidJSValue and protecting from errors in valueFromAST.

Author: leebyron

src/execution/__tests__/variables-test.js

@@ -613,7 +613,7 @@ describe('Execute: Handles inputs', () => {
           {
             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]',
+              '"String", found [1,2,3]; String cannot represent an array value: [1,2,3]',
             locations: [{ line: 2, column: 31 }],
             path: undefined,
           },

src/subscription/__tests__/subscribe-test.js

@@ -475,7 +475,7 @@ describe('Subscription Initialization Phase', () => {
         {
           message:
             'Variable "$priority" got invalid value "meow".\nExpected ' +
-            'type "Int", found "meow": Int cannot represent non 32-bit signed ' +
+            'type "Int", found "meow"; Int cannot represent non 32-bit signed ' +
             'integer value: meow',
           locations: [{ line: 2, column: 21 }],
           path: undefined,

src/type/definition.js

@@ -332,11 +332,6 @@ export class GraphQLScalarType {
     return serializer(value);
   }
 
-  // Determines if an internal value is valid for this type.
-  isValidValue(value: mixed): boolean {
-    return !isInvalid(this.parseValue(value));
-  }
-
   // Parses an externally provided value to use as an input.
   parseValue(value: mixed): mixed {
     const parser = this._scalarConfig.parseValue;
@@ -346,11 +341,6 @@ export class GraphQLScalarType {
     return parser ? parser(value) : value;
   }
 
-  // Determines if an internal value is valid for this type.
-  isValidLiteral(valueNode: ValueNode, variables: ?ObjMap<mixed>): boolean {
-    return !isInvalid(this.parseLiteral(valueNode, variables));
-  }
-
   // Parses an externally provided literal value to use as an input.
   parseLiteral(valueNode: ValueNode, variables: ?ObjMap<mixed>): mixed {
     const parser = this._scalarConfig.parseLiteral;
@@ -919,12 +909,6 @@ export class GraphQLEnumType /* <T> */ {
     }
   }
 
-  isValidValue(value: mixed): boolean {
-    return (
-      typeof value === 'string' && this._getNameLookup()[value] !== undefined
-    );
-  }
-
   parseValue(value: mixed): ?any /* T */ {
     if (typeof value === 'string') {
       const enumValue = this._getNameLookup()[value];
@@ -934,14 +918,6 @@ export class GraphQLEnumType /* <T> */ {
     }
   }
 
-  isValidLiteral(valueNode: ValueNode, _variables: ?ObjMap<mixed>): boolean {
-    // Note: variables will be resolved to a value before calling this function.
-    return (
-      valueNode.kind === Kind.ENUM &&
-      this._getNameLookup()[valueNode.value] !== undefined
-    );
-  }
-
   parseLiteral(valueNode: ValueNode, _variables: ?ObjMap<mixed>): ?any /* T */ {
     // Note: variables will be resolved to a value before calling this function.
     if (valueNode.kind === Kind.ENUM) {

src/utilities/isValidJSValue.js

@@ -10,6 +10,7 @@
 import { forEach, isCollection } from 'iterall';
 
 import invariant from '../jsutils/invariant';
+import isInvalid from '../jsutils/isInvalid';
 import isNullish from '../jsutils/isNullish';
 import {
   GraphQLScalarType,
@@ -89,23 +90,28 @@ export function isValidJSValue(
     return errors;
   }
 
-  invariant(
-    type instanceof GraphQLScalarType || type instanceof GraphQLEnumType,
-    'Must be input type',
-  );
+  // Enum types only accept certain values
+  if (type instanceof GraphQLEnumType) {
+    if (typeof value !== 'string' || !type.getValue(value)) {
+      const printed = JSON.stringify(value);
+      return [`Expected type "${type.name}", found ${printed}.`];
+    }
+
+    return [];
+  }
+
+  invariant(type instanceof GraphQLScalarType, 'Must be scalar type');
 
-  // Scalar/Enum input checks to ensure the type can parse the value to
-  // a non-null value.
+  // Scalars determine if a value is valid via parseValue().
   try {
     const parseResult = type.parseValue(value);
-    if (isNullish(parseResult) && !type.isValidValue(value)) {
+    if (isInvalid(parseResult)) {
       return [`Expected type "${type.name}", found ${JSON.stringify(value)}.`];
     }
   } catch (error) {
-    return [
-      `Expected type "${type.name}", found ${JSON.stringify(value)}: ` +
-        error.message,
-    ];
+    const printed = JSON.stringify(value);
+    const message = error.message;
+    return [`Expected type "${type.name}", found ${printed}; ${message}`];
   }
 
   return [];

src/utilities/isValidLiteralValue.js

@@ -23,6 +23,7 @@ import {
 } from '../type/definition';
 import type { GraphQLInputType } from '../type/definition';
 import invariant from '../jsutils/invariant';
+import isInvalid from '../jsutils/isInvalid';
 import keyMap from '../jsutils/keyMap';
 
 /**
@@ -100,14 +101,26 @@ export function isValidLiteralValue(
     return errors;
   }
 
-  invariant(
-    type instanceof GraphQLScalarType || type instanceof GraphQLEnumType,
-    'Must be input type',
-  );
+  if (type instanceof GraphQLEnumType) {
+    if (valueNode.kind !== Kind.ENUM || !type.getValue(valueNode.value)) {
+      return [`Expected type "${type.name}", found ${print(valueNode)}.`];
+    }
+
+    return [];
+  }
+
+  invariant(type instanceof GraphQLScalarType, 'Must be a scalar type');
 
-  // Scalars determine if a literal values is valid.
-  if (!type.isValidLiteral(valueNode, null)) {
-    return [`Expected type "${type.name}", found ${print(valueNode)}.`];
+  // 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 [];

src/utilities/valueFromAST.js

@@ -144,18 +144,32 @@ export function valueFromAST(
     return coercedObj;
   }
 
-  invariant(
-    type instanceof GraphQLScalarType || type instanceof GraphQLEnumType,
-    'Must be input type',
-  );
-
-  // Scalar and Enum values implement methods which perform this translation.
-  if (type.isValidLiteral(valueNode, variables)) {
-    return type.parseLiteral(valueNode, variables);
+  if (type instanceof GraphQLEnumType) {
+    if (valueNode.kind !== Kind.ENUM) {
+      return; // Invalid: intentionally return no value.
+    }
+    const enumValue = type.getValue(valueNode.value);
+    if (!enumValue) {
+      return; // Invalid: intentionally return no value.
+    }
+    return enumValue.value;
   }
 
+  invariant(type instanceof GraphQLScalarType, 'Must be scalar type');
+
+  // Scalars fulfill parsing a literal value via parseLiteral().
   // Invalid values represent a failure to parse correctly, in which case
   // no value is returned.
+  let result;
+  try {
+    result = type.parseLiteral(valueNode, variables);
+  } catch (_error) {
+    return; // Invalid: intentionally return no value.
+  }
+  if (isInvalid(result)) {
+    return; // Invalid: intentionally return no value.
+  }
+  return result;
 }
 
 // Returns true if the provided valueNode is a variable which is not defined

src/validation/__tests__/harness.js

@@ -29,6 +29,7 @@ import {
   GraphQLIncludeDirective,
   GraphQLSkipDirective,
 } from '../../type/directives';
+import { GraphQLScalarType } from '../../type/definition';
 
 const Being = new GraphQLInterfaceType({
   name: 'Being',
@@ -269,6 +270,19 @@ const ComplicatedArgs = new GraphQLObjectType({
   }),
 });
 
+const InvalidScalar = new GraphQLScalarType({
+  name: 'Invalid',
+  serialize(value) {
+    return value;
+  },
+  parseLiteral(node) {
+    throw new Error('Invalid scalar is always invalid: ' + node.value);
+  },
+  parseValue(node) {
+    throw new Error('Invalid scalar is always invalid: ' + node);
+  },
+});
+
 const QueryRoot = new GraphQLObjectType({
   name: 'QueryRoot',
   fields: () => ({
@@ -284,6 +298,12 @@ const QueryRoot = new GraphQLObjectType({
     dogOrHuman: { type: DogOrHuman },
     humanOrAlien: { type: HumanOrAlien },
     complicatedArgs: { type: ComplicatedArgs },
+    invalidArg: {
+      args: {
+        arg: { type: InvalidScalar },
+      },
+      type: GraphQLString,
+    },
   }),
 });
 

src/validation/__tests__/validation-test.js

@@ -36,6 +36,26 @@ describe('Validate: Supports full validation', () => {
     );
   });
 
+  it('detects bad scalar parse', () => {
+    const doc = `
+      query {
+        invalidArg(arg: "bad value")
+      }
+    `;
+
+    const errors = validate(testSchema, parse(doc));
+    expect(errors).to.deep.equal([
+      {
+        locations: [{ line: 3, column: 25 }],
+        message:
+          'Argument "arg" has invalid value "bad value".\n' +
+          'Expected type "Invalid", found "bad value"; ' +
+          'Invalid scalar is always invalid: bad value',
+        path: undefined,
+      },
+    ]);
+  });
+
   // NOTE: experimental
   it('validates using a custom TypeInfo', () => {
     // This TypeInfo will never return a valid field.