Skip to content

Commit 2ada5c6

Browse files
freiksenetfreiksenet
committed
Make input objects and lists return better errors
Add index of invalid element or field of invalid field and make it recurse into children until faulty place is found.
1 parent a941554 commit 2ada5c6

File tree

8 files changed

+160
-50
lines changed

8 files changed

+160
-50
lines changed

‎src/execution/__tests__/variables.js

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,14 @@ var TestInputObject = new GraphQLInputObjectType({
5656
}
5757
});
5858

59+
var TestNestedInputObject = new GraphQLInputObjectType({
60+
name: 'TestNestedInputObject',
61+
fields: {
62+
na: { type: new GraphQLNonNull(TestInputObject) },
63+
nb: { type: new GraphQLNonNull(GraphQLString) },
64+
},
65+
});
66+
5967
var TestType = new GraphQLObjectType({
6068
name: 'TestType',
6169
fields: {
@@ -79,6 +87,15 @@ var TestType = new GraphQLObjectType({
7987
args: { input: { type: GraphQLString, defaultValue: 'Hello World' } },
8088
resolve: (_, { input }) => input && JSON.stringify(input)
8189
},
90+
fieldWithNestedInputObject: {
91+
type: GraphQLString,
92+
args: {
93+
input: {
94+
type: TestNestedInputObject, defaultValue: 'Hello World'
95+
}
96+
},
97+
resolve: (_, { input }) => input && JSON.stringify(input)
98+
},
8299
list: {
83100
type: GraphQLString,
84101
args: { input: { type: new GraphQLList(GraphQLString) } },
@@ -232,7 +249,8 @@ describe('Execute: Handles inputs', () => {
232249
locations: [ { line: 2, column: 17 } ],
233250
message:
234251
'Variable "$input" expected value of type "TestInputObject" but ' +
235-
'got: {"a":"foo","b":"bar","c":null}.'
252+
'got: {"a":"foo","b":"bar","c":null}. ' +
253+
'In field c: Expected non-null value.'
236254
});
237255
});
238256

@@ -250,7 +268,7 @@ describe('Execute: Handles inputs', () => {
250268
locations: [ { line: 2, column: 17 } ],
251269
message:
252270
'Variable "$input" expected value of type "TestInputObject" but ' +
253-
'got: "foo bar".'
271+
'got: "foo bar". Not an object.'
254272
});
255273
});
256274

@@ -268,10 +286,37 @@ describe('Execute: Handles inputs', () => {
268286
locations: [ { line: 2, column: 17 } ],
269287
message:
270288
'Variable "$input" expected value of type "TestInputObject" but ' +
271-
'got: {"a":"foo","b":"bar"}.'
289+
'got: {"a":"foo","b":"bar"}. In field c: Expected non-null value.'
272290
});
273291
});
274292

293+
it('errors on deep nested errors and with many errors', async () => {
294+
var nestedDoc = `
295+
query q($input: TestNestedInputObject) {
296+
fieldWithNestedObjectInput(input: $input)
297+
}
298+
`;
299+
var nestedAst = parse(nestedDoc);
300+
var params = { input: { na: { a: 'foo' } } };
301+
302+
var caughtError;
303+
try {
304+
execute(schema, nestedAst, null, params);
305+
} catch (error) {
306+
caughtError = error;
307+
}
308+
309+
expect(caughtError).to.containSubset({
310+
locations: [ { line: 2, column: 19 } ],
311+
message:
312+
'Variable "$input" expected value of type "TestNestedInputObject" but ' +
313+
'got: {"na":{"a":"foo"}}.' +
314+
' In field na: In field c: Expected non-null value.' +
315+
' In field nb: Expected non-null value.'
316+
});
317+
318+
});
319+
275320
it('errors on addition of unknown input field', async () => {
276321
var params = { input: { a: 'foo', b: 'bar', c: 'baz', d: 'dog' } };
277322

@@ -643,7 +688,7 @@ describe('Execute: Handles inputs', () => {
643688
locations: [ { line: 2, column: 17 } ],
644689
message:
645690
'Variable "$input" expected value of type "[String!]" but got: ' +
646-
'["A",null,"B"].'
691+
'["A",null,"B"]. In element #1: Expected non-null value.'
647692
});
648693
});
649694

@@ -706,7 +751,7 @@ describe('Execute: Handles inputs', () => {
706751
locations: [ { line: 2, column: 17 } ],
707752
message:
708753
'Variable "$input" expected value of type "[String!]!" but got: ' +
709-
'["A",null,"B"].'
754+
'["A",null,"B"]. In element #1: Expected non-null value.'
710755
});
711756
});
712757

‎src/execution/values.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ function getVariableValue(
9494
);
9595
}
9696
var inputType: GraphQLInputType = (type: any);
97-
if (isValidJSValue(input, inputType)) {
97+
var errors = isValidJSValue(input, inputType);
98+
if (!errors.length) {
9899
if (isNullish(input)) {
99100
var defaultValue = definitionAST.defaultValue;
100101
if (defaultValue) {
@@ -110,9 +111,11 @@ function getVariableValue(
110111
[ definitionAST ]
111112
);
112113
}
114+
var message = errors ? '\n' + errors.join('\n') : '';
113115
throw new GraphQLError(
114116
`Variable "$${variable.name.value}" expected value of type ` +
115-
`"${print(definitionAST.type)}" but got: ${JSON.stringify(input)}.`,
117+
`"${print(definitionAST.type)}" but got: ${JSON.stringify(input)}.` +
118+
`${message}`,
116119
[ definitionAST ]
117120
);
118121
}

‎src/utilities/isValidJSValue.js

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,45 +25,58 @@ import type { GraphQLInputType } from '../type/definition';
2525
* accepted for that type. This is primarily useful for validating the
2626
* runtime values of query variables.
2727
*/
28-
export function isValidJSValue(value: any, type: GraphQLInputType): boolean {
28+
export function isValidJSValue(value: any, type: GraphQLInputType): [ string ] {
2929
// A value must be provided if the type is non-null.
3030
if (type instanceof GraphQLNonNull) {
3131
if (isNullish(value)) {
32-
return false;
32+
return [ 'Expected non-null value.' ];
3333
}
3434
var nullableType: GraphQLInputType = (type.ofType: any);
3535
return isValidJSValue(value, nullableType);
3636
}
3737

3838
if (isNullish(value)) {
39-
return true;
39+
return [];
4040
}
4141

4242
// Lists accept a non-list value as a list of one.
4343
if (type instanceof GraphQLList) {
4444
var itemType: GraphQLInputType = (type.ofType: any);
4545
if (Array.isArray(value)) {
46-
return value.every(item => isValidJSValue(item, itemType));
46+
return value.reduce((acc, item, index) => {
47+
var errors = isValidJSValue(item, itemType);
48+
return acc.concat(errors.map(error =>
49+
`In element #${index}: ${error}`
50+
));
51+
}, []);
4752
}
4853
return isValidJSValue(value, itemType);
4954
}
5055

5156
// Input objects check each defined field.
5257
if (type instanceof GraphQLInputObjectType) {
5358
if (typeof value !== 'object') {
54-
return false;
59+
return [ 'Not an object.' ];
5560
}
5661
var fields = type.getFields();
5762

63+
var errors = [];
64+
5865
// Ensure every provided field is defined.
59-
if (Object.keys(value).some(fieldName => !fields[fieldName])) {
60-
return false;
66+
for (var providedField of Object.keys(value)) {
67+
if (!fields[providedField]) {
68+
errors.push('Unknown field "${providedField}".');
69+
}
6170
}
6271

6372
// Ensure every defined field is valid.
64-
return Object.keys(fields).every(
65-
fieldName => isValidJSValue(value[fieldName], fields[fieldName].type)
66-
);
73+
for (var fieldName of Object.keys(fields)) {
74+
var newErrors = isValidJSValue(value[fieldName], fields[fieldName].type);
75+
errors.push(...(newErrors.map(error =>
76+
`In field "${fieldName}": ${error}`
77+
)));
78+
}
79+
return errors;
6780
}
6881

6982
invariant(
@@ -73,5 +86,10 @@ export function isValidJSValue(value: any, type: GraphQLInputType): boolean {
7386

7487
// Scalar/Enum input checks to ensure the type can parse the value to
7588
// a non-null value.
76-
return !isNullish(type.parseValue(value));
89+
var parseResult = type.parseValue(value);
90+
if (isNullish(parseResult)) {
91+
return [ `Expected type "${type.name}", found "{JSON.strigify(value)}".` ];
92+
}
93+
94+
return [];
7795
}

‎src/utilities/isValidLiteralValue.js

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

11+
import { print } from '../language/printer';
1112
import type { Value, ListValue, ObjectValue } from '../language/ast';
1213
import {
1314
VARIABLE,
@@ -37,56 +38,70 @@ import isNullish from '../jsutils/isNullish';
3738
export function isValidLiteralValue(
3839
type: GraphQLInputType,
3940
valueAST: Value
40-
): boolean {
41+
): [ string ] {
4142
// A value must be provided if the type is non-null.
4243
if (type instanceof GraphQLNonNull) {
4344
if (!valueAST) {
44-
return false;
45+
return [ 'Expected non-null value.' ];
4546
}
4647
var ofType: GraphQLInputType = (type.ofType: any);
4748
return isValidLiteralValue(ofType, valueAST);
4849
}
4950

5051
if (!valueAST) {
51-
return true;
52+
return [];
5253
}
5354

5455
// This function only tests literals, and assumes variables will provide
5556
// values of the correct type.
5657
if (valueAST.kind === VARIABLE) {
57-
return true;
58+
return [];
5859
}
5960

6061
// Lists accept a non-list value as a list of one.
6162
if (type instanceof GraphQLList) {
6263
var itemType: GraphQLInputType = (type.ofType: any);
6364
if (valueAST.kind === LIST) {
64-
return (valueAST: ListValue).values.every(
65-
itemAST => isValidLiteralValue(itemType, itemAST)
66-
);
65+
return (valueAST: ListValue).values.reduce((acc, itemAST, index) => {
66+
var errors = isValidLiteralValue(itemType, itemAST);
67+
return acc.concat(errors.map(error =>
68+
`In element #${index}: ${error}`
69+
));
70+
}, []);
6771
}
6872
return isValidLiteralValue(itemType, valueAST);
6973
}
7074

7175
// Input objects check each defined field and look for undefined fields.
7276
if (type instanceof GraphQLInputObjectType) {
7377
if (valueAST.kind !== OBJECT) {
74-
return false;
78+
return [ 'Not an object.' ];
7579
}
7680
var fields = type.getFields();
7781

82+
var errors = [];
83+
7884
// Ensure every provided field is defined.
7985
var fieldASTs = (valueAST: ObjectValue).fields;
80-
if (fieldASTs.some(fieldAST => !fields[fieldAST.name.value])) {
81-
return false;
86+
for (var providedFieldAST of fieldASTs) {
87+
if (!fields[providedFieldAST.name.value]) {
88+
errors.push(`Unknown field "${providedFieldAST.name.value}".`);
89+
}
8290
}
8391

8492
// Ensure every defined field is valid.
8593
var fieldASTMap = keyMap(fieldASTs, fieldAST => fieldAST.name.value);
86-
return Object.keys(fields).every(fieldName => isValidLiteralValue(
87-
fields[fieldName].type,
88-
fieldASTMap[fieldName] && fieldASTMap[fieldName].value
89-
));
94+
for (var fieldName of Object.keys(fields)) {
95+
var result = isValidLiteralValue(
96+
fields[fieldName].type,
97+
fieldASTMap[fieldName] && fieldASTMap[fieldName].value
98+
);
99+
errors.push(...(result.map(error =>
100+
`In field "${fieldName}": ${error}`
101+
)));
102+
}
103+
104+
return errors;
90105
}
91106

92107
invariant(
@@ -96,5 +111,10 @@ export function isValidLiteralValue(
96111

97112
// Scalar/Enum input checks to ensure the type can parse the value to
98113
// a non-null value.
99-
return !isNullish(type.parseLiteral(valueAST));
114+
var parseResult = type.parseLiteral(valueAST);
115+
if (isNullish(parseResult)) {
116+
return [ `Expected type "${type.name}", found ${print(valueAST)}.` ];
117+
}
118+
119+
return [];
100120
}

‎src/validation/__tests__/ArgumentsOfCorrectType.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ import {
1515
} from '../rules/ArgumentsOfCorrectType';
1616

1717

18-
function badValue(argName, typeName, value, line, column) {
18+
function badValue(argName, typeName, value, line, column, errors) {
1919
return {
20-
message: badValueMessage(argName, typeName, value),
20+
message: badValueMessage(argName, typeName, value, errors),
2121
locations: [ { line, column } ],
2222
};
2323
}
@@ -725,7 +725,9 @@ describe('Validate: Argument values of correct type', () => {
725725
}
726726
}
727727
`, [
728-
badValue('complexArg', 'ComplexInput', '{intField: 4}', 4, 41),
728+
badValue('complexArg', 'ComplexInput', '{intField: 4}', 4, 41, [
729+
'In field requiredField: Expected non-null value.'
730+
]),
729731
]);
730732
});
731733

@@ -766,7 +768,8 @@ describe('Validate: Argument values of correct type', () => {
766768
'ComplexInput',
767769
'{requiredField: true, unknownField: "value"}',
768770
4,
769-
41
771+
41,
772+
[ 'Unknown field unknownField.' ]
770773
),
771774
]);
772775
});

‎src/validation/__tests__/DefaultValuesOfCorrectType.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ function defaultForNonNullArg(varName, typeName, guessTypeName, line, column) {
2323
};
2424
}
2525

26-
function badValue(varName, typeName, val, line, column) {
26+
function badValue(varName, typeName, val, line, column, errors) {
2727
return {
28-
message: badValueForDefaultArgMessage(varName, typeName, val),
28+
message: badValueForDefaultArgMessage(varName, typeName, val, errors),
2929
locations: [ { line, column } ],
3030
};
3131
}
@@ -83,7 +83,9 @@ describe('Validate: Variable default values of correct type', () => {
8383
`, [
8484
badValue('a', 'Int', '"one"', 3, 19),
8585
badValue('b', 'String', '4', 4, 22),
86-
badValue('c', 'ComplexInput', '"notverycomplex"', 5, 28)
86+
badValue('c', 'ComplexInput', '"notverycomplex"', 5, 28, [
87+
'Not an object.'
88+
])
8789
]);
8890
});
8991

@@ -93,7 +95,9 @@ describe('Validate: Variable default values of correct type', () => {
9395
dog { name }
9496
}
9597
`, [
96-
badValue('a', 'ComplexInput', '{intField: 3}', 2, 53)
98+
badValue('a', 'ComplexInput', '{intField: 3}', 2, 53, [
99+
'In field requiredField: Expected non-null value.'
100+
])
97101
]);
98102
});
99103

0 commit comments

Comments
 (0)