From 6cecfdd9af5cf9cf9554210378fefb521e0e9dd3 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Tue, 11 May 2021 15:06:20 -0700 Subject: [PATCH] Preserve sources of variable values Depends on #3074 By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`. While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review --- src/execution/execute.d.ts | 4 +- src/execution/execute.js | 5 +- src/execution/values.d.ts | 19 +++++-- src/execution/values.js | 46 ++++++++++----- src/type/definition.d.ts | 8 +-- src/type/definition.js | 9 ++- src/utilities/__tests__/valueFromAST-test.js | 59 +++++++++++++------- src/utilities/valueFromAST.d.ts | 5 +- src/utilities/valueFromAST.js | 15 ++--- src/utilities/valueFromASTUntyped.d.ts | 4 +- src/utilities/valueFromASTUntyped.js | 4 +- 11 files changed, 116 insertions(+), 62 deletions(-) diff --git a/src/execution/execute.d.ts b/src/execution/execute.d.ts index 1db6f20b06..7386a31b36 100644 --- a/src/execution/execute.d.ts +++ b/src/execution/execute.d.ts @@ -23,6 +23,8 @@ import { GraphQLObjectType, } from '../type/definition'; +import { VariableValues } from './values'; + /** * Terminology * @@ -55,7 +57,7 @@ export interface ExecutionContext { contextValue: unknown; fragments: ObjMap; operation: OperationDefinitionNode; - variableValues: { [key: string]: unknown }; + variableValues: VariableValues; fieldResolver: GraphQLFieldResolver; typeResolver: GraphQLTypeResolver; errors: Array; diff --git a/src/execution/execute.js b/src/execution/execute.js index f3a88c8c0f..5fd064df21 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -60,6 +60,7 @@ import { import { typeFromAST } from '../utilities/typeFromAST'; import { getOperationRootType } from '../utilities/getOperationRootType'; +import type { VariableValues } from './values'; import { getVariableValues, getArgumentValues, @@ -98,7 +99,7 @@ export type ExecutionContext = {| rootValue: mixed, contextValue: mixed, operation: OperationDefinitionNode, - variableValues: { [variable: string]: mixed, ... }, + variableValues: VariableValues, fieldResolver: GraphQLFieldResolver, typeResolver: GraphQLTypeResolver, errors: Array, @@ -679,7 +680,7 @@ export function buildResolveInfo( fragments: exeContext.fragments, rootValue: exeContext.rootValue, operation: exeContext.operation, - variableValues: exeContext.variableValues, + variableValues: exeContext.variableValues.coerced, }; } diff --git a/src/execution/values.d.ts b/src/execution/values.d.ts index b88943a04d..32b6e901d7 100644 --- a/src/execution/values.d.ts +++ b/src/execution/values.d.ts @@ -1,5 +1,5 @@ import { Maybe } from '../jsutils/Maybe'; -import { ObjMap } from '../jsutils/ObjMap'; +import { ReadOnlyObjMap } from '../jsutils/ObjMap'; import { GraphQLError } from '../error/GraphQLError'; import { @@ -10,11 +10,20 @@ import { import { GraphQLDirective } from '../type/directives'; import { GraphQLSchema } from '../type/schema'; -import { GraphQLField } from '../type/definition'; +import { GraphQLField, GraphQLInputType } from '../type/definition'; + +export type VariableValues = { + readonly sources: ReadOnlyObjMap<{ + readonly variable: VariableDefinitionNode; + readonly type: GraphQLInputType; + readonly value: unknown; + }>; + readonly coerced: ReadOnlyObjMap; +}; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } - | { errors?: never; coerced: { [key: string]: unknown } }; + | { errors?: never; coerced: VariableValues }; /** * Prepares an object map of variableValues of the correct type based on the @@ -43,7 +52,7 @@ export function getVariableValues( export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - variableValues?: Maybe>, + variableValues?: Maybe, ): { [key: string]: unknown }; /** @@ -62,5 +71,5 @@ export function getDirectiveValues( node: { readonly directives?: ReadonlyArray; }, - variableValues?: Maybe>, + variableValues?: Maybe, ): undefined | { [key: string]: unknown }; diff --git a/src/execution/values.js b/src/execution/values.js index 173f04c65a..e04139247d 100644 --- a/src/execution/values.js +++ b/src/execution/values.js @@ -1,6 +1,6 @@ -import type { ObjMap } from '../jsutils/ObjMap'; -import { keyMap } from '../jsutils/keyMap'; +import type { ReadOnlyObjMap, ReadOnlyObjMapLike } from '../jsutils/ObjMap'; import { inspect } from '../jsutils/inspect'; +import { keyMap } from '../jsutils/keyMap'; import { printPathArray } from '../jsutils/printPathArray'; import { GraphQLError } from '../error/GraphQLError'; @@ -14,7 +14,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'; import { getCoercedDefaultValue } from '../type/defaultValues'; @@ -23,9 +23,18 @@ import { typeFromAST } from '../utilities/typeFromAST'; import { valueFromAST } from '../utilities/valueFromAST'; import { coerceInputValue } from '../utilities/coerceInputValue'; +export type VariableValues = {| + +sources: ReadOnlyObjMap<{| + +variable: VariableDefinitionNode, + +type: GraphQLInputType, + +value: mixed, + |}>, + +coerced: ReadOnlyObjMap, +|}; + type CoercedVariableValues = | {| errors: $ReadOnlyArray |} - | {| coerced: { [variable: string]: mixed, ... } |}; + | {| coerced: VariableValues |}; /** * Prepares an object map of variableValues of the correct type based on the @@ -41,7 +50,7 @@ type CoercedVariableValues = export function getVariableValues( schema: GraphQLSchema, varDefNodes: $ReadOnlyArray, - inputs: { +[variable: string]: mixed, ... }, + inputs: ReadOnlyObjMapLike, options?: {| maxErrors?: number |}, ): CoercedVariableValues { const errors = []; @@ -74,10 +83,11 @@ export function getVariableValues( function coerceVariableValues( schema: GraphQLSchema, varDefNodes: $ReadOnlyArray, - inputs: { +[variable: string]: mixed, ... }, + inputs: ReadOnlyObjMapLike, onError: (error: GraphQLError) => void, -): { [variable: string]: mixed, ... } { - const coercedValues = {}; +): VariableValues { + const sources = Object.create(null); + const coerced = Object.create(null); for (const varDefNode of varDefNodes) { const varName = varDefNode.variable.name.value; const varType = typeFromAST(schema, varDefNode.type); @@ -96,7 +106,12 @@ function coerceVariableValues( if (!hasOwnProperty(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + sources[varName] = { + variable: varDefNode, + type: varType, + value: undefined, + }; + coerced[varName] = valueFromAST(varDefNode.defaultValue, varType); } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -121,7 +136,8 @@ function coerceVariableValues( continue; } - coercedValues[varName] = coerceInputValue( + sources[varName] = { variable: varDefNode, type: varType, value }; + coerced[varName] = coerceInputValue( value, varType, (path, invalidValue, error) => { @@ -144,7 +160,7 @@ function coerceVariableValues( ); } - return coercedValues; + return { sources, coerced }; } /** @@ -160,7 +176,7 @@ function coerceVariableValues( export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, - variableValues?: ?ObjMap, + variableValues?: ?VariableValues, ): { [argument: string]: mixed, ... } { const coercedValues = {}; @@ -196,7 +212,7 @@ export function getArgumentValues( const variableName = valueNode.name.value; if ( variableValues == null || - !hasOwnProperty(variableValues, variableName) + variableValues.coerced[variableName] === undefined ) { if (argDef.defaultValue) { coercedValues[name] = getCoercedDefaultValue( @@ -212,7 +228,7 @@ export function getArgumentValues( } continue; } - isNull = variableValues[variableName] == null; + isNull = variableValues.coerced[variableName] == null; } if (isNull && isNonNullType(argType)) { @@ -252,7 +268,7 @@ export function getArgumentValues( export function getDirectiveValues( directiveDef: GraphQLDirective, node: { +directives?: $ReadOnlyArray, ... }, - variableValues?: ?ObjMap, + variableValues?: ?VariableValues, ): void | { [argument: string]: mixed, ... } { // istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2203') const directiveNode = node.directives?.find( diff --git a/src/type/definition.d.ts b/src/type/definition.d.ts index 0acd9b3d4e..0caa767531 100644 --- a/src/type/definition.d.ts +++ b/src/type/definition.d.ts @@ -5,7 +5,7 @@ import { Maybe } from '../jsutils/Maybe'; import { PromiseOrValue } from '../jsutils/PromiseOrValue'; import { Path } from '../jsutils/Path'; -import { ObjMap } from '../jsutils/ObjMap'; +import { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap'; import { ScalarTypeDefinitionNode, @@ -345,7 +345,7 @@ export type GraphQLScalarValueParser = ( ) => Maybe; export type GraphQLScalarLiteralParser = ( valueNode: ValueNode, - variables: Maybe>, + variables: Maybe>, ) => Maybe; export interface GraphQLScalarTypeConfig { @@ -487,7 +487,7 @@ export interface GraphQLResolveInfo { readonly fragments: ObjMap; readonly rootValue: unknown; readonly operation: OperationDefinitionNode; - readonly variableValues: { [variableName: string]: unknown }; + readonly variableValues: ReadOnlyObjMap; } /** @@ -790,7 +790,7 @@ export class GraphQLEnumType { parseValue(value: unknown): Maybe; parseLiteral( valueNode: ValueNode, - _variables: Maybe>, + _variables: Maybe>, ): Maybe; toConfig(): GraphQLEnumTypeConfig & { diff --git a/src/type/definition.js b/src/type/definition.js index 226ef414d0..6fee6b6948 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -641,7 +641,7 @@ export type GraphQLScalarValueParser = ( export type GraphQLScalarLiteralParser = ( valueNode: ValueNode, - variables: ?ObjMap, + variables: ?ReadOnlyObjMap, ) => ?TInternal; export type GraphQLScalarTypeConfig = {| @@ -909,7 +909,7 @@ export type GraphQLResolveInfo = {| +fragments: ObjMap, +rootValue: mixed, +operation: OperationDefinitionNode, - +variableValues: { [variable: string]: mixed, ... }, + +variableValues: ReadOnlyObjMap, |}; export type GraphQLFieldConfig< @@ -1349,7 +1349,10 @@ export class GraphQLEnumType /* */ { return enumValue.value; } - parseLiteral(valueNode: ValueNode, _variables: ?ObjMap): ?any /* T */ { + parseLiteral( + valueNode: ValueNode, + _variables: ?ReadOnlyObjMap, + ): ?any /* T */ { // Note: variables will be resolved to a value before calling this function. if (valueNode.kind !== Kind.ENUM) { const valueStr = print(valueNode); diff --git a/src/utilities/__tests__/valueFromAST-test.js b/src/utilities/__tests__/valueFromAST-test.js index 187472d0d6..b1d092c2ae 100644 --- a/src/utilities/__tests__/valueFromAST-test.js +++ b/src/utilities/__tests__/valueFromAST-test.js @@ -1,11 +1,11 @@ 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 { parseValue } from '../../language/parser'; +import { parseValue, Parser } from '../../language/parser'; import type { GraphQLInputType } from '../../type/definition'; import { @@ -22,6 +22,9 @@ import { GraphQLEnumType, GraphQLInputObjectType, } from '../../type/definition'; +import { GraphQLSchema } from '../../type/schema'; + +import { getVariableValues } from '../../execution/values'; import { valueFromAST } from '../valueFromAST'; @@ -29,9 +32,22 @@ describe('valueFromAST', () => { function expectValueFrom( valueText: string, type: GraphQLInputType, - variables?: ObjMap, + variableDefs?: string, + variableValues?: ReadOnlyObjMap, ) { const ast = parseValue(valueText); + let variables; + if (variableValues && variableDefs !== undefined) { + const parser = new Parser(variableDefs); + parser.expectToken(''); + const coercedVariables = getVariableValues( + new GraphQLSchema({}), + parser.parseVariableDefinitions(), + variableValues, + ); + invariant(coercedVariables.coerced); + variables = coercedVariables.coerced; + } const value = valueFromAST(ast, type, variables); return expect(value); } @@ -239,38 +255,43 @@ describe('valueFromAST', () => { }); it('accepts variable values assuming already coerced', () => { - expectValueFrom('$var', GraphQLBoolean, {}).to.equal(undefined); - expectValueFrom('$var', GraphQLBoolean, { var: true }).to.equal(true); - expectValueFrom('$var', GraphQLBoolean, { var: null }).to.equal(null); - expectValueFrom('$var', nonNullBool, { var: null }).to.equal(undefined); + expectValueFrom('$var', GraphQLBoolean).to.equal(undefined); + expectValueFrom('$var', GraphQLBoolean, '($var: Boolean)', { + var: true, + }).to.equal(true); + expectValueFrom('$var', GraphQLBoolean, '($var: Boolean)', { + var: null, + }).to.equal(null); + expectValueFrom('$var', nonNullBool, '($var: Boolean)', { + var: null, + }).to.equal(undefined); }); it('asserts variables are provided as items in lists', () => { - expectValueFrom('[ $foo ]', listOfBool, {}).to.deep.equal([null]); - expectValueFrom('[ $foo ]', listOfNonNullBool, {}).to.equal(undefined); - expectValueFrom('[ $foo ]', listOfNonNullBool, { + expectValueFrom('[ $foo ]', listOfBool).to.deep.equal([null]); + expectValueFrom('[ $foo ]', listOfNonNullBool).to.equal(undefined); + expectValueFrom('[ $foo ]', listOfNonNullBool, '($foo: Boolean)', { foo: true, }).to.deep.equal([true]); // Note: variables are expected to have already been coerced, so we // do not expect the singleton wrapping behavior for variables. - expectValueFrom('$foo', listOfNonNullBool, { foo: true }).to.equal(true); - expectValueFrom('$foo', listOfNonNullBool, { foo: [true] }).to.deep.equal([ - true, - ]); + expectValueFrom('$foo', listOfNonNullBool, '($foo: Boolean)', { + foo: true, + }).to.equal(true); + expectValueFrom('$foo', listOfNonNullBool, '($foo: [Boolean])', { + foo: [true], + }).to.deep.equal([true]); }); it('omits input object fields for unprovided variables', () => { expectValueFrom( '{ int: $foo, bool: $foo, requiredBool: true }', testInputObj, - {}, ).to.deep.equal({ int: 42, requiredBool: true }); - expectValueFrom('{ requiredBool: $foo }', testInputObj, {}).to.equal( - undefined, - ); + expectValueFrom('{ requiredBool: $foo }', testInputObj).to.equal(undefined); - expectValueFrom('{ requiredBool: $foo }', testInputObj, { + expectValueFrom('{ requiredBool: $foo }', testInputObj, '($foo: Boolean)', { foo: true, }).to.deep.equal({ int: 42, diff --git a/src/utilities/valueFromAST.d.ts b/src/utilities/valueFromAST.d.ts index 6ea38000e1..8fb88c0c68 100644 --- a/src/utilities/valueFromAST.d.ts +++ b/src/utilities/valueFromAST.d.ts @@ -1,9 +1,10 @@ import { Maybe } from '../jsutils/Maybe'; -import { ObjMap } from '../jsutils/ObjMap'; import { ValueNode } from '../language/ast'; import { GraphQLInputType } from '../type/definition'; +import { VariableValues } from '../execution/values'; + /** * Produces a JavaScript value given a GraphQL Value AST. * @@ -27,5 +28,5 @@ import { GraphQLInputType } from '../type/definition'; export function valueFromAST( valueNode: Maybe, type: GraphQLInputType, - variables?: Maybe>, + variables?: Maybe, ): unknown; diff --git a/src/utilities/valueFromAST.js b/src/utilities/valueFromAST.js index 19081ea96e..c85997bd56 100644 --- a/src/utilities/valueFromAST.js +++ b/src/utilities/valueFromAST.js @@ -1,4 +1,3 @@ -import type { ObjMap } from '../jsutils/ObjMap'; import { keyMap } from '../jsutils/keyMap'; import { inspect } from '../jsutils/inspect'; import { invariant } from '../jsutils/invariant'; @@ -14,6 +13,8 @@ import { isNonNullType, } from '../type/definition'; +import type { VariableValues } from '../execution/values'; + /** * Produces a JavaScript value given a GraphQL Value AST. * @@ -37,7 +38,7 @@ import { export function valueFromAST( valueNode: ?ValueNode, type: GraphQLInputType, - variables?: ?ObjMap, + variables?: ?VariableValues, ): mixed | void { if (!valueNode) { // When there is no node, then there is also no value. @@ -47,11 +48,11 @@ export function valueFromAST( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - if (variables == null || variables[variableName] === undefined) { + if (variables == null || variables.coerced[variableName] === undefined) { // No valid return value. return; } - const variableValue = variables[variableName]; + const variableValue = variables.coerced[variableName]; if (variableValue === null && isNonNullType(type)) { return; // Invalid: intentionally return no value. } @@ -139,7 +140,7 @@ export function valueFromAST( // no value is returned. let result; try { - result = type.parseLiteral(valueNode, variables); + result = type.parseLiteral(valueNode, variables?.coerced); } catch (_error) { return; // Invalid: intentionally return no value. } @@ -157,10 +158,10 @@ export function valueFromAST( // in the set of variables. function isMissingVariable( valueNode: ValueNode, - variables: ?ObjMap, + variables: ?VariableValues, ): boolean { return ( valueNode.kind === Kind.VARIABLE && - (variables == null || variables[valueNode.name.value] === undefined) + (variables == null || variables.coerced[valueNode.name.value] === undefined) ); } diff --git a/src/utilities/valueFromASTUntyped.d.ts b/src/utilities/valueFromASTUntyped.d.ts index 495e279723..1e173cc5c4 100644 --- a/src/utilities/valueFromASTUntyped.d.ts +++ b/src/utilities/valueFromASTUntyped.d.ts @@ -1,5 +1,5 @@ import { Maybe } from '../jsutils/Maybe'; -import { ObjMap } from '../jsutils/ObjMap'; +import { ReadOnlyObjMap } from '../jsutils/ObjMap'; import { ValueNode } from '../language/ast'; @@ -21,5 +21,5 @@ import { ValueNode } from '../language/ast'; */ export function valueFromASTUntyped( valueNode: ValueNode, - variables?: Maybe>, + variables?: Maybe>, ): unknown; diff --git a/src/utilities/valueFromASTUntyped.js b/src/utilities/valueFromASTUntyped.js index 05f5db71b2..cadcc1b907 100644 --- a/src/utilities/valueFromASTUntyped.js +++ b/src/utilities/valueFromASTUntyped.js @@ -1,4 +1,4 @@ -import type { ObjMap } from '../jsutils/ObjMap'; +import type { ReadOnlyObjMap } from '../jsutils/ObjMap'; import { inspect } from '../jsutils/inspect'; import { invariant } from '../jsutils/invariant'; import { keyValMap } from '../jsutils/keyValMap'; @@ -24,7 +24,7 @@ import type { ValueNode } from '../language/ast'; */ export function valueFromASTUntyped( valueNode: ValueNode, - variables?: ?ObjMap, + variables?: ?ReadOnlyObjMap, ): mixed { switch (valueNode.kind) { case Kind.NULL: