Skip to content

Commit 14f260b

Browse files
IvanGoncharovIvanGoncharov
authored
Limits errors in getVariableValues() (#2062)
Based on #2037
1 parent 18371cb commit 14f260b

File tree

11 files changed

+503
-265
lines changed

11 files changed

+503
-265
lines changed

‎src/execution/__tests__/variables-test.js

+47-6
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import { expect } from 'chai';
44
import { describe, it } from 'mocha';
55

66
import inspect from '../../jsutils/inspect';
7+
import invariant from '../../jsutils/invariant';
78

9+
import { Kind } from '../../language/kinds';
810
import { parse } from '../../language/parser';
911

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

2123
import { execute } from '../execute';
24+
import { getVariableValues } from '../values';
2225

2326
const TestComplexScalar = new GraphQLScalarType({
2427
name: 'ComplexScalar',
@@ -369,7 +372,7 @@ describe('Execute: Handles inputs', () => {
369372
errors: [
370373
{
371374
message:
372-
'Variable "$input" got invalid value { a: "foo", b: "bar", c: null }; Expected non-nullable type String! not to be null at value.c.',
375+
'Variable "$input" got invalid value null at "input.c"; Expected non-nullable type String! not to be null.',
373376
locations: [{ line: 2, column: 16 }],
374377
},
375378
],
@@ -397,7 +400,7 @@ describe('Execute: Handles inputs', () => {
397400
errors: [
398401
{
399402
message:
400-
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field of required type String! was not provided at value.c.',
403+
'Variable "$input" got invalid value { a: "foo", b: "bar" }; Field c of required type String! was not provided.',
401404
locations: [{ line: 2, column: 16 }],
402405
},
403406
],
@@ -416,12 +419,12 @@ describe('Execute: Handles inputs', () => {
416419
errors: [
417420
{
418421
message:
419-
'Variable "$input" got invalid value { na: { a: "foo" } }; Field of required type String! was not provided at value.na.c.',
422+
'Variable "$input" got invalid value { a: "foo" } at "input.na"; Field c of required type String! was not provided.',
420423
locations: [{ line: 2, column: 18 }],
421424
},
422425
{
423426
message:
424-
'Variable "$input" got invalid value { na: { a: "foo" } }; Field of required type String! was not provided at value.nb.',
427+
'Variable "$input" got invalid value { na: { a: "foo" } }; Field nb of required type String! was not provided.',
425428
locations: [{ line: 2, column: 18 }],
426429
},
427430
],
@@ -830,7 +833,7 @@ describe('Execute: Handles inputs', () => {
830833
errors: [
831834
{
832835
message:
833-
'Variable "$input" got invalid value ["A", null, "B"]; Expected non-nullable type String! not to be null at value[1].',
836+
'Variable "$input" got invalid value null at "input[1]"; Expected non-nullable type String! not to be null.',
834837
locations: [{ line: 2, column: 16 }],
835838
},
836839
],
@@ -879,7 +882,7 @@ describe('Execute: Handles inputs', () => {
879882
errors: [
880883
{
881884
message:
882-
'Variable "$input" got invalid value ["A", null, "B"]; Expected non-nullable type String! not to be null at value[1].',
885+
'Variable "$input" got invalid value null at "input[1]"; Expected non-nullable type String! not to be null.',
883886
locations: [{ line: 2, column: 16 }],
884887
},
885888
],
@@ -986,4 +989,42 @@ describe('Execute: Handles inputs', () => {
986989
});
987990
});
988991
});
992+
993+
describe('getVariableValues: limit maximum number of coercion errors', () => {
994+
it('when values are invalid', () => {
995+
const doc = parse(`
996+
query ($input: [String!]) {
997+
listNN(input: $input)
998+
}
999+
`);
1000+
const operation = doc.definitions[0];
1001+
invariant(operation.kind === Kind.OPERATION_DEFINITION);
1002+
1003+
const result = getVariableValues(
1004+
schema,
1005+
operation.variableDefinitions || [],
1006+
{ input: [0, 1, 2] },
1007+
{ maxErrors: 2 },
1008+
);
1009+
1010+
expect(result).to.deep.equal({
1011+
errors: [
1012+
{
1013+
message:
1014+
'Variable "$input" got invalid value 0 at "input[0]"; Expected type String. String cannot represent a non string value: 0',
1015+
locations: [{ line: 2, column: 16 }],
1016+
},
1017+
{
1018+
message:
1019+
'Variable "$input" got invalid value 1 at "input[1]"; Expected type String. String cannot represent a non string value: 1',
1020+
locations: [{ line: 2, column: 16 }],
1021+
},
1022+
{
1023+
message:
1024+
'Too many errors processing variables, error limit reached. Execution aborted.',
1025+
},
1026+
],
1027+
});
1028+
});
1029+
});
9891030
});

‎src/execution/execute.js

+1
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,7 @@ export function buildExecutionContext(
319319
schema,
320320
operation.variableDefinitions || [],
321321
rawVariableValues || {},
322+
{ maxErrors: 50 },
322323
);
323324

324325
if (coercedVariableValues.errors) {

‎src/execution/values.js

+55-17
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import find from '../polyfills/find';
55
import keyMap from '../jsutils/keyMap';
66
import inspect from '../jsutils/inspect';
77
import { type ObjMap } from '../jsutils/ObjMap';
8+
import printPathArray from '../jsutils/printPathArray';
89

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

@@ -24,9 +25,9 @@ import {
2425
isNonNullType,
2526
} from '../type/definition';
2627

27-
import { coerceValue } from '../utilities/coerceValue';
2828
import { typeFromAST } from '../utilities/typeFromAST';
2929
import { valueFromAST } from '../utilities/valueFromAST';
30+
import { coerceInputValue } from '../utilities/coerceInputValue';
3031

3132
type CoercedVariableValues =
3233
| {| errors: $ReadOnlyArray<GraphQLError> |}
@@ -45,8 +46,36 @@ export function getVariableValues(
4546
schema: GraphQLSchema,
4647
varDefNodes: $ReadOnlyArray<VariableDefinitionNode>,
4748
inputs: { +[variable: string]: mixed, ... },
49+
options?: {| maxErrors?: number |},
4850
): CoercedVariableValues {
51+
const maxErrors = options && options.maxErrors;
4952
const errors = [];
53+
try {
54+
const coerced = coerceVariableValues(schema, varDefNodes, inputs, error => {
55+
if (maxErrors != null && errors.length >= maxErrors) {
56+
throw new GraphQLError(
57+
'Too many errors processing variables, error limit reached. Execution aborted.',
58+
);
59+
}
60+
errors.push(error);
61+
});
62+
63+
if (errors.length === 0) {
64+
return { coerced };
65+
}
66+
} catch (error) {
67+
errors.push(error);
68+
}
69+
70+
return { errors };
71+
}
72+
73+
function coerceVariableValues(
74+
schema: GraphQLSchema,
75+
varDefNodes: $ReadOnlyArray<VariableDefinitionNode>,
76+
inputs: { +[variable: string]: mixed, ... },
77+
onError: GraphQLError => void,
78+
): { [variable: string]: mixed, ... } {
5079
const coercedValues = {};
5180
for (const varDefNode of varDefNodes) {
5281
const varName = varDefNode.variable.name.value;
@@ -55,7 +84,7 @@ export function getVariableValues(
5584
// Must use input types for variables. This should be caught during
5685
// validation, however is checked again here for safety.
5786
const varTypeStr = print(varDefNode.type);
58-
errors.push(
87+
onError(
5988
new GraphQLError(
6089
`Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`,
6190
varDefNode.type,
@@ -71,7 +100,7 @@ export function getVariableValues(
71100

72101
if (isNonNullType(varType)) {
73102
const varTypeStr = inspect(varType);
74-
errors.push(
103+
onError(
75104
new GraphQLError(
76105
`Variable "$${varName}" of required type "${varTypeStr}" was not provided.`,
77106
varDefNode,
@@ -84,7 +113,7 @@ export function getVariableValues(
84113
const value = inputs[varName];
85114
if (value === null && isNonNullType(varType)) {
86115
const varTypeStr = inspect(varType);
87-
errors.push(
116+
onError(
88117
new GraphQLError(
89118
`Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.`,
90119
varDefNode,
@@ -93,21 +122,30 @@ export function getVariableValues(
93122
continue;
94123
}
95124

96-
const coerced = coerceValue(value, varType, varDefNode);
97-
if (coerced.errors) {
98-
for (const error of coerced.errors) {
99-
error.message =
100-
`Variable "$${varName}" got invalid value ${inspect(value)}; ` +
101-
error.message;
102-
}
103-
errors.push(...coerced.errors);
104-
continue;
105-
}
106-
107-
coercedValues[varName] = coerced.value;
125+
coercedValues[varName] = coerceInputValue(
126+
value,
127+
varType,
128+
(path, invalidValue, error) => {
129+
let prefix =
130+
`Variable "$${varName}" got invalid value ` + inspect(invalidValue);
131+
if (path.length > 0) {
132+
prefix += ` at "${varName}${printPathArray(path)}"`;
133+
}
134+
onError(
135+
new GraphQLError(
136+
prefix + '; ' + error.message,
137+
varDefNode,
138+
undefined,
139+
undefined,
140+
undefined,
141+
error.originalError,
142+
),
143+
);
144+
},
145+
);
108146
}
109147

110-
return errors.length === 0 ? { coerced: coercedValues } : { errors };
148+
return coercedValues;
111149
}
112150

113151
/**

‎src/index.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -386,8 +386,10 @@ export {
386386
// the GraphQL type system.
387387
TypeInfo,
388388
// Coerces a JavaScript value to a GraphQL type, or produces errors.
389+
coerceInputValue,
390+
// @deprecated use coerceInputValue - will be removed in v15
389391
coerceValue,
390-
// @deprecated use coerceValue - will be removed in v15
392+
// @deprecated use coerceInputValue - will be removed in v15
391393
isValidJSValue,
392394
// @deprecated use validation - will be removed in v15
393395
isValidLiteralValue,

‎src/jsutils/Path.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export function addPath(prev: $ReadOnly<Path> | void, key: string | number) {
1515
/**
1616
* Given a Path, return an Array of the path keys.
1717
*/
18-
export function pathToArray(path: $ReadOnly<Path>): Array<string | number> {
18+
export function pathToArray(path: ?$ReadOnly<Path>): Array<string | number> {
1919
const flattened = [];
2020
let curr = path;
2121
while (curr) {

‎src/jsutils/printPathArray.js

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// @flow strict
2+
3+
/**
4+
* Build a string describing the path.
5+
*/
6+
export default function printPathArray(
7+
path: $ReadOnlyArray<string | number>,
8+
): string {
9+
return path
10+
.map(key =>
11+
typeof key === 'number' ? '[' + key.toString() + ']' : '.' + key,
12+
)
13+
.join('');
14+
}

0 commit comments

Comments
 (0)