Skip to content

Add suggestions for invalid values #1153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/execution/__tests__/variables-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ describe('Execute: Handles inputs', () => {
message:
'Variable "$input" got invalid value ' +
'{"a":"foo","b":"bar","c":null}; ' +
'Expected non-nullable type String! at value.c.',
'Expected non-nullable type String! not to be null at value.c.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -313,7 +313,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value "foo bar"; ' +
'Expected object type TestInputObject.',
'Expected type TestInputObject to be an object.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -535,7 +535,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$value" got invalid value null; ' +
'Expected non-nullable type String!.',
'Expected non-nullable type String! not to be null.',
locations: [{ line: 2, column: 31 }],
path: undefined,
},
Expand Down Expand Up @@ -734,7 +734,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String]!.',
'Expected non-nullable type [String]! not to be null.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -825,7 +825,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value ["A",null,"B"]; ' +
'Expected non-nullable type String! at value[1].',
'Expected non-nullable type String! not to be null at value[1].',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand All @@ -847,7 +847,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String!]!.',
'Expected non-nullable type [String!]! not to be null.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -887,7 +887,7 @@ describe('Execute: Handles inputs', () => {
{
message:
'Variable "$input" got invalid value ["A",null,"B"]; ' +
'Expected non-nullable type String! at value[1].',
'Expected non-nullable type String! not to be null at value[1].',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down
17 changes: 16 additions & 1 deletion src/jsutils/suggestionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,34 @@ export default function suggestionList(
* insertion, deletion, or substitution of a single character, or a swap of two
* adjacent characters.
*
* Includes a custom alteration from Damerau-Levenshtein to treat case changes
* as a single edit which helps identify mis-cased values with an edit distance
* of 1.
*
* This distance can be useful for detecting typos in input or sorting
*
* @param {string} a
* @param {string} b
* @return {int} distance in number of edits
*/
function lexicalDistance(a, b) {
function lexicalDistance(aStr, bStr) {
if (aStr === bStr) {
return 0;
}

let i;
let j;
const d = [];
const a = aStr.toLowerCase();
const b = bStr.toLowerCase();
const aLength = a.length;
const bLength = b.length;

// Any case change counts as a single edit
if (a === b) {
return 1;
}

for (i = 0; i <= aLength; i++) {
d[i] = [i];
}
Expand Down
18 changes: 16 additions & 2 deletions src/type/__tests__/enumType-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('Type System: Enum Values', () => {
errors: [
{
message:
'Expected type Color, found "GREEN"; Did you mean the enum value: GREEN?',
'Expected type Color, found "GREEN"; Did you mean the enum value GREEN?',
locations: [{ line: 1, column: 23 }],
},
],
Expand All @@ -175,7 +175,21 @@ describe('Type System: Enum Values', () => {
errors: [
{
message:
'Expected type Color, found GREENISH; Did you mean the enum value: GREEN?',
'Expected type Color, found GREENISH; Did you mean the enum value GREEN?',
locations: [{ line: 1, column: 23 }],
},
],
});
});

it('does not accept values with incorrect casing', async () => {
expect(
await graphql(schema, '{ colorEnum(fromEnum: green) }'),
).to.jsonEqual({
errors: [
{
message:
'Expected type Color, found green; Did you mean the enum value GREEN?',
locations: [{ line: 1, column: 23 }],
},
],
Expand Down
110 changes: 109 additions & 1 deletion src/utilities/__tests__/coerceValue-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,24 @@
import { describe, it } from 'mocha';
import { expect } from 'chai';
import { coerceValue } from '../coerceValue';
import { GraphQLInt, GraphQLFloat, GraphQLString } from '../../type';
import {
GraphQLInt,
GraphQLFloat,
GraphQLString,
GraphQLEnumType,
GraphQLInputObjectType,
GraphQLNonNull,
} from '../../type';

function expectNoErrors(result) {
expect(result.errors).to.equal(undefined);
expect(result.value).not.to.equal(undefined);
}

function expectError(result, expected) {
const messages = result.errors && result.errors.map(error => error.message);
expect(messages).to.deep.equal([expected]);
expect(result.value).to.equal(undefined);
}

describe('coerceValue', () => {
Expand Down Expand Up @@ -130,4 +139,103 @@ describe('coerceValue', () => {
);
});
});

describe('for GraphQLEnum', () => {
const TestEnum = new GraphQLEnumType({
name: 'TestEnum',
values: {
FOO: { value: 'InternalFoo' },
BAR: { value: 123456789 },
},
});

it('returns no error for a known enum name', () => {
const fooResult = coerceValue('FOO', TestEnum);
expectNoErrors(fooResult);
expect(fooResult.value).to.equal('InternalFoo');

const barResult = coerceValue('BAR', TestEnum);
expectNoErrors(barResult);
expect(barResult.value).to.equal(123456789);
});

it('results error for misspelled enum value', () => {
const result = coerceValue('foo', TestEnum);
expectError(result, 'Expected type TestEnum; did you mean FOO?');
});

it('results error for incorrect value type', () => {
const result1 = coerceValue(123, TestEnum);
expectError(result1, 'Expected type TestEnum.');

const result2 = coerceValue({ field: 'value' }, TestEnum);
expectError(result2, 'Expected type TestEnum.');
});
});

describe('for GraphQLInputObject', () => {
const TestInputObject = new GraphQLInputObjectType({
name: 'TestInputObject',
fields: {
foo: { type: GraphQLNonNull(GraphQLInt) },
bar: { type: GraphQLInt },
},
});

it('returns no error for a valid input', () => {
const result = coerceValue({ foo: 123 }, TestInputObject);
expectNoErrors(result);
expect(result.value).to.deep.equal({ foo: 123 });
});

it('returns no error for a non-object type', () => {
const result = coerceValue(123, TestInputObject);
expectError(result, 'Expected type TestInputObject to be an object.');
});

it('returns no error for an invalid field', () => {
const result = coerceValue({ foo: 'abc' }, TestInputObject);
expectError(
result,
'Expected type Int at value.foo; Int cannot represent non 32-bit signed integer value: abc',
);
});

it('returns multiple errors for multiple invalid fields', () => {
const result = coerceValue({ foo: 'abc', bar: 'def' }, TestInputObject);
expect(
result.errors && result.errors.map(error => error.message),
).to.deep.equal([
'Expected type Int at value.foo; Int cannot represent non 32-bit signed integer value: abc',
'Expected type Int at value.bar; Int cannot represent non 32-bit signed integer value: def',
]);
});

it('returns error for a missing required field', () => {
const result = coerceValue({ bar: 123 }, TestInputObject);
expectError(
result,
'Field value.foo of required type Int! was not provided.',
);
});

it('returns error for an unknown field', () => {
const result = coerceValue(
{ foo: 123, unknownField: 123 },
TestInputObject,
);
expectError(
result,
'Field "unknownField" is not defined by type TestInputObject.',
);
});

it('returns error for a misspelled field', () => {
const result = coerceValue({ foo: 123, bart: 123 }, TestInputObject);
expectError(
result,
'Field "bart" is not defined by type TestInputObject; did you mean bar?',
);
});
});
});
40 changes: 32 additions & 8 deletions src/utilities/coerceValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import { forEach, isCollection } from 'iterall';
import isInvalid from '../jsutils/isInvalid';
import isNullish from '../jsutils/isNullish';
import orList from '../jsutils/orList';
import suggestionList from '../jsutils/suggestionList';
import { GraphQLError } from '../error';
import type { ASTNode } from '../language/ast';
import {
Expand Down Expand Up @@ -46,7 +48,7 @@ export function coerceValue(
if (isNullish(value)) {
return ofErrors([
coercionError(
`Expected non-nullable type ${String(type)}`,
`Expected non-nullable type ${String(type)} not to be null`,
blameNode,
path,
),
Expand Down Expand Up @@ -74,7 +76,13 @@ export function coerceValue(
return ofValue(parseResult);
} catch (error) {
return ofErrors([
coercionError(`Expected type ${type.name}`, blameNode, path, error),
coercionError(
`Expected type ${type.name}`,
blameNode,
path,
error.message,
error,
),
]);
}
}
Expand All @@ -86,8 +94,16 @@ export function coerceValue(
return ofValue(enumValue.value);
}
}
const suggestions = suggestionList(
String(value),
type.getValues().map(enumValue => enumValue.name),
);
const didYouMean =
suggestions.length !== 0
? `did you mean ${orList(suggestions)}?`
: undefined;
return ofErrors([
coercionError(`Expected type ${type.name}`, blameNode, path),
coercionError(`Expected type ${type.name}`, blameNode, path, didYouMean),
]);
}

Expand Down Expand Up @@ -119,7 +135,11 @@ export function coerceValue(
if (isInputObjectType(type)) {
if (typeof value !== 'object') {
return ofErrors([
coercionError(`Expected object type ${type.name}`, blameNode, path),
coercionError(
`Expected type ${type.name} to be an object`,
blameNode,
path,
),
]);
}
let errors;
Expand Down Expand Up @@ -164,12 +184,18 @@ export function coerceValue(
for (const fieldName in value) {
if (hasOwnProperty.call(value, fieldName)) {
if (!fields[fieldName]) {
const suggestions = suggestionList(fieldName, Object.keys(fields));
const didYouMean =
suggestions.length !== 0
? `did you mean ${orList(suggestions)}?`
: undefined;
errors = add(
errors,
coercionError(
`Field "${fieldName}" is not defined by type ${type.name}`,
blameNode,
path,
didYouMean,
),
);
}
Expand Down Expand Up @@ -199,15 +225,13 @@ function atPath(prev, key) {
return { prev, key };
}

function coercionError(message, blameNode, path, originalError) {
function coercionError(message, blameNode, path, subMessage, originalError) {
const pathStr = printPath(path);
// Return a GraphQLError instance
return new GraphQLError(
message +
(pathStr ? ' at ' + pathStr : '') +
(originalError && originalError.message
? '; ' + originalError.message
: '.'),
(subMessage ? '; ' + subMessage : '.'),
blameNode,
undefined,
undefined,
Expand Down
Loading