From a78e156c5d250d27182329c5b27117c1f40e929f Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Thu, 7 Dec 2017 00:22:37 -0800 Subject: [PATCH] Fix unhandled error when parsing custom scalar literals. 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. --- src/execution/__tests__/variables-test.js | 2 +- src/subscription/__tests__/subscribe-test.js | 2 +- src/type/definition.js | 24 ---------------- src/utilities/isValidJSValue.js | 28 +++++++++++------- src/utilities/isValidLiteralValue.js | 27 +++++++++++++----- src/utilities/valueFromAST.js | 30 ++++++++++++++------ src/validation/__tests__/harness.js | 20 +++++++++++++ src/validation/__tests__/validation-test.js | 20 +++++++++++++ 8 files changed, 101 insertions(+), 52 deletions(-) diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index c8cd497e82..01645fbbc1 100644 --- a/src/execution/__tests__/variables-test.js +++ b/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, }, diff --git a/src/subscription/__tests__/subscribe-test.js b/src/subscription/__tests__/subscribe-test.js index 548018cb19..87e14227dd 100644 --- a/src/subscription/__tests__/subscribe-test.js +++ b/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, diff --git a/src/type/definition.js b/src/type/definition.js index 17d9bc7d06..40eb5c43e8 100644 --- a/src/type/definition.js +++ b/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): boolean { - return !isInvalid(this.parseLiteral(valueNode, variables)); - } - // Parses an externally provided literal value to use as an input. parseLiteral(valueNode: ValueNode, variables: ?ObjMap): mixed { const parser = this._scalarConfig.parseLiteral; @@ -919,12 +909,6 @@ export class GraphQLEnumType /* */ { } } - 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 /* */ { } } - isValidLiteral(valueNode: ValueNode, _variables: ?ObjMap): 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): ?any /* T */ { // Note: variables will be resolved to a value before calling this function. if (valueNode.kind === Kind.ENUM) { diff --git a/src/utilities/isValidJSValue.js b/src/utilities/isValidJSValue.js index 708469b48a..04e03d20c1 100644 --- a/src/utilities/isValidJSValue.js +++ b/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 []; diff --git a/src/utilities/isValidLiteralValue.js b/src/utilities/isValidLiteralValue.js index 34df248ad2..0adfd970d1 100644 --- a/src/utilities/isValidLiteralValue.js +++ b/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 []; diff --git a/src/utilities/valueFromAST.js b/src/utilities/valueFromAST.js index 87c96d84ae..2f2fcb9c39 100644 --- a/src/utilities/valueFromAST.js +++ b/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 diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js index 00b6af831d..e6ebc0c879 100644 --- a/src/validation/__tests__/harness.js +++ b/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, + }, }), }); diff --git a/src/validation/__tests__/validation-test.js b/src/validation/__tests__/validation-test.js index 4b83be8a87..78690fe3d9 100644 --- a/src/validation/__tests__/validation-test.js +++ b/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.