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
graphql · May 13, 2021 · abdec72 · abdec72
1 parent 05db9bd
commit abdec72
Show file tree

Hide file tree

Showing 11 changed files with 116 additions and 62 deletions.
diff --git 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<FragmentDefinitionNode>;
   operation: OperationDefinitionNode;
-  variableValues: { [key: string]: unknown };
+  variableValues: VariableValues;
   fieldResolver: GraphQLFieldResolver<any, any>;
   typeResolver: GraphQLTypeResolver<any, any>;
   errors: Array<GraphQLError>;

diff --git 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<any, any>,
   typeResolver: GraphQLTypeResolver<any, any>,
   errors: Array<GraphQLError>,
@@ -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
@@ -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<unknown>;
+};
 
 type CoercedVariableValues =
   | { errors: ReadonlyArray<GraphQLError>; 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<unknown, unknown> | GraphQLDirective,
   node: FieldNode | DirectiveNode,
-  variableValues?: Maybe<ObjMap<unknown>>,
+  variableValues?: Maybe<VariableValues>,
 ): { [key: string]: unknown };
 
 /**
@@ -62,5 +71,5 @@ export function getDirectiveValues(
   node: {
     readonly directives?: ReadonlyArray<DirectiveNode>;
   },
-  variableValues?: Maybe<ObjMap<unknown>>,
+  variableValues?: Maybe<VariableValues>,
 ): undefined | { [key: string]: unknown };
diff --git 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<mixed>,
+|};
+
 type CoercedVariableValues =
   | {| errors: $ReadOnlyArray<GraphQLError> |}
-  | {| 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<VariableDefinitionNode>,
-  inputs: { +[variable: string]: mixed, ... },
+  inputs: ReadOnlyObjMapLike<mixed>,
   options?: {| maxErrors?: number |},
 ): CoercedVariableValues {
   const errors = [];
@@ -74,10 +83,11 @@ export function getVariableValues(
 function coerceVariableValues(
   schema: GraphQLSchema,
   varDefNodes: $ReadOnlyArray<VariableDefinitionNode>,
-  inputs: { +[variable: string]: mixed, ... },
+  inputs: ReadOnlyObjMapLike<mixed>,
   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<mixed, mixed> | GraphQLDirective,
   node: FieldNode | DirectiveNode,
-  variableValues?: ?ObjMap<mixed>,
+  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<DirectiveNode>, ... },
-  variableValues?: ?ObjMap<mixed>,
+  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
@@ -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<TInternal> = (
 ) => Maybe<TInternal>;
 export type GraphQLScalarLiteralParser<TInternal> = (
   valueNode: ValueNode,
-  variables: Maybe<ObjMap<unknown>>,
+  variables: Maybe<ReadOnlyObjMap<unknown>>,
 ) => Maybe<TInternal>;
 
 export interface GraphQLScalarTypeConfig<TInternal, TExternal> {
@@ -487,7 +487,7 @@ export interface GraphQLResolveInfo {
   readonly fragments: ObjMap<FragmentDefinitionNode>;
   readonly rootValue: unknown;
   readonly operation: OperationDefinitionNode;
-  readonly variableValues: { [variableName: string]: unknown };
+  readonly variableValues: ReadOnlyObjMap<unknown>;
 }
 
 /**
@@ -790,7 +790,7 @@ export class GraphQLEnumType {
   parseValue(value: unknown): Maybe<any>;
   parseLiteral(
     valueNode: ValueNode,
-    _variables: Maybe<ObjMap<unknown>>,
+    _variables: Maybe<ReadOnlyObjMap<unknown>>,
   ): Maybe<any>;
 
   toConfig(): GraphQLEnumTypeConfig & {

diff --git a/src/type/definition.js b/src/type/definition.js
@@ -641,7 +641,7 @@ export type GraphQLScalarValueParser<TInternal> = (
 
 export type GraphQLScalarLiteralParser<TInternal> = (
   valueNode: ValueNode,
-  variables: ?ObjMap<mixed>,
+  variables: ?ReadOnlyObjMap<mixed>,
 ) => ?TInternal;
 
 export type GraphQLScalarTypeConfig<TInternal, TExternal> = {|
@@ -909,7 +909,7 @@ export type GraphQLResolveInfo = {|
   +fragments: ObjMap<FragmentDefinitionNode>,
   +rootValue: mixed,
   +operation: OperationDefinitionNode,
-  +variableValues: { [variable: string]: mixed, ... },
+  +variableValues: ReadOnlyObjMap<mixed>,
 |};
 
 export type GraphQLFieldConfig<
@@ -1349,7 +1349,10 @@ export class GraphQLEnumType /* <T> */ {
     return enumValue.value;
   }
 
-  parseLiteral(valueNode: ValueNode, _variables: ?ObjMap<mixed>): ?any /* T */ {
+  parseLiteral(
+    valueNode: ValueNode,
+    _variables: ?ReadOnlyObjMap<mixed>,
+  ): ?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
@@ -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,16 +22,32 @@ import {
   GraphQLEnumType,
   GraphQLInputObjectType,
 } from '../../type/definition';
+import { GraphQLSchema } from '../../type/schema';
+
+import { getVariableValues } from '../../execution/values';
 
 import { valueFromAST } from '../valueFromAST';
 
 describe('valueFromAST', () => {
   function expectValueFrom(
     valueText: string,
     type: GraphQLInputType,
-    variables?: ObjMap<mixed>,
+    variableDefs?: string,
+    variableValues?: ReadOnlyObjMap<mixed>,
   ) {
     const ast = parseValue(valueText);
+    let variables;
+    if (variableValues && variableDefs !== undefined) {
+      const parser = new Parser(variableDefs);
+      parser.expectToken('<SOF>');
+      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,