chore: collect fields alternative (#3)

* patch

* commit progress

* this should be spec compliant

* another alternative

* linting

* update tests

* add test that Matt mentioned

* remove only

Author: JoviDeCroock

src/execution/__tests__/variables-test.ts

@@ -1198,11 +1198,12 @@ describe('Execute: Handles inputs', () => {
           fieldWithNullableStringInput(input: $value)
         }
       `);
-      expect(result).to.deep.equal({
-        data: {
-          fieldWithNullableStringInput: null,
-        },
-      });
+
+      expect(result).to.have.property('errors');
+      expect(result.errors).to.have.length(1);
+      expect(result.errors?.at(0)?.message).to.match(
+        /Argument "value" of required type "String!"/,
+      );
     });
 
     it('when the definition has a default and is provided', () => {
@@ -1237,22 +1238,42 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
-    it('when the definition has a non-nullable default and is provided null', () => {
+    it('when a definition has a default, is not provided, and spreads another fragment', () => {
       const result = executeQueryWithFragmentArguments(`
         query {
-          ...a(value: null)
+          ...a
         }
-        fragment a($value: String! = "B") on TestType {
-          fieldWithNullableStringInput(input: $value)
+        fragment a($a: String! = "B") on TestType {
+          ...b(b: $a)
+        }
+        fragment b($b: String!) on TestType {
+          fieldWithNonNullableStringInput(input: $b)
         }
       `);
       expect(result).to.deep.equal({
         data: {
-          fieldWithNullableStringInput: 'null',
+          fieldWithNonNullableStringInput: '"B"',
         },
       });
     });
 
+    it('when the definition has a non-nullable default and is provided null', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(value: null)
+        }
+        fragment a($value: String! = "B") on TestType {
+          fieldWithNullableStringInput(input: $value)
+        }
+      `);
+
+      expect(result).to.have.property('errors');
+      expect(result.errors).to.have.length(1);
+      expect(result.errors?.at(0)?.message).to.match(
+        /Argument "value" of non-null type "String!"/,
+      );
+    });
+
     it('when the definition has no default and is not provided', () => {
       const result = executeQueryWithFragmentArguments(`
         query {
@@ -1303,6 +1324,27 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
+    it ('when a fragment-variable is shadowed by an intermediate fragment-spread but defined in the operation-variables', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query($x: String = "A") {
+          ...a
+        }
+        fragment a($x: String) on TestType {
+          ...b
+        }
+
+        fragment b on TestType {
+          fieldWithNullableStringInput(input: $x)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNullableStringInput:
+            '"A"',
+        },
+      });
+    });
+
     it('when a fragment is used with different args', () => {
       const result = executeQueryWithFragmentArguments(`
         query($x: String = "Hello") {

src/execution/collectFields.ts

@@ -22,10 +22,9 @@ import {
 } from '../type/directives.js';
 import type { GraphQLSchema } from '../type/schema.js';
 
-import { substituteFragmentArguments } from '../utilities/substituteFragmentArguments.js';
 import { typeFromAST } from '../utilities/typeFromAST.js';
 
-import { getDirectiveValues } from './values.js';
+import { getArgumentValuesFromSpread, getDirectiveValues } from './values.js';
 
 export interface DeferUsage {
   label: string | undefined;
@@ -35,6 +34,7 @@ export interface DeferUsage {
 export interface FieldDetails {
   node: FieldNode;
   deferUsage: DeferUsage | undefined;
+  fragmentVariableValues?: ObjMap<unknown> | undefined;
 }
 
 export type FieldGroup = ReadonlyArray<FieldDetails>;
@@ -44,10 +44,10 @@ export type GroupedFieldSet = ReadonlyMap<string, FieldGroup>;
 interface CollectFieldsContext {
   schema: GraphQLSchema;
   fragments: ObjMap<FragmentDefinitionNode>;
-  variableValues: { [variable: string]: unknown };
   operation: OperationDefinitionNode;
   runtimeType: GraphQLObjectType;
   visitedFragmentNames: Set<string>;
+  variableValues: { [variable: string]: unknown };
 }
 
 /**
@@ -74,8 +74,8 @@ export function collectFields(
   const context: CollectFieldsContext = {
     schema,
     fragments,
-    variableValues,
     runtimeType,
+    variableValues,
     operation,
     visitedFragmentNames: new Set(),
   };
@@ -85,6 +85,7 @@ export function collectFields(
     operation.selectionSet,
     groupedFieldSet,
     newDeferUsages,
+    variableValues,
   );
   return { groupedFieldSet, newDeferUsages };
 }
@@ -114,8 +115,8 @@ export function collectSubfields(
   const context: CollectFieldsContext = {
     schema,
     fragments,
-    variableValues,
     runtimeType: returnType,
+    variableValues,
     operation,
     visitedFragmentNames: new Set(),
   };
@@ -130,6 +131,7 @@ export function collectSubfields(
         node.selectionSet,
         subGroupedFieldSet,
         newDeferUsages,
+        undefined,
         fieldDetail.deferUsage,
       );
     }
@@ -141,31 +143,35 @@ export function collectSubfields(
   };
 }
 
+// eslint-disable-next-line max-params
 function collectFieldsImpl(
   context: CollectFieldsContext,
   selectionSet: SelectionSetNode,
   groupedFieldSet: AccumulatorMap<string, FieldDetails>,
   newDeferUsages: Array<DeferUsage>,
+  fragmentVariableValues?: ObjMap<unknown>,
   deferUsage?: DeferUsage,
 ): void {
   const {
     schema,
     fragments,
-    variableValues,
     runtimeType,
+    variableValues,
     operation,
     visitedFragmentNames,
   } = context;
 
   for (const selection of selectionSet.selections) {
     switch (selection.kind) {
       case Kind.FIELD: {
-        if (!shouldIncludeNode(variableValues, selection)) {
+        const vars = fragmentVariableValues ?? variableValues;
+        if (!shouldIncludeNode(vars, selection)) {
           continue;
         }
         groupedFieldSet.add(getFieldEntryKey(selection), {
           node: selection,
           deferUsage,
+          fragmentVariableValues: fragmentVariableValues ?? undefined,
         });
         break;
       }
@@ -190,6 +196,7 @@ function collectFieldsImpl(
             selection.selectionSet,
             groupedFieldSet,
             newDeferUsages,
+            fragmentVariableValues,
             deferUsage,
           );
         } else {
@@ -199,6 +206,7 @@ function collectFieldsImpl(
             selection.selectionSet,
             groupedFieldSet,
             newDeferUsages,
+            fragmentVariableValues,
             newDeferUsage,
           );
         }
@@ -231,27 +239,43 @@ function collectFieldsImpl(
           continue;
         }
 
-        const fragmentSelectionSet = substituteFragmentArguments(
-          fragment,
-          selection,
-        );
+        // We need to introduce a concept of shadowing:
+        //
+        // - when a fragment defines a variable that is in the parent scope but not given
+        //   in the fragment-spread we need to look at this variable as undefined and check
+        //   whether the definition has a defaultValue, if not remove it from the variableValues.
+        // - when a fragment does not define a variable we need to copy it over from the parent
+        //   scope as that variable can still get used in spreads later on in the selectionSet.
+        // - when a value is passed in through the fragment-spread we need to copy over the key-value
+        //   into our variable-values.
+        const fragmentArgValues = fragment.variableDefinitions
+          ? getArgumentValuesFromSpread(
+              selection,
+              schema,
+              fragment.variableDefinitions,
+              variableValues,
+              fragmentVariableValues,
+            )
+          : undefined;
 
         if (!newDeferUsage) {
           visitedFragmentNames.add(fragmentName);
           collectFieldsImpl(
             context,
-            fragmentSelectionSet,
+            fragment.selectionSet,
             groupedFieldSet,
             newDeferUsages,
+            fragmentArgValues,
             deferUsage,
           );
         } else {
           newDeferUsages.push(newDeferUsage);
           collectFieldsImpl(
             context,
-            fragmentSelectionSet,
+            fragment.selectionSet,
             groupedFieldSet,
             newDeferUsages,
+            fragmentArgValues,
             newDeferUsage,
           );
         }

src/execution/execute.ts

@@ -721,9 +721,10 @@ function executeField(
     // variables scope to fulfill any variable references.
     // TODO: find a way to memoize, in case this field is within a List type.
     const args = getArgumentValues(
-      fieldDef,
       fieldGroup[0].node,
+      fieldDef.args,
       exeContext.variableValues,
+      fieldGroup[0].fragmentVariableValues,
     );
 
     // The resolve function's optional third argument is a context value that
@@ -1028,7 +1029,7 @@ function getStreamUsage(
   const stream = getDirectiveValues(
     GraphQLStreamDirective,
     fieldGroup[0].node,
-    exeContext.variableValues,
+    fieldGroup[0].fragmentVariableValues ?? exeContext.variableValues,
   );
 
   if (!stream) {
@@ -2051,7 +2052,12 @@ function executeSubscription(
 
     // Build a JS object of arguments from the field.arguments AST, using the
     // variables scope to fulfill any variable references.
-    const args = getArgumentValues(fieldDef, fieldNodes[0], variableValues);
+    const args = getArgumentValues(
+      fieldNodes[0],
+      fieldDef.args,
+      variableValues,
+      undefined,
+    );
 
     // The resolve function's optional third argument is a context value that
     // is provided to every resolve function within an execution. It is commonly