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

NaN values should be checked inside scalar's serialize method #2438

Merged
merged 1 commit into from
Feb 8, 2020
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
8 changes: 3 additions & 5 deletions src/execution/execute.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import inspect from '../jsutils/inspect';
import memoize3 from '../jsutils/memoize3';
import invariant from '../jsutils/invariant';
import devAssert from '../jsutils/devAssert';
import isInvalid from '../jsutils/isInvalid';
import isNullish from '../jsutils/isNullish';
import isPromise from '../jsutils/isPromise';
import { type ObjMap } from '../jsutils/ObjMap';
import isObjectLike from '../jsutils/isObjectLike';
Expand Down Expand Up @@ -833,8 +831,8 @@ function completeValue(
return completed;
}

// If result value is null-ish (null, undefined, or NaN) then return null.
if (isNullish(result)) {
// If result value is null or undefined then return null.
if (result == null) {
return null;
}

Expand Down Expand Up @@ -940,7 +938,7 @@ function completeListValue(
*/
function completeLeafValue(returnType: GraphQLLeafType, result: mixed): mixed {
const serializedResult = returnType.serialize(result);
if (isInvalid(serializedResult)) {
if (serializedResult === undefined) {
throw new Error(
`Expected a value of type "${inspect(returnType)}" but ` +
`received: ${inspect(result)}`,
Expand Down
8 changes: 0 additions & 8 deletions src/jsutils/isInvalid.js

This file was deleted.

8 changes: 0 additions & 8 deletions src/jsutils/isNullish.js

This file was deleted.

10 changes: 7 additions & 3 deletions src/utilities/__tests__/astFromValue-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ describe('astFromValue', () => {

expect(astFromValue(undefined, GraphQLBoolean)).to.deep.equal(null);

expect(astFromValue(NaN, GraphQLInt)).to.deep.equal(null);

expect(astFromValue(null, GraphQLBoolean)).to.deep.equal({
kind: 'NullValue',
});
Expand Down Expand Up @@ -83,6 +81,10 @@ describe('astFromValue', () => {
expect(() => astFromValue(1e40, GraphQLInt)).to.throw(
'Int cannot represent non 32-bit signed integer value: 1e+40',
);

expect(() => astFromValue(NaN, GraphQLInt)).to.throw(
'Int cannot represent non-integer value: NaN',
);
});

it('converts Float values to Int/Float ASTs', () => {
Expand Down Expand Up @@ -205,7 +207,9 @@ describe('astFromValue', () => {
value: 'value',
});

expect(astFromValue(NaN, passthroughScalar)).to.equal(null);
expect(() => astFromValue(NaN, passthroughScalar)).to.throw(
'Cannot convert value to AST: NaN.',
);
expect(() => astFromValue(Infinity, passthroughScalar)).to.throw(
'Cannot convert value to AST: Infinity.',
);
Expand Down
2 changes: 2 additions & 0 deletions src/utilities/__tests__/valueFromASTUntyped-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ describe('valueFromASTUntyped', () => {
{ a: ['foo'] },
);
testCaseWithVars('$testVariable', { testVariable: null }, null);
testCaseWithVars('$testVariable', { testVariable: NaN }, NaN);
testCaseWithVars('$testVariable', {}, undefined);
testCaseWithVars('$testVariable', null, undefined);
});
});
8 changes: 3 additions & 5 deletions src/utilities/astFromValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import objectValues from '../polyfills/objectValues';

import inspect from '../jsutils/inspect';
import invariant from '../jsutils/invariant';
import isNullish from '../jsutils/isNullish';
import isInvalid from '../jsutils/isInvalid';
import isObjectLike from '../jsutils/isObjectLike';
import isCollection from '../jsutils/isCollection';

Expand Down Expand Up @@ -59,8 +57,8 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
return { kind: Kind.NULL };
}

// undefined, NaN
if (isInvalid(value)) {
// undefined
if (value === undefined) {
return null;
}

Expand Down Expand Up @@ -107,7 +105,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
// Since value is an internally represented value, it must be serialized
// to an externally represented value before converting into an AST.
const serialized = type.serialize(value);
if (isNullish(serialized)) {
if (serialized == null) {
return null;
}

Expand Down
11 changes: 5 additions & 6 deletions src/utilities/valueFromAST.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import objectValues from '../polyfills/objectValues';
import keyMap from '../jsutils/keyMap';
import inspect from '../jsutils/inspect';
import invariant from '../jsutils/invariant';
import isInvalid from '../jsutils/isInvalid';
import { type ObjMap } from '../jsutils/ObjMap';

import { Kind } from '../language/kinds';
Expand Down Expand Up @@ -52,7 +51,7 @@ export function valueFromAST(

if (valueNode.kind === Kind.VARIABLE) {
const variableName = valueNode.name.value;
if (!variables || isInvalid(variables[variableName])) {
if (variables == null || variables[variableName] === undefined) {
// No valid return value.
return;
}
Expand Down Expand Up @@ -92,7 +91,7 @@ export function valueFromAST(
coercedValues.push(null);
} else {
const itemValue = valueFromAST(itemNode, itemType, variables);
if (isInvalid(itemValue)) {
if (itemValue === undefined) {
return; // Invalid: intentionally return no value.
}
coercedValues.push(itemValue);
Expand All @@ -101,7 +100,7 @@ export function valueFromAST(
return coercedValues;
}
const coercedValue = valueFromAST(valueNode, itemType, variables);
if (isInvalid(coercedValue)) {
if (coercedValue === undefined) {
return; // Invalid: intentionally return no value.
}
return [coercedValue];
Expand All @@ -124,7 +123,7 @@ export function valueFromAST(
continue;
}
const fieldValue = valueFromAST(fieldNode.value, field.type, variables);
if (isInvalid(fieldValue)) {
if (fieldValue === undefined) {
return; // Invalid: intentionally return no value.
}
coercedObj[field.name] = fieldValue;
Expand Down Expand Up @@ -157,6 +156,6 @@ export function valueFromAST(
function isMissingVariable(valueNode, variables) {
return (
valueNode.kind === Kind.VARIABLE &&
(!variables || isInvalid(variables[valueNode.name.value]))
(variables == null || variables[valueNode.name.value] === undefined)
);
}
9 changes: 2 additions & 7 deletions src/utilities/valueFromASTUntyped.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import inspect from '../jsutils/inspect';
import invariant from '../jsutils/invariant';
import keyValMap from '../jsutils/keyValMap';
import isInvalid from '../jsutils/isInvalid';
import { type ObjMap } from '../jsutils/ObjMap';

import { Kind } from '../language/kinds';
Expand Down Expand Up @@ -48,12 +47,8 @@ export function valueFromASTUntyped(
field => field.name.value,
field => valueFromASTUntyped(field.value, variables),
);
case Kind.VARIABLE: {
const variableName = valueNode.name.value;
return variables && !isInvalid(variables[variableName])
? variables[variableName]
: undefined;
}
case Kind.VARIABLE:
return variables?.[valueNode.name.value];
}

// Not reachable. All possible value nodes have been considered.
Expand Down
3 changes: 1 addition & 2 deletions src/validation/rules/ValuesOfCorrectTypeRule.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import objectValues from '../../polyfills/objectValues';

import keyMap from '../../jsutils/keyMap';
import inspect from '../../jsutils/inspect';
import isInvalid from '../../jsutils/isInvalid';
import didYouMean from '../../jsutils/didYouMean';
import suggestionList from '../../jsutils/suggestionList';

Expand Down Expand Up @@ -130,7 +129,7 @@ function isValidValueNode(context: ValidationContext, node: ValueNode): void {
// may throw or return an invalid value to indicate failure.
try {
const parseResult = type.parseLiteral(node, undefined /* variables */);
if (isInvalid(parseResult)) {
if (parseResult === undefined) {
const typeStr = inspect(locationType);
context.reportError(
new GraphQLError(
Expand Down