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

astFromValue - JavaScript BigInt support #4088

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+89 −15
Open

astFromValue - JavaScript BigInt support #4088

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9284dab
JavaScript BigInt support
ardatan May 8, 2024
1781164
Add more tests
ardatan May 9, 2024
47f71f8
reduce diff, don't modify existing tests
yaacovCR Oct 13, 2024
e78d663
add a few tests for scalars
yaacovCR Oct 13, 2024
62f92fe
add failing test
yaacovCR Oct 13, 2024
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: 7 additions & 0 deletions src/jsutils/isInteger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export function isInteger(value: unknown): value is number | bigint {
const valueTypeOf = typeof value;
if (valueTypeOf === 'number') {
return Number.isInteger(value);
}
return valueTypeOf === 'bigint';
}
7 changes: 7 additions & 0 deletions src/jsutils/isNumeric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export function isNumeric(value: unknown): value is number | bigint {
const valueTypeOf = typeof value;
if (valueTypeOf === 'number') {
return Number.isFinite(value);
}
return valueTypeOf === 'bigint';
}
10 changes: 10 additions & 0 deletions src/type/__tests__/scalars-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ describe('Type System: Specified scalar types', () => {
expect(parseValue(1)).to.equal(1);
expect(parseValue(0)).to.equal(0);
expect(parseValue(-1)).to.equal(-1);
expect(parseValue(1n)).to.equal(1);

expect(() => parseValue(9876504321)).to.throw(
'Int cannot represent non 32-bit signed integer value: 9876504321',
Expand Down Expand Up @@ -119,6 +120,7 @@ describe('Type System: Specified scalar types', () => {
expect(serialize(1e5)).to.equal(100000);
expect(serialize(false)).to.equal(0);
expect(serialize(true)).to.equal(1);
expect(serialize(1n)).to.equal(1);

const customValueOfObj = {
value: 5,
Expand Down Expand Up @@ -280,6 +282,7 @@ describe('Type System: Specified scalar types', () => {
expect(serialize('-1.1')).to.equal(-1.1);
expect(serialize(false)).to.equal(0.0);
expect(serialize(true)).to.equal(1.0);
expect(serialize(1n)).to.equal(1n);

const customValueOfObj = {
value: 5.5,
Expand Down Expand Up @@ -380,6 +383,7 @@ describe('Type System: Specified scalar types', () => {
expect(serialize(-1.1)).to.equal('-1.1');
expect(serialize(true)).to.equal('true');
expect(serialize(false)).to.equal('false');
expect(serialize(9007199254740993n)).to.equal('9007199254740993');

const valueOf = () => 'valueOf string';
const toJSON = () => 'toJSON string';
Expand Down Expand Up @@ -493,6 +497,8 @@ describe('Type System: Specified scalar types', () => {

expect(serialize(1)).to.equal(true);
expect(serialize(0)).to.equal(false);
expect(serialize(1n)).to.equal(true);
expect(serialize(0n)).to.equal(false);
expect(serialize(true)).to.equal(true);
expect(serialize(false)).to.equal(false);
expect(
Expand Down Expand Up @@ -539,6 +545,9 @@ describe('Type System: Specified scalar types', () => {
expect(parseValue(9007199254740991)).to.equal('9007199254740991');
expect(parseValue(-9007199254740991)).to.equal('-9007199254740991');

// Can handle bigint in JS
expect(parseValue(9007199254740993n)).to.equal('9007199254740993');

expect(() => parseValue(undefined)).to.throw(
'ID cannot represent value: undefined',
);
Expand Down Expand Up @@ -614,6 +623,7 @@ describe('Type System: Specified scalar types', () => {
expect(serialize(123)).to.equal('123');
expect(serialize(0)).to.equal('0');
expect(serialize(-1)).to.equal('-1');
expect(serialize(9007199254740993n)).to.equal('9007199254740993');

const valueOf = () => 'valueOf ID';
const toJSON = () => 'toJSON ID';
Expand Down
34 changes: 19 additions & 15 deletions src/type/scalars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { inspect } from '../jsutils/inspect.js';
import { isInteger } from '../jsutils/isInteger.js';
import { isNumeric } from '../jsutils/isNumeric.js';
import { isObjectLike } from '../jsutils/isObjectLike.js';

import { GraphQLError } from '../error/GraphQLError.js';
Expand Down Expand Up @@ -40,7 +42,7 @@ export const GraphQLInt = new GraphQLScalarType<number>({
num = Number(coercedValue);
}

if (typeof num !== 'number' || !Number.isInteger(num)) {
if (!isInteger(num)) {
throw new GraphQLError(
`Int cannot represent non-integer value: ${inspect(coercedValue)}`,
);
Expand All @@ -51,21 +53,22 @@ export const GraphQLInt = new GraphQLScalarType<number>({
inspect(coercedValue),
);
}
return num;
return Number(num);
},

parseValue(inputValue) {
if (typeof inputValue !== 'number' || !Number.isInteger(inputValue)) {
if (!isInteger(inputValue)) {
throw new GraphQLError(
`Int cannot represent non-integer value: ${inspect(inputValue)}`,
);
}
if (inputValue > GRAPHQL_MAX_INT || inputValue < GRAPHQL_MIN_INT) {
const coercedVal = Number(inputValue);
if (coercedVal > GRAPHQL_MAX_INT || coercedVal < GRAPHQL_MIN_INT) {
throw new GraphQLError(
`Int cannot represent non 32-bit signed integer value: ${inputValue}`,
);
}
return inputValue;
return coercedVal;
},

parseConstLiteral(valueNode) {
Expand Down Expand Up @@ -96,7 +99,7 @@ export const GraphQLInt = new GraphQLScalarType<number>({
},
});

export const GraphQLFloat = new GraphQLScalarType<number>({
export const GraphQLFloat = new GraphQLScalarType<number | bigint>({
name: 'Float',
description:
'The `Float` scalar type represents signed double-precision fractional values as specified by [IEEE 754](https://en.wikipedia.org/wiki/IEEE_floating_point).',
Expand All @@ -113,16 +116,17 @@ export const GraphQLFloat = new GraphQLScalarType<number>({
num = Number(coercedValue);
}

if (typeof num !== 'number' || !Number.isFinite(num)) {
if (!isNumeric(num)) {
throw new GraphQLError(
`Float cannot represent non numeric value: ${inspect(coercedValue)}`,
);
}

return num;
},

parseValue(inputValue) {
if (typeof inputValue !== 'number' || !Number.isFinite(inputValue)) {
if (!isNumeric(inputValue)) {
throw new GraphQLError(
`Float cannot represent non numeric value: ${inspect(inputValue)}`,
);
Expand Down Expand Up @@ -163,8 +167,8 @@ export const GraphQLString = new GraphQLScalarType<string>({
if (typeof coercedValue === 'boolean') {
return coercedValue ? 'true' : 'false';
}
if (typeof coercedValue === 'number' && Number.isFinite(coercedValue)) {
return coercedValue.toString();
if (isNumeric(coercedValue)) {
return String(coercedValue);
}
throw new GraphQLError(
`String cannot represent value: ${inspect(outputValue)}`,
Expand Down Expand Up @@ -207,8 +211,8 @@ export const GraphQLBoolean = new GraphQLScalarType<boolean>({
if (typeof coercedValue === 'boolean') {
return coercedValue;
}
if (Number.isFinite(coercedValue)) {
return coercedValue !== 0;
if (isNumeric(coercedValue)) {
return Number(coercedValue) !== 0;
}
throw new GraphQLError(
`Boolean cannot represent a non boolean value: ${inspect(coercedValue)}`,
Expand Down Expand Up @@ -252,7 +256,7 @@ export const GraphQLID = new GraphQLScalarType<string>({
if (typeof coercedValue === 'string') {
return coercedValue;
}
if (Number.isInteger(coercedValue)) {
if (isInteger(coercedValue)) {
return String(coercedValue);
}
throw new GraphQLError(
Expand All @@ -264,8 +268,8 @@ export const GraphQLID = new GraphQLScalarType<string>({
if (typeof inputValue === 'string') {
return inputValue;
}
if (typeof inputValue === 'number' && Number.isInteger(inputValue)) {
return inputValue.toString();
if (isInteger(inputValue)) {
return String(inputValue);
}
throw new GraphQLError(`ID cannot represent value: ${inspect(inputValue)}`);
},
Expand Down
40 changes: 40 additions & 0 deletions src/utilities/__tests__/astFromValue-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ describe('astFromValue', () => {
value: true,
});

expect(astFromValue(0n, GraphQLBoolean)).to.deep.equal({
kind: 'BooleanValue',
value: false,
});

expect(astFromValue(1n, GraphQLBoolean)).to.deep.equal({
kind: 'BooleanValue',
value: true,
});

const NonNullBoolean = new GraphQLNonNull(GraphQLBoolean);
expect(astFromValue(0, NonNullBoolean)).to.deep.equal({
kind: 'BooleanValue',
Expand All @@ -69,6 +79,11 @@ describe('astFromValue', () => {
value: '10000',
});

expect(astFromValue(1n, GraphQLInt)).to.deep.equal({
saihaj marked this conversation as resolved.
Show resolved Hide resolved
kind: 'IntValue',
value: '1',
});

// GraphQL spec does not allow coercing non-integer values to Int to avoid
// accidental data loss.
expect(() => astFromValue(123.5, GraphQLInt)).to.throw(
Expand All @@ -80,6 +95,16 @@ describe('astFromValue', () => {
'Int cannot represent non 32-bit signed integer value: 1e+40',
);

// Note: outside the bounds of 32bit signed int.
expect(() => astFromValue(9007199254740991, GraphQLInt)).to.throw(
'Int cannot represent non 32-bit signed integer value: 9007199254740991',
);

// Note: outside the bounds of 32bit signed int as BigInt.
expect(() => astFromValue(9007199254740991n, GraphQLInt)).to.throw(
'Int cannot represent non 32-bit signed integer value: 9007199254740991',
);

expect(() => astFromValue(NaN, GraphQLInt)).to.throw(
'Int cannot represent non-integer value: NaN',
);
Expand All @@ -96,6 +121,11 @@ describe('astFromValue', () => {
value: '123',
});

expect(astFromValue(9007199254740993n, GraphQLFloat)).to.deep.equal({
Copy link
Contributor

Choose a reason for hiding this comment

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

The change in type of this test is because astFromValue is not safe, set to be replaced by valueToLiteral() for all remaining uses in #3814

But a bigint of sufficient size, -- as far as I know -- cannot be converted to a JS float.

We also in this PR do not properly handle GraphQLFloat scalar parsing and serialization of small BigInts, I will push a failing test to demonstrate what I mean.

We also probably should update the logic within the new valueToLiteral() utility. It's possible that overall we can just delay landing this PR until after #3814

kind: 'IntValue',
value: '9007199254740993',
});

expect(astFromValue(123.5, GraphQLFloat)).to.deep.equal({
kind: 'FloatValue',
value: '123.5',
Expand Down Expand Up @@ -133,6 +163,11 @@ describe('astFromValue', () => {
value: '123',
});

expect(astFromValue(9007199254740993n, GraphQLString)).to.deep.equal({
kind: 'StringValue',
value: '9007199254740993',
});

expect(astFromValue(false, GraphQLString)).to.deep.equal({
kind: 'StringValue',
value: 'false',
Expand Down Expand Up @@ -183,6 +218,11 @@ describe('astFromValue', () => {
value: '01',
});

expect(astFromValue(9007199254740993n, GraphQLID)).to.deep.equal({
kind: 'IntValue',
value: '9007199254740993',
});

expect(() => astFromValue(false, GraphQLID)).to.throw(
'ID cannot represent value: false',
);
Expand Down
5 changes: 5 additions & 0 deletions src/utilities/astFromValue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ export function astFromValue(
: { kind: Kind.FLOAT, value: stringNum };
}

if (typeof serialized === 'bigint') {
const stringNum = String(serialized);
return { kind: Kind.INT, value: stringNum };
}

if (typeof serialized === 'string') {
// Enum types use Enum literals.
if (isEnumType(type)) {
Expand Down
Loading