BREAKING/BUGFIX Strict coercion of scalar types

src/execution/__tests__/executor-test.js

@@ -279,7 +279,7 @@ describe('Execute: Handles basic execution tasks', () => {
 
     const rootValue = { root: 'val' };
 
-    execute(schema, ast, rootValue, null, { var: 123 });
+    execute(schema, ast, rootValue, null, { var: 'abc' });
 
     expect(Object.keys(info)).to.deep.equal([
       'fieldName',
@@ -304,7 +304,7 @@ describe('Execute: Handles basic execution tasks', () => {
     expect(info.schema).to.equal(schema);
     expect(info.rootValue).to.equal(rootValue);
     expect(info.operation).to.equal(ast.definitions[0]);
-    expect(info.variableValues).to.deep.equal({ var: '123' });
+    expect(info.variableValues).to.deep.equal({ var: 'abc' });
   });
 
   it('threads root value context correctly', () => {

src/execution/__tests__/variables-test.js

@@ -694,7 +694,7 @@ describe('Execute: Handles inputs', () => {
           {
             message:
               'Variable "$value" got invalid value [1, 2, 3]; Expected type ' +
-              'String; String cannot represent an array value: [1, 2, 3]',
+              'String; String cannot represent a non string value: [1, 2, 3]',
             locations: [{ line: 2, column: 16 }],
           },
         ],

src/jsutils/isFinite.js

@@ -0,0 +1,20 @@
+/**
+ * Copyright (c) 2018-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow strict
+ */
+
+declare function isFinite(value: mixed): boolean %checks(typeof value ===
+  'number');
+
+/* eslint-disable no-redeclare */
+// $FlowFixMe workaround for: https://github.com/facebook/flow/issues/4441
+const isFinite =
+  Number.isFinite ||
+  function(value) {
+    return typeof value === 'number' && isFinite(value);
+  };
+export default isFinite;

src/type/__tests__/serialization-test.js

@@ -60,7 +60,7 @@ describe('Type System: Scalar coercion', () => {
     );
     // Doesn't represent number
     expect(() => GraphQLInt.serialize('')).to.throw(
-      'Int cannot represent non-integer value: (empty string)',
+      'Int cannot represent non-integer value: ""',
     );
     expect(() => GraphQLInt.serialize(NaN)).to.throw(
       'Int cannot represent non-integer value: NaN',
@@ -95,26 +95,24 @@ describe('Type System: Scalar coercion', () => {
       'Float cannot represent non numeric value: "one"',
     );
     expect(() => GraphQLFloat.serialize('')).to.throw(
-      'Float cannot represent non numeric value: (empty string)',
+      'Float cannot represent non numeric value: ""',
     );
     expect(() => GraphQLFloat.serialize([5])).to.throw(
       'Float cannot represent an array value: [5]',
     );
   });
 
-  for (const scalar of [GraphQLString, GraphQLID]) {
-    it(`serializes output as ${scalar}`, () => {
-      expect(scalar.serialize('string')).to.equal('string');
-      expect(scalar.serialize(1)).to.equal('1');
-      expect(scalar.serialize(-1.1)).to.equal('-1.1');
-      expect(scalar.serialize(true)).to.equal('true');
-      expect(scalar.serialize(false)).to.equal('false');
+  it(`serializes output as String`, () => {
+    expect(GraphQLString.serialize('string')).to.equal('string');
+    expect(GraphQLString.serialize(1)).to.equal('1');
+    expect(GraphQLString.serialize(-1.1)).to.equal('-1.1');
+    expect(GraphQLString.serialize(true)).to.equal('true');
+    expect(GraphQLString.serialize(false)).to.equal('false');
 
-      expect(() => scalar.serialize([1])).to.throw(
-        'String cannot represent an array value: [1]',
-      );
-    });
-  }
+    expect(() => GraphQLString.serialize([1])).to.throw(
+      'String cannot represent an array value: [1]',
+    );
+  });
 
   it('serializes output as Boolean', () => {
     expect(GraphQLBoolean.serialize('string')).to.equal(true);
@@ -129,4 +127,46 @@ describe('Type System: Scalar coercion', () => {
       'Boolean cannot represent an array value: [false]',
     );
   });
+
+  it('serializes output as ID', () => {
+    expect(GraphQLID.serialize('string')).to.equal('string');
+    expect(GraphQLID.serialize('false')).to.equal('false');
+    expect(GraphQLID.serialize('')).to.equal('');
+    expect(GraphQLID.serialize(123)).to.equal('123');
+    expect(GraphQLID.serialize(0)).to.equal('0');
+
+    const objValue = {
+      _id: 123,
+      valueOf() {
+        return this._id;
+      },
+    };
+    expect(GraphQLID.serialize(objValue)).to.equal('123');
+
+    const badObjValue = {
+      _id: false,
+      valueOf() {
+        return this._id;
+      },
+    };
+    expect(() => GraphQLID.serialize(badObjValue)).to.throw(
+      'ID cannot represent value: {_id: false, valueOf: [function valueOf]}',
+    );
+
+    expect(() => GraphQLID.serialize(true)).to.throw(
+      'ID cannot represent value: true',
+    );
+
+    expect(() => GraphQLID.serialize(-1.1)).to.throw(
+      'ID cannot represent value: -1.1',
+    );
+
+    expect(() => GraphQLID.serialize({})).to.throw(
+      'ID cannot represent value: {}',
+    );
+
+    expect(() => GraphQLID.serialize(['abc'])).to.throw(
+      'ID cannot represent value: ["abc"]',
+    );
+  });
 });

src/type/scalars.js

@@ -8,6 +8,7 @@
  */
 
 import inspect from '../jsutils/inspect';
+import isFinite from '../jsutils/isFinite';
 import isInteger from '../jsutils/isInteger';
 import { GraphQLScalarType, isNamedType } from './definition';
 import { Kind } from '../language/kinds';
@@ -20,38 +21,46 @@ import { Kind } from '../language/kinds';
 const MAX_INT = 2147483647;
 const MIN_INT = -2147483648;
 
-function coerceInt(value: mixed): number {
+function serializeInt(value: mixed): number {
   if (Array.isArray(value)) {
     throw new TypeError(
-      `Int cannot represent an array value: [${String(value)}]`,
+      `Int cannot represent an array value: ${inspect(value)}`,
     );
   }
-  if (value === '') {
+  const num = Number(value);
+  if (value === '' || !isInteger(num)) {
     throw new TypeError(
-      'Int cannot represent non-integer value: (empty string)',
+      `Int cannot represent non-integer value: ${inspect(value)}`,
     );
   }
-  const num = Number(value);
-  if (!isInteger(num)) {
+  if (num > MAX_INT || num < MIN_INT) {
     throw new TypeError(
-      'Int cannot represent non-integer value: ' + inspect(value),
+      `Int cannot represent non 32-bit signed integer value: ${inspect(value)}`,
     );
   }
+  return num;
+}
 
-  if (num > MAX_INT || num < MIN_INT) {
+function coerceInt(value: mixed): number {
+  if (!isInteger(value)) {
     throw new TypeError(
-      'Int cannot represent non 32-bit signed integer value: ' + inspect(value),
+      `Int cannot represent non-integer value: ${inspect(value)}`,
     );
   }
-  return num;
+  if (value > MAX_INT || value < MIN_INT) {
+    throw new TypeError(
+      `Int cannot represent non 32-bit signed integer value: ${inspect(value)}`,
+    );
+  }
+  return value;
 }
 
 export const GraphQLInt = new GraphQLScalarType({
   name: 'Int',
   description:
     'The `Int` scalar type represents non-fractional signed whole numeric ' +
     'values. Int can represent values between -(2^31) and 2^31 - 1. ',
-  serialize: coerceInt,
+  serialize: serializeInt,
   parseValue: coerceInt,
   parseLiteral(ast) {
     if (ast.kind === Kind.INT) {
@@ -64,24 +73,28 @@ export const GraphQLInt = new GraphQLScalarType({
   },
 });
 
-function coerceFloat(value: mixed): number {
+function serializeFloat(value: mixed): number {
   if (Array.isArray(value)) {
     throw new TypeError(
-      `Float cannot represent an array value: [${String(value)}]`,
+      `Float cannot represent an array value: ${inspect(value)}`,
     );
   }
-  if (value === '') {
+  const num = Number(value);
+  if (value === '' || !isFinite(num)) {
     throw new TypeError(
-      'Float cannot represent non numeric value: (empty string)',
+      `Float cannot represent non numeric value: ${inspect(value)}`,
     );
   }
-  const num = Number(value);
-  if (isFinite(num)) {
-    return num;
+  return num;
+}
+
+function coerceFloat(value: mixed): number {
+  if (!isFinite(value)) {
+    throw new TypeError(
+      `Float cannot represent non numeric value: ${inspect(value)}`,
+    );
   }
-  throw new TypeError(
-    'Float cannot represent non numeric value: ' + inspect(value),
-  );
+  return value;
 }
 
 export const GraphQLFloat = new GraphQLScalarType({
@@ -90,7 +103,7 @@ export const GraphQLFloat = new GraphQLScalarType({
     'The `Float` scalar type represents signed double-precision fractional ' +
     'values as specified by ' +
     '[IEEE 754](http://en.wikipedia.org/wiki/IEEE_floating_point). ',
-  serialize: coerceFloat,
+  serialize: serializeFloat,
   parseValue: coerceFloat,
   parseLiteral(ast) {
     return ast.kind === Kind.FLOAT || ast.kind === Kind.INT
@@ -99,7 +112,7 @@ export const GraphQLFloat = new GraphQLScalarType({
   },
 });
 
-function coerceString(value: mixed): string {
+function serializeString(value: mixed): string {
   if (Array.isArray(value)) {
     throw new TypeError(
       `String cannot represent an array value: ${inspect(value)}`,
@@ -108,38 +121,84 @@ function coerceString(value: mixed): string {
   return String(value);
 }
 
+function coerceString(value: mixed): string {
+  if (typeof value !== 'string') {
+    throw new TypeError(
+      `String cannot represent a non string value: ${inspect(value)}`,
+    );
+  }
+  return value;
+}
+
 export const GraphQLString = new GraphQLScalarType({
   name: 'String',
   description:
     'The `String` scalar type represents textual data, represented as UTF-8 ' +
     'character sequences. The String type is most often used by GraphQL to ' +
     'represent free-form human-readable text.',
-  serialize: coerceString,
+  serialize: serializeString,
   parseValue: coerceString,
   parseLiteral(ast) {
     return ast.kind === Kind.STRING ? ast.value : undefined;
   },
 });
 
-function coerceBoolean(value: mixed): boolean {
+function serializeBoolean(value: mixed): boolean {
   if (Array.isArray(value)) {
     throw new TypeError(
-      `Boolean cannot represent an array value: [${String(value)}]`,
+      `Boolean cannot represent an array value: ${inspect(value)}`,
     );
   }
   return Boolean(value);
 }
 
+function coerceBoolean(value: mixed): boolean {
+  if (typeof value !== 'boolean') {
+    throw new TypeError(
+      `Boolean cannot represent a non boolean value: ${inspect(value)}`,
+    );
+  }
+  return value;
+}
+
 export const GraphQLBoolean = new GraphQLScalarType({
   name: 'Boolean',
   description: 'The `Boolean` scalar type represents `true` or `false`.',
-  serialize: coerceBoolean,
+  serialize: serializeBoolean,
   parseValue: coerceBoolean,
   parseLiteral(ast) {
     return ast.kind === Kind.BOOLEAN ? ast.value : undefined;
   },
 });
 
+function serializeID(value: mixed): string {
+  // Support serializing objects with custom valueOf() functions - a common way
+  // to represent an object identifier (ex. MongoDB).
+  const result =
+    value &&
+    typeof value.valueOf === 'function' &&
+    value.valueOf !== Object.prototype.valueOf
+      ? value.valueOf()
+      : value;
+  if (
+    typeof result !== 'string' &&
+    (typeof result !== 'number' || !isInteger(result))
+  ) {
+    throw new TypeError(`ID cannot represent value: ${inspect(value)}`);
+  }
+  return String(result);
+}
+
+function coerceID(value: mixed): string {
+  if (
+    typeof value !== 'string' &&
+    (typeof value !== 'number' || !isInteger(value))
+  ) {
+    throw new TypeError(`ID cannot represent value: ${inspect(value)}`);
+  }
+  return String(value);
+}
+
 export const GraphQLID = new GraphQLScalarType({
   name: 'ID',
   description:
@@ -148,8 +207,8 @@ export const GraphQLID = new GraphQLScalarType({
     'response as a String; however, it is not intended to be human-readable. ' +
     'When expected as an input type, any string (such as `"4"`) or integer ' +
     '(such as `4`) input value will be accepted as an ID.',
-  serialize: coerceString,
-  parseValue: coerceString,
+  serialize: serializeID,
+  parseValue: coerceID,
   parseLiteral(ast) {
     return ast.kind === Kind.STRING || ast.kind === Kind.INT
       ? ast.value

src/utilities/__tests__/astFromValue-test.js

@@ -183,10 +183,9 @@ describe('astFromValue', () => {
       value: '01',
     });
 
-    expect(astFromValue(false, GraphQLID)).to.deep.equal({
-      kind: 'StringValue',
-      value: 'false',
-    });
+    expect(() => astFromValue(false, GraphQLID)).to.throw(
+      'ID cannot represent value: false',
+    );
 
     expect(astFromValue(null, GraphQLID)).to.deep.equal({ kind: 'NullValue' });