fix(replaceVariables): preserve sources for fragment variable values (#4389)

The new flow for coercing input literals -- see
#3814 -- introduces a
`replaceVariables()` prior to calling `coerceInputLiteral()` such that
we go:

from `ValueNode` => `ConstValueNode` => coerced value

The main advantages being that:
(1) we can trivially support embedding variables within complex scalars
-- despite this being non-spec defined behavior!
(2) userland implementations of custom scalar `coerceInputLiteral()`
methods do not need to worry about handling variables or fragment
variables.

Prior to this PR, we 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.

As a post-script, because within `getFragmentVariableValues()` we now
end up within a `ValueNode` stored on source, to coerce this value, we
can extract a new helper `coerceArgumentValue()` from
`experimentalGetArgumentValues()`, rather than calling it after we are
done for all the values, which has some additional overhead.

This has the side benefit of removing the need for a separate
`experimentalGetArgumentValues()` function altogether. We initially
introduced it to have a more flexible signature for
`getArgumentValues()` that encompasses argument for fragment spreads,
but now this is taken care of using our more directed helper.

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,94 @@ 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 non-embedded variables', () => {
+      const result = executeQuery(
+        `
+          query ($input: JSONScalar) {
+            fieldWithJSONScalarInput(input: $input)
+          }
+        `,
+        { 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" }',
+        },
+      });
+    });
+
+    it('allows custom scalars with embedded nested fragment variables', () => {
+      const result = executeQueryWithFragmentArguments(`
+        {
+          ...JSONFragment(input1: "foo")
+        }
+        fragment JSONFragment($input1: String) on TestType {
+          ...JSONNestedFragment(input2: $input1)
+        }
+        fragment JSONNestedFragment($input2: String) on TestType {
+          fieldWithJSONScalarInput(input: { a: $input2, 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,
@@ -25,7 +26,7 @@ import { typeFromAST } from '../utilities/typeFromAST.js';
 import type { GraphQLVariableSignature } from './getVariableSignature.js';
 import type { VariableValues } from './values.js';
 import {
-  experimentalGetArgumentValues,
+  getArgumentValues,
   getDirectiveValues,
   getFragmentVariableValues,
 } from './values.js';
@@ -35,10 +36,21 @@ 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;
+  readonly fragmentVariableValues?: FragmentVariableValues;
+}
+
 export interface FieldDetails {
   node: FieldNode;
   deferUsage?: DeferUsage | undefined;
-  fragmentVariableValues?: VariableValues | undefined;
+  fragmentVariableValues?: FragmentVariableValues | undefined;
 }
 
 export type FieldDetailsList = ReadonlyArray<FieldDetails>;
@@ -168,7 +180,7 @@ function collectFieldsImpl(
   groupedFieldSet: AccumulatorMap<string, FieldDetails>,
   newDeferUsages: Array<DeferUsage>,
   deferUsage?: DeferUsage,
-  fragmentVariableValues?: VariableValues,
+  fragmentVariableValues?: FragmentVariableValues,
 ): void {
   const {
     schema,
@@ -273,7 +285,7 @@ function collectFieldsImpl(
         );
 
         const fragmentVariableSignatures = fragment.variableSignatures;
-        let newFragmentVariableValues: VariableValues | undefined;
+        let newFragmentVariableValues: FragmentVariableValues | undefined;
         if (fragmentVariableSignatures) {
           newFragmentVariableValues = getFragmentVariableValues(
             selection,
@@ -318,7 +330,7 @@ function collectFieldsImpl(
  */
 function getDeferUsage(
   variableValues: VariableValues,
-  fragmentVariableValues: VariableValues | undefined,
+  fragmentVariableValues: FragmentVariableValues | undefined,
   node: FragmentSpreadNode | InlineFragmentNode,
   parentDeferUsage: DeferUsage | undefined,
 ): DeferUsage | undefined {
@@ -351,7 +363,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,
@@ -361,9 +373,9 @@ function shouldIncludeNode(
     return false;
   }
   const skip = skipDirectiveNode
-    ? experimentalGetArgumentValues(
+    ? getArgumentValues(
+        GraphQLSkipDirective,
         skipDirectiveNode,
-        GraphQLSkipDirective.args,
         variableValues,
         fragmentVariableValues,
         context.hideSuggestions,
@@ -381,9 +393,9 @@ function shouldIncludeNode(
     return false;
   }
   const include = includeDirectiveNode
-    ? experimentalGetArgumentValues(
+    ? getArgumentValues(
+        GraphQLIncludeDirective,
         includeDirectiveNode,
-        GraphQLIncludeDirective.args,
         variableValues,
         fragmentVariableValues,
         context.hideSuggestions,

src/execution/execute.ts

@@ -85,7 +85,6 @@ import type {
 import { DeferredFragmentRecord } from './types.js';
 import type { VariableValues } from './values.js';
 import {
-  experimentalGetArgumentValues,
   getArgumentValues,
   getDirectiveValues,
   getVariableValues,
@@ -877,9 +876,9 @@ function executeField(
     // Build a JS object of arguments from the field.arguments AST, using the
     // variables scope to fulfill any variable references.
     // TODO: find a way to memoize, in case this field is within a List type.
-    const args = experimentalGetArgumentValues(
+    const args = getArgumentValues(
+      fieldDef,
       fieldDetailsList[0].node,
-      fieldDef.args,
       variableValues,
       fieldDetailsList[0].fragmentVariableValues,
       hideSuggestions,
@@ -2298,6 +2297,7 @@ function executeSubscription(
       fieldDef,
       fieldNodes[0],
       variableValues,
+      fieldDetailsList[0].fragmentVariableValues,
       hideSuggestions,
     );