Skip to content

Commit 2536f14

Browse files
authored
Preserve original coercion errors, improve error quality. (#1133)
This is a fairly major refactoring of coerceValue which returns an Either so it can return a complete collection of errors. This allows originalError to be preserved for scalar coercion errors and ensures *all* errors are represented in the response. This had a minor change to the logic in execute / subscribe to allow for buildExecutionContext to abrupt complete with multiple errors.
1 parent 2d08496 commit 2536f14

File tree

12 files changed

+542
-423
lines changed

12 files changed

+542
-423
lines changed

src/execution/__tests__/variables-test.js

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import { expect } from 'chai';
1212
import { describe, it } from 'mocha';
1313
import { execute } from '../execute';
14-
import { coerceValue } from '../values';
1514
import { parse } from '../../language';
1615
import {
1716
GraphQLSchema,
@@ -301,8 +300,8 @@ describe('Execute: Handles inputs', () => {
301300
{
302301
message:
303302
'Variable "$input" got invalid value ' +
304-
'{"a":"foo","b":"bar","c":null}.' +
305-
'\nIn field "c": Expected "String!", found null.',
303+
'{"a":"foo","b":"bar","c":null}; ' +
304+
'Expected non-nullable type String! at value.c.',
306305
locations: [{ line: 2, column: 17 }],
307306
path: undefined,
308307
},
@@ -318,8 +317,8 @@ describe('Execute: Handles inputs', () => {
318317
errors: [
319318
{
320319
message:
321-
'Variable "$input" got invalid value "foo bar".' +
322-
'\nExpected "TestInputObject", found not an object.',
320+
'Variable "$input" got invalid value "foo bar"; ' +
321+
'Expected object type TestInputObject.',
323322
locations: [{ line: 2, column: 17 }],
324323
path: undefined,
325324
},
@@ -335,8 +334,8 @@ describe('Execute: Handles inputs', () => {
335334
errors: [
336335
{
337336
message:
338-
'Variable "$input" got invalid value {"a":"foo","b":"bar"}.' +
339-
'\nIn field "c": Expected "String!", found null.',
337+
'Variable "$input" got invalid value {"a":"foo","b":"bar"}; ' +
338+
'Field value.c of required type String! was not provided.',
340339
locations: [{ line: 2, column: 17 }],
341340
path: undefined,
342341
},
@@ -358,10 +357,15 @@ describe('Execute: Handles inputs', () => {
358357
errors: [
359358
{
360359
message:
361-
'Variable "$input" got invalid value {"na":{"a":"foo"}}.' +
362-
'\nIn field "na": In field "c": Expected "String!", ' +
363-
'found null.' +
364-
'\nIn field "nb": Expected "String!", found null.',
360+
'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
361+
'Field value.na.c of required type String! was not provided.',
362+
locations: [{ line: 2, column: 19 }],
363+
path: undefined,
364+
},
365+
{
366+
message:
367+
'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
368+
'Field value.nb of required type String! was not provided.',
365369
locations: [{ line: 2, column: 19 }],
366370
path: undefined,
367371
},
@@ -380,8 +384,8 @@ describe('Execute: Handles inputs', () => {
380384
{
381385
message:
382386
'Variable "$input" got invalid value ' +
383-
'{"a":"foo","b":"bar","c":"baz","extra":"dog"}.' +
384-
'\nIn field "extra": Unknown field.',
387+
'{"a":"foo","b":"bar","c":"baz","extra":"dog"}; ' +
388+
'Field "extra" is not defined by type TestInputObject.',
385389
locations: [{ line: 2, column: 17 }],
386390
path: undefined,
387391
},
@@ -535,8 +539,8 @@ describe('Execute: Handles inputs', () => {
535539
errors: [
536540
{
537541
message:
538-
'Variable "$value" got invalid value null.\n' +
539-
'Expected "String!", found null.',
542+
'Variable "$value" got invalid value null; ' +
543+
'Expected non-nullable type String!.',
540544
locations: [{ line: 2, column: 31 }],
541545
path: undefined,
542546
},
@@ -608,31 +612,21 @@ describe('Execute: Handles inputs', () => {
608612
const ast = parse(doc);
609613
const variables = { value: [1, 2, 3] };
610614

611-
expect(await execute(schema, ast, null, null, variables)).to.deep.equal({
615+
const result = await execute(schema, ast, null, null, variables);
616+
617+
expect(result).to.deep.equal({
612618
errors: [
613619
{
614620
message:
615-
'Variable "$value" got invalid value [1,2,3].\nExpected type ' +
616-
'"String", found [1,2,3]; String cannot represent an array value: [1,2,3]',
621+
'Variable "$value" got invalid value [1,2,3]; Expected type ' +
622+
'String; String cannot represent an array value: [1,2,3]',
617623
locations: [{ line: 2, column: 31 }],
618624
path: undefined,
619625
},
620626
],
621627
});
622-
});
623-
624-
it('coercing an array to GraphQLString throws TypeError', async () => {
625-
let caughtError;
626-
try {
627-
coerceValue(GraphQLString, [1, 2, 3]);
628-
} catch (error) {
629-
caughtError = error;
630-
}
631628

632-
expect(caughtError instanceof TypeError).to.equal(true);
633-
expect(caughtError && caughtError.message).to.equal(
634-
'String cannot represent an array value: [1,2,3]',
635-
);
629+
expect(result.errors[0].originalError).not.to.equal(undefined);
636630
});
637631

638632
it('serializing an array via GraphQLString throws TypeError', async () => {
@@ -744,8 +738,8 @@ describe('Execute: Handles inputs', () => {
744738
errors: [
745739
{
746740
message:
747-
'Variable "$input" got invalid value null.\n' +
748-
'Expected "[String]!", found null.',
741+
'Variable "$input" got invalid value null; ' +
742+
'Expected non-nullable type [String]!.',
749743
locations: [{ line: 2, column: 17 }],
750744
path: undefined,
751745
},
@@ -835,8 +829,8 @@ describe('Execute: Handles inputs', () => {
835829
errors: [
836830
{
837831
message:
838-
'Variable "$input" got invalid value ["A",null,"B"].' +
839-
'\nIn element #1: Expected "String!", found null.',
832+
'Variable "$input" got invalid value ["A",null,"B"]; ' +
833+
'Expected non-nullable type String! at value[1].',
840834
locations: [{ line: 2, column: 17 }],
841835
path: undefined,
842836
},
@@ -857,8 +851,8 @@ describe('Execute: Handles inputs', () => {
857851
errors: [
858852
{
859853
message:
860-
'Variable "$input" got invalid value null.\n' +
861-
'Expected "[String!]!", found null.',
854+
'Variable "$input" got invalid value null; ' +
855+
'Expected non-nullable type [String!]!.',
862856
locations: [{ line: 2, column: 17 }],
863857
path: undefined,
864858
},
@@ -897,8 +891,8 @@ describe('Execute: Handles inputs', () => {
897891
errors: [
898892
{
899893
message:
900-
'Variable "$input" got invalid value ["A",null,"B"].' +
901-
'\nIn element #1: Expected "String!", found null.',
894+
'Variable "$input" got invalid value ["A",null,"B"]; ' +
895+
'Expected non-nullable type String! at value[1].',
902896
locations: [{ line: 2, column: 17 }],
903897
path: undefined,
904898
},

src/execution/execute.js

Lines changed: 55 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -187,19 +187,19 @@ function executeImpl(
187187

188188
// If a valid context cannot be created due to incorrect arguments,
189189
// a "Response" with only errors is returned.
190-
let context;
191-
try {
192-
context = buildExecutionContext(
193-
schema,
194-
document,
195-
rootValue,
196-
contextValue,
197-
variableValues,
198-
operationName,
199-
fieldResolver,
200-
);
201-
} catch (error) {
202-
return { errors: [error] };
190+
const context = buildExecutionContext(
191+
schema,
192+
document,
193+
rootValue,
194+
contextValue,
195+
variableValues,
196+
operationName,
197+
fieldResolver,
198+
);
199+
200+
// Return early errors if execution context failed.
201+
if (Array.isArray(context)) {
202+
return { errors: context };
203203
}
204204

205205
// Return a Promise that will eventually resolve to the data described by
@@ -291,20 +291,18 @@ export function buildExecutionContext(
291291
rawVariableValues: ?ObjMap<mixed>,
292292
operationName: ?string,
293293
fieldResolver: ?GraphQLFieldResolver<any, any>,
294-
): ExecutionContext {
294+
): $ReadOnlyArray<GraphQLError> | ExecutionContext {
295295
const errors: Array<GraphQLError> = [];
296-
let operation: ?OperationDefinitionNode;
296+
let operation: OperationDefinitionNode | void;
297+
let hasMultipleAssumedOperations = false;
297298
const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
298-
document.definitions.forEach(definition => {
299+
for (let i = 0; i < document.definitions.length; i++) {
300+
const definition = document.definitions[i];
299301
switch (definition.kind) {
300302
case Kind.OPERATION_DEFINITION:
301303
if (!operationName && operation) {
302-
throw new GraphQLError(
303-
'Must provide operation name if query contains ' +
304-
'multiple operations.',
305-
);
306-
}
307-
if (
304+
hasMultipleAssumedOperations = true;
305+
} else if (
308306
!operationName ||
309307
(definition.name && definition.name.value === operationName)
310308
) {
@@ -315,19 +313,46 @@ export function buildExecutionContext(
315313
fragments[definition.name.value] = definition;
316314
break;
317315
}
318-
});
316+
}
317+
319318
if (!operation) {
320319
if (operationName) {
321-
throw new GraphQLError(`Unknown operation named "${operationName}".`);
320+
errors.push(
321+
new GraphQLError(`Unknown operation named "${operationName}".`),
322+
);
322323
} else {
323-
throw new GraphQLError('Must provide an operation.');
324+
errors.push(new GraphQLError('Must provide an operation.'));
324325
}
326+
} else if (hasMultipleAssumedOperations) {
327+
errors.push(
328+
new GraphQLError(
329+
'Must provide operation name if query contains ' +
330+
'multiple operations.',
331+
),
332+
);
325333
}
326-
const variableValues = getVariableValues(
327-
schema,
328-
operation.variableDefinitions || [],
329-
rawVariableValues || {},
330-
);
334+
335+
let variableValues;
336+
if (operation) {
337+
const coercedVariableValues = getVariableValues(
338+
schema,
339+
operation.variableDefinitions || [],
340+
rawVariableValues || {},
341+
);
342+
343+
if (coercedVariableValues.errors) {
344+
errors.push(...coercedVariableValues.errors);
345+
} else {
346+
variableValues = coercedVariableValues.coerced;
347+
}
348+
}
349+
350+
if (errors.length !== 0) {
351+
return errors;
352+
}
353+
354+
invariant(operation, 'Has operation if no errors.');
355+
invariant(variableValues, 'Has variables if no errors.');
331356

332357
return {
333358
schema,

0 commit comments

Comments
 (0)