Skip to content

Scalar coercion cleanup #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 20, 2018
Merged

Scalar coercion cleanup #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a5e611
Cleanup + restrict Boolean serialize coercion to numbers
IvanGoncharov Jul 4, 2018
f8e8f90
Make coercion code more explicit.
IvanGoncharov Jul 10, 2018
7f631d2
Address review comments
IvanGoncharov Jul 19, 2018
0ed49bf
fixing const num that's not const
mjmahone Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/execution/__tests__/schema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('Execute: Handles execution with a complex schema', () => {
function article(id) {
return {
id,
isPublished: 'true',
Copy link
Member Author

@IvanGoncharov IvanGoncharov Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more example why current coercion rules looks strange and confusing:
"true" => true
"false" => true

isPublished: true,
author: johnSmith,
title: 'My Article ' + id,
body: 'This is a post',
Expand Down
48 changes: 31 additions & 17 deletions src/type/__tests__/serialization-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]',
);
});

Expand Down Expand Up @@ -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]',
);
});

Expand All @@ -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';
Expand All @@ -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);
Copy link
Member Author

@IvanGoncharov IvanGoncharov Jul 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting non-empty strings to true is confusing because GraphQLString coerce true into "true". Also, spec doesn't mention this coercion:
http://facebook.github.io/graphql/June2018/#sec-Boolean

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: {}',
);
});

Expand All @@ -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,
Expand All @@ -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',
Copy link
Contributor

@mjmahone mjmahone Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a great change: making the test clear the reason it fails is floating-point coercion not negative-number coercion.

expect(() => GraphQLID.serialize(3.14)).to.throw(
'ID cannot represent value: 3.14',
);

expect(() => GraphQLID.serialize({})).to.throw(
Expand Down
86 changes: 48 additions & 38 deletions src/type/scalars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ 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)}`,
);
if (typeof value === 'boolean') {
return value ? 1 : 0;
}
const num = Number(value);
if (value === '' || !isInteger(num)) {

let num = value;
if (typeof value === 'string' && value !== '') {
num = Number(value);
}

if (!isInteger(num)) {
throw new TypeError(
`Int cannot represent non-integer value: ${inspect(value)}`,
);
Expand Down Expand Up @@ -74,13 +77,15 @@ 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)}`,
);
if (typeof value === 'boolean') {
return value ? 1 : 0;
}

let num = value;
if (typeof value === 'string' && value !== '') {
num = Number(value);
}
const num = Number(value);
if (value === '' || !isFinite(num)) {
if (!isFinite(num)) {
throw new TypeError(
`Float cannot represent non numeric value: ${inspect(value)}`,
);
Expand Down Expand Up @@ -118,16 +123,18 @@ 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'
) {
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 {
Expand All @@ -153,12 +160,15 @@ export const GraphQLString = new GraphQLScalarType({
});

function serializeBoolean(value: mixed): boolean {
if (Array.isArray(value)) {
throw new TypeError(
`Boolean cannot represent an array 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 {
Expand All @@ -185,23 +195,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' &&
(typeof result !== 'number' || !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' &&
(typeof value !== 'number' || !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({
Expand Down