Skip to content

Commit

Permalink
Limits errors in getVariableValues() (#2062)
Browse files Browse the repository at this point in the history 
Based on #2037
  • Loading branch information
@IvanGoncharov
IvanGoncharov committed Aug 7, 2019
1 parent 18371cb commit 14f260b
Show file tree
Hide file tree
Showing 11 changed files with 503 additions and 265 deletions.
53 changes: 47 additions & 6 deletions src/execution/__tests__/variables-test.js
View file
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
View file
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
View file
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
View file
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
View file
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
View file
@@ -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('');
}

0 comments on commit 14f260b

Please sign in to comment.