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

Limits errors in getVariableValues() #2062

Merged
merged 1 commit into from Aug 7, 2019
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 47 additions & 6 deletions src/execution/__tests__/variables-test.js
Expand Up @@ -4,7 +4,9 @@ import { expect } from 'chai';
import { describe, it } from 'mocha';

import inspect from '../../jsutils/inspect';
import invariant from '../../jsutils/invariant';

import { Kind } from '../../language/kinds';
import { parse } from '../../language/parser';

import { GraphQLSchema } from '../../type/schema';
Expand All @@ -19,6 +21,7 @@ import {
} from '../../type/definition';

import { execute } from '../execute';
import { getVariableValues } from '../values';

const TestComplexScalar = new GraphQLScalarType({
name: 'ComplexScalar',
Expand Down Expand Up @@ -369,7 +372,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value { a: "foo", b: "bar", c: null }; Expected non-nullable type String! not to be null at value.c.',
'Variable "$input" got invalid value null at "input.c"; Expected non-nullable type String! not to be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -397,7 +400,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field of required type String! was not provided at value.c.',
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field c of required type String! was not provided.',
locations: [{ line: 2, column: 16 }],
},
],
Expand All @@ -416,12 +419,12 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value { na: { a: "foo" } }; Field of required type String! was not provided at value.na.c.',
'Variable "$input" got invalid value { a: "foo" } at "input.na"; Field c of required type String! was not provided.',
locations: [{ line: 2, column: 18 }],
},
{
message:
'Variable "$input" got invalid value { na: { a: "foo" } }; Field of required type String! was not provided at value.nb.',
'Variable "$input" got invalid value { na: { a: "foo" } }; Field nb of required type String! was not provided.',
locations: [{ line: 2, column: 18 }],
},
],
Expand Down Expand Up @@ -830,7 +833,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value ["A", null, "B"]; Expected non-nullable type String! not to be null at value[1].',
'Variable "$input" got invalid value null at "input[1]"; Expected non-nullable type String! not to be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -879,7 +882,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value ["A", null, "B"]; Expected non-nullable type String! not to be null at value[1].',
'Variable "$input" got invalid value null at "input[1]"; Expected non-nullable type String! not to be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -986,4 +989,42 @@ describe('Execute: Handles inputs', () => {
});
});
});

describe('getVariableValues: limit maximum number of coercion errors', () => {
it('when values are invalid', () => {
const doc = parse(`
query ($input: [String!]) {
listNN(input: $input)
}
`);
const operation = doc.definitions[0];
invariant(operation.kind === Kind.OPERATION_DEFINITION);

const result = getVariableValues(
schema,
operation.variableDefinitions || [],
{ input: [0, 1, 2] },
{ maxErrors: 2 },
);

expect(result).to.deep.equal({
errors: [
{
message:
'Variable "$input" got invalid value 0 at "input[0]"; Expected type String. String cannot represent a non string value: 0',
locations: [{ line: 2, column: 16 }],
},
{
message:
'Variable "$input" got invalid value 1 at "input[1]"; Expected type String. String cannot represent a non string value: 1',
locations: [{ line: 2, column: 16 }],
},
{
message:
'Too many errors processing variables, error limit reached. Execution aborted.',
},
],
});
});
});
});
1 change: 1 addition & 0 deletions src/execution/execute.js
Expand Up @@ -319,6 +319,7 @@ export function buildExecutionContext(
schema,
operation.variableDefinitions || [],
rawVariableValues || {},
{ maxErrors: 50 },
);

if (coercedVariableValues.errors) {
Expand Down
72 changes: 55 additions & 17 deletions src/execution/values.js
Expand Up @@ -5,6 +5,7 @@ import find from '../polyfills/find';
import keyMap from '../jsutils/keyMap';
import inspect from '../jsutils/inspect';
import { type ObjMap } from '../jsutils/ObjMap';
import printPathArray from '../jsutils/printPathArray';

import { GraphQLError } from '../error/GraphQLError';

Expand All @@ -24,9 +25,9 @@ import {
isNonNullType,
} from '../type/definition';

import { coerceValue } from '../utilities/coerceValue';
import { typeFromAST } from '../utilities/typeFromAST';
import { valueFromAST } from '../utilities/valueFromAST';
import { coerceInputValue } from '../utilities/coerceInputValue';

type CoercedVariableValues =
| {| errors: $ReadOnlyArray<GraphQLError> |}
Expand All @@ -45,8 +46,36 @@ export function getVariableValues(
schema: GraphQLSchema,
varDefNodes: $ReadOnlyArray<VariableDefinitionNode>,
inputs: { +[variable: string]: mixed, ... },
options?: {| maxErrors?: number |},
): CoercedVariableValues {
const maxErrors = options && options.maxErrors;
const errors = [];
try {
const coerced = coerceVariableValues(schema, varDefNodes, inputs, error => {
if (maxErrors != null && errors.length >= maxErrors) {
throw new GraphQLError(
'Too many errors processing variables, error limit reached. Execution aborted.',
);
}
errors.push(error);
});

if (errors.length === 0) {
return { coerced };
}
} catch (error) {
errors.push(error);
}

return { errors };
}

function coerceVariableValues(
schema: GraphQLSchema,
varDefNodes: $ReadOnlyArray<VariableDefinitionNode>,
inputs: { +[variable: string]: mixed, ... },
onError: GraphQLError => void,
): { [variable: string]: mixed, ... } {
const coercedValues = {};
for (const varDefNode of varDefNodes) {
const varName = varDefNode.variable.name.value;
Expand All @@ -55,7 +84,7 @@ export function getVariableValues(
// Must use input types for variables. This should be caught during
// validation, however is checked again here for safety.
const varTypeStr = print(varDefNode.type);
errors.push(
onError(
new GraphQLError(
`Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`,
varDefNode.type,
Expand All @@ -71,7 +100,7 @@ export function getVariableValues(

if (isNonNullType(varType)) {
const varTypeStr = inspect(varType);
errors.push(
onError(
new GraphQLError(
`Variable "$${varName}" of required type "${varTypeStr}" was not provided.`,
varDefNode,
Expand All @@ -84,7 +113,7 @@ export function getVariableValues(
const value = inputs[varName];
if (value === null && isNonNullType(varType)) {
const varTypeStr = inspect(varType);
errors.push(
onError(
new GraphQLError(
`Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.`,
varDefNode,
Expand All @@ -93,21 +122,30 @@ export function getVariableValues(
continue;
}

const coerced = coerceValue(value, varType, varDefNode);
if (coerced.errors) {
for (const error of coerced.errors) {
error.message =
`Variable "$${varName}" got invalid value ${inspect(value)}; ` +
error.message;
}
errors.push(...coerced.errors);
continue;
}

coercedValues[varName] = coerced.value;
coercedValues[varName] = coerceInputValue(
value,
varType,
(path, invalidValue, error) => {
let prefix =
`Variable "$${varName}" got invalid value ` + inspect(invalidValue);
if (path.length > 0) {
prefix += ` at "${varName}${printPathArray(path)}"`;
}
onError(
new GraphQLError(
prefix + '; ' + error.message,
varDefNode,
undefined,
undefined,
undefined,
error.originalError,
),
);
},
);
}

return errors.length === 0 ? { coerced: coercedValues } : { errors };
return coercedValues;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/index.js
Expand Up @@ -386,8 +386,10 @@ export {
// the GraphQL type system.
TypeInfo,
// Coerces a JavaScript value to a GraphQL type, or produces errors.
coerceInputValue,
// @deprecated use coerceInputValue - will be removed in v15
coerceValue,
// @deprecated use coerceValue - will be removed in v15
// @deprecated use coerceInputValue - will be removed in v15
isValidJSValue,
// @deprecated use validation - will be removed in v15
isValidLiteralValue,
Expand Down
2 changes: 1 addition & 1 deletion src/jsutils/Path.js
Expand Up @@ -15,7 +15,7 @@ export function addPath(prev: $ReadOnly<Path> | void, key: string | number) {
/**
* Given a Path, return an Array of the path keys.
*/
export function pathToArray(path: $ReadOnly<Path>): Array<string | number> {
export function pathToArray(path: ?$ReadOnly<Path>): Array<string | number> {
const flattened = [];
let curr = path;
while (curr) {
Expand Down
14 changes: 14 additions & 0 deletions src/jsutils/printPathArray.js
@@ -0,0 +1,14 @@
// @flow strict

/**
* Build a string describing the path.
*/
export default function printPathArray(
path: $ReadOnlyArray<string | number>,
): string {
return path
.map(key =>
typeof key === 'number' ? '[' + key.toString() + ']' : '.' + key,
)
.join('');
}