Skip to content
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

Change serialization for Int, Float and Boolean to work with objects #2382

Merged
merged 5 commits into from
Jan 23, 2020
+49 −18
Merged

Change serialization for Int, Float and Boolean to work with objects #2382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f60bbb6
Change serialization for int and float to work with objectsIt is good…
Jan 22, 2020
e4a18d8
Add tests for float and int serialization to work with objects
Jan 22, 2020
947683e
Change object serialization to calling of common method
Jan 22, 2020
f33a40a
Fix Int serialization test
Jan 22, 2020
4b17186
Add serialization from object for boolean type
Jan 23, 2020
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
24 changes: 24 additions & 0 deletions src/type/__tests__/serialization-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ describe('Type System: Scalar coercion', () => {
expect(GraphQLInt.serialize(1e5)).to.equal(100000);
expect(GraphQLInt.serialize(false)).to.equal(0);
expect(GraphQLInt.serialize(true)).to.equal(1);
expect(
GraphQLInt.serialize({
value: 5,
valueOf() {
return this.value;
},
}),
).to.equal(5);

// The GraphQL specification does not allow serializing non-integer values
// as Int to avoid accidental data loss.
Expand Down Expand Up @@ -79,6 +87,14 @@ describe('Type System: Scalar coercion', () => {
expect(GraphQLFloat.serialize('-1.1')).to.equal(-1.1);
expect(GraphQLFloat.serialize(false)).to.equal(0.0);
expect(GraphQLFloat.serialize(true)).to.equal(1.0);
expect(
GraphQLFloat.serialize({
value: 5.5,
valueOf() {
return this.value;
},
}),
).to.equal(5.5);

expect(() => GraphQLFloat.serialize(NaN)).to.throw(
'Float cannot represent non numeric value: NaN',
Expand Down Expand Up @@ -139,6 +155,14 @@ describe('Type System: Scalar coercion', () => {
expect(GraphQLBoolean.serialize(0)).to.equal(false);
expect(GraphQLBoolean.serialize(true)).to.equal(true);
expect(GraphQLBoolean.serialize(false)).to.equal(false);
expect(
GraphQLBoolean.serialize({
value: true,
valueOf() {
return this.value;
},
}),
).to.equal(true);

expect(() => GraphQLBoolean.serialize(NaN)).to.throw(
'Boolean cannot represent a non boolean value: NaN',
Expand Down
43 changes: 25 additions & 18 deletions src/type/scalars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,26 @@ const MAX_INT = 2147483647;
const MIN_INT = -2147483648;

function serializeInt(outputValue: mixed): number {
if (typeof outputValue === 'boolean') {
return outputValue ? 1 : 0;
const coercedValue = serializeObject(outputValue);

if (typeof coercedValue === 'boolean') {
return coercedValue ? 1 : 0;
}

let num = outputValue;
if (typeof outputValue === 'string' && outputValue !== '') {
num = Number(outputValue);
let num = coercedValue;
if (typeof coercedValue === 'string' && coercedValue !== '') {
num = Number(coercedValue);
}
Copy link
Member

@IvanGoncharov IvanGoncharov Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please use serializeObject instead and add it in Int, Float and Boolean.
Also please update tests in https://github.com/graphql/graphql-js/blob/master/src/type/__tests__/serialization-test.js

So basically, please replicate how this is done for ID and String.

Copy link
Contributor Author

@alex-knyazev alex-knyazev Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@IvanGoncharov I did it.
Is it good idea to squash my 4 commits to one?

Copy link
Member

Choose a reason for hiding this comment

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

Is it good idea to squash my 4 commits to one?

@alex-knyazev Both work, since it's small commit I can just "squash and merge" in GitHub UI.
Please don't forget to make the same changes and tests for GraphQLBoolean type.

Copy link
Contributor Author

@alex-knyazev alex-knyazev Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@IvanGoncharov I've added the same logic for boolean type


if (!isInteger(num)) {
throw new GraphQLError(
`Int cannot represent non-integer value: ${inspect(outputValue)}`,
`Int cannot represent non-integer value: ${inspect(coercedValue)}`,
);
}
if (num > MAX_INT || num < MIN_INT) {
throw new GraphQLError(
'Int cannot represent non 32-bit signed integer value: ' +
inspect(outputValue),
inspect(coercedValue),
);
}
return num;
Expand Down Expand Up @@ -84,17 +86,20 @@ export const GraphQLInt = new GraphQLScalarType({
});

function serializeFloat(outputValue: mixed): number {
if (typeof outputValue === 'boolean') {
return outputValue ? 1 : 0;
const coercedValue = serializeObject(outputValue);

if (typeof coercedValue === 'boolean') {
return coercedValue ? 1 : 0;
}

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

if (!isFinite(num)) {
throw new GraphQLError(
`Float cannot represent non numeric value: ${inspect(outputValue)}`,
`Float cannot represent non numeric value: ${inspect(coercedValue)}`,
);
}
return num;
Expand Down Expand Up @@ -191,14 +196,16 @@ export const GraphQLString = new GraphQLScalarType({
});

function serializeBoolean(outputValue: mixed): boolean {
if (typeof outputValue === 'boolean') {
return outputValue;
const coercedValue = serializeObject(outputValue);

if (typeof coercedValue === 'boolean') {
return coercedValue;
}
if (isFinite(outputValue)) {
return outputValue !== 0;
if (isFinite(coercedValue)) {
return coercedValue !== 0;
}
throw new GraphQLError(
`Boolean cannot represent a non boolean value: ${inspect(outputValue)}`,
`Boolean cannot represent a non boolean value: ${inspect(coercedValue)}`,
);
}

Expand Down