Updates to get to 100% code coverage, plus simplify visit so it works even with nested variable inputs

Author: mjmahone

src/execution/__tests__/variables-test.ts

@@ -1014,6 +1014,23 @@ describe('Execute: Handles inputs', () => {
   });
 
   describe('using fragment arguments', () => {
+    it('when there are no fragment arguments', () => {
+      const result = executeQuery(`
+        query {
+          ...a
+        }
+
+        fragment a on TestType {
+          fieldWithNonNullableStringInput(input: "A")
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNonNullableStringInput: '"A"',
+        },
+      });
+    });
+
     it('when a value is required and provided', () => {
       const result = executeQueryWithFragmentArguments(`
         query {
@@ -1031,6 +1048,33 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
+    it('when a value is required and not provided', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a
+        }
+
+        fragment a($value: String!) on TestType {
+          fieldWithNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNullableStringInput: null,
+        },
+        errors: [
+          {
+            message:
+              'Fragment argument "$value" on fragment "a" is required but not provided.',
+            locations: [
+              { line: 3, column: 11 },
+              { line: 6, column: 20 },
+            ],
+          },
+        ],
+      });
+    });
+
     it('when the definition has a default and is provided', () => {
       const result = executeQueryWithFragmentArguments(`
         query {
@@ -1065,6 +1109,33 @@ describe('Execute: Handles inputs', () => {
       });
     });
 
+    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.deep.equal({
+        data: {
+          fieldWithNullableStringInput: 'null',
+        },
+        errors: [
+          {
+            message:
+              'Fragment argument "$value" on fragment "a" is non-null, but null was provided.',
+            locations: [
+              { line: 3, column: 16 },
+              { line: 6, column: 20 },
+            ],
+          },
+        ],
+      });
+    });
+
     it('when the definition has no default and is not provided', () => {
       const result = executeQueryWithFragmentArguments(`
         query {
@@ -1082,6 +1153,44 @@ describe('Execute: Handles inputs', () => {
         },
       });
     });
+
+    it('when the argument variable is nested in a complex type', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(value: "C")
+        }
+
+        fragment a($value: String) on TestType {
+          list(input: ["A", "B", $value, "D"])
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          list: '["A", "B", "C", "D"]',
+        },
+      });
+    });
+
+    it('when argument variables are used recursively', () => {
+      const result = executeQueryWithFragmentArguments(`
+        query {
+          ...a(aValue: "C")
+        }
+
+        fragment a($aValue: String) on TestType {
+          ...b(bValue: $aValue)
+        }
+
+        fragment b($bValue: String) on TestType {
+          list(input: ["A", "B", $bValue, "D"])
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          list: '["A", "B", "C", "D"]',
+        },
+      });
+    });
   });
 
   describe('getVariableValues: limit maximum number of coercion errors', () => {

src/execution/execute.ts

@@ -538,20 +538,24 @@ function selectionSetWithFragmentArgumentsApplied(
   fragment: FragmentDefinitionNode,
   fragmentSpread: FragmentSpreadNode,
 ): SelectionSetNode {
-  if (fragment.variableDefinitions == null) {
+  const variableDefinitions = fragment.variableDefinitions;
+  if (variableDefinitions == null) {
     return fragment.selectionSet;
   }
 
   const providedArguments: Map<string, ArgumentNode> = new Map();
   for (const arg of fragmentSpread.arguments ?? []) {
     providedArguments.set(arg.name.value, arg);
   }
-  const fragmentArgumentValues: Map<string, ValueNode | null> = new Map();
-  for (const argDef of fragment.variableDefinitions ?? []) {
+  const fragmentArgumentValues: Map<string, ValueNode> = new Map();
+  for (const argDef of variableDefinitions) {
     const variableName = argDef.variable.name.value;
     const providedArg = providedArguments.get(variableName);
     if (providedArg != null) {
-      if (providedArg.value.kind === Kind.NULL && isNonNullType(argDef.type)) {
+      if (
+        providedArg.value.kind === Kind.NULL &&
+        argDef.type.kind === Kind.NON_NULL_TYPE
+      ) {
         exeContext.errors.push(
           new GraphQLError(
             `Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is non-null, but null was provided.`,
@@ -562,34 +566,22 @@ function selectionSetWithFragmentArgumentsApplied(
       fragmentArgumentValues.set(variableName, providedArg.value);
     } else if (argDef.defaultValue != null) {
       fragmentArgumentValues.set(variableName, argDef.defaultValue);
-    } else {
-      if (isNonNullType(argDef.type)) {
-        exeContext.errors.push(
-          new GraphQLError(
-            `Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is required but not provided.`,
-            [fragmentSpread, argDef],
-          ),
-        );
-      }
-      fragmentArgumentValues.set(variableName, null);
+    } else if (argDef.type.kind === Kind.NON_NULL_TYPE) {
+      exeContext.errors.push(
+        new GraphQLError(
+          `Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is required but not provided.`,
+          [fragmentSpread, argDef],
+        ),
+      );
     }
+    // Otherwise just preserve the variable as-is: it will be treated as unset by the executor.
   }
 
   return visit(fragment.selectionSet, {
-    Argument(node) {
-      if (node.value.kind === Kind.VARIABLE) {
-        const variable = node.value;
-        if (fragmentArgumentValues.has(variable.name.value)) {
-          const replacementValue = fragmentArgumentValues.get(
-            variable.name.value,
-          );
-          if (replacementValue == null) {
-            // Remove the argument if the provided value is unset.
-            return null;
-          }
-
-          return { ...node, value: replacementValue };
-        }
+    Variable(node) {
+      const replacementValue = fragmentArgumentValues.get(node.name.value);
+      if (replacementValue != null) {
+        return replacementValue;
       }
     },
   });

src/language/parser.ts

@@ -449,17 +449,21 @@ export class Parser {
 
     const hasTypeCondition = this.expectOptionalKeyword('on');
     if (!hasTypeCondition && this.peek(TokenKind.NAME)) {
-      if (this._options?.allowFragmentArguments === true) {
+      const name = this.parseFragmentName();
+      if (
+        this._options?.allowFragmentArguments === true &&
+        this.peek(TokenKind.PAREN_L)
+      ) {
         return this.node<FragmentSpreadNode>(start, {
           kind: Kind.FRAGMENT_SPREAD,
-          name: this.parseFragmentName(),
+          name,
           arguments: this.parseArguments(false),
           directives: this.parseDirectives(false),
         });
       }
       return this.node<FragmentSpreadNode>(start, {
         kind: Kind.FRAGMENT_SPREAD,
-        name: this.parseFragmentName(),
+        name,
         directives: this.parseDirectives(false),
       });
     }