Skip to content

Commit

Permalink
Fix unhandled error when parsing custom scalar literals.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
leebyron committed Dec 7, 2017
1 parent 007407d commit fb55817
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 48 deletions.
2 changes: 1 addition & 1 deletion src/execution/__tests__/variables-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down
2 changes: 1 addition & 1 deletion src/subscription/__tests__/subscribe-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
24 changes: 0 additions & 24 deletions src/type/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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];
Expand All @@ -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) {
Expand Down
28 changes: 17 additions & 11 deletions src/utilities/isValidJSValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 [];
Expand Down
27 changes: 20 additions & 7 deletions src/utilities/isValidLiteralValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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 [];
Expand Down
12 changes: 8 additions & 4 deletions src/utilities/valueFromAST.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,16 @@ export function valueFromAST(
);

// Scalar and Enum values implement methods which perform this translation.
if (type.isValidLiteral(valueNode, variables)) {
return type.parseLiteral(valueNode, variables);
}

// Invalid values represent a failure to parse correctly, in which case
// no value is returned.
try {
const result = type.parseLiteral(valueNode, variables);
if (!isInvalid(result)) {
return result;
}
} catch (_error) {
// Do nothing, no value is returned.
}
}

// Returns true if the provided valueNode is a variable which is not defined
Expand Down
20 changes: 20 additions & 0 deletions src/validation/__tests__/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
GraphQLIncludeDirective,
GraphQLSkipDirective,
} from '../../type/directives';
import { GraphQLScalarType } from '../../type/definition';

const Being = new GraphQLInterfaceType({
name: 'Being',
Expand Down Expand Up @@ -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: () => ({
Expand All @@ -284,6 +298,12 @@ const QueryRoot = new GraphQLObjectType({
dogOrHuman: { type: DogOrHuman },
humanOrAlien: { type: HumanOrAlien },
complicatedArgs: { type: ComplicatedArgs },
invalidArg: {
args: {
arg: { type: InvalidScalar },
},
type: GraphQLString,
},
}),
});

Expand Down
20 changes: 20 additions & 0 deletions src/validation/__tests__/validation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit fb55817

Please sign in to comment.