From a30ced7f7af65e6174714338d6b89a14b7d849a4 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Sat, 28 Jul 2018 18:39:01 -0400 Subject: [PATCH 1/3] [RFC] Allow directives on variable definitions --- src/language/__tests__/parser-test.js | 6 ++++++ src/language/__tests__/printer-test.js | 9 +++++++++ src/language/ast.js | 1 + src/language/directiveLocation.js | 1 + src/language/parser.js | 3 ++- src/language/printer.js | 8 ++++++-- src/language/visitor.js | 2 +- 7 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/language/__tests__/parser-test.js b/src/language/__tests__/parser-test.js index 59eeda2049..5e8498d6e1 100644 --- 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 @bar = false) { 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 index d782554e66..18f7d18377 100644 --- 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 @testDirective(if: true) @test = {a: 123}) { id }', + ); + expect(print(queryAstWithVariableDirective)).to.equal(dedent` + query ($foo: TestType @testDirective(if: true) @test = {a: 123}) { + id + } + `); + const mutationAstWithArtifacts = parse( 'mutation ($foo: TestType) @testDirective { id, name }', ); diff --git a/src/language/ast.js b/src/language/ast.js index 000bc3bdad..83b6025421 100644 --- a/src/language/ast.js +++ b/src/language/ast.js @@ -224,6 +224,7 @@ export type VariableDefinitionNode = { +loc?: Location, +variable: VariableNode, +type: TypeNode, + +directives?: $ReadOnlyArray, +defaultValue?: ValueNode, }; diff --git a/src/language/directiveLocation.js b/src/language/directiveLocation.js index 0a87001c36..e63ec67c0d 100644 --- 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 index fdbfce0812..40d9120d17 100644 --- a/src/language/parser.js +++ b/src/language/parser.js @@ -332,7 +332,7 @@ function parseVariableDefinitions( } /** - * VariableDefinition : Variable : Type DefaultValue? + * VariableDefinition : Variable : Type Directives? DefaultValue? */ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode { const start = lexer.token; @@ -340,6 +340,7 @@ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode { kind: Kind.VARIABLE_DEFINITION, variable: parseVariable(lexer), type: (expect(lexer, TokenKind.COLON), parseTypeReference(lexer)), + directives: parseDirectives(lexer, false), defaultValue: skip(lexer, TokenKind.EQUALS) ? parseValueLiteral(lexer, true) : undefined, diff --git a/src/language/printer.js b/src/language/printer.js index 0242898f52..af4398e7e7 100644 --- a/src/language/printer.js +++ b/src/language/printer.js @@ -36,8 +36,12 @@ const printDocASTReducer = { : join([op, join([name, varDefs]), directives, selectionSet], ' '); }, - VariableDefinition: ({ variable, type, defaultValue }) => - variable + ': ' + type + wrap(' = ', defaultValue), + VariableDefinition: ({ variable, type, directives, defaultValue }) => + variable + + ': ' + + type + + wrap(' ', join(directives, ' ')) + + wrap(' = ', defaultValue), SelectionSet: ({ selections }) => block(selections), diff --git a/src/language/visitor.js b/src/language/visitor.js index 32c1af78ba..7c5c96734c 100644 --- 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', 'directives', 'defaultValue'], Variable: ['name'], SelectionSet: ['selections'], Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], From c233c78159b28ed33ce29a05dfa9836acb8708c9 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Sun, 29 Jul 2018 14:43:32 -0400 Subject: [PATCH 2/3] Make parser expect const directives, and move to match position with InputValueDefinition --- src/language/__tests__/parser-test.js | 2 +- src/language/__tests__/printer-test.js | 4 ++-- src/language/ast.js | 2 +- src/language/parser.js | 4 ++-- src/language/printer.js | 7 +++---- src/language/visitor.js | 2 +- 6 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/language/__tests__/parser-test.js b/src/language/__tests__/parser-test.js index 5e8498d6e1..e6811a0fe0 100644 --- a/src/language/__tests__/parser-test.js +++ b/src/language/__tests__/parser-test.js @@ -108,7 +108,7 @@ describe('Parser', () => { it('parses variable definition directives', () => { expect(() => - parse('query Foo($x: Boolean @bar = false) { field }'), + parse('query Foo($x: Boolean = false @bar) { field }'), ).to.not.throw(); }); diff --git a/src/language/__tests__/printer-test.js b/src/language/__tests__/printer-test.js index 18f7d18377..1bcb35f55f 100644 --- a/src/language/__tests__/printer-test.js +++ b/src/language/__tests__/printer-test.js @@ -65,10 +65,10 @@ describe('Printer: Query document', () => { `); const queryAstWithVariableDirective = parse( - 'query ($foo: TestType @testDirective(if: true) @test = {a: 123}) { id }', + 'query ($foo: TestType = {a: 123} @testDirective(if: true) @test) { id }', ); expect(print(queryAstWithVariableDirective)).to.equal(dedent` - query ($foo: TestType @testDirective(if: true) @test = {a: 123}) { + query ($foo: TestType = {a: 123} @testDirective(if: true) @test) { id } `); diff --git a/src/language/ast.js b/src/language/ast.js index 83b6025421..21bc1797db 100644 --- a/src/language/ast.js +++ b/src/language/ast.js @@ -224,8 +224,8 @@ export type VariableDefinitionNode = { +loc?: Location, +variable: VariableNode, +type: TypeNode, - +directives?: $ReadOnlyArray, +defaultValue?: ValueNode, + +directives?: $ReadOnlyArray, }; export type VariableNode = { diff --git a/src/language/parser.js b/src/language/parser.js index 40d9120d17..21241ec17e 100644 --- a/src/language/parser.js +++ b/src/language/parser.js @@ -332,7 +332,7 @@ function parseVariableDefinitions( } /** - * VariableDefinition : Variable : Type Directives? DefaultValue? + * VariableDefinition : Variable : Type DefaultValue? Directives[Const]? */ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode { const start = lexer.token; @@ -340,10 +340,10 @@ function parseVariableDefinition(lexer: Lexer<*>): VariableDefinitionNode { kind: Kind.VARIABLE_DEFINITION, variable: parseVariable(lexer), type: (expect(lexer, TokenKind.COLON), parseTypeReference(lexer)), - directives: parseDirectives(lexer, false), 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 index af4398e7e7..5254f46ee5 100644 --- a/src/language/printer.js +++ b/src/language/printer.js @@ -36,13 +36,12 @@ const printDocASTReducer = { : join([op, join([name, varDefs]), directives, selectionSet], ' '); }, - VariableDefinition: ({ variable, type, directives, defaultValue }) => + VariableDefinition: ({ variable, type, defaultValue, directives }) => variable + ': ' + type + - wrap(' ', join(directives, ' ')) + - wrap(' = ', defaultValue), - + 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 index 7c5c96734c..54bc026672 100644 --- a/src/language/visitor.js +++ b/src/language/visitor.js @@ -64,7 +64,7 @@ export const QueryDocumentKeys = { 'directives', 'selectionSet', ], - VariableDefinition: ['variable', 'type', 'directives', 'defaultValue'], + VariableDefinition: ['variable', 'type', 'defaultValue', 'directives'], Variable: ['name'], SelectionSet: ['selections'], Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], From 978e4c9fa7dc439e20f23f0e4bb99afb32571f8a Mon Sep 17 00:00:00 2001 From: mjmahone Date: Sun, 29 Jul 2018 17:17:16 -0400 Subject: [PATCH 3/3] Adding to introspection query, and the KnownDirectives validation rule --- src/type/__tests__/introspection-test.js | 5 +++++ src/type/introspection.js | 4 ++++ src/utilities/__tests__/schemaPrinter-test.js | 6 ++++++ src/validation/__tests__/KnownDirectives-test.js | 11 ++++++----- src/validation/__tests__/harness.js | 4 ++++ src/validation/rules/KnownDirectives.js | 2 ++ 6 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/type/__tests__/introspection-test.js b/src/type/__tests__/introspection-test.js index 2d52922c5d..2ffb1f9f8c 100644 --- 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 index 5f56f72062..8e5ae586ee 100644 --- 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 index de0e4e1129..2ff9717cd8 100644 --- 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 index 0e0daf5641..e43f96aa04 100644 --- a/src/validation/__tests__/KnownDirectives-test.js +++ b/src/validation/__tests__/KnownDirectives-test.js @@ -102,8 +102,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) @@ -120,8 +120,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 } @@ -130,7 +130,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 index 6d428faa79..48b91618a0 100644 --- 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'], + }), new GraphQLDirective({ name: 'onSchema', locations: ['SCHEMA'], diff --git a/src/validation/rules/KnownDirectives.js b/src/validation/rules/KnownDirectives.js index 05c12f39f6..e00c46412f 100644 --- a/src/validation/rules/KnownDirectives.js +++ b/src/validation/rules/KnownDirectives.js @@ -82,6 +82,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;