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,15 +34,16 @@ export interface DeferUsage {
 export interface FieldDetails {
   node: FieldNode;
   deferUsage: DeferUsage | undefined;
+  fragmentVariableValues?: ObjMap<unknown> | undefined;
 }
 
 interface CollectFieldsContext {
   schema: GraphQLSchema;
   fragments: ObjMap<FragmentDefinitionNode>;
-  variableValues: { [variable: string]: unknown };
   operation: OperationDefinitionNode;
   runtimeType: GraphQLObjectType;
   visitedFragmentNames: Set<string>;
+  variableValues: { [variable: string]: unknown };
 }
 
 /**
@@ -66,13 +66,18 @@ export function collectFields(
   const context: CollectFieldsContext = {
     schema,
     fragments,
-    variableValues,
     runtimeType,
+    variableValues,
     operation,
     visitedFragmentNames: new Set(),
   };
 
-  collectFieldsImpl(context, operation.selectionSet, groupedFieldSet);
+  collectFieldsImpl(
+    context,
+    operation.selectionSet,
+    groupedFieldSet,
+    variableValues,
+  );
   return groupedFieldSet;
 }
 
@@ -98,8 +103,8 @@ export function collectSubfields(
   const context: CollectFieldsContext = {
     schema,
     fragments,
-    variableValues,
     runtimeType: returnType,
+    variableValues,
     operation,
     visitedFragmentNames: new Set(),
   };
@@ -112,6 +117,7 @@ export function collectSubfields(
         context,
         node.selectionSet,
         subGroupedFieldSet,
+        undefined,
         fieldDetail.deferUsage,
       );
     }
@@ -120,31 +126,35 @@ export function collectSubfields(
   return subGroupedFieldSet;
 }
 
+// eslint-disable-next-line max-params
 function collectFieldsImpl(
   context: CollectFieldsContext,
   selectionSet: SelectionSetNode,
   groupedFieldSet: AccumulatorMap<string, FieldDetails>,
+  fragmentVariableValues?: ObjMap<unknown>,
   parentDeferUsage?: DeferUsage,
   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: deferUsage ?? parentDeferUsage,
+          fragmentVariableValues: fragmentVariableValues ?? undefined,
         });
         break;
       }
@@ -167,6 +177,7 @@ function collectFieldsImpl(
           context,
           selection.selectionSet,
           groupedFieldSet,
+          fragmentVariableValues,
           parentDeferUsage,
           newDeferUsage ?? deferUsage,
         );
@@ -203,18 +214,34 @@ function collectFieldsImpl(
           visitedFragmentNames.add(fragmentName);
         }
 
-        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;
 
         collectFieldsImpl(
           context,
-          fragmentSelectionSet,
+          fragment.selectionSet,
           groupedFieldSet,
+          fragmentArgValues,
           parentDeferUsage,
           newDeferUsage ?? deferUsage,
         );
+
         break;
       }
     }

src/execution/execute.ts

@@ -624,9 +624,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.fields[0].node,
+      fieldDef.args,
       exeContext.variableValues,
+      fieldGroup.fields[0].fragmentVariableValues,
     );
 
     // The resolve function's optional third argument is a context value that
@@ -927,7 +928,7 @@ function getStreamUsage(
   const stream = getDirectiveValues(
     GraphQLStreamDirective,
     fieldGroup.fields[0].node,
-    exeContext.variableValues,
+    fieldGroup.fields[0].fragmentVariableValues ?? exeContext.variableValues,
   );
 
   if (!stream) {
@@ -1846,7 +1847,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