From 8dbec4e86083b07c96dcbf232633d4390c39fa06 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 15 Dec 2017 13:05:39 -0800 Subject: [PATCH] Add suggestions for invalid values For misspelled enums or field names, these suggestions can be helpful. This also changes the suggestions algorithm to better detect case-sensitivity mistakes, which are common --- src/execution/__tests__/variables-test.js | 14 +-- src/jsutils/suggestionList.js | 17 ++- src/type/__tests__/enumType-test.js | 18 ++- src/utilities/__tests__/coerceValue-test.js | 110 +++++++++++++++++- src/utilities/coerceValue.js | 40 +++++-- .../__tests__/ValuesOfCorrectType-test.js | 26 ++++- src/validation/rules/ValuesOfCorrectType.js | 47 +++++--- 7 files changed, 232 insertions(+), 40 deletions(-) diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index 162990e74f..8d7c8c12ed 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -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, }, @@ -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, }, @@ -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, }, @@ -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, }, @@ -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, }, @@ -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, }, @@ -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, }, diff --git a/src/jsutils/suggestionList.js b/src/jsutils/suggestionList.js index 5072db9017..3d03591eca 100644 --- a/src/jsutils/suggestionList.js +++ b/src/jsutils/suggestionList.js @@ -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]; } diff --git a/src/type/__tests__/enumType-test.js b/src/type/__tests__/enumType-test.js index e1ba722c5e..e97d0ff2aa 100644 --- a/src/type/__tests__/enumType-test.js +++ b/src/type/__tests__/enumType-test.js @@ -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 }], }, ], @@ -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 }], }, ], diff --git a/src/utilities/__tests__/coerceValue-test.js b/src/utilities/__tests__/coerceValue-test.js index 451c2dad76..3bad8ab0c2 100644 --- a/src/utilities/__tests__/coerceValue-test.js +++ b/src/utilities/__tests__/coerceValue-test.js @@ -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', () => { @@ -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?', + ); + }); + }); }); diff --git a/src/utilities/coerceValue.js b/src/utilities/coerceValue.js index 4467fb0cab..c5ba791044 100644 --- a/src/utilities/coerceValue.js +++ b/src/utilities/coerceValue.js @@ -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 { @@ -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, ), @@ -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, + ), ]); } } @@ -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), ]); } @@ -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; @@ -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, ), ); } @@ -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, diff --git a/src/validation/__tests__/ValuesOfCorrectType-test.js b/src/validation/__tests__/ValuesOfCorrectType-test.js index 21f1b4f7c2..ba2ed1b04d 100644 --- a/src/validation/__tests__/ValuesOfCorrectType-test.js +++ b/src/validation/__tests__/ValuesOfCorrectType-test.js @@ -31,9 +31,9 @@ function requiredField(typeName, fieldName, fieldTypeName, line, column) { }; } -function unknownField(typeName, fieldName, line, column) { +function unknownField(typeName, fieldName, line, column, message) { return { - message: unknownFieldMessage(typeName, fieldName), + message: unknownFieldMessage(typeName, fieldName, message), locations: [{ line, column }], path: undefined, }; @@ -543,7 +543,7 @@ describe('Validate: Values of correct type', () => { '"SIT"', 4, 41, - 'Did you mean the enum value: SIT?', + 'Did you mean the enum value SIT?', ), ], ); @@ -587,7 +587,15 @@ describe('Validate: Values of correct type', () => { } } `, - [badValue('DogCommand', 'sit', 4, 41)], + [ + badValue( + 'DogCommand', + 'sit', + 4, + 41, + 'Did you mean the enum value SIT?', + ), + ], ); }); }); @@ -989,7 +997,15 @@ describe('Validate: Values of correct type', () => { } } `, - [unknownField('ComplexInput', 'unknownField', 6, 15)], + [ + unknownField( + 'ComplexInput', + 'unknownField', + 6, + 15, + 'Did you mean intField or booleanField?', + ), + ], ); }); diff --git a/src/validation/rules/ValuesOfCorrectType.js b/src/validation/rules/ValuesOfCorrectType.js index 34fbefe289..d8cfe53079 100644 --- a/src/validation/rules/ValuesOfCorrectType.js +++ b/src/validation/rules/ValuesOfCorrectType.js @@ -20,7 +20,7 @@ import { getNullableType, getNamedType, } from '../../type/definition'; -import type { GraphQLEnumType } from '../../type/definition'; +import type { GraphQLType } from '../../type/definition'; import isInvalid from '../../jsutils/isInvalid'; import keyMap from '../../jsutils/keyMap'; import orList from '../../jsutils/orList'; @@ -51,8 +51,12 @@ export function requiredFieldMessage( export function unknownFieldMessage( typeName: string, fieldName: string, + message?: string, ): string { - return `Field "${fieldName}" is not defined by type ${typeName}.`; + return ( + `Field "${fieldName}" is not defined by type ${typeName}` + + (message ? `; ${message}` : '.') + ); } /** @@ -105,10 +109,18 @@ export function ValuesOfCorrectType(context: ValidationContext): any { ObjectField(node) { const parentType = getNamedType(context.getParentInputType()); const fieldType = context.getInputType(); - if (!fieldType && parentType) { + if (!fieldType && isInputObjectType(parentType)) { + const suggestions = suggestionList( + node.name.value, + Object.keys(parentType.getFields()), + ); + const didYouMean = + suggestions.length !== 0 + ? `Did you mean ${orList(suggestions)}?` + : undefined; context.reportError( new GraphQLError( - unknownFieldMessage(parentType.name, node.name.value), + unknownFieldMessage(parentType.name, node.name.value, didYouMean), node, ), ); @@ -152,12 +164,13 @@ function isValidScalar(context: ValidationContext, node: ValueNode): void { const type = getNamedType(locationType); if (!isScalarType(type)) { - const suggestions = isEnumType(type) - ? enumTypeSuggestion(type, node) - : undefined; context.reportError( new GraphQLError( - badValueMessage(String(locationType), print(node), suggestions), + badValueMessage( + String(locationType), + print(node), + enumTypeSuggestion(type, node), + ), node, ), ); @@ -191,12 +204,14 @@ function isValidScalar(context: ValidationContext, node: ValueNode): void { } } -function enumTypeSuggestion(type: GraphQLEnumType, node: ValueNode): string { - const suggestions = suggestionList( - print(node), - type.getValues().map(value => value.name), - ); - return suggestions.length === 0 - ? '' - : `Did you mean the enum value: ${orList(suggestions)}?`; +function enumTypeSuggestion(type: GraphQLType, node: ValueNode): string | void { + if (isEnumType(type)) { + const suggestions = suggestionList( + print(node), + type.getValues().map(value => value.name), + ); + if (suggestions.length !== 0) { + return `Did you mean the enum value ${orList(suggestions)}?`; + } + } }