enum: 'parseLiteral/parseValue/serialize' now throw on invalid values

IvanGoncharov · IvanGoncharov · commit 8057e3946b58 · 2020-01-06T15:34:41.000+07:00
Continuation of #1821
diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js
@@ -14,6 +14,7 @@ import { GraphQLInt, GraphQLBoolean, GraphQLString } from '../../type/scalars';
 import {
   GraphQLList,
   GraphQLNonNull,
+  GraphQLScalarType,
   GraphQLInterfaceType,
   GraphQLObjectType,
 } from '../../type/definition';
@@ -1063,6 +1064,39 @@ describe('Execute: Handles basic execution tasks', () => {
     expect(asyncResult).to.deep.equal(asyncResult);
   });
 
+  it('fails when serialize of custom scalar does not return a value', () => {
+    const customScalar = new GraphQLScalarType({
+      name: 'CustomScalar',
+      serialize() {
+        /* returns nothing */
+      },
+    });
+    const schema = new GraphQLSchema({
+      query: new GraphQLObjectType({
+        name: 'Query',
+        fields: {
+          customScalar: {
+            type: customScalar,
+            resolve: () => 'CUSTOM_VALUE',
+          },
+        },
+      }),
+    });
+
+    const result = execute({ schema, document: parse('{ customScalar }') });
+    expect(result).to.deep.equal({
+      data: { customScalar: null },
+      errors: [
+        {
+          message:
+            'Expected a value of type "CustomScalar" but received: "CUSTOM_VALUE"',
+          locations: [{ line: 1, column: 3 }],
+          path: ['customScalar'],
+        },
+      ],
+    });
+  });
+
   it('executes ignoring invalid non-executable definitions', () => {
     const schema = new GraphQLSchema({
       query: new GraphQLObjectType({
diff --git a/src/type/__tests__/enumType-test.js b/src/type/__tests__/enumType-test.js
@@ -150,7 +150,7 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Expected value of type "Color", found "GREEN". Did you mean the enum value "GREEN"?',
+            'Enum "Color" cannot represent non-enum value: "GREEN". Did you mean the enum value "GREEN"?',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -164,7 +164,7 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Expected value of type "Color", found GREENISH. Did you mean the enum value "GREEN"?',
+            'Value "GREENISH" does not exist in "Color" enum. Did you mean the enum value "GREEN"?',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -178,7 +178,7 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Expected value of type "Color", found green. Did you mean the enum value "GREEN"?',
+            'Value "green" does not exist in "Color" enum. Did you mean the enum value "GREEN"?',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -192,7 +192,7 @@ describe('Type System: Enum Values', () => {
       data: { colorEnum: null },
       errors: [
         {
-          message: 'Expected a value of type "Color" but received: "GREEN"',
+          message: 'Enum "Color" cannot represent value: "GREEN"',
           locations: [{ line: 1, column: 3 }],
           path: ['colorEnum'],
         },
@@ -206,7 +206,7 @@ describe('Type System: Enum Values', () => {
     expect(result).to.deep.equal({
       errors: [
         {
-          message: 'Expected value of type "Color", found 1.',
+          message: 'Enum "Color" cannot represent non-enum value: 1.',
           locations: [{ line: 1, column: 23 }],
         },
       ],
@@ -262,7 +262,7 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Variable "$color" got invalid value 2; Expected type "Color".',
+            'Variable "$color" got invalid value 2; Enum "Color" cannot represent non-string value: 2.',
           locations: [{ line: 1, column: 8 }],
         },
       ],
@@ -390,7 +390,7 @@ describe('Type System: Enum Values', () => {
       errors: [
         {
           message:
-            'Expected a value of type "Complex" but received: { someRandomValue: 123 }',
+            'Enum "Complex" cannot represent value: { someRandomValue: 123 }',
           locations: [{ line: 6, column: 9 }],
           path: ['bad'],
         },
diff --git a/src/type/definition.js b/src/type/definition.js
@@ -10,9 +10,11 @@ import { type Path } from '../jsutils/Path';
 import devAssert from '../jsutils/devAssert';
 import keyValMap from '../jsutils/keyValMap';
 import instanceOf from '../jsutils/instanceOf';
+import didYouMean from '../jsutils/didYouMean';
 import isObjectLike from '../jsutils/isObjectLike';
 import identityFunc from '../jsutils/identityFunc';
 import defineToJSON from '../jsutils/defineToJSON';
+import suggestionList from '../jsutils/suggestionList';
 import defineToStringTag from '../jsutils/defineToStringTag';
 import { type PromiseOrValue } from '../jsutils/PromiseOrValue';
 import {
@@ -22,6 +24,7 @@ import {
 } from '../jsutils/ObjMap';
 
 import { Kind } from '../language/kinds';
+import { print } from '../language/printer';
 import {
   type ScalarTypeDefinitionNode,
   type ObjectTypeDefinitionNode,
@@ -44,6 +47,8 @@ import {
   type ValueNode,
 } from '../language/ast';
 
+import { GraphQLError } from '../error/GraphQLError';
+
 import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped';
 
 import { type GraphQLSchema } from './schema';
@@ -1234,28 +1239,54 @@ export class GraphQLEnumType /* <T> */ {
 
   serialize(outputValue: mixed /* T */): ?string {
     const enumValue = this._valueLookup.get(outputValue);
-    if (enumValue) {
-      return enumValue.name;
+    if (enumValue === undefined) {
+      throw new GraphQLError(
+        `Enum "${this.name}" cannot represent value: ${inspect(outputValue)}`,
+      );
     }
+    return enumValue.name;
   }
 
   parseValue(inputValue: mixed): ?any /* T */ {
-    if (typeof inputValue === 'string') {
-      const enumValue = this.getValue(inputValue);
-      if (enumValue) {
-        return enumValue.value;
-      }
+    if (typeof inputValue !== 'string') {
+      const valueStr = inspect(inputValue);
+      throw new GraphQLError(
+        `Enum "${this.name}" cannot represent non-string value: ${valueStr}.` +
+          didYouMeanEnumValue(this, valueStr),
+      );
     }
+
+    const enumValue = this.getValue(inputValue);
+    if (enumValue == null) {
+      throw new GraphQLError(
+        `Value "${inputValue}" does not exist in "${this.name}" enum.` +
+          didYouMeanEnumValue(this, inputValue),
+      );
+    }
+    return enumValue.value;
   }
 
   parseLiteral(valueNode: ValueNode, _variables: ?ObjMap<mixed>): ?any /* T */ {
     // Note: variables will be resolved to a value before calling this function.
-    if (valueNode.kind === Kind.ENUM) {
-      const enumValue = this.getValue(valueNode.value);
-      if (enumValue) {
-        return enumValue.value;
-      }
+    if (valueNode.kind !== Kind.ENUM) {
+      const valueStr = print(valueNode);
+      throw new GraphQLError(
+        `Enum "${this.name}" cannot represent non-enum value: ${valueStr}.` +
+          didYouMeanEnumValue(this, valueStr),
+        valueNode,
+      );
     }
+
+    const enumValue = this.getValue(valueNode.value);
+    if (enumValue == null) {
+      const valueStr = print(valueNode);
+      throw new GraphQLError(
+        `Value "${valueStr}" does not exist in "${this.name}" enum.` +
+          didYouMeanEnumValue(this, valueStr),
+        valueNode,
+      );
+    }
+    return enumValue.value;
   }
 
   toConfig(): {|
@@ -1294,6 +1325,16 @@ export class GraphQLEnumType /* <T> */ {
 defineToStringTag(GraphQLEnumType);
 defineToJSON(GraphQLEnumType);
 
+function didYouMeanEnumValue(
+  enumType: GraphQLEnumType,
+  unknownValueStr: string,
+): string {
+  const allNames = enumType.getValues().map(value => value.name);
+  const suggestedValues = suggestionList(unknownValueStr, allNames);
+
+  return didYouMean('the enum value', suggestedValues);
+}
+
 function defineEnumValues(
   typeName: string,
   valueMap: GraphQLEnumValueConfigMap /* <T> */,
diff --git a/src/utilities/__tests__/astFromValue-test.js b/src/utilities/__tests__/astFromValue-test.js
@@ -219,10 +219,14 @@ describe('astFromValue', () => {
     });
 
     // Note: case sensitive
-    expect(astFromValue('hello', myEnum)).to.deep.equal(null);
+    expect(() => astFromValue('hello', myEnum)).to.throw(
+      'Enum "MyEnum" cannot represent value: "hello"',
+    );
 
     // Note: Not a valid enum value
-    expect(astFromValue('VALUE', myEnum)).to.deep.equal(null);
+    expect(() => astFromValue('UNKNOWN_VALUE', myEnum)).to.throw(
+      'Enum "MyEnum" cannot represent value: "UNKNOWN_VALUE"',
+    );
   });
 
   it('converts array values to List ASTs', () => {
diff --git a/src/utilities/__tests__/coerceInputValue-test.js b/src/utilities/__tests__/coerceInputValue-test.js
@@ -140,7 +140,8 @@ describe('coerceInputValue', () => {
       const result = coerceValue('foo', TestEnum);
       expectErrors(result).to.deep.equal([
         {
-          error: 'Expected type "TestEnum". Did you mean the enum value "FOO"?',
+          error:
+            'Value "foo" does not exist in "TestEnum" enum. Did you mean the enum value "FOO"?',
           path: [],
           value: 'foo',
         },
@@ -151,7 +152,7 @@ describe('coerceInputValue', () => {
       const result1 = coerceValue(123, TestEnum);
       expectErrors(result1).to.deep.equal([
         {
-          error: 'Expected type "TestEnum".',
+          error: 'Enum "TestEnum" cannot represent non-string value: 123.',
           path: [],
           value: 123,
         },
@@ -160,7 +161,8 @@ describe('coerceInputValue', () => {
       const result2 = coerceValue({ field: 'value' }, TestEnum);
       expectErrors(result2).to.deep.equal([
         {
-          error: 'Expected type "TestEnum".',
+          error:
+            'Enum "TestEnum" cannot represent non-string value: { field: "value" }.',
           path: [],
           value: { field: 'value' },
         },
diff --git a/src/utilities/coerceInputValue.js b/src/utilities/coerceInputValue.js
@@ -15,8 +15,7 @@ import { type Path, addPath, pathToArray } from '../jsutils/Path';
 import { GraphQLError } from '../error/GraphQLError';
 import {
   type GraphQLInputType,
-  isScalarType,
-  isEnumType,
+  isLeafType,
   isInputObjectType,
   isListType,
   isNonNullType,
@@ -157,7 +156,7 @@ function coerceInputValueImpl(
     return coercedValue;
   }
 
-  if (isScalarType(type)) {
+  if (isLeafType(type)) {
     let parseResult;
 
     // Scalars determine if a input value is valid via parseValue(), which can
@@ -194,28 +193,6 @@ function coerceInputValueImpl(
     return parseResult;
   }
 
-  if (isEnumType(type)) {
-    if (typeof inputValue === 'string') {
-      const enumValue = type.getValue(inputValue);
-      if (enumValue) {
-        return enumValue.value;
-      }
-    }
-    const suggestions = suggestionList(
-      String(inputValue),
-      type.getValues().map(enumValue => enumValue.name),
-    );
-    onError(
-      pathToArray(path),
-      inputValue,
-      new GraphQLError(
-        `Expected type "${type.name}".` +
-          didYouMean('the enum value', suggestions),
-      ),
-    );
-    return;
-  }
-
   // Not reachable. All possible input types have been considered.
   invariant(false, 'Unexpected input type: ' + inspect((type: empty)));
 }
diff --git a/src/utilities/valueFromAST.js b/src/utilities/valueFromAST.js
@@ -13,8 +13,7 @@ import { type ValueNode } from '../language/ast';
 
 import {
   type GraphQLInputType,
-  isScalarType,
-  isEnumType,
+  isLeafType,
   isInputObjectType,
   isListType,
   isNonNullType,
@@ -133,18 +132,7 @@ export function valueFromAST(
     return coercedObj;
   }
 
-  if (isEnumType(type)) {
-    if (valueNode.kind !== Kind.ENUM) {
-      return; // Invalid: intentionally return no value.
-    }
-    const enumValue = type.getValue(valueNode.value);
-    if (!enumValue) {
-      return; // Invalid: intentionally return no value.
-    }
-    return enumValue.value;
-  }
-
-  if (isScalarType(type)) {
+  if (isLeafType(type)) {
     // Scalars fulfill parsing a literal value via parseLiteral().
     // Invalid values represent a failure to parse correctly, in which case
     // no value is returned.
@@ -154,7 +142,7 @@ export function valueFromAST(
     } catch (_error) {
       return; // Invalid: intentionally return no value.
     }
-    if (isInvalid(result)) {
+    if (result === undefined) {
       return; // Invalid: intentionally return no value.
     }
     return result;
diff --git a/src/validation/__tests__/ValuesOfCorrectType-test.js b/src/validation/__tests__/ValuesOfCorrectType-test.js
diff --git a/src/validation/rules/ValuesOfCorrectType.js b/src/validation/rules/ValuesOfCorrectType.js
