fix embedding fragment variables within custom scalars

yaacovCR · yaacovCR · commit 32541f694296 · 2025-05-06T11:00:20.000+03:00
diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts
@@ -2,6 +2,7 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap.js';
 import type { ObjMap, ReadOnlyObjMap } from '../jsutils/ObjMap.js';
 
 import type {
+  ConstValueNode,
   DirectiveNode,
   FieldNode,
   FragmentDefinitionNode,
@@ -42,7 +43,7 @@ export interface FragmentVariableValues {
 
 interface FragmentVariableValueSource {
   readonly signature: GraphQLVariableSignature;
-  readonly value?: unknown;
+  readonly value?: ConstValueNode;
 }
 
 export interface FieldDetails {
@@ -283,7 +284,7 @@ function collectFieldsImpl(
         );
 
         const fragmentVariableSignatures = fragment.variableSignatures;
-        let newFragmentVariableValues: VariableValues | undefined;
+        let newFragmentVariableValues: FragmentVariableValues | undefined;
         if (fragmentVariableSignatures) {
           newFragmentVariableValues = getFragmentVariableValues(
             selection,
diff --git a/src/execution/values.ts b/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,6 +28,7 @@ import {
   coerceInputLiteral,
   coerceInputValue,
 } from '../utilities/coerceInputValue.js';
+import { replaceVariables } from '../utilities/replaceVariables.js';
 import {
   validateInputLiteral,
   validateInputValue,
@@ -158,16 +160,26 @@ export function getFragmentVariableValues(
   fragmentVariableValues?: Maybe<FragmentVariableValues>,
   hideSuggestions?: Maybe<boolean>,
 ): 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(
diff --git a/src/utilities/__tests__/replaceVariables-test.ts b/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 }'),
       );
diff --git a/src/utilities/replaceVariables.ts b/src/utilities/replaceVariables.ts
@@ -28,25 +28,36 @@ export function replaceVariables(
   switch (valueNode.kind) {
     case Kind.VARIABLE: {
       const varName = valueNode.name.value;
-      const scopedVariableValues = fragmentVariableValues?.sources[varName]
-        ? fragmentVariableValues
-        : variableValues;
+      const fragmentVariableValueSource =
+        fragmentVariableValues?.sources[varName];
 
-      const scopedVariableSource = scopedVariableValues?.sources[varName];
-      if (scopedVariableSource == null) {
+      if (fragmentVariableValueSource) {
+        const value = fragmentVariableValueSource.value;
+        if (value === undefined) {
+          const defaultValue = fragmentVariableValueSource.signature.default;
+          if (defaultValue !== undefined) {
+            return defaultValue.literal;
+          }
+          return { kind: Kind.NULL };
+        }
+        return value;
+      }
+
+      const variableValueSource = variableValues?.sources[varName];
+      if (variableValueSource == null) {
         return { kind: Kind.NULL };
       }
 
-      if (scopedVariableSource.value === undefined) {
-        const defaultValue = scopedVariableSource.signature.default;
+      if (variableValueSource.value === undefined) {
+        const defaultValue = variableValueSource.signature.default;
         if (defaultValue !== undefined) {
           return defaultValue.literal;
         }
       }
 
       return valueToLiteral(
-        scopedVariableSource.value,
-        scopedVariableSource.signature.type,
+        variableValueSource.value,
+        variableValueSource.signature.type,
       ) as ConstValueNode;
     }
     case Kind.OBJECT: {
