Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate literals in a single rule with finer precision #1144

Merged
merged 1 commit into from
Dec 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/execution/__tests__/variables-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }],
},
],
});
Expand Down Expand Up @@ -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'],
},
Expand Down
8 changes: 4 additions & 4 deletions src/execution/values.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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],
);
}
Expand Down
4 changes: 2 additions & 2 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,6 @@ export {
// All validation rules in the GraphQL Specification.
specifiedRules,
// Individual validation rules.
ArgumentsOfCorrectTypeRule,
DefaultValuesOfCorrectTypeRule,
FieldsOnCorrectTypeRule,
FragmentsOnCompositeTypesRule,
KnownArgumentNamesRule,
Expand All @@ -285,7 +283,9 @@ export {
UniqueInputFieldNamesRule,
UniqueOperationNamesRule,
UniqueVariableNamesRule,
ValuesOfCorrectTypeRule,
VariablesAreInputTypesRule,
VariablesDefaultValueAllowedRule,
VariablesInAllowedPositionRule,
} from './validation';

Expand Down
24 changes: 24 additions & 0 deletions src/jsutils/orList.js
Original file line number Diff line number Diff line change
@@ -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,
);
}
13 changes: 2 additions & 11 deletions src/jsutils/quotedOrList.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}"`));
}
26 changes: 18 additions & 8 deletions src/type/__tests__/enumType-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }],
},
],
Expand All @@ -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'],
},
],
});
Expand All @@ -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 }],
},
],
Expand All @@ -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 }],
},
],
Expand Down
27 changes: 23 additions & 4 deletions src/utilities/TypeInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Expand All @@ -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 {
Expand All @@ -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];
Expand Down Expand Up @@ -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:
Expand Down
30 changes: 30 additions & 0 deletions src/utilities/__tests__/isValidLiteralValue-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* 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.
*/

import { expect } from 'chai';
import { describe, it } from 'mocha';
import { isValidLiteralValue } from '../isValidLiteralValue';
import { parseValue } from '../../language';
import { GraphQLInt } from '../../type';

describe('isValidLiteralValue', () => {
it('Returns no errors for a valid value', () => {
expect(isValidLiteralValue(GraphQLInt, parseValue('123'))).to.deep.equal(
[],
);
});

it('Returns errors for an invalid value', () => {
expect(isValidLiteralValue(GraphQLInt, parseValue('"abc"'))).to.deep.equal([
{
message: 'Expected type Int, found "abc".',
locations: [{ line: 1, column: 1 }],
path: undefined,
},
]);
});
});
129 changes: 18 additions & 111 deletions src/utilities/isValidLiteralValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Loading