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

Author: mjmahone

src/execution/__tests__/variables-test.ts

@@ -130,14 +130,6 @@ function executeQuery(
   return executeSync({ schema, document, variableValues });
 }
 
-function executeQueryWithFragmentArguments(
-  query: string,
-  variableValues?: { [variable: string]: unknown },
-) {
-  const document = parse(query, { allowFragmentArguments: true });
-  return executeSync({ schema, document, variableValues });
-}
-
 describe('Execute: Handles inputs', () => {
   describe('Handles objects and nullability', () => {
     describe('using inline structs', () => {
@@ -1032,7 +1024,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when a value is required and provided', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a(value: "A")
         }
@@ -1049,7 +1041,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when a value is required and not provided', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a
         }
@@ -1076,7 +1068,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when the definition has a default and is provided', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a(value: "A")
         }
@@ -1093,7 +1085,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when the definition has a default and is not provided', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a
         }
@@ -1110,7 +1102,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when the definition has a non-nullable default and is provided null', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a(value: null)
         }
@@ -1137,7 +1129,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when the definition has no default and is not provided', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a
         }
@@ -1155,7 +1147,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when the argument variable is nested in a complex type', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a(value: "C")
         }
@@ -1172,7 +1164,7 @@ describe('Execute: Handles inputs', () => {
     });
 
     it('when argument variables are used recursively', () => {
-      const result = executeQueryWithFragmentArguments(`
+      const result = executeQuery(`
         query {
           ...a(aValue: "C")
         }

src/language/__tests__/parser-test.ts

@@ -366,19 +366,13 @@ describe('Parser', () => {
   it('allows parsing fragment defined variables', () => {
     const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
 
-    expect(() =>
-      parse(document, { allowFragmentArguments: 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, { allowFragmentArguments: true }),
-    ).to.not.throw();
-    expect(() => parse(document)).to.throw('Syntax Error');
+    expect(() => parse(document)).to.not.throw();
   });
 
   it('contains location information that only stringifies start/end', () => {

src/language/__tests__/printer-test.ts

@@ -116,7 +116,6 @@ describe('Printer: Query document', () => {
   it('prints fragment with variable directives', () => {
     const queryASTWithVariableDirective = parse(
       'fragment Foo($foo: TestType @test) on TestType @testDirective { id }',
-      { allowFragmentArguments: true },
     );
     expect(print(queryASTWithVariableDirective)).to.equal(dedent`
       fragment Foo($foo: TestType @test) on TestType @testDirective {
@@ -132,7 +131,6 @@ describe('Printer: Query document', () => {
           id
         }
       `,
-      { allowFragmentArguments: true },
     );
     expect(print(fragmentWithVariable)).to.equal(dedent`
       fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
@@ -144,7 +142,6 @@ describe('Printer: Query document', () => {
   it('prints fragment spread with arguments', () => {
     const queryASTWithVariableDirective = parse(
       'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }',
-      { allowFragmentArguments: true },
     );
     expect(print(queryASTWithVariableDirective)).to.equal(dedent`
       fragment Foo on TestType {
@@ -156,7 +153,6 @@ describe('Printer: Query document', () => {
   it('prints fragment spread with multi-line arguments', () => {
     const queryASTWithVariableDirective = 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") }',
-      { allowFragmentArguments: true },
     );
     expect(print(queryASTWithVariableDirective)).to.equal(dedent`
       fragment Foo on TestType {

src/language/__tests__/visitor-test.ts

@@ -465,7 +465,6 @@ describe('Visitor', () => {
   it('visits variables defined in fragments', () => {
     const ast = parse('fragment a($v: Boolean = false) on t { f }', {
       noLocation: true,
-      allowFragmentArguments: true,
     });
     const visited: Array<any> = [];
 
@@ -515,7 +514,6 @@ describe('Visitor', () => {
   it('visits arguments on fragment spreads', () => {
     const ast = parse('fragment a on t { ...s(v: false) }', {
       noLocation: true,
-      allowFragmentArguments: true,
     });
     const visited: Array<any> = [];
 

src/language/parser.ts

@@ -80,24 +80,6 @@ export interface ParseOptions {
    * disables that behavior for performance or testing.
    */
   noLocation?: boolean;
-
-  /**
-   * If enabled, the parser will understand and parse variable definitions
-   * contained in a fragment definition. They'll be represented in the
-   * `variableDefinitions` field of the FragmentDefinitionNode.
-   *
-   * Additionally, the parser will understand arguments passed to fragment
-   * spreads. They'll be represented in the `arguments` field of the
-   * FragmentSpreadNode.
-   *
-   * The syntax is identical to normal, query-defined variables. For example:
-   *
-   *   fragment A($var: Boolean = false) on T  {
-   *     ...
-   *   }
-   *
-   */
-  allowFragmentArguments?: boolean;
 }
 
 /**
@@ -450,10 +432,7 @@ export class Parser {
     const hasTypeCondition = this.expectOptionalKeyword('on');
     if (!hasTypeCondition && this.peek(TokenKind.NAME)) {
       const name = this.parseFragmentName();
-      if (
-        this._options?.allowFragmentArguments === true &&
-        this.peek(TokenKind.PAREN_L)
-      ) {
+      if (this.peek(TokenKind.PAREN_L)) {
         return this.node<FragmentSpreadNode>(start, {
           kind: Kind.FRAGMENT_SPREAD,
           name,
@@ -484,13 +463,11 @@ export class Parser {
   parseFragmentDefinition(): FragmentDefinitionNode {
     const start = this._lexer.token;
     this.expectKeyword('fragment');
-    // Legacy support for defining variables within fragments changes
-    // the grammar of FragmentDefinition:
-    //   - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet
-    if (this._options?.allowFragmentArguments === true) {
+    const name = this.parseFragmentName();
+    if (this.peek(TokenKind.PAREN_L)) {
       return this.node<FragmentDefinitionNode>(start, {
         kind: Kind.FRAGMENT_DEFINITION,
-        name: this.parseFragmentName(),
+        name,
         variableDefinitions: this.parseVariableDefinitions(),
         typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
         directives: this.parseDirectives(false),
@@ -499,7 +476,7 @@ export class Parser {
     }
     return this.node<FragmentDefinitionNode>(start, {
       kind: Kind.FRAGMENT_DEFINITION,
-      name: this.parseFragmentName(),
+      name,
       typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
       directives: this.parseDirectives(false),
       selectionSet: this.parseSelectionSet(),

src/utilities/buildASTSchema.ts

@@ -99,7 +99,6 @@ export function buildSchema(
 ): GraphQLSchema {
   const document = parse(source, {
     noLocation: options?.noLocation,
-    allowFragmentArguments: options?.allowFragmentArguments,
   });
 
   return buildASTSchema(document, {

src/validation/__tests__/NoFragmentArgumentUsageRule-test.ts

@@ -0,0 +1,50 @@
+import { describe, it } from 'mocha';
+
+import { NoFragmentArgumentUsageRule } from '../rules/custom/NoFragmentArgumentUsageRule';
+
+import { expectValidationErrors } from './harness';
+
+function expectErrors(queryStr: string) {
+  return expectValidationErrors(NoFragmentArgumentUsageRule, queryStr);
+}
+
+function expectValid(queryStr: string) {
+  expectErrors(queryStr).to.deep.equal([]);
+}
+
+describe('Validate: Known fragment names', () => {
+  it('known fragment names are valid', () => {
+    expectValid(`
+      {
+        ...HumanFields
+      }
+      fragment HumanFields on Query {
+        human(id: 4) {
+          name
+        }
+      }
+    `);
+  });
+
+  it('unknown fragment names are invalid', () => {
+    expectErrors(`
+      {
+        ...HumanFields(x: 4)
+      }
+      fragment HumanFields($x: ID) on Query {
+        human(id: $x) {
+          name
+        }
+      }
+    `).to.deep.equal([
+      {
+        message: 'Fragment arguments are not enabled.',
+        locations: [{ line: 3, column: 24 }],
+      },
+      {
+        message: 'Fragment argument definitions are not enabled.',
+        locations: [{ line: 5, column: 28 }],
+      },
+    ]);
+  });
+});

src/validation/index.ts

@@ -96,3 +96,4 @@ export { PossibleTypeExtensionsRule } from './rules/PossibleTypeExtensionsRule';
 /** Optional rules not defined by the GraphQL Specification */
 export { NoDeprecatedCustomRule } from './rules/custom/NoDeprecatedCustomRule';
 export { NoSchemaIntrospectionCustomRule } from './rules/custom/NoSchemaIntrospectionCustomRule';
+export { NoFragmentArgumentUsageRule } from './rules/custom/NoFragmentArgumentUsageRule';

src/validation/rules/custom/NoFragmentArgumentUsageRule.ts

@@ -0,0 +1,35 @@
+import { GraphQLError } from '../../../error/GraphQLError';
+
+import type { ASTVisitor } from '../../../language/visitor';
+
+import type { ASTValidationContext } from '../../ValidationContext';
+
+/**
+ * Fragment arguments are, by default, not allowed to be used.
+ */
+export function NoFragmentArgumentUsageRule(
+  context: ASTValidationContext,
+): ASTVisitor {
+  return {
+    FragmentDefinition(node) {
+      if (node.variableDefinitions && node.variableDefinitions.length > 0) {
+        context.reportError(
+          new GraphQLError(
+            'Fragment argument definitions are not enabled.',
+            node.variableDefinitions[0],
+          ),
+        );
+      }
+    },
+    FragmentSpread(node) {
+      if (node.arguments && node.arguments.length > 0) {
+        context.reportError(
+          new GraphQLError(
+            'Fragment arguments are not enabled.',
+            node.arguments[0],
+          ),
+        );
+      }
+    },
+  };
+}

src/validation/specifiedRules.ts

@@ -93,6 +93,9 @@ import { UniqueFieldDefinitionNamesRule } from './rules/UniqueFieldDefinitionNam
 import { UniqueDirectiveNamesRule } from './rules/UniqueDirectiveNamesRule';
 import { PossibleTypeExtensionsRule } from './rules/PossibleTypeExtensionsRule';
 
+// Prevent not-yet-in-spec usage of Fragment Arguments by default
+import { NoFragmentArgumentUsageRule } from './rules/custom/NoFragmentArgumentUsageRule';
+
 /**
  * This set includes all validation rules defined by the GraphQL spec.
  *
@@ -114,6 +117,7 @@ export const specifiedRules: ReadonlyArray<ValidationRule> = Object.freeze([
   NoUnusedFragmentsRule,
   PossibleFragmentSpreadsRule,
   NoFragmentCyclesRule,
+  NoFragmentArgumentUsageRule,
   UniqueVariableNamesRule,
   NoUndefinedVariablesRule,
   NoUnusedVariablesRule,