Add fragment arguments to AST and related utility functions

Fragment Args rebased for 2022

Add fragment arguments to AST and related utility functions

Replace fragment argument variables at execution time with passed-in arguments.

Ensure arguments for unset fragment argument variables are removed so they execute as "unset" rather than as an invalid, undefined variable.

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

Remove flag for parsing, but add default validation to indicate fragment args are not yet spec-supported, and may be missing validation.

Update collectFields: no longer throws errors that validation should catch

Back to main: quicker to reset and manually redo than to deal with stacked rebase conflicts. Will squash this commit away

Re-apply changes, address olds comments, but do not start on functional work

Author: mjmahone

src/execution/__tests__/variables-test.ts

@@ -1006,6 +1006,156 @@ 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 = executeQuery(`
+        query {
+          ...a(value: "A")
+        }
+        fragment a($value: String!) on TestType {
+          fieldWithNonNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNonNullableStringInput: '"A"',
+        },
+      });
+    });
+
+    it('when a value is required and not provided', () => {
+      const result = executeQuery(`
+        query {
+          ...a
+        }
+        fragment a($value: String!) on TestType {
+          fieldWithNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNullableStringInput: null,
+        },
+      });
+    });
+
+    it('when the definition has a default and is provided', () => {
+      const result = executeQuery(`
+        query {
+          ...a(value: "A")
+        }
+        fragment a($value: String! = "B") on TestType {
+          fieldWithNonNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNonNullableStringInput: '"A"',
+        },
+      });
+    });
+
+    it('when the definition has a default and is not provided', () => {
+      const result = executeQuery(`
+        query {
+          ...a
+        }
+        fragment a($value: String! = "B") on TestType {
+          fieldWithNonNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNonNullableStringInput: '"B"',
+        },
+      });
+    });
+
+    it('when the definition has a non-nullable default and is provided null', () => {
+      const result = executeQuery(`
+        query {
+          ...a(value: null)
+        }
+        fragment a($value: String! = "B") on TestType {
+          fieldWithNullableStringInput(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNullableStringInput: 'null',
+        },
+      });
+    });
+
+    it('when the definition has no default and is not provided', () => {
+      const result = executeQuery(`
+        query {
+          ...a
+        }
+        fragment a($value: String) on TestType {
+          fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value)
+        }
+      `);
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithNonNullableStringInputAndDefaultArgumentValue:
+            '"Hello World"',
+        },
+      });
+    });
+
+    it('when the argument variable is nested in a complex type', () => {
+      const result = executeQuery(`
+        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 = executeQuery(`
+        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', () => {
     const doc = parse(`
       query ($input: [String!]) {

src/execution/collectFields.ts

@@ -2,14 +2,17 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap.js';
 import type { ObjMap } from '../jsutils/ObjMap.js';
 
 import type {
+  ArgumentNode,
   FieldNode,
   FragmentDefinitionNode,
   FragmentSpreadNode,
   InlineFragmentNode,
   SelectionSetNode,
+  ValueNode,
+  VariableDefinitionNode,
 } from '../language/ast.js';
 import { Kind } from '../language/kinds.js';
-
+import { visit } from '../language/visitor.js';
 import type { GraphQLObjectType } from '../type/definition.js';
 import { isAbstractType } from '../type/definition.js';
 import {
@@ -186,6 +189,12 @@ function collectFieldsImpl(
         ) {
           continue;
         }
+        const selectionSetWithAppliedArguments =
+          selectionSetWithFragmentArgumentsApplied(
+            fragment.argumentDefinitions,
+            selection.arguments,
+            fragment.selectionSet,
+          );
 
         if (!defer) {
           visitedFragmentNames.add(fragName);
@@ -198,7 +207,7 @@ function collectFieldsImpl(
             fragments,
             variableValues,
             runtimeType,
-            fragment.selectionSet,
+            selectionSetWithAppliedArguments,
             patchFields,
             patches,
             visitedFragmentNames,
@@ -213,7 +222,7 @@ function collectFieldsImpl(
             fragments,
             variableValues,
             runtimeType,
-            fragment.selectionSet,
+            selectionSetWithAppliedArguments,
             fields,
             patches,
             visitedFragmentNames,
@@ -225,6 +234,41 @@ function collectFieldsImpl(
   }
 }
 
+function selectionSetWithFragmentArgumentsApplied(
+  argumentDefinitions: Readonly<Array<VariableDefinitionNode>> | undefined,
+  fragmentArguments: Readonly<Array<ArgumentNode>> | undefined,
+  selectionSet: SelectionSetNode,
+): SelectionSetNode {
+  const providedArguments = new Map<string, ArgumentNode>();
+  if (fragmentArguments) {
+    for (const argument of fragmentArguments) {
+      providedArguments.set(argument.name.value, argument);
+    }
+  }
+
+  const fragmentArgumentValues = new Map<string, ValueNode>();
+  if (argumentDefinitions) {
+    for (const argumentDef of argumentDefinitions) {
+      const variableName = argumentDef.variable.name.value;
+      const providedArg = providedArguments.get(variableName);
+      if (providedArg) {
+        // Not valid if the providedArg is null and argumentDef is non-null
+        fragmentArgumentValues.set(variableName, providedArg.value);
+      } else if (argumentDef.defaultValue) {
+        fragmentArgumentValues.set(variableName, argumentDef.defaultValue);
+      }
+      // If argumentDef is non-null, expect a provided arg or non-null default value.
+      // Otherwise just preserve the variable as-is: it will be treated as unset by the executor.
+    }
+  }
+
+  return visit(selectionSet, {
+    Variable(node) {
+      return fragmentArgumentValues.get(node.name.value);
+    },
+  });
+}
+
 /**
  * Returns an object containing the `@defer` arguments if a field should be
  * deferred based on the experimental flag, defer directive present and

src/language/__tests__/parser-test.ts

@@ -607,13 +607,16 @@ describe('Parser', () => {
     expect('loc' in result).to.equal(false);
   });
 
-  it('Legacy: allows parsing fragment defined variables', () => {
+  it('allows parsing fragment defined arguments', () => {
     const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
 
-    expect(() =>
-      parse(document, { allowLegacyFragmentVariables: true }),
-    ).to.not.throw();
-    expect(() => parse(document)).to.throw('Syntax Error');
+    expect(() => parse(document)).to.not.throw();
+  });
+
+  it('allows parsing fragment spread arguments', () => {
+    const document = 'fragment a on t { ...b(v: $v) }';
+
+    expect(() => parse(document)).to.not.throw();
   });
 
   it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => {

src/language/__tests__/printer-test.ts

@@ -110,34 +110,58 @@ describe('Printer: Query document', () => {
     `);
   });
 
-  it('Legacy: prints fragment with variable directives', () => {
-    const queryASTWithVariableDirective = parse(
+  it('prints fragment with argument definition directives', () => {
+    const fragmentWithArgumentDefinitionDirective = parse(
       'fragment Foo($foo: TestType @test) on TestType @testDirective { id }',
-      { allowLegacyFragmentVariables: true },
     );
-    expect(print(queryASTWithVariableDirective)).to.equal(dedent`
+    expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent`
       fragment Foo($foo: TestType @test) on TestType @testDirective {
         id
       }
     `);
   });
 
-  it('Legacy: correctly prints fragment defined variables', () => {
-    const fragmentWithVariable = parse(
+  it('correctly prints fragment defined arguments', () => {
+    const fragmentWithArgumentDefinition = parse(
       `
         fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
           id
         }
       `,
-      { allowLegacyFragmentVariables: true },
     );
-    expect(print(fragmentWithVariable)).to.equal(dedent`
+    expect(print(fragmentWithArgumentDefinition)).to.equal(dedent`
       fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
         id
       }
     `);
   });
 
+  it('prints fragment spread with arguments', () => {
+    const fragmentSpreadWithArguments = parse(
+      'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }',
+    );
+    expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
+      fragment Foo on TestType {
+        ...Bar(a: { x: $x }, b: true)
+      }
+    `);
+  });
+
+  it('prints fragment spread with multi-line arguments', () => {
+    const fragmentSpreadWithArguments = parse(
+      'fragment Foo on TestType { ...Bar(a: {x: $x, y: $y, z: $z, xy: $xy}, b: true, c: "a long string extending arguments over max length") }',
+    );
+    expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
+      fragment Foo on TestType {
+        ...Bar(
+          a: { x: $x, y: $y, z: $z, xy: $xy }
+          b: true
+          c: "a long string extending arguments over max length"
+        )
+      }
+    `);
+  });
+
   it('prints kitchen sink without altering ast', () => {
     const ast = parse(kitchenSinkQuery, {
       noLocation: true,

src/language/__tests__/visitor-test.ts

@@ -455,10 +455,9 @@ describe('Visitor', () => {
     ]);
   });
 
-  it('Legacy: visits variables defined in fragments', () => {
+  it('visits arguments defined on fragments', () => {
     const ast = parse('fragment a($v: Boolean = false) on t { f }', {
       noLocation: true,
-      allowLegacyFragmentVariables: true,
     });
     const visited: Array<any> = [];
 
@@ -505,6 +504,49 @@ describe('Visitor', () => {
     ]);
   });
 
+  it('visits arguments on fragment spreads', () => {
+    const ast = parse('fragment a on t { ...s(v: false) }', {
+      noLocation: true,
+    });
+    const visited: Array<any> = [];
+
+    visit(ast, {
+      enter(node) {
+        checkVisitorFnArgs(ast, arguments);
+        visited.push(['enter', node.kind, getValue(node)]);
+      },
+      leave(node) {
+        checkVisitorFnArgs(ast, arguments);
+        visited.push(['leave', node.kind, getValue(node)]);
+      },
+    });
+
+    expect(visited).to.deep.equal([
+      ['enter', 'Document', undefined],
+      ['enter', 'FragmentDefinition', undefined],
+      ['enter', 'Name', 'a'],
+      ['leave', 'Name', 'a'],
+      ['enter', 'NamedType', undefined],
+      ['enter', 'Name', 't'],
+      ['leave', 'Name', 't'],
+      ['leave', 'NamedType', undefined],
+      ['enter', 'SelectionSet', undefined],
+      ['enter', 'FragmentSpread', undefined],
+      ['enter', 'Name', 's'],
+      ['leave', 'Name', 's'],
+      ['enter', 'Argument', { kind: 'BooleanValue', value: false }],
+      ['enter', 'Name', 'v'],
+      ['leave', 'Name', 'v'],
+      ['enter', 'BooleanValue', false],
+      ['leave', 'BooleanValue', false],
+      ['leave', 'Argument', { kind: 'BooleanValue', value: false }],
+      ['leave', 'FragmentSpread', undefined],
+      ['leave', 'SelectionSet', undefined],
+      ['leave', 'FragmentDefinition', undefined],
+      ['leave', 'Document', undefined],
+    ]);
+  });
+
   it('n', () => {
     const ast = parse(kitchenSinkQuery, {
       experimentalClientControlledNullability: true,