Apply changes from v17 branch

Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>

Author: JoviDeCroock

src/execution/__tests__/variables-test.ts

@@ -1201,7 +1201,7 @@ describe('Execute: Handles inputs', () => {
 
       expect(result).to.have.property('errors');
       expect(result.errors).to.have.length(1);
-      expect(result.errors?.[0]?.message).to.match(
+      expect(result.errors?.[0].message).to.match(
         /Argument "value" of required type "String!"/,
       );
     });
@@ -1269,7 +1269,7 @@ describe('Execute: Handles inputs', () => {
 
       expect(result).to.have.property('errors');
       expect(result.errors).to.have.length(1);
-      expect(result.errors?.[0]?.message).to.match(
+      expect(result.errors?.[0].message).to.match(
         /Argument "value" of non-null type "String!"/,
       );
     });
@@ -1307,6 +1307,22 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
+    it('when a nullable argument without a field default is not provided and shadowed by an operation variable', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query($x: String = "A") {
+          ...a
+        }
+        fragment a($x: String) on TestType {
+          fieldWithNullableStringInput(input: $x)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNullableStringInput: null,
+        },
+      });
+    });
+
     it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => {
       const result = executeQueryWithFragmentArguments(`
         query($x: String = "A") {
@@ -1411,6 +1427,27 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
+    it('when argument variables with the same name are used directly and recursively', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(value: "A")
+        }
+        fragment a($value: String!) on TestType {
+          ...b(value: "B")
+          fieldInFragmentA: fieldWithNonNullableStringInput(input: $value)
+        }
+        fragment b($value: String!) on TestType {
+          fieldInFragmentB: fieldWithNonNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldInFragmentA: '"A"',
+          fieldInFragmentB: '"B"',
+        },
+      });
+    });
+
     it('when argument passed in as list', () => {
       const result = executeQueryWithFragmentArguments(`
         query Q($opValue: String = "op") {
@@ -1433,5 +1470,33 @@ describe('Execute: Handles inputs', () => {
         },
       });
     });
+
+    it('when argument passed to a directive', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(value: true)
+        }
+        fragment a($value: Boolean!) on TestType {
+          fieldWithNonNullableStringInput @skip(if: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {},
+      });
+    });
+
+    it('when argument passed to a directive on a nested field', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(value: true)
+        }
+        fragment a($value: Boolean!) on TestType {
+          nested { echo(input: "echo") @skip(if: $value) }
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: { nested: {} },
+      });
+    });
   });
 });

src/execution/collectFields.ts

@@ -17,13 +17,24 @@ import {
 } from '../type/directives';
 import type { GraphQLSchema } from '../type/schema';
 
+import type { GraphQLVariableSignature } from '../utilities/getVariableSignature';
 import { typeFromAST } from '../utilities/typeFromAST';
 
-import { getArgumentValuesFromSpread, getDirectiveValues } from './values';
+import { experimentalGetArgumentValues, getDirectiveValues } from './values';
+
+export interface FragmentVariables {
+  signatures: ObjMap<GraphQLVariableSignature>;
+  values: ObjMap<unknown>;
+}
 
 export interface FieldDetails {
   node: FieldNode;
-  fragmentVariableValues?: ObjMap<unknown> | undefined;
+  fragmentVariables?: FragmentVariables | undefined;
+}
+
+export interface FragmentDetails {
+  definition: FragmentDefinitionNode;
+  variableSignatures?: ObjMap<GraphQLVariableSignature> | undefined;
 }
 
 /**
@@ -37,7 +48,7 @@ export interface FieldDetails {
  */
 export function collectFields(
   schema: GraphQLSchema,
-  fragments: ObjMap<FragmentDefinitionNode>,
+  fragments: ObjMap<FragmentDetails>,
   variableValues: { [variable: string]: unknown },
   runtimeType: GraphQLObjectType,
   selectionSet: SelectionSetNode,
@@ -68,7 +79,7 @@ export function collectFields(
  */
 export function collectSubfields(
   schema: GraphQLSchema,
-  fragments: ObjMap<FragmentDefinitionNode>,
+  fragments: ObjMap<FragmentDetails>,
   variableValues: { [variable: string]: unknown },
   returnType: GraphQLObjectType,
   fieldEntries: ReadonlyArray<FieldDetails>,
@@ -85,6 +96,7 @@ export function collectSubfields(
         entry.node.selectionSet,
         subFieldEntries,
         visitedFragmentNames,
+        entry.fragmentVariables,
       );
     }
   }
@@ -93,41 +105,40 @@ export function collectSubfields(
 
 function collectFieldsImpl(
   schema: GraphQLSchema,
-  fragments: ObjMap<FragmentDefinitionNode>,
+  fragments: ObjMap<FragmentDetails>,
   variableValues: { [variable: string]: unknown },
   runtimeType: GraphQLObjectType,
   selectionSet: SelectionSetNode,
   fields: Map<string, Array<FieldDetails>>,
   visitedFragmentNames: Set<string>,
-  localVariableValues?: { [variable: string]: unknown },
+  fragmentVariables?: FragmentVariables,
 ): void {
   for (const selection of selectionSet.selections) {
     switch (selection.kind) {
       case Kind.FIELD: {
-        const vars = localVariableValues ?? variableValues;
-        if (!shouldIncludeNode(vars, selection)) {
+        if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) {
           continue;
         }
         const name = getFieldEntryKey(selection);
         const fieldList = fields.get(name);
         if (fieldList !== undefined) {
           fieldList.push({
             node: selection,
-            fragmentVariableValues: localVariableValues ?? undefined,
+            fragmentVariables,
           });
         } else {
           fields.set(name, [
             {
               node: selection,
-              fragmentVariableValues: localVariableValues ?? undefined,
+              fragmentVariables,
             },
           ]);
         }
         break;
       }
       case Kind.INLINE_FRAGMENT: {
         if (
-          !shouldIncludeNode(variableValues, selection) ||
+          !shouldIncludeNode(selection, variableValues, fragmentVariables) ||
           !doesFragmentConditionMatch(schema, selection, runtimeType)
         ) {
           continue;
@@ -140,54 +151,50 @@ function collectFieldsImpl(
           selection.selectionSet,
           fields,
           visitedFragmentNames,
+          fragmentVariables,
         );
         break;
       }
       case Kind.FRAGMENT_SPREAD: {
         const fragName = selection.name.value;
         if (
           visitedFragmentNames.has(fragName) ||
-          !shouldIncludeNode(variableValues, selection)
+          !shouldIncludeNode(selection, variableValues, fragmentVariables)
         ) {
           continue;
         }
         visitedFragmentNames.add(fragName);
         const fragment = fragments[fragName];
         if (
           !fragment ||
-          !doesFragmentConditionMatch(schema, fragment, runtimeType)
+          !doesFragmentConditionMatch(schema, fragment.definition, runtimeType)
         ) {
           continue;
         }
 
-        // 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 fragmentVariableValues = fragment.variableDefinitions
-          ? getArgumentValuesFromSpread(
+        const fragmentVariableSignatures = fragment.variableSignatures;
+        let newFragmentVariables: FragmentVariables | undefined;
+        if (fragmentVariableSignatures) {
+          newFragmentVariables = {
+            signatures: fragmentVariableSignatures,
+            values: experimentalGetArgumentValues(
               selection,
-              schema,
-              fragment.variableDefinitions,
+              Object.values(fragmentVariableSignatures),
               variableValues,
-              localVariableValues,
-            )
-          : undefined;
+              fragmentVariables,
+            ),
+          };
+        }
 
         collectFieldsImpl(
           schema,
           fragments,
           variableValues,
           runtimeType,
-          fragment.selectionSet,
+          fragment.definition.selectionSet,
           fields,
           visitedFragmentNames,
-          fragmentVariableValues,
+          newFragmentVariables,
         );
         break;
       }
@@ -200,10 +207,16 @@ function collectFieldsImpl(
  * directives, where `@skip` has higher precedence than `@include`.
  */
 function shouldIncludeNode(
-  variableValues: { [variable: string]: unknown },
   node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
+  variableValues: { [variable: string]: unknown },
+  fragmentVariables: FragmentVariables | undefined,
 ): boolean {
-  const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues);
+  const skip = getDirectiveValues(
+    GraphQLSkipDirective,
+    node,
+    variableValues,
+    fragmentVariables,
+  );
   if (skip?.if === true) {
     return false;
   }
@@ -212,6 +225,7 @@ function shouldIncludeNode(
     GraphQLIncludeDirective,
     node,
     variableValues,
+    fragmentVariables,
   );
   if (include?.if === false) {
     return false;

src/execution/execute.ts

@@ -4,6 +4,7 @@ import { invariant } from '../jsutils/invariant';
 import { isIterableObject } from '../jsutils/isIterableObject';
 import { isObjectLike } from '../jsutils/isObjectLike';
 import { isPromise } from '../jsutils/isPromise';
+import { mapValue } from '../jsutils/mapValue';
 import type { Maybe } from '../jsutils/Maybe';
 import { memoize3 } from '../jsutils/memoize3';
 import type { ObjMap } from '../jsutils/ObjMap';
@@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError';
 import type {
   DocumentNode,
   FieldNode,
-  FragmentDefinitionNode,
   OperationDefinitionNode,
 } from '../language/ast';
 import { OperationTypeNode } from '../language/ast';
@@ -52,12 +52,14 @@ import {
 import type { GraphQLSchema } from '../type/schema';
 import { assertValidSchema } from '../type/validate';
 
-import type { FieldDetails } from './collectFields';
+import { getVariableSignature } from '../utilities/getVariableSignature';
+
+import type { FieldDetails, FragmentDetails } from './collectFields';
 import {
   collectFields,
   collectSubfields as _collectSubfields,
 } from './collectFields';
-import { getArgumentValues, getVariableValues } from './values';
+import { experimentalGetArgumentValues, getVariableValues } from './values';
 
 /**
  * A memoized collection of relevant subfields with regard to the return
@@ -107,7 +109,7 @@ const collectSubfields = memoize3(
  */
 export interface ExecutionContext {
   schema: GraphQLSchema;
-  fragments: ObjMap<FragmentDefinitionNode>;
+  fragments: ObjMap<FragmentDetails>;
   rootValue: unknown;
   contextValue: unknown;
   operation: OperationDefinitionNode;
@@ -290,7 +292,7 @@ export function buildExecutionContext(
   } = args;
 
   let operation: OperationDefinitionNode | undefined;
-  const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
+  const fragments: ObjMap<FragmentDetails> = Object.create(null);
   for (const definition of document.definitions) {
     switch (definition.kind) {
       case Kind.OPERATION_DEFINITION:
@@ -307,9 +309,18 @@ export function buildExecutionContext(
           operation = definition;
         }
         break;
-      case Kind.FRAGMENT_DEFINITION:
-        fragments[definition.name.value] = definition;
+      case Kind.FRAGMENT_DEFINITION: {
+        let variableSignatures;
+        if (definition.variableDefinitions) {
+          variableSignatures = Object.create(null);
+          for (const varDef of definition.variableDefinitions) {
+            const signature = getVariableSignature(schema, varDef);
+            variableSignatures[signature.name] = signature;
+          }
+        }
+        fragments[definition.name.value] = { definition, variableSignatures };
         break;
+      }
       default:
       // ignore non-executable definitions
     }
@@ -519,11 +530,11 @@ 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 = getArgumentValues(
+    const args = experimentalGetArgumentValues(
       fieldEntries[0].node,
       fieldDef.args,
       exeContext.variableValues,
-      fieldEntries[0].fragmentVariableValues,
+      fieldEntries[0].fragmentVariables,
     );
 
     // The resolve function's optional third argument is a context value that
@@ -598,7 +609,10 @@ export function buildResolveInfo(
     parentType,
     path,
     schema: exeContext.schema,
-    fragments: exeContext.fragments,
+    fragments: mapValue(
+      exeContext.fragments,
+      (fragment) => fragment.definition,
+    ),
     rootValue: exeContext.rootValue,
     operation: exeContext.operation,
     variableValues: exeContext.variableValues,