Skip to content

Commit

Permalink
Preserve original coercion errors, improve error quality.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@leebyron
leebyron committed Dec 11, 2017
1 parent 2d08496 commit 65283d6
Show file tree
Hide file tree
Showing 12 changed files with 542 additions and 423 deletions.
72 changes: 33 additions & 39 deletions src/execution/__tests__/variables-test.js
View file
Expand Up @@ -11,7 +11,6 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';
import { execute } from '../execute';
import { coerceValue } from '../values';
import { parse } from '../../language';
import {
GraphQLSchema,
Expand Down Expand Up @@ -301,8 +300,8 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value ' +
'{"a":"foo","b":"bar","c":null}.' +
'\nIn field "c": Expected "String!", found null.',
'{"a":"foo","b":"bar","c":null}; ' +
'Expected non-nullable type String! at value.c.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -318,8 +317,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value "foo bar".' +
'\nExpected "TestInputObject", found not an object.',
'Variable "$input" got invalid value "foo bar"; ' +
'Expected object type TestInputObject.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -335,8 +334,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value {"a":"foo","b":"bar"}.' +
'\nIn field "c": Expected "String!", found null.',
'Variable "$input" got invalid value {"a":"foo","b":"bar"}; ' +
'Field value.c of required type String! was not provided.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -358,10 +357,15 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value {"na":{"a":"foo"}}.' +
'\nIn field "na": In field "c": Expected "String!", ' +
'found null.' +
'\nIn field "nb": Expected "String!", found null.',
'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
'Field value.na.c of required type String! was not provided.',
locations: [{ line: 2, column: 19 }],
path: undefined,
},
{
message:
'Variable "$input" got invalid value {"na":{"a":"foo"}}; ' +
'Field value.nb of required type String! was not provided.',
locations: [{ line: 2, column: 19 }],
path: undefined,
},
Expand All @@ -380,8 +384,8 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value ' +
'{"a":"foo","b":"bar","c":"baz","extra":"dog"}.' +
'\nIn field "extra": Unknown field.',
'{"a":"foo","b":"bar","c":"baz","extra":"dog"}; ' +
'Field "extra" is not defined by type TestInputObject.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -535,8 +539,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$value" got invalid value null.\n' +
'Expected "String!", found null.',
'Variable "$value" got invalid value null; ' +
'Expected non-nullable type String!.',
locations: [{ line: 2, column: 31 }],
path: undefined,
},
Expand Down Expand Up @@ -608,31 +612,21 @@ describe('Execute: Handles inputs', () => {
const ast = parse(doc);
const variables = { value: [1, 2, 3] };

expect(await execute(schema, ast, null, null, variables)).to.deep.equal({
const result = await execute(schema, ast, null, null, variables);

expect(result).to.deep.equal({
errors: [
{
message:
'Variable "$value" got invalid value [1,2,3].\nExpected type ' +
'"String", found [1,2,3]; String cannot represent an array value: [1,2,3]',
'Variable "$value" got invalid value [1,2,3]; Expected type ' +
'String; String cannot represent an array value: [1,2,3]',
locations: [{ line: 2, column: 31 }],
path: undefined,
},
],
});
});

it('coercing an array to GraphQLString throws TypeError', async () => {
let caughtError;
try {
coerceValue(GraphQLString, [1, 2, 3]);
} catch (error) {
caughtError = error;
}

expect(caughtError instanceof TypeError).to.equal(true);
expect(caughtError && caughtError.message).to.equal(
'String cannot represent an array value: [1,2,3]',
);
expect(result.errors[0].originalError).not.to.equal(undefined);
});

it('serializing an array via GraphQLString throws TypeError', async () => {
Expand Down Expand Up @@ -744,8 +738,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value null.\n' +
'Expected "[String]!", found null.',
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String]!.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -835,8 +829,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value ["A",null,"B"].' +
'\nIn element #1: Expected "String!", found null.',
'Variable "$input" got invalid value ["A",null,"B"]; ' +
'Expected non-nullable type String! at value[1].',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -857,8 +851,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value null.\n' +
'Expected "[String!]!", found null.',
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String!]!.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -897,8 +891,8 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value ["A",null,"B"].' +
'\nIn element #1: Expected "String!", found null.',
'Variable "$input" got invalid value ["A",null,"B"]; ' +
'Expected non-nullable type String! at value[1].',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down
85 changes: 55 additions & 30 deletions src/execution/execute.js
View file
Expand Up @@ -187,19 +187,19 @@ function executeImpl(

// If a valid context cannot be created due to incorrect arguments,
// a "Response" with only errors is returned.
let context;
try {
context = buildExecutionContext(
schema,
document,
rootValue,
contextValue,
variableValues,
operationName,
fieldResolver,
);
} catch (error) {
return { errors: [error] };
const context = buildExecutionContext(
schema,
document,
rootValue,
contextValue,
variableValues,
operationName,
fieldResolver,
);

// Return early errors if execution context failed.
if (Array.isArray(context)) {
return { errors: context };
}

// Return a Promise that will eventually resolve to the data described by
Expand Down Expand Up @@ -291,20 +291,18 @@ export function buildExecutionContext(
rawVariableValues: ?ObjMap<mixed>,
operationName: ?string,
fieldResolver: ?GraphQLFieldResolver<any, any>,
): ExecutionContext {
): $ReadOnlyArray<GraphQLError> | ExecutionContext {
const errors: Array<GraphQLError> = [];
let operation: ?OperationDefinitionNode;
let operation: OperationDefinitionNode | void;
let hasMultipleAssumedOperations = false;
const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
document.definitions.forEach(definition => {
for (let i = 0; i < document.definitions.length; i++) {
const definition = document.definitions[i];
switch (definition.kind) {
case Kind.OPERATION_DEFINITION:
if (!operationName && operation) {
throw new GraphQLError(
'Must provide operation name if query contains ' +
'multiple operations.',
);
}
if (
hasMultipleAssumedOperations = true;
} else if (
!operationName ||
(definition.name && definition.name.value === operationName)
) {
Expand All @@ -315,19 +313,46 @@ export function buildExecutionContext(
fragments[definition.name.value] = definition;
break;
}
});
}

if (!operation) {
if (operationName) {
throw new GraphQLError(`Unknown operation named "${operationName}".`);
errors.push(
new GraphQLError(`Unknown operation named "${operationName}".`),
);
} else {
throw new GraphQLError('Must provide an operation.');
errors.push(new GraphQLError('Must provide an operation.'));
}
} else if (hasMultipleAssumedOperations) {
errors.push(
new GraphQLError(
'Must provide operation name if query contains ' +
'multiple operations.',
),
);
}
const variableValues = getVariableValues(
schema,
operation.variableDefinitions || [],
rawVariableValues || {},
);

let variableValues;
if (operation) {
const coercedVariableValues = getVariableValues(
schema,
operation.variableDefinitions || [],
rawVariableValues || {},
);

if (coercedVariableValues.errors) {
errors.push(...coercedVariableValues.errors);
} else {
variableValues = coercedVariableValues.coerced;
}
}

if (errors.length !== 0) {
return errors;
}

invariant(operation, 'Has operation if no errors.');
invariant(variableValues, 'Has variables if no errors.');

return {
schema,
Expand Down

0 comments on commit 65283d6

Please sign in to comment.