BREAKING/BUGFIX Strict coercion of scalar types (#1382)

* BREAKING/BUGFIX Strict coercion of scalar types

This no longer accepts incoming variable values in a potentially lossy way, mirroring the existing behavior for literals. This fixes an issue with GraphQL.js being not spec compliant.

This is breaking since servers which used to accept incorrect variable values will now return errors to clients.

Serialization of values is not affected in the same way, since this is not a client-visible behavior.

As a bonus, this adds unique serialization and coercion functions for the ID type, allowing to be more restrictive on numeric types and un-stringable object types, while directly supporting valueOf() methods (ala MongoDB). The changes to how the ID type serializes and coerces data could be potentially breaking.

Fixes #1324

* Updates from review. Simplified ID serialization and added similar logic to string serialization

Author: leebyron

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,38 @@ 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 value: [1]',
+    );
+
+    const badObjValue = {};
+    expect(() => GraphQLString.serialize(badObjValue)).to.throw(
+      'String cannot represent value: {}',
+    );
+
+    const stringableObjValue = {
+      valueOf() {
+        return 'something useful';
+      },
+    };
+    expect(GraphQLString.serialize(stringableObjValue)).to.equal(
+      'something useful',
+    );
+  });
 
   it('serializes output as Boolean', () => {
     expect(GraphQLBoolean.serialize('string')).to.equal(true);
@@ -129,4 +141,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,13 +112,31 @@ export const GraphQLFloat = new GraphQLScalarType({
   },
 });
 
+function serializeString(value: mixed): string {
+  // Support serializing objects with custom valueOf() functions - a common way
+  // to represent an complex value which can be represented as a string
+  // (ex: MongoDB id objects).
+  const result =
+    value && typeof value.valueOf === 'function' ? value.valueOf() : value;
+  // Serialize string, number, and boolean values to a string, but do not
+  // attempt to coerce object, function, symbol, or other types as strings.
+  if (
+    typeof result !== 'string' &&
+    typeof result !== 'number' &&
+    typeof result !== 'boolean'
+  ) {
+    throw new TypeError(`String cannot represent value: ${inspect(result)}`);
+  }
+  return String(result);
+}
+
 function coerceString(value: mixed): string {
-  if (Array.isArray(value)) {
+  if (typeof value !== 'string') {
     throw new TypeError(
-      `String cannot represent an array value: ${inspect(value)}`,
+      `String cannot represent a non string value: ${inspect(value)}`,
     );
   }
-  return String(value);
+  return value;
 }
 
 export const GraphQLString = new GraphQLScalarType({
@@ -114,32 +145,65 @@ export const GraphQLString = new GraphQLScalarType({
     '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() : 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 +212,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' });