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.

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

Fix 'astFromValue' to correctly handle integers and strings #1181

Merged
merged 1 commit into from
Jan 8, 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
86 changes: 40 additions & 46 deletions src/type/__tests__/introspection-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,7 @@ describe('Introspection', () => {
const TestInputObject = new GraphQLInputObjectType({
name: 'TestInputObject',
fields: {
a: { type: GraphQLString, defaultValue: 'foo' },
a: { type: GraphQLString, defaultValue: 'tes\t de\fault' },
b: { type: GraphQLList(GraphQLString) },
c: { type: GraphQLString, defaultValue: null },
},
Expand All @@ -839,15 +839,13 @@ describe('Introspection', () => {
const schema = new GraphQLSchema({ query: TestType });
const request = `
{
__schema {
types {
kind
__type(name: "TestInputObject") {
kind
name
inputFields {
name
inputFields {
name
type { ...TypeRef }
defaultValue
}
type { ...TypeRef }
defaultValue
}
}
}
Expand All @@ -870,46 +868,42 @@ describe('Introspection', () => {
}
`;

return expect(graphqlSync(schema, request)).to.containSubset({
return expect(graphqlSync(schema, request)).to.deep.equal({
data: {
__schema: {
types: [
__type: {
kind: 'INPUT_OBJECT',
name: 'TestInputObject',
inputFields: [
{
kind: 'INPUT_OBJECT',
name: 'TestInputObject',
inputFields: [
{
name: 'a',
type: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
defaultValue: '"foo"',
},
{
name: 'b',
type: {
kind: 'LIST',
name: null,
ofType: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
},
defaultValue: null,
},
{
name: 'c',
type: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
defaultValue: 'null',
name: 'a',
type: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
defaultValue: '"tes\\t de\\fault"',
},
{
name: 'b',
type: {
kind: 'LIST',
name: null,
ofType: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
],
},
defaultValue: null,
},
{
name: 'c',
type: {
kind: 'SCALAR',
name: 'String',
ofType: null,
},
defaultValue: 'null',
},
],
},
Expand Down
29 changes: 27 additions & 2 deletions src/utilities/__tests__/astFromValue-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ describe('astFromValue', () => {
});

it('converts Int values to Int ASTs', () => {
expect(astFromValue(-1, GraphQLInt)).to.deep.equal({
kind: 'IntValue',
value: '-1',
});

expect(astFromValue(123.0, GraphQLInt)).to.deep.equal({
kind: 'IntValue',
value: '123',
Expand All @@ -81,6 +86,11 @@ describe('astFromValue', () => {
});

it('converts Float values to Int/Float ASTs', () => {
expect(astFromValue(-1, GraphQLFloat)).to.deep.equal({
kind: 'IntValue',
value: '-1',
});

expect(astFromValue(123.0, GraphQLFloat)).to.deep.equal({
kind: 'IntValue',
value: '123',
Expand Down Expand Up @@ -115,7 +125,7 @@ describe('astFromValue', () => {

expect(astFromValue('VA\nLUE', GraphQLString)).to.deep.equal({
kind: 'StringValue',
value: 'VA\\nLUE',
value: 'VA\nLUE',
});

expect(astFromValue(123, GraphQLString)).to.deep.equal({
Expand Down Expand Up @@ -149,15 +159,30 @@ describe('astFromValue', () => {
// Note: EnumValues cannot contain non-identifier characters
expect(astFromValue('VA\nLUE', GraphQLID)).to.deep.equal({
kind: 'StringValue',
value: 'VA\\nLUE',
value: 'VA\nLUE',
});

// Note: IntValues are used when possible.
expect(astFromValue(-1, GraphQLID)).to.deep.equal({
kind: 'IntValue',
value: '-1',
});

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

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

expect(astFromValue('01', GraphQLID)).to.deep.equal({
kind: 'StringValue',
value: '01',
});

expect(astFromValue(false, GraphQLID)).to.deep.equal({
kind: 'StringValue',
value: 'false',
Expand Down
16 changes: 16 additions & 0 deletions src/utilities/__tests__/schemaPrinter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,22 @@ describe('Type System Printer', () => {
`);
});

it('Prints String Field With String Arg With Default', () => {
const output = printSingleFieldSchema({
type: GraphQLString,
args: { argOne: { type: GraphQLString, defaultValue: 'tes\t de\fault' } },
});
expect(output).to.equal(dedent`
schema {
query: Root
}

type Root {
singleField(argOne: String = "tes\t de\fault"): String
}
`);
});

it('Prints String Field With Int Arg With Default Null', () => {
const output = printSingleFieldSchema({
type: GraphQLString,
Expand Down
47 changes: 21 additions & 26 deletions src/utilities/astFromValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,7 @@ import { forEach, isCollection } from 'iterall';

import isNullish from '../jsutils/isNullish';
import isInvalid from '../jsutils/isInvalid';
import type {
ValueNode,
IntValueNode,
FloatValueNode,
StringValueNode,
BooleanValueNode,
NullValueNode,
EnumValueNode,
ListValueNode,
ObjectValueNode,
} from '../language/ast';
import type { ValueNode } from '../language/ast';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning up these flow types!

import * as Kind from '../language/kinds';
import type { GraphQLInputType } from '../type/definition';
import {
Expand Down Expand Up @@ -64,7 +54,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {

// only explicit null, not undefined, NaN
if (_value === null) {
return ({ kind: Kind.NULL }: NullValueNode);
return { kind: Kind.NULL };
}

// undefined, NaN
Expand All @@ -84,7 +74,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
valuesNodes.push(itemNode);
}
});
return ({ kind: Kind.LIST, values: valuesNodes }: ListValueNode);
return { kind: Kind.LIST, values: valuesNodes };
}
return astFromValue(_value, itemType);
}
Expand All @@ -108,7 +98,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
});
}
});
return ({ kind: Kind.OBJECT, fields: fieldNodes }: ObjectValueNode);
return { kind: Kind.OBJECT, fields: fieldNodes };
}

if (isScalarType(type) || isEnumType(type)) {
Expand All @@ -121,34 +111,32 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {

// Others serialize based on their corresponding JavaScript scalar types.
if (typeof serialized === 'boolean') {
return ({ kind: Kind.BOOLEAN, value: serialized }: BooleanValueNode);
return { kind: Kind.BOOLEAN, value: serialized };
}

// JavaScript numbers can be Int or Float values.
if (typeof serialized === 'number') {
const stringNum = String(serialized);
return /^[0-9]+$/.test(stringNum)
? ({ kind: Kind.INT, value: stringNum }: IntValueNode)
: ({ kind: Kind.FLOAT, value: stringNum }: FloatValueNode);
return integerStringRegExp.test(stringNum)
? { kind: Kind.INT, value: stringNum }
: { kind: Kind.FLOAT, value: stringNum };
}

if (typeof serialized === 'string') {
// Enum types use Enum literals.
if (isEnumType(type)) {
return ({ kind: Kind.ENUM, value: serialized }: EnumValueNode);
return { kind: Kind.ENUM, value: serialized };
}

// ID types can use Int literals.
if (type === GraphQLID && /^[0-9]+$/.test(serialized)) {
return ({ kind: Kind.INT, value: serialized }: IntValueNode);
if (type === GraphQLID && integerStringRegExp.test(serialized)) {
return { kind: Kind.INT, value: serialized };
}

// Use JSON stringify, which uses the same string encoding as GraphQL,
// then remove the quotes.
return ({
return {
kind: Kind.STRING,
value: JSON.stringify(serialized).slice(1, -1),
}: StringValueNode);
value: serialized,
};
}

throw new TypeError('Cannot convert value to AST: ' + String(serialized));
Expand All @@ -157,3 +145,10 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
/* istanbul ignore next */
throw new Error(`Unknown type: ${(type: empty)}.`);
}

/**
* IntValue:
* - NegativeSign? 0
* - NegativeSign? NonZeroDigit ( Digit+ )?
*/
const integerStringRegExp = /^-?(0|[1-9][0-9]*)$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better. Thanks for including the comment with the lexeme rule