Skip to content

Scalars: reject array serialization/coercion #1336

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.

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 5 commits into from
Jun 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 0 additions & 7 deletions src/execution/__tests__/variables-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -648,13 +648,6 @@ describe('Execute: Handles inputs', () => {
expect(result.errors[0].originalError).not.to.equal(undefined);
});

it('serializing an array via GraphQLString throws TypeError', () => {
expect(() => GraphQLString.serialize([1, 2, 3])).to.throw(
TypeError,
'String cannot represent an array value: [1,2,3]',
);
});

it('reports error for non-provided variables for non-nullable inputs', () => {
// Note: this test would typically fail validation before encountering
// this execution error, however for queries which previously validated
Expand Down
53 changes: 38 additions & 15 deletions src/type/__tests__/serialization-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,27 @@
* LICENSE file in the root directory of this source tree.
*/

import { GraphQLInt, GraphQLFloat, GraphQLString, GraphQLBoolean } from '../';
import {
GraphQLInt,
GraphQLID,
GraphQLFloat,
GraphQLString,
GraphQLBoolean,
} from '../';

import { describe, it } from 'mocha';
import { expect } from 'chai';

describe('Type System: Scalar coercion', () => {
it('serializes output int', () => {
it('serializes output as Int', () => {
expect(GraphQLInt.serialize(1)).to.equal(1);
expect(GraphQLInt.serialize('123')).to.equal(123);
expect(GraphQLInt.serialize(0)).to.equal(0);
expect(GraphQLInt.serialize(-1)).to.equal(-1);
expect(GraphQLInt.serialize(1e5)).to.equal(100000);
expect(GraphQLInt.serialize(false)).to.equal(0);
expect(GraphQLInt.serialize(true)).to.equal(1);

// The GraphQL specification does not allow serializing non-integer values
// as Int to avoid accidental data loss.
expect(() => GraphQLInt.serialize(0.1)).to.throw(
Expand Down Expand Up @@ -49,17 +58,19 @@ describe('Type System: Scalar coercion', () => {
expect(() => GraphQLInt.serialize('one')).to.throw(
'Int cannot represent non 32-bit signed integer value: one',
);
expect(GraphQLInt.serialize(false)).to.equal(0);
expect(GraphQLInt.serialize(true)).to.equal(1);
// Doesn't represent number
expect(() => GraphQLInt.serialize('')).to.throw(
'Int cannot represent non 32-bit signed integer value: (empty string)',
);
expect(() => GraphQLInt.serialize(NaN)).to.throw(
'Int cannot represent non 32-bit signed integer value: NaN',
);
expect(() => GraphQLInt.serialize([5])).to.throw(
'Int cannot represent an array value: [5]',
);
});

it('serializes output float', () => {
it('serializes output as Float', () => {
expect(GraphQLFloat.serialize(1)).to.equal(1.0);
expect(GraphQLFloat.serialize(0)).to.equal(0.0);
expect(GraphQLFloat.serialize('123.5')).to.equal(123.5);
Expand All @@ -74,30 +85,42 @@ describe('Type System: Scalar coercion', () => {
expect(() => GraphQLFloat.serialize(NaN)).to.throw(
'Float cannot represent non numeric value: NaN',
);

expect(() => GraphQLFloat.serialize('one')).to.throw(
'Float cannot represent non numeric value: one',
);

expect(() => GraphQLFloat.serialize('')).to.throw(
'Float cannot represent non numeric value: (empty string)',
);
expect(() => GraphQLFloat.serialize([5])).to.throw(
'Float cannot represent an array value: [5]',
);
});

it('serializes output strings', () => {
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');
});
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 boolean', () => {
expect(() => scalar.serialize([1])).to.throw(
'String cannot represent an array value: [1]',
);
});
}

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([false])).to.throw(
'Boolean cannot represent an array value: [false]',
);
});
});
33 changes: 26 additions & 7 deletions src/type/scalars.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ import { Kind } from '../language/kinds';
const MAX_INT = 2147483647;
const MIN_INT = -2147483648;

function coerceInt(value: mixed): ?number {
function coerceInt(value: mixed): number {
if (Array.isArray(value)) {
throw new TypeError(
`Int cannot represent an array value: [${String(value)}]`,
);
}
if (value === '') {
throw new TypeError(
'Int cannot represent non 32-bit signed integer value: (empty string)',
Expand Down Expand Up @@ -57,7 +62,12 @@ export const GraphQLInt = new GraphQLScalarType({
},
});

function coerceFloat(value: mixed): ?number {
function coerceFloat(value: mixed): number {
if (Array.isArray(value)) {
throw new TypeError(
`Float cannot represent an array value: [${String(value)}]`,
);
}
if (value === '') {
throw new TypeError(
'Float cannot represent non numeric value: (empty string)',
Expand Down Expand Up @@ -87,7 +97,7 @@ export const GraphQLFloat = new GraphQLScalarType({
},
});

function coerceString(value: mixed): ?string {
function coerceString(value: mixed): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting: technically the spec lets us coerce arrays into strings and booleans: https://github.com/facebook/graphql/blob/master/spec/Section%203%20--%20Type%20System.md#string

However it does NOT allow us to coerce arrays into floats or ints.

if (Array.isArray(value)) {
throw new TypeError(
`String cannot represent an array value: [${String(value)}]`,
Expand All @@ -109,11 +119,20 @@ export const GraphQLString = new GraphQLScalarType({
},
});

function coerceBoolean(value: mixed): boolean {
if (Array.isArray(value)) {
throw new TypeError(
`Boolean cannot represent an array value: [${String(value)}]`,
);
}
return Boolean(value);
}

export const GraphQLBoolean = new GraphQLScalarType({
name: 'Boolean',
description: 'The `Boolean` scalar type represents `true` or `false`.',
serialize: Boolean,
parseValue: Boolean,
serialize: coerceBoolean,
parseValue: coerceBoolean,
parseLiteral(ast) {
return ast.kind === Kind.BOOLEAN ? ast.value : undefined;
},
Expand All @@ -127,8 +146,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: String,
parseValue: String,
serialize: coerceString,
parseValue: coerceString,
parseLiteral(ast) {
return ast.kind === Kind.STRING || ast.kind === Kind.INT
? ast.value
Expand Down
Loading