fix: preserve sources for fragment variable values

the new flow for coercing input literals (see graphql#3814 introduces a `replaceVariables()` prior to calling `coerceInputLiteral()` such that we go from `ValueNode` => `ConstValueNode` => coerced value. (With the main advantages being (1) that we can trivially support embedding variables within complex scalars -- despite this being non-spec defined behavior -- and (2) that implementations of custom scalar `coerceInputLiteral()` methods do not need to worry about handling variables or fragment variables.

We previously did not properly handle this in the case of fragment definition variables/fragment spread arguments. `replaceVariableValues()` assumes that the uncoerced value is preserved as source, but instead of grabbing this value from the argument on the spread, we were incorrectly attempting to retrieve the already stored value from existing variables.

This was not caught because we previously did not have any actual tests for this custom unspecified behavior and were using incorrect types and bespoke builders in our tests for `replaceVariables()`.

This PR:

(a) fixes the bug by storing the argument from the spread
(b) fixes our bespoke builders in `replaceVariables()`
(c) add explicit tests for embedding variables within custom scalars to protect against future changes.

To consider:

Within `getFragmentVariableValues()`, because we end up within a `ConstValueNode` stored on source, to  coerce this value, we could consider extracting only the functionality we need from `experimentalGetArgumentValues()` (which also handles variables). This would seem to also have the side benefit of removing the need for a separate new signature for this function that can handle fragment spread arguments and we could stick with the old signature/name of just `getArgumentValues()`.

Author: yaacovCR

src/execution/__tests__/variables-test.ts

@@ -30,6 +30,8 @@ import {
 import { GraphQLBoolean, GraphQLString } from '../../type/scalars.js';
 import { GraphQLSchema } from '../../type/schema.js';
 
+import { valueFromASTUntyped } from '../../utilities/valueFromASTUntyped.js';
+
 import { executeSync, experimentalExecuteIncrementally } from '../execute.js';
 import { getVariableValues } from '../values.js';
 
@@ -64,6 +66,16 @@ const TestComplexScalar = new GraphQLScalarType({
   },
 });
 
+const TestJSONScalar = new GraphQLScalarType({
+  name: 'JSONScalar',
+  coerceInputValue(value) {
+    return value;
+  },
+  coerceInputLiteral(value) {
+    return valueFromASTUntyped(value);
+  },
+});
+
 const NestedType: GraphQLObjectType = new GraphQLObjectType({
   name: 'NestedType',
   fields: {
@@ -151,6 +163,7 @@ const TestType = new GraphQLObjectType({
     fieldWithNestedInputObject: fieldWithInputArg({
       type: TestNestedInputObject,
     }),
+    fieldWithJSONScalarInput: fieldWithInputArg({ type: TestJSONScalar }),
     list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }),
     nested: {
       type: NestedType,
@@ -859,6 +872,57 @@ describe('Execute: Handles inputs', () => {
     });
   });
 
+  // Note: the below is non-specified custom graphql-js behavior.
+  describe('Handles custom scalars with embedded variables', () => {
+    it('allows custom scalars', () => {
+      const result = executeQuery(`
+        {
+          fieldWithJSONScalarInput(input: { a: "foo", b: ["bar"], c: "baz" })
+        }
+      `);
+
+      expectJSON(result).toDeepEqual({
+        data: {
+          fieldWithJSONScalarInput: '{ a: "foo", b: ["bar"], c: "baz" }',
+        },
+      });
+    });
+
+    it('allows custom scalars with embedded operation variables', () => {
+      const result = executeQuery(
+        `
+          query ($input: String) {
+            fieldWithJSONScalarInput(input: { a: $input, b: ["bar"], c: "baz" })
+          }
+        `,
+        { input: 'foo' },
+      );
+
+      expectJSON(result).toDeepEqual({
+        data: {
+          fieldWithJSONScalarInput: '{ a: "foo", b: ["bar"], c: "baz" }',
+        },
+      });
+    });
+
+    it('allows custom scalars with embedded fragment variables', () => {
+      const result = executeQueryWithFragmentArguments(`
+        {
+          ...JSONFragment(input: "foo")
+        }
+        fragment JSONFragment($input: String) on TestType {
+          fieldWithJSONScalarInput(input: { a: $input, b: ["bar"], c: "baz" })
+        }
+      `);
+
+      expectJSON(result).toDeepEqual({
+        data: {
+          fieldWithJSONScalarInput: '{ a: "foo", b: ["bar"], c: "baz" }',
+        },
+      });
+    });
+  });
+
   describe('Handles lists and nullability', () => {
     it('allows lists to be null', () => {
       const doc = `

src/execution/collectFields.ts

@@ -1,7 +1,8 @@
 import { AccumulatorMap } from '../jsutils/AccumulatorMap.js';
-import type { ObjMap } from '../jsutils/ObjMap.js';
+import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap.js';
 
 import type {
+  ConstValueNode,
   DirectiveNode,
   FieldNode,
   FragmentDefinitionNode,
@@ -35,10 +36,20 @@ export interface DeferUsage {
   parentDeferUsage: DeferUsage | undefined;
 }
 
+export interface FragmentVariableValues {
+  readonly sources: ReadOnlyObjMap<FragmentVariableValueSource>;
+  readonly coerced: ReadOnlyObjMap<unknown>;
+}
+
+interface FragmentVariableValueSource {
+  readonly signature: GraphQLVariableSignature;
+  readonly value?: ConstValueNode;
+}
+
 export interface FieldDetails {
   node: FieldNode;
   deferUsage?: DeferUsage | undefined;
-  fragmentVariableValues?: VariableValues | undefined;
+  fragmentVariableValues?: FragmentVariableValues | undefined;
 }
 
 export type FieldDetailsList = ReadonlyArray<FieldDetails>;
@@ -168,7 +179,7 @@ function collectFieldsImpl(
   groupedFieldSet: AccumulatorMap<string, FieldDetails>,
   newDeferUsages: Array<DeferUsage>,
   deferUsage?: DeferUsage,
-  fragmentVariableValues?: VariableValues,
+  fragmentVariableValues?: FragmentVariableValues,
 ): void {
   const {
     schema,
@@ -273,7 +284,7 @@ function collectFieldsImpl(
         );
 
         const fragmentVariableSignatures = fragment.variableSignatures;
-        let newFragmentVariableValues: VariableValues | undefined;
+        let newFragmentVariableValues: FragmentVariableValues | undefined;
         if (fragmentVariableSignatures) {
           newFragmentVariableValues = getFragmentVariableValues(
             selection,
@@ -318,7 +329,7 @@ function collectFieldsImpl(
  */
 function getDeferUsage(
   variableValues: VariableValues,
-  fragmentVariableValues: VariableValues | undefined,
+  fragmentVariableValues: FragmentVariableValues | undefined,
   node: FragmentSpreadNode | InlineFragmentNode,
   parentDeferUsage: DeferUsage | undefined,
 ): DeferUsage | undefined {
@@ -351,7 +362,7 @@ function shouldIncludeNode(
   context: CollectFieldsContext,
   node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
   variableValues: VariableValues,
-  fragmentVariableValues: VariableValues | undefined,
+  fragmentVariableValues: FragmentVariableValues | undefined,
 ): boolean {
   const skipDirectiveNode = node.directives?.find(
     (directive) => directive.name.value === GraphQLSkipDirective.name,

src/execution/values.ts

@@ -1,4 +1,5 @@
 import { invariant } from '../jsutils/invariant.js';
+import { keyMap } from '../jsutils/keyMap.js';
 import type { Maybe } from '../jsutils/Maybe.js';
 import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap.js';
 import { printPathArray } from '../jsutils/printPathArray.js';
@@ -27,11 +28,13 @@ import {
   coerceInputLiteral,
   coerceInputValue,
 } from '../utilities/coerceInputValue.js';
+import { replaceVariables } from '../utilities/replaceVariables.js';
 import {
   validateInputLiteral,
   validateInputValue,
 } from '../utilities/validateInputValue.js';
 
+import type { FragmentVariableValues } from './collectFields.js';
 import type { GraphQLVariableSignature } from './getVariableSignature.js';
 import { getVariableSignature } from './getVariableSignature.js';
 
@@ -154,19 +157,29 @@ export function getFragmentVariableValues(
   fragmentSpreadNode: FragmentSpreadNode,
   fragmentSignatures: ReadOnlyObjMap<GraphQLVariableSignature>,
   variableValues: VariableValues,
-  fragmentVariableValues?: Maybe<VariableValues>,
+  fragmentVariableValues?: Maybe<FragmentVariableValues>,
   hideSuggestions?: Maybe<boolean>,
-): VariableValues {
+): FragmentVariableValues {
+  const args = keyMap(
+    fragmentSpreadNode.arguments ?? [],
+    (arg) => arg.name.value,
+  );
   const varSignatures: Array<GraphQLVariableSignature> = [];
   const sources = Object.create(null);
   for (const [varName, varSignature] of Object.entries(fragmentSignatures)) {
     varSignatures.push(varSignature);
     sources[varName] = {
       signature: varSignature,
-      value:
-        fragmentVariableValues?.sources[varName]?.value ??
-        variableValues.sources[varName]?.value,
     };
+    const arg = args[varName];
+    if (arg !== undefined) {
+      const value = arg.value;
+      sources[varName].value = replaceVariables(
+        value,
+        variableValues,
+        fragmentVariableValues,
+      );
+    }
   }
 
   const coerced = experimentalGetArgumentValues(
@@ -207,7 +220,7 @@ export function experimentalGetArgumentValues(
   node: FieldNode | DirectiveNode | FragmentSpreadNode,
   argDefs: ReadonlyArray<GraphQLArgument | GraphQLVariableSignature>,
   variableValues: Maybe<VariableValues>,
-  fragmentVariableValues?: Maybe<VariableValues>,
+  fragmentVariableValues?: Maybe<FragmentVariableValues>,
   hideSuggestions?: Maybe<boolean>,
 ): { [argument: string]: unknown } {
   const coercedValues: { [argument: string]: unknown } = {};
@@ -306,7 +319,7 @@ export function getDirectiveValues(
   directiveDef: GraphQLDirective,
   node: { readonly directives?: ReadonlyArray<DirectiveNode> | undefined },
   variableValues?: Maybe<VariableValues>,
-  fragmentVariableValues?: Maybe<VariableValues>,
+  fragmentVariableValues?: Maybe<FragmentVariableValues>,
   hideSuggestions?: Maybe<boolean>,
 ): undefined | { [argument: string]: unknown } {
   const directiveNode = node.directives?.find(

src/utilities/__tests__/replaceVariables-test.ts

@@ -4,14 +4,24 @@ import { describe, it } from 'mocha';
 import { invariant } from '../../jsutils/invariant.js';
 import type { ReadOnlyObjMap } from '../../jsutils/ObjMap.js';
 
-import type { ValueNode } from '../../language/ast.js';
+import type {
+  FragmentArgumentNode,
+  FragmentSpreadNode,
+  ValueNode,
+  VariableDefinitionNode,
+} from '../../language/ast.js';
+import { Kind } from '../../language/kinds.js';
 import { Parser, parseValue as _parseValue } from '../../language/parser.js';
 import { TokenKind } from '../../language/tokenKind.js';
 
 import { GraphQLInt } from '../../type/scalars.js';
 import { GraphQLSchema } from '../../type/schema.js';
 
-import { getVariableValues } from '../../execution/values.js';
+import { getVariableSignature } from '../../execution/getVariableSignature.js';
+import {
+  getFragmentVariableValues,
+  getVariableValues,
+} from '../../execution/values.js';
 
 import { replaceVariables } from '../replaceVariables.js';
 
@@ -20,17 +30,51 @@ function parseValue(ast: string): ValueNode {
 }
 
 function testVariables(variableDefs: string, inputs: ReadOnlyObjMap<unknown>) {
-  const parser = new Parser(variableDefs, { noLocation: true });
-  parser.expectToken(TokenKind.SOF);
   const variableValuesOrErrors = getVariableValues(
     new GraphQLSchema({ types: [GraphQLInt] }),
-    parser.parseVariableDefinitions() ?? [],
+    parseVariableDefinitions(variableDefs),
     inputs,
   );
   invariant(variableValuesOrErrors.variableValues !== undefined);
   return variableValuesOrErrors.variableValues;
 }
 
+function parseVariableDefinitions(
+  variableDefs: string,
+): ReadonlyArray<VariableDefinitionNode> {
+  const parser = new Parser(variableDefs, { noLocation: true });
+  parser.expectToken(TokenKind.SOF);
+  return parser.parseVariableDefinitions() ?? [];
+}
+
+function testFragmentVariables(variableDefs: string, fragmentArgs: string) {
+  const schema = new GraphQLSchema({ types: [GraphQLInt] });
+  const fragmentSignatures = Object.create(null);
+  for (const varDef of parseVariableDefinitions(variableDefs)) {
+    const signature = getVariableSignature(schema, varDef);
+    fragmentSignatures[signature.name] = signature;
+  }
+  const spread: FragmentSpreadNode = {
+    kind: Kind.FRAGMENT_SPREAD,
+    name: { kind: Kind.NAME, value: 'TestFragment' },
+    arguments: parseFragmentArguments(fragmentArgs),
+  };
+  return getFragmentVariableValues(
+    spread,
+    fragmentSignatures,
+    Object.create(null),
+    undefined,
+  );
+}
+
+function parseFragmentArguments(
+  fragmentArguments: string,
+): ReadonlyArray<FragmentArgumentNode> {
+  const parser = new Parser(fragmentArguments, { noLocation: true });
+  parser.expectToken(TokenKind.SOF);
+  return parser.parseFragmentArguments() ?? [];
+}
+
 describe('replaceVariables', () => {
   describe('Operation Variables', () => {
     it('does not change simple AST', () => {
@@ -96,7 +140,7 @@ describe('replaceVariables', () => {
   describe('Fragment Variables', () => {
     it('replaces simple Fragment Variables', () => {
       const ast = parseValue('$var');
-      const fragmentVars = testVariables('($var: Int)', { var: 123 });
+      const fragmentVars = testFragmentVariables('($var: Int)', `(var: 123)`);
       expect(replaceVariables(ast, undefined, fragmentVars)).to.deep.equal(
         parseValue('123'),
       );
@@ -105,15 +149,15 @@ describe('replaceVariables', () => {
     it('replaces simple Fragment Variables even when overlapping with Operation Variables', () => {
       const ast = parseValue('$var');
       const operationVars = testVariables('($var: Int)', { var: 123 });
-      const fragmentVars = testVariables('($var: Int)', { var: 456 });
+      const fragmentVars = testFragmentVariables('($var: Int)', '(var: 456)');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('456'),
       );
     });
 
     it('replaces Fragment Variables with default values', () => {
       const ast = parseValue('$var');
-      const fragmentVars = testVariables('($var: Int = 123)', {});
+      const fragmentVars = testFragmentVariables('($var: Int = 123)', '');
       expect(replaceVariables(ast, undefined, fragmentVars)).to.deep.equal(
         parseValue('123'),
       );
@@ -122,15 +166,15 @@ describe('replaceVariables', () => {
     it('replaces Fragment Variables with default values even when overlapping with Operation Variables', () => {
       const ast = parseValue('$var');
       const operationVars = testVariables('($var: Int = 123)', {});
-      const fragmentVars = testVariables('($var: Int = 456)', {});
+      const fragmentVars = testFragmentVariables('($var: Int = 456)', '');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('456'),
       );
     });
 
     it('replaces nested Fragment Variables', () => {
       const ast = parseValue('{ foo: [ $var ], bar: $var }');
-      const fragmentVars = testVariables('($var: Int)', { var: 123 });
+      const fragmentVars = testFragmentVariables('($var: Int)', '(var: 123)');
       expect(replaceVariables(ast, undefined, fragmentVars)).to.deep.equal(
         parseValue('{ foo: [ 123 ], bar: 123 }'),
       );
@@ -139,7 +183,7 @@ describe('replaceVariables', () => {
     it('replaces nested Fragment Variables even when overlapping with Operation Variables', () => {
       const ast = parseValue('{ foo: [ $var ], bar: $var }');
       const operationVars = testVariables('($var: Int)', { var: 123 });
-      const fragmentVars = testVariables('($var: Int)', { var: 456 });
+      const fragmentVars = testFragmentVariables('($var: Int)', '(var: 456)');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('{ foo: [ 456 ], bar: 456 }'),
       );
@@ -155,7 +199,7 @@ describe('replaceVariables', () => {
     it('replaces missing Fragment Variables with null even when overlapping with Operation Variables', () => {
       const ast = parseValue('$var');
       const operationVars = testVariables('($var: Int)', { var: 123 });
-      const fragmentVars = testVariables('($var: Int)', {});
+      const fragmentVars = testFragmentVariables('($var: Int)', '');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('null'),
       );
@@ -171,7 +215,7 @@ describe('replaceVariables', () => {
     it('replaces missing Fragment Variables in lists with null even when overlapping with Operation Variables', () => {
       const ast = parseValue('[1, $var]');
       const operationVars = testVariables('($var: Int)', { var: 123 });
-      const fragmentVars = testVariables('($var: Int)', {});
+      const fragmentVars = testFragmentVariables('($var: Int)', '');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('[1, null]'),
       );
@@ -187,7 +231,7 @@ describe('replaceVariables', () => {
     it('omits missing Fragment Variables from objects even when overlapping with Operation Variables', () => {
       const ast = parseValue('{ foo: 1, bar: $var }');
       const operationVars = testVariables('($var: Int)', { var: 123 });
-      const fragmentVars = testVariables('($var: Int)', {});
+      const fragmentVars = testFragmentVariables('($var: Int)', '');
       expect(replaceVariables(ast, operationVars, fragmentVars)).to.deep.equal(
         parseValue('{ foo: 1 }'),
       );