[RFC] Allow directives on variable definitions (#1437)

* [RFC] Allow directives on variable definitions * Make parser expect const directives, and move to match position with InputValueDefinition * Adding to introspection query, and the KnownDirectives validation rule
graphql · Aug 7, 2018 · 1d7efa9 · 1d7efa9
1 parent 3217802
commit 1d7efa9
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 10 deletions.
diff --git a/src/language/__tests__/parser-test.js b/src/language/__tests__/parser-test.js
@@ -106,6 +106,12 @@ describe('Parser', () => {
     );
   });
 
+  it('parses variable definition directives', () => {
+    expect(() =>
+      parse('query Foo($x: Boolean = false @bar) { field }'),
+    ).to.not.throw();
+  });
+
   it('does not accept fragments named "on"', () => {
     expectSyntaxError('fragment on on on { on }', 'Unexpected Name "on"', {
       line: 1,

diff --git a/src/language/__tests__/printer-test.js b/src/language/__tests__/printer-test.js
@@ -64,6 +64,15 @@ describe('Printer: Query document', () => {
       }
     `);
 
+    const queryAstWithVariableDirective = parse(
+      'query ($foo: TestType = {a: 123} @testDirective(if: true) @test) { id }',
+    );
+    expect(print(queryAstWithVariableDirective)).to.equal(dedent`
+      query ($foo: TestType = {a: 123} @testDirective(if: true) @test) {
+        id
+      }
+    `);
+
     const mutationAstWithArtifacts = parse(
       'mutation ($foo: TestType) @testDirective { id, name }',
     );

diff --git a/src/language/ast.js b/src/language/ast.js
@@ -225,6 +225,7 @@ export type VariableDefinitionNode = {
   +variable: VariableNode,
   +type: TypeNode,
   +defaultValue?: ValueNode,
+  +directives?: $ReadOnlyArray<DirectiveNode>,
 };
 
 export type VariableNode = {

diff --git a/src/language/directiveLocation.js b/src/language/directiveLocation.js
@@ -19,6 +19,7 @@ export const DirectiveLocation = Object.freeze({
   FRAGMENT_DEFINITION: 'FRAGMENT_DEFINITION',
   FRAGMENT_SPREAD: 'FRAGMENT_SPREAD',
   INLINE_FRAGMENT: 'INLINE_FRAGMENT',
+  VARIABLE_DEFINITION: 'VARIABLE_DEFINITION',
   // Type System Definitions
   SCHEMA: 'SCHEMA',
   SCALAR: 'SCALAR',

diff --git a/src/language/parser.js b/src/language/parser.js
@@ -332,7 +332,7 @@ function parseVariableDefinitions(
 }
 
 /**
- * VariableDefinition : Variable : Type DefaultValue?
+ * VariableDefinition : Variable : Type DefaultValue? Directives[Const]?
  */
 function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode {
   const start = lexer.token;
@@ -343,6 +343,7 @@ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode {
     defaultValue: skip(lexer, TokenKind.EQUALS)
       ? parseValueLiteral(lexer, true)
       : undefined,
+    directives: parseDirectives(lexer, true),
     loc: loc(lexer, start),
   };
 }

diff --git a/src/language/printer.js b/src/language/printer.js
@@ -36,9 +36,12 @@ const printDocASTReducer = {
       : join([op, join([name, varDefs]), directives, selectionSet], ' ');
   },
 
-  VariableDefinition: ({ variable, type, defaultValue }) =>
-    variable + ': ' + type + wrap(' = ', defaultValue),
-
+  VariableDefinition: ({ variable, type, defaultValue, directives }) =>
+    variable +
+    ': ' +
+    type +
+    wrap(' = ', defaultValue) +
+    wrap(' ', join(directives, ' ')),
   SelectionSet: ({ selections }) => block(selections),
 
   Field: ({ alias, name, arguments: args, directives, selectionSet }) =>

diff --git a/src/language/visitor.js b/src/language/visitor.js
@@ -64,7 +64,7 @@ export const QueryDocumentKeys = {
     'directives',
     'selectionSet',
   ],
-  VariableDefinition: ['variable', 'type', 'defaultValue'],
+  VariableDefinition: ['variable', 'type', 'defaultValue', 'directives'],
   Variable: ['name'],
   SelectionSet: ['selections'],
   Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'],

diff --git a/src/type/__tests__/introspection-test.js b/src/type/__tests__/introspection-test.js
@@ -746,6 +746,11 @@ describe('Introspection', () => {
                   isDeprecated: false,
                   deprecationReason: null,
                 },
+                {
+                  name: 'VARIABLE_DEFINITION',
+                  isDeprecated: false,
+                  deprecationReason: null,
+                },
                 {
                   name: 'SCHEMA',
                   isDeprecated: false,

diff --git a/src/type/introspection.js b/src/type/introspection.js
@@ -135,6 +135,10 @@ export const __DirectiveLocation = new GraphQLEnumType({
       value: DirectiveLocation.INLINE_FRAGMENT,
       description: 'Location adjacent to an inline fragment.',
     },
+    VARIABLE_DEFINITION: {
+      value: DirectiveLocation.VARIABLE_DEFINITION,
+      description: 'Location adjacent to a variable definition.',
+    },
     SCHEMA: {
       value: DirectiveLocation.SCHEMA,
       description: 'Location adjacent to a schema definition.',

diff --git a/src/utilities/__tests__/schemaPrinter-test.js b/src/utilities/__tests__/schemaPrinter-test.js
@@ -676,6 +676,9 @@ describe('Type System Printer', () => {
         """Location adjacent to an inline fragment."""
         INLINE_FRAGMENT
 
+        """Location adjacent to a variable definition."""
+        VARIABLE_DEFINITION
+
         """Location adjacent to a schema definition."""
         SCHEMA
 
@@ -904,6 +907,9 @@ describe('Type System Printer', () => {
         # Location adjacent to an inline fragment.
         INLINE_FRAGMENT
 
+        # Location adjacent to a variable definition.
+        VARIABLE_DEFINITION
+
         # Location adjacent to a schema definition.
         SCHEMA
 

diff --git a/src/validation/__tests__/KnownDirectives-test.js b/src/validation/__tests__/KnownDirectives-test.js
@@ -127,8 +127,8 @@ describe('Validate: Known directives', () => {
     expectPassesRule(
       KnownDirectives,
       `
-      query Foo @onQuery {
-        name @include(if: true)
+      query Foo($var: Boolean @onVariableDefinition) @onQuery {
+        name @include(if: $var)
         ...Frag @include(if: true)
         skippedField @skip(if: true)
         ...SkippedFrag @skip(if: true)
@@ -145,8 +145,8 @@ describe('Validate: Known directives', () => {
     expectFailsRule(
       KnownDirectives,
       `
-      query Foo @include(if: true) {
-        name @onQuery
+      query Foo($var: Boolean @onField) @include(if: true) {
+        name @onQuery @include(if: $var)
         ...Frag @onQuery
       }
 
@@ -155,7 +155,8 @@ describe('Validate: Known directives', () => {
       }
     `,
       [
-        misplacedDirective('include', 'QUERY', 2, 17),
+        misplacedDirective('onField', 'VARIABLE_DEFINITION', 2, 31),
+        misplacedDirective('include', 'QUERY', 2, 41),
         misplacedDirective('onQuery', 'FIELD', 3, 14),
         misplacedDirective('onQuery', 'FRAGMENT_SPREAD', 4, 17),
         misplacedDirective('onQuery', 'MUTATION', 7, 20),

diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js
@@ -374,6 +374,10 @@ export const testSchema = new GraphQLSchema({
       name: 'onInlineFragment',
       locations: ['INLINE_FRAGMENT'],
     }),
+    new GraphQLDirective({
+      name: 'onVariableDefinition',
+      locations: ['VARIABLE_DEFINITION'],
+    }),
   ],
 });
 

diff --git a/src/validation/rules/KnownDirectives.js b/src/validation/rules/KnownDirectives.js
@@ -98,6 +98,8 @@ function getDirectiveLocationForASTPath(ancestors) {
         return DirectiveLocation.INLINE_FRAGMENT;
       case Kind.FRAGMENT_DEFINITION:
         return DirectiveLocation.FRAGMENT_DEFINITION;
+      case Kind.VARIABLE_DEFINITION:
+        return DirectiveLocation.VARIABLE_DEFINITION;
       case Kind.SCHEMA_DEFINITION:
       case Kind.SCHEMA_EXTENSION:
         return DirectiveLocation.SCHEMA;