Skip to content

Commit 6dce0c9

Browse files
committed
Enforces input coercion rules.
Before this diff, bad input to arguments and variables was often ignored and replaced with `null` rather than rejected. Now that `null` has a semantic meaning, and thanks to some recent changes to the spec (graphql/graphql-spec#221) - changes are necessary in order to enforce the stricter coercion rules. This diff does the following: * Implements the CoerceArgumentValues as described in the spec. * Implements the CoerceVariablesValues as described in the spec. * Alters valueFromAST and coerceValue (dual functions) to strictly enforce coercion, returning `undefined` implicitly when they fail to do so. It also fixes issues where undefined returns were being ignored as items in a list or fields in an input object.
1 parent 2b717f1 commit 6dce0c9

File tree

4 files changed

+200
-107
lines changed

4 files changed

+200
-107
lines changed

src/execution/execute.js

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,8 @@ function shouldIncludeNode(
458458
);
459459
if (skipAST) {
460460
const { if: skipIf } = getArgumentValues(
461-
GraphQLSkipDirective.args,
462-
skipAST.arguments,
461+
GraphQLSkipDirective,
462+
skipAST,
463463
exeContext.variableValues
464464
);
465465
if (skipIf === true) {
@@ -473,8 +473,8 @@ function shouldIncludeNode(
473473
);
474474
if (includeAST) {
475475
const { if: includeIf } = getArgumentValues(
476-
GraphQLIncludeDirective.args,
477-
includeAST.arguments,
476+
GraphQLIncludeDirective,
477+
includeAST,
478478
exeContext.variableValues
479479
);
480480
if (includeIf === false) {
@@ -563,8 +563,8 @@ function resolveField(
563563
// variables scope to fulfill any variable references.
564564
// TODO: find a way to memoize, in case this field is within a List type.
565565
const args = getArgumentValues(
566-
fieldDef.args,
567-
fieldAST.arguments,
566+
fieldDef,
567+
fieldAST,
568568
exeContext.variableValues
569569
);
570570

src/execution/values.js

Lines changed: 150 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* of patent rights can be found in the PATENTS file in the same directory.
99
*/
1010

11-
import { forEach, isCollection } from 'iterall';
11+
import { createIterator, isCollection } from 'iterall';
1212

1313
import { GraphQLError } from '../error';
1414
import invariant from '../jsutils/invariant';
@@ -18,6 +18,8 @@ import keyMap from '../jsutils/keyMap';
1818
import { typeFromAST } from '../utilities/typeFromAST';
1919
import { valueFromAST } from '../utilities/valueFromAST';
2020
import { isValidJSValue } from '../utilities/isValidJSValue';
21+
import { isValidLiteralValue } from '../utilities/isValidLiteralValue';
22+
import * as Kind from '../language/kinds';
2123
import { print } from '../language/printer';
2224
import {
2325
isInputType,
@@ -27,9 +29,18 @@ import {
2729
GraphQLList,
2830
GraphQLNonNull,
2931
} from '../type/definition';
30-
import type { GraphQLInputType, GraphQLArgument } from '../type/definition';
32+
import type {
33+
GraphQLInputType,
34+
GraphQLFieldDefinition
35+
} from '../type/definition';
36+
import type { GraphQLDirective } from '../type/directives';
3137
import type { GraphQLSchema } from '../type/schema';
32-
import type { Argument, VariableDefinition } from '../language/ast';
38+
import type {
39+
Field,
40+
Directive,
41+
Variable,
42+
VariableDefinition,
43+
} from '../language/ast';
3344

3445

3546
/**
@@ -42,84 +53,113 @@ export function getVariableValues(
4253
definitionASTs: Array<VariableDefinition>,
4354
inputs: { [key: string]: mixed }
4455
): { [key: string]: mixed } {
45-
return definitionASTs.reduce((values, defAST) => {
46-
const varName = defAST.variable.name.value;
47-
values[varName] = getVariableValue(schema, defAST, inputs[varName]);
48-
return values;
49-
}, {});
50-
}
56+
const coercedValues = Object.create(null);
57+
for (let i = 0; i < definitionASTs.length; i++) {
58+
const definitionAST = definitionASTs[i];
59+
const varName = definitionAST.variable.name.value;
60+
let varType = typeFromAST(schema, definitionAST.type);
61+
if (!isInputType(varType)) {
62+
throw new GraphQLError(
63+
`Variable "$${varName}" expected value of type ` +
64+
`"${String(varType)}" which cannot be used as an input type.`,
65+
[ definitionAST.type ]
66+
);
67+
}
68+
varType = ((varType: any): GraphQLInputType);
5169

70+
const value = inputs[varName];
71+
if (isInvalid(value)) {
72+
const defaultValue = definitionAST.defaultValue;
73+
if (!isInvalid(defaultValue)) {
74+
coercedValues[varName] = valueFromAST(defaultValue, varType);
75+
}
76+
if (varType instanceof GraphQLNonNull) {
77+
throw new GraphQLError(
78+
`Variable "$${varName}" of required type ` +
79+
`"${String(varType)}" was not provided.`,
80+
[ definitionAST ]
81+
);
82+
}
83+
} else {
84+
const coercedValue = coerceValue(varType, value);
85+
if (isInvalid(coercedValue)) {
86+
const errors = isValidJSValue(value, varType);
87+
const message = errors ? '\n' + errors.join('\n') : '';
88+
throw new GraphQLError(
89+
`Variable "${varName}" got invalid value ` +
90+
`${JSON.stringify(value)}.${message}`,
91+
[ definitionAST ]
92+
);
93+
}
94+
coercedValues[varName] = coercedValue;
95+
}
96+
}
97+
return coercedValues;
98+
}
5299

53100
/**
54101
* Prepares an object map of argument values given a list of argument
55102
* definitions and list of argument AST nodes.
56103
*/
57104
export function getArgumentValues(
58-
argDefs: ?Array<GraphQLArgument>,
59-
argASTs: ?Array<Argument>,
105+
def: GraphQLFieldDefinition | GraphQLDirective,
106+
node: Field | Directive,
60107
variableValues?: ?{ [key: string]: mixed }
61108
): { [key: string]: mixed } {
109+
const argDefs = def.args;
110+
const argASTs = node.arguments;
62111
if (!argDefs || !argASTs) {
63112
return {};
64113
}
114+
const coercedValues = Object.create(null);
65115
const argASTMap = keyMap(argASTs, arg => arg.name.value);
66-
return argDefs.reduce((result, argDef) => {
116+
for (let i = 0; i < argDefs.length; i++) {
117+
const argDef = argDefs[i];
67118
const name = argDef.name;
68-
const valueAST = argASTMap[name] ? argASTMap[name].value : null;
69-
let value = valueFromAST(valueAST, argDef.type, variableValues);
70-
if (isInvalid(value)) {
71-
value = argDef.defaultValue;
72-
}
73-
if (!isInvalid(value)) {
74-
result[name] = value;
75-
}
76-
return result;
77-
}, {});
78-
}
79-
80-
81-
/**
82-
* Given a variable definition, and any value of input, return a value which
83-
* adheres to the variable definition, or throw an error.
84-
*/
85-
function getVariableValue(
86-
schema: GraphQLSchema,
87-
definitionAST: VariableDefinition,
88-
input: mixed
89-
): mixed {
90-
const type = typeFromAST(schema, definitionAST.type);
91-
const variable = definitionAST.variable;
92-
if (!type || !isInputType(type)) {
93-
throw new GraphQLError(
94-
`Variable "$${variable.name.value}" expected value of type ` +
95-
`"${print(definitionAST.type)}" which cannot be used as an input type.`,
96-
[ definitionAST ]
97-
);
98-
}
99-
const inputType = ((type: any): GraphQLInputType);
100-
const errors = isValidJSValue(input, inputType);
101-
if (!errors.length) {
102-
if (isInvalid(input)) {
103-
const defaultValue = definitionAST.defaultValue;
104-
if (defaultValue) {
105-
return valueFromAST(defaultValue, inputType);
119+
const argType = argDef.type;
120+
const argumentAST = argASTMap[name];
121+
const defaultValue = argDef.defaultValue;
122+
if (!argumentAST) {
123+
if (!isInvalid(defaultValue)) {
124+
coercedValues[name] = defaultValue;
125+
} else if (argType instanceof GraphQLNonNull) {
126+
throw new GraphQLError(
127+
`Argument "${name}" of required type ` +
128+
`"${String(argType)}" was not provided.`,
129+
[ node ]
130+
);
131+
}
132+
} else if (argumentAST.value.kind === Kind.VARIABLE) {
133+
const variableName = (argumentAST.value: Variable).name.value;
134+
if (variableValues && !isInvalid(variableValues[variableName])) {
135+
// Note: this does not check that this variable value is correct.
136+
// This assumes that this query has been validated and the variable
137+
// usage here is of the correct type.
138+
coercedValues[name] = variableValues[variableName];
139+
} else if (!isInvalid(defaultValue)) {
140+
coercedValues[name] = defaultValue;
141+
} else if (argType instanceof GraphQLNonNull) {
142+
throw new GraphQLError(
143+
`Argument "${name}" of required type "${String(argType)}" was ` +
144+
`provided the variable "$${variableName}" without a runtime value.`,
145+
[ argumentAST.value ]
146+
);
106147
}
148+
} else {
149+
const valueAST = argumentAST.value;
150+
const coercedValue = valueFromAST(valueAST, argType, variableValues);
151+
if (isInvalid(coercedValue)) {
152+
const errors = isValidLiteralValue(argType, valueAST);
153+
const message = errors ? '\n' + errors.join('\n') : '';
154+
throw new GraphQLError(
155+
`Argument "${name}" got invalid value ${print(valueAST)}.${message}`,
156+
[ def ]
157+
);
158+
}
159+
coercedValues[name] = coercedValue;
107160
}
108-
return coerceValue(inputType, input);
109-
}
110-
if (isNullish(input)) {
111-
throw new GraphQLError(
112-
`Variable "$${variable.name.value}" of required type ` +
113-
`"${print(definitionAST.type)}" was not provided.`,
114-
[ definitionAST ]
115-
);
116161
}
117-
const message = errors ? '\n' + errors.join('\n') : '';
118-
throw new GraphQLError(
119-
`Variable "$${variable.name.value}" got invalid value ` +
120-
`${JSON.stringify(input)}.${message}`,
121-
[ definitionAST ]
122-
);
162+
return coercedValues;
123163
}
124164

125165
/**
@@ -135,42 +175,66 @@ function coerceValue(type: GraphQLInputType, value: mixed): mixed {
135175
return coerceValue(type.ofType, _value);
136176
}
137177

138-
if (_value === null) {
139-
return null;
178+
if (isInvalid(_value)) {
179+
// Intentionally teturn no value rather than the value null.
180+
return;
140181
}
141182

142-
if (isInvalid(_value)) {
143-
return undefined;
183+
if (_value === null) {
184+
// Intentionally return the value null.
185+
return null;
144186
}
145187

146188
if (type instanceof GraphQLList) {
147189
const itemType = type.ofType;
148190
if (isCollection(_value)) {
149191
const coercedValues = [];
150-
forEach((_value: any), item => {
151-
coercedValues.push(coerceValue(itemType, item));
152-
});
192+
const valueIter = createIterator(_value);
193+
if (!valueIter) {
194+
return; // Intentionally return no value.
195+
}
196+
let step;
197+
while (!(step = valueIter.next()).done) {
198+
const itemValue = coerceValue(itemType, step.value);
199+
if (isInvalid(itemValue)) {
200+
return; // Intentionally return no value.
201+
}
202+
coercedValues.push(itemValue);
203+
}
153204
return coercedValues;
154205
}
206+
const coercedValue = coerceValue(itemType, _value);
207+
if (isInvalid(coercedValue)) {
208+
return; // Intentionally return no value.
209+
}
155210
return [ coerceValue(itemType, _value) ];
156211
}
157212

158213
if (type instanceof GraphQLInputObjectType) {
159214
if (typeof _value !== 'object' || _value === null) {
160-
return null;
215+
return; // Intentionally return no value.
161216
}
217+
const coercedObj = Object.create(null);
162218
const fields = type.getFields();
163-
return Object.keys(fields).reduce((obj, fieldName) => {
219+
const fieldNames = Object.keys(fields);
220+
for (let i = 0; i < fieldNames.length; i++) {
221+
const fieldName = fieldNames[i];
164222
const field = fields[fieldName];
165-
let fieldValue = coerceValue(field.type, _value[fieldName]);
166-
if (isInvalid(fieldValue)) {
167-
fieldValue = field.defaultValue;
223+
if (isInvalid(_value[fieldName])) {
224+
if (!isInvalid(field.defaultValue)) {
225+
coercedObj[fieldName] = field.defaultValue;
226+
} else if (field.type instanceof GraphQLNonNull) {
227+
return; // Intentionally return no value.
228+
}
229+
continue;
168230
}
169-
if (!isInvalid(fieldValue)) {
170-
obj[fieldName] = fieldValue;
231+
const fieldValue = coerceValue(field.type, _value[fieldName]);
232+
if (isInvalid(fieldValue)) {
233+
return; // Intentionally return no value.
171234
}
172-
return obj;
173-
}, {});
235+
coercedObj[fieldName] = fieldValue;
236+
}
237+
return coercedObj;
174238
}
175239

176240
invariant(
@@ -179,7 +243,11 @@ function coerceValue(type: GraphQLInputType, value: mixed): mixed {
179243
);
180244

181245
const parsed = type.parseValue(_value);
182-
if (!isNullish(parsed)) {
183-
return parsed;
246+
if (isNullish(parsed)) {
247+
// null or invalid values represent a failure to parse correctly,
248+
// in which case no value is returned.
249+
return;
184250
}
251+
252+
return parsed;
185253
}

src/utilities/buildASTSchema.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,8 +469,8 @@ function getDeprecationReason(directives: ?Array<Directive>): ?string {
469469
return;
470470
}
471471
const { reason } = getArgumentValues(
472-
GraphQLDeprecatedDirective.args,
473-
deprecatedAST.arguments
472+
GraphQLDeprecatedDirective,
473+
deprecatedAST
474474
);
475475
return (reason: any);
476476
}

0 commit comments

Comments
 (0)