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

Preserve sources of variable values #3077

Closed
wants to merge 1 commit into from
Closed
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
13 changes: 7 additions & 6 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
import { typeFromAST } from '../utilities/typeFromAST';
import { getOperationRootType } from '../utilities/getOperationRootType';

import type { VariableValues } from './values';
import {
getVariableValues,
getArgumentValues,
Expand Down Expand Up @@ -99,7 +100,7 @@ export interface ExecutionContext {
rootValue: unknown;
contextValue: unknown;
operation: OperationDefinitionNode;
variableValues: { [variable: string]: unknown };
variableValues: VariableValues;
fieldResolver: GraphQLFieldResolver<any, any>;
typeResolver: GraphQLTypeResolver<any, any>;
errors: Array<GraphQLError>;
Expand Down Expand Up @@ -302,15 +303,15 @@ export function buildExecutionContext(
// istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2203')
const variableDefinitions = operation.variableDefinitions ?? [];

const coercedVariableValues = getVariableValues(
const variableValuesOrErrors = getVariableValues(
schema,
variableDefinitions,
rawVariableValues ?? {},
{ maxErrors: 50 },
);

if (coercedVariableValues.errors) {
return coercedVariableValues.errors;
if (variableValuesOrErrors.errors) {
return variableValuesOrErrors.errors;
}

return {
Expand All @@ -319,7 +320,7 @@ export function buildExecutionContext(
rootValue,
contextValue,
operation,
variableValues: coercedVariableValues.coerced,
variableValues: variableValuesOrErrors.variableValues,
fieldResolver: fieldResolver ?? defaultFieldResolver,
typeResolver: typeResolver ?? defaultTypeResolver,
errors: [],
Expand Down Expand Up @@ -682,7 +683,7 @@ export function buildResolveInfo(
fragments: exeContext.fragments,
rootValue: exeContext.rootValue,
operation: exeContext.operation,
variableValues: exeContext.variableValues,
variableValues: exeContext.variableValues.coerced,
};
}

Expand Down
60 changes: 38 additions & 22 deletions src/execution/values.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ObjMap } from '../jsutils/ObjMap';
import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap';
import type { Maybe } from '../jsutils/Maybe';
import { keyMap } from '../jsutils/keyMap';
import { inspect } from '../jsutils/inspect';
Expand All @@ -15,7 +15,7 @@ import { Kind } from '../language/kinds';
import { print } from '../language/printer';

import type { GraphQLSchema } from '../type/schema';
import type { GraphQLField } from '../type/definition';
import type { GraphQLInputType, GraphQLField } from '../type/definition';
import type { GraphQLDirective } from '../type/directives';
import { isInputType, isNonNullType } from '../type/definition';

Expand All @@ -26,9 +26,20 @@ import {
coerceDefaultValue,
} from '../utilities/coerceInputValue';

type CoercedVariableValues =
| { errors: ReadonlyArray<GraphQLError>; coerced?: never }
| { coerced: { [variable: string]: unknown }; errors?: never };
export interface VariableValues {
readonly sources: ReadOnlyObjMap<VariableValueSource>;
readonly coerced: ReadOnlyObjMap<unknown>;
}

interface VariableValueSource {
readonly variable: VariableDefinitionNode;
readonly type: GraphQLInputType;
readonly value: unknown;
}

type VariableValuesOrErrors =
| { variableValues: VariableValues; errors?: never }
| { errors: ReadonlyArray<GraphQLError>; variableValues?: never };

/**
* Prepares an object map of variableValues of the correct type based on the
Expand All @@ -46,11 +57,11 @@ export function getVariableValues(
varDefNodes: ReadonlyArray<VariableDefinitionNode>,
inputs: { readonly [variable: string]: unknown },
options?: { maxErrors?: number },
): CoercedVariableValues {
const errors = [];
): VariableValuesOrErrors {
const errors: Array<GraphQLError> = [];
const maxErrors = options?.maxErrors;
try {
const coerced = coerceVariableValues(
const variableValues = coerceVariableValues(
schema,
varDefNodes,
inputs,
Expand All @@ -65,7 +76,7 @@ export function getVariableValues(
);

if (errors.length === 0) {
return { coerced };
return { variableValues };
}
} catch (error) {
errors.push(error);
Expand All @@ -79,8 +90,9 @@ function coerceVariableValues(
varDefNodes: ReadonlyArray<VariableDefinitionNode>,
inputs: { readonly [variable: string]: unknown },
onError: (error: GraphQLError) => void,
): { [variable: string]: unknown } {
const coercedValues: { [variable: string]: unknown } = {};
): VariableValues {
const sources: ObjMap<VariableValueSource> = Object.create(null);
const coerced: ObjMap<unknown> = Object.create(null);
for (const varDefNode of varDefNodes) {
const varName = varDefNode.variable.name.value;
const varType = typeFromAST(schema, varDefNode.type);
Expand All @@ -98,11 +110,14 @@ function coerceVariableValues(
}

if (!hasOwnProperty(inputs, varName)) {
if (varDefNode.defaultValue) {
coercedValues[varName] = coerceInputLiteral(
varDefNode.defaultValue,
varType,
);
const defaultValue = varDefNode.defaultValue;
if (defaultValue) {
sources[varName] = {
variable: varDefNode,
type: varType,
value: undefined,
};
coerced[varName] = coerceInputLiteral(defaultValue, varType);
} else if (isNonNullType(varType)) {
onError(
new GraphQLError(
Expand All @@ -125,7 +140,8 @@ function coerceVariableValues(
continue;
}

coercedValues[varName] = coerceInputValue(
sources[varName] = { variable: varDefNode, type: varType, value };
coerced[varName] = coerceInputValue(
value,
varType,
(path, invalidValue, error) => {
Expand All @@ -148,7 +164,7 @@ function coerceVariableValues(
);
}

return coercedValues;
return { sources, coerced };
}

/**
Expand All @@ -164,7 +180,7 @@ function coerceVariableValues(
export function getArgumentValues(
def: GraphQLField<unknown, unknown> | GraphQLDirective,
node: FieldNode | DirectiveNode,
variableValues?: Maybe<ObjMap<unknown>>,
variableValues?: Maybe<VariableValues>,
): { [argument: string]: unknown } {
const coercedValues: { [argument: string]: unknown } = {};

Expand Down Expand Up @@ -199,7 +215,7 @@ export function getArgumentValues(
const variableName = valueNode.name.value;
if (
variableValues == null ||
!hasOwnProperty(variableValues, variableName)
variableValues.coerced[variableName] === undefined
) {
if (argDef.defaultValue) {
coercedValues[name] = coerceDefaultValue(
Expand All @@ -215,7 +231,7 @@ export function getArgumentValues(
}
continue;
}
isNull = variableValues[variableName] == null;
isNull = variableValues.coerced[variableName] == null;
}

if (isNull && isNonNullType(argType)) {
Expand Down Expand Up @@ -256,7 +272,7 @@ export function getArgumentValues(
export function getDirectiveValues(
directiveDef: GraphQLDirective,
node: { readonly directives?: ReadonlyArray<DirectiveNode> },
variableValues?: Maybe<ObjMap<unknown>>,
variableValues?: Maybe<VariableValues>,
): undefined | { [argument: string]: unknown } {
// istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2203')
const directiveNode = node.directives?.find(
Expand Down
84 changes: 68 additions & 16 deletions src/utilities/__tests__/coerceInputValue-test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';

import type { ObjMap } from '../../jsutils/ObjMap';
import type { ReadOnlyObjMap } from '../../jsutils/ObjMap';
import { invariant } from '../../jsutils/invariant';
import { identityFunc } from '../../jsutils/identityFunc';

import { print } from '../../language/printer';
import { parseValue } from '../../language/parser';
import { parseValue, Parser } from '../../language/parser';

import type { GraphQLInputType } from '../../type/definition';
import {
Expand All @@ -23,6 +23,10 @@ import {
GraphQLScalarType,
GraphQLInputObjectType,
} from '../../type/definition';
import { GraphQLSchema } from '../../type/schema';

import type { VariableValues } from '../../execution/values';
import { getVariableValues } from '../../execution/values';

import {
coerceInputValue,
Expand Down Expand Up @@ -450,20 +454,29 @@ describe('coerceInputLiteral', () => {
valueText: string,
type: GraphQLInputType,
expected: unknown,
variables?: ObjMap<unknown>,
variableValues?: VariableValues,
) {
const ast = parseValue(valueText);
const value = coerceInputLiteral(ast, type, variables);
const value = coerceInputLiteral(ast, type, variableValues);
expect(value).to.deep.equal(expected);
}

function testWithVariables(
variables: ObjMap<unknown>,
variableDefs: string,
inputs: ReadOnlyObjMap<unknown>,
valueText: string,
type: GraphQLInputType,
expected: unknown,
) {
test(valueText, type, expected, variables);
const parser = new Parser(variableDefs);
parser.expectToken('<SOF>');
const variableValuesOrErrors = getVariableValues(
new GraphQLSchema({}),
parser.parseVariableDefinitions(),
inputs,
);
invariant(variableValuesOrErrors.variableValues);
test(valueText, type, expected, variableValuesOrErrors.variableValues);
}

it('converts according to input coercion rules', () => {
Expand Down Expand Up @@ -662,19 +675,55 @@ describe('coerceInputLiteral', () => {

it('accepts variable values assuming already coerced', () => {
test('$var', GraphQLBoolean, undefined);
testWithVariables({ var: true }, '$var', GraphQLBoolean, true);
testWithVariables({ var: null }, '$var', GraphQLBoolean, null);
testWithVariables({ var: null }, '$var', nonNullBool, undefined);
testWithVariables(
'($var: Boolean)',
{ var: true },
'$var',
GraphQLBoolean,
true,
);
testWithVariables(
'($var: Boolean)',
{ var: null },
'$var',
GraphQLBoolean,
null,
);
testWithVariables(
'($var: Boolean)',
{ var: null },
'$var',
nonNullBool,
undefined,
);
});

it('asserts variables are provided as items in lists', () => {
test('[ $foo ]', listOfBool, [null]);
test('[ $foo ]', listOfNonNullBool, undefined);
testWithVariables({ foo: true }, '[ $foo ]', listOfNonNullBool, [true]);
testWithVariables(
'($foo: Boolean)',
{ foo: true },
'[ $foo ]',
listOfNonNullBool,
[true],
);
// Note: variables are expected to have already been coerced, so we
// do not expect the singleton wrapping behavior for variables.
testWithVariables({ foo: true }, '$foo', listOfNonNullBool, true);
testWithVariables({ foo: [true] }, '$foo', listOfNonNullBool, [true]);
testWithVariables(
'($foo: Boolean)',
{ foo: true },
'$foo',
listOfNonNullBool,
true,
);
testWithVariables(
'($foo: [Boolean])',
{ foo: [true] },
'$foo',
listOfNonNullBool,
[true],
);
});

it('omits input object fields for unprovided variables', () => {
Expand All @@ -683,10 +732,13 @@ describe('coerceInputLiteral', () => {
requiredBool: true,
});
test('{ requiredBool: $foo }', testInputObj, undefined);
testWithVariables({ foo: true }, '{ requiredBool: $foo }', testInputObj, {
int: 42,
requiredBool: true,
});
testWithVariables(
'($foo: Boolean)',
{ foo: true },
'{ requiredBool: $foo }',
testInputObj,
{ int: 42, requiredBool: true },
);
});
});

Expand Down
Loading