From 1a5e611e43e32f361c6dd875cf789fb1eda0ae07 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 4 Jul 2018 15:26:55 +0300 Subject: [PATCH 1/4] Cleanup + restrict Boolean serialize coercion to numbers --- src/execution/__tests__/schema-test.js | 2 +- src/type/__tests__/serialization-test.js | 44 ++++++++++++++++-------- src/type/scalars.js | 20 ++++------- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/execution/__tests__/schema-test.js b/src/execution/__tests__/schema-test.js index c62ca9b78f..a62c9964da 100644 --- a/src/execution/__tests__/schema-test.js +++ b/src/execution/__tests__/schema-test.js @@ -91,7 +91,7 @@ describe('Execute: Handles execution with a complex schema', () => { function article(id) { return { id, - isPublished: 'true', + isPublished: true, author: johnSmith, title: 'My Article ' + id, body: 'This is a post', diff --git a/src/type/__tests__/serialization-test.js b/src/type/__tests__/serialization-test.js index 0944303d0c..b77556b11f 100644 --- a/src/type/__tests__/serialization-test.js +++ b/src/type/__tests__/serialization-test.js @@ -109,15 +109,6 @@ describe('Type System: Scalar coercion', () => { expect(GraphQLString.serialize(true)).to.equal('true'); expect(GraphQLString.serialize(false)).to.equal('false'); - 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'; @@ -126,19 +117,41 @@ describe('Type System: Scalar coercion', () => { expect(GraphQLString.serialize(stringableObjValue)).to.equal( 'something useful', ); + + expect(() => GraphQLString.serialize(NaN)).to.throw( + 'String cannot represent value: NaN', + ); + + expect(() => GraphQLString.serialize([1])).to.throw( + 'String cannot represent value: [1]', + ); + + const badObjValue = {}; + expect(() => GraphQLString.serialize(badObjValue)).to.throw( + 'String cannot represent value: {}', + ); }); it('serializes output as Boolean', () => { - expect(GraphQLBoolean.serialize('string')).to.equal(true); - expect(GraphQLBoolean.serialize('false')).to.equal(true); - expect(GraphQLBoolean.serialize('')).to.equal(false); expect(GraphQLBoolean.serialize(1)).to.equal(true); expect(GraphQLBoolean.serialize(0)).to.equal(false); expect(GraphQLBoolean.serialize(true)).to.equal(true); expect(GraphQLBoolean.serialize(false)).to.equal(false); + expect(() => GraphQLBoolean.serialize(NaN)).to.throw( + 'Boolean cannot represent a non boolean value: NaN', + ); + expect(() => GraphQLBoolean.serialize('')).to.throw( + 'Boolean cannot represent a non boolean value: ""', + ); + expect(() => GraphQLBoolean.serialize('true')).to.throw( + 'Boolean cannot represent a non boolean value: "true"', + ); expect(() => GraphQLBoolean.serialize([false])).to.throw( - 'Boolean cannot represent an array value: [false]', + 'Boolean cannot represent a non boolean value: [false]', + ); + expect(() => GraphQLBoolean.serialize({})).to.throw( + 'Boolean cannot represent a non boolean value: {}', ); }); @@ -148,6 +161,7 @@ describe('Type System: Scalar coercion', () => { expect(GraphQLID.serialize('')).to.equal(''); expect(GraphQLID.serialize(123)).to.equal('123'); expect(GraphQLID.serialize(0)).to.equal('0'); + expect(GraphQLID.serialize(-1)).to.equal('-1'); const objValue = { _id: 123, @@ -171,8 +185,8 @@ describe('Type System: Scalar coercion', () => { 'ID cannot represent value: true', ); - expect(() => GraphQLID.serialize(-1.1)).to.throw( - 'ID cannot represent value: -1.1', + expect(() => GraphQLID.serialize(3.14)).to.throw( + 'ID cannot represent value: 3.14', ); expect(() => GraphQLID.serialize({})).to.throw( diff --git a/src/type/scalars.js b/src/type/scalars.js index 9962e52940..7bb0167607 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -118,12 +118,12 @@ function serializeString(value: mixed): 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 + // Serialize string, boolean and number 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' + typeof result !== 'boolean' && + !isFinite(result) ) { throw new TypeError(`String cannot represent value: ${inspect(result)}`); } @@ -153,9 +153,9 @@ export const GraphQLString = new GraphQLScalarType({ }); function serializeBoolean(value: mixed): boolean { - if (Array.isArray(value)) { + if (typeof value !== 'boolean' && !isFinite(value)) { throw new TypeError( - `Boolean cannot represent an array value: ${inspect(value)}`, + `Boolean cannot represent a non boolean value: ${inspect(value)}`, ); } return Boolean(value); @@ -185,20 +185,14 @@ function serializeID(value: mixed): string { // 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)) - ) { + if (typeof result !== 'string' && !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)) - ) { + if (typeof value !== 'string' && !isInteger(value)) { throw new TypeError(`ID cannot represent value: ${inspect(value)}`); } return String(value); From f8e8f90dd402deec96e3f903d4bcdcddcf04614a Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 10 Jul 2018 16:51:41 +0300 Subject: [PATCH 2/4] Make coercion code more explicit. --- src/type/__tests__/serialization-test.js | 4 +- src/type/scalars.js | 75 ++++++++++++++---------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/src/type/__tests__/serialization-test.js b/src/type/__tests__/serialization-test.js index b77556b11f..989bc7c950 100644 --- a/src/type/__tests__/serialization-test.js +++ b/src/type/__tests__/serialization-test.js @@ -69,7 +69,7 @@ describe('Type System: Scalar coercion', () => { 'Int cannot represent non-integer value: Infinity', ); expect(() => GraphQLInt.serialize([5])).to.throw( - 'Int cannot represent an array value: [5]', + 'Int cannot represent non-integer value: [5]', ); }); @@ -98,7 +98,7 @@ describe('Type System: Scalar coercion', () => { 'Float cannot represent non numeric value: ""', ); expect(() => GraphQLFloat.serialize([5])).to.throw( - 'Float cannot represent an array value: [5]', + 'Float cannot represent non numeric value: [5]', ); }); diff --git a/src/type/scalars.js b/src/type/scalars.js index 7bb0167607..7b91be5a2c 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -22,13 +22,14 @@ const MAX_INT = 2147483647; const MIN_INT = -2147483648; function serializeInt(value: mixed): number { - if (Array.isArray(value)) { - throw new TypeError( - `Int cannot represent an array value: ${inspect(value)}`, - ); + let num = value; + if (typeof value === 'string' && value !== '') { + num = Number(value); + } else if (typeof value === 'boolean') { + return value ? 1 : 0; } - const num = Number(value); - if (value === '' || !isInteger(num)) { + + if (!isInteger(num)) { throw new TypeError( `Int cannot represent non-integer value: ${inspect(value)}`, ); @@ -74,13 +75,14 @@ export const GraphQLInt = new GraphQLScalarType({ }); function serializeFloat(value: mixed): number { - if (Array.isArray(value)) { - throw new TypeError( - `Float cannot represent an array value: ${inspect(value)}`, - ); + let num = value; + if (typeof value === 'string' && value !== '') { + num = Number(value); + } else if (typeof value === 'boolean') { + return value ? 1 : 0; } - const num = Number(value); - if (value === '' || !isFinite(num)) { + + if (!isFinite(num)) { throw new TypeError( `Float cannot represent non numeric value: ${inspect(value)}`, ); @@ -120,14 +122,16 @@ function serializeString(value: mixed): string { value && typeof value.valueOf === 'function' ? value.valueOf() : value; // Serialize string, boolean and number values to a string, but do not // attempt to coerce object, function, symbol, or other types as strings. - if ( - typeof result !== 'string' && - typeof result !== 'boolean' && - !isFinite(result) - ) { - throw new TypeError(`String cannot represent value: ${inspect(result)}`); - } - return String(result); + if (typeof result === 'string') { + return result; + } + if (typeof result === 'boolean') { + return result ? 'true' : 'false'; + } + if (isFinite(result)) { + return result.toString(); + } + throw new TypeError(`String cannot represent value: ${inspect(value)}`); } function coerceString(value: mixed): string { @@ -153,12 +157,15 @@ export const GraphQLString = new GraphQLScalarType({ }); function serializeBoolean(value: mixed): boolean { - if (typeof value !== 'boolean' && !isFinite(value)) { - throw new TypeError( - `Boolean cannot represent a non boolean value: ${inspect(value)}`, - ); + if (typeof value === 'boolean') { + return value; } - return Boolean(value); + if (isFinite(value)) { + return value !== 0; + } + throw new TypeError( + `Boolean cannot represent a non boolean value: ${inspect(value)}`, + ); } function coerceBoolean(value: mixed): boolean { @@ -185,17 +192,23 @@ function serializeID(value: mixed): string { // to represent an object identifier (ex. MongoDB). const result = value && typeof value.valueOf === 'function' ? value.valueOf() : value; - if (typeof result !== 'string' && !isInteger(result)) { - throw new TypeError(`ID cannot represent value: ${inspect(value)}`); + if (typeof result === 'string') { + return result; } - return String(result); + if (isInteger(result)) { + return String(result); + } + throw new TypeError(`ID cannot represent value: ${inspect(value)}`); } function coerceID(value: mixed): string { - if (typeof value !== 'string' && !isInteger(value)) { - throw new TypeError(`ID cannot represent value: ${inspect(value)}`); + if (typeof value === 'string') { + return value; + } + if (isInteger(value)) { + return value.toString(); } - return String(value); + throw new TypeError(`ID cannot represent value: ${inspect(value)}`); } export const GraphQLID = new GraphQLScalarType({ From 7f631d2313db2cff91c741fceb2da457d5fe1a01 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 19 Jul 2018 19:33:52 +0300 Subject: [PATCH 3/4] Address review comments --- src/type/scalars.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/type/scalars.js b/src/type/scalars.js index 7b91be5a2c..b09e31963d 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -22,11 +22,13 @@ const MAX_INT = 2147483647; const MIN_INT = -2147483648; function serializeInt(value: mixed): number { - let num = value; + if (typeof value === 'boolean') { + return value ? 1 : 0; + } + + const num = value; if (typeof value === 'string' && value !== '') { num = Number(value); - } else if (typeof value === 'boolean') { - return value ? 1 : 0; } if (!isInteger(num)) { @@ -75,13 +77,14 @@ export const GraphQLInt = new GraphQLScalarType({ }); function serializeFloat(value: mixed): number { + if (typeof value === 'boolean') { + return value ? 1 : 0; + } + let num = value; if (typeof value === 'string' && value !== '') { num = Number(value); - } else if (typeof value === 'boolean') { - return value ? 1 : 0; } - if (!isFinite(num)) { throw new TypeError( `Float cannot represent non numeric value: ${inspect(value)}`, From 0ed49bf16d1d2a1e737ba35d9695e7ebf6d55e30 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 19 Jul 2018 20:57:10 -0400 Subject: [PATCH 4/4] fixing const num that's not const --- src/type/scalars.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/type/scalars.js b/src/type/scalars.js index b09e31963d..89f6ed252b 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -26,7 +26,7 @@ function serializeInt(value: mixed): number { return value ? 1 : 0; } - const num = value; + let num = value; if (typeof value === 'string' && value !== '') { num = Number(value); }