From e89c19d2ce584f924c2e0e472a61bb805ce260d4 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Thu, 17 Mar 2016 16:33:28 -0700 Subject: [PATCH] [RFC] Proposed change to directive location introspection This proposes a change to how we represent the ability to validate the locations of directives via introspection. Specifically, this deprecates `onField`, `onFragment`, and `onOperation` in favor of `locations` which is a list of `__DirectiveLocation`. **Rationale:** This allows for a more fine-grained validation of directive placement, now you can assert that a directive is allowed on queries but not mutations, or allowed on fragment definitions but not on fragment spreads. Also, this makes expanding the locations a directive is allowed to be placed easier to do, as future expansions will not affect the introspection API. This should be considered a prereq to #265. Finally, this is a prereq to a forthcoming RFC to add directives to the type schema language, one of the last missing pieces to represent a full schema using this language. Currently considering something like: ``` directive @skip(if: Boolean) on FIELD, FRAGMENT_SPREAD, INLINE_FRAGMENT ``` **Drawbacks:** Any change to the introspection API is a challenge. Especially so for graphql-js which is used as both client tools via Graph*i*QL and as a node.js server. To account for this, I've left the existing fields as deprecated, and continued to support these deprecated fields in `buildClientSchema`, which is used by Graph*i*QL. While graphql-js will likely continue to expose these deprecated fields for some time to come, the spec itself should not include these fields if this change is reflected. --- src/__tests__/starWarsIntrospectionTests.js | 3 + src/index.js | 3 + src/type/__tests__/introspection.js | 80 +++++++++++++++++--- src/type/definition.js | 11 +-- src/type/directives.js | 48 ++++++++---- src/type/introspection.js | 69 ++++++++++++++++- src/utilities/__tests__/buildClientSchema.js | 71 ++++++++++++++++- src/utilities/__tests__/schemaPrinter.js | 11 +++ src/utilities/assertValidName.js | 22 ++++++ src/utilities/buildClientSchema.js | 25 +++++- src/utilities/index.js | 3 + src/utilities/introspectionQuery.js | 11 ++- src/validation/__tests__/KnownDirectives.js | 6 +- src/validation/__tests__/harness.js | 2 +- src/validation/rules/KnownDirectives.js | 59 ++++++++------- 15 files changed, 340 insertions(+), 84 deletions(-) create mode 100644 src/utilities/assertValidName.js diff --git a/src/__tests__/starWarsIntrospectionTests.js b/src/__tests__/starWarsIntrospectionTests.js index 718ee8a618..68f3734b76 100644 --- a/src/__tests__/starWarsIntrospectionTests.js +++ b/src/__tests__/starWarsIntrospectionTests.js @@ -71,6 +71,9 @@ describe('Star Wars Introspection Tests', () => { }, { name: '__Directive' + }, + { + name: '__DirectiveLocation' } ] } diff --git a/src/index.js b/src/index.js index 38b39b9ca9..949eaae073 100644 --- a/src/index.js +++ b/src/index.js @@ -161,4 +161,7 @@ export { isEqualType, isTypeSubTypeOf, doTypesOverlap, + + // Asserts a string is a valid GraphQL name. + assertValidName, } from './utilities'; diff --git a/src/type/__tests__/introspection.js b/src/type/__tests__/introspection.js index 04efc05543..64aa6f7c4b 100644 --- a/src/type/__tests__/introspection.js +++ b/src/type/__tests__/introspection.js @@ -640,6 +640,28 @@ describe('Introspection', () => { isDeprecated: false, deprecationReason: null }, + { + name: 'locations', + args: [], + type: { + kind: 'NON_NULL', + name: null, + ofType: { + kind: 'LIST', + name: null, + ofType: { + kind: 'NON_NULL', + name: null, + ofType: { + kind: 'ENUM', + name: '__DirectiveLocation' + } + } + } + }, + isDeprecated: false, + deprecationReason: null + }, { name: 'args', args: [], @@ -674,8 +696,8 @@ describe('Introspection', () => { ofType: null, }, }, - isDeprecated: false, - deprecationReason: null + isDeprecated: true, + deprecationReason: 'Use `locations`.' }, { name: 'onFragment', @@ -689,8 +711,8 @@ describe('Introspection', () => { ofType: null, }, }, - isDeprecated: false, - deprecationReason: null + isDeprecated: true, + deprecationReason: 'Use `locations`.' }, { name: 'onField', @@ -704,19 +726,58 @@ describe('Introspection', () => { ofType: null, }, }, - isDeprecated: false, - deprecationReason: null + isDeprecated: true, + deprecationReason: 'Use `locations`.' } ], inputFields: null, interfaces: [], enumValues: null, possibleTypes: null, + }, + { + kind: 'ENUM', + name: '__DirectiveLocation', + fields: null, + inputFields: null, + interfaces: null, + enumValues: [ + { + name: 'QUERY', + isDeprecated: false + }, + { + name: 'MUTATION', + isDeprecated: false + }, + { + name: 'SUBSCRIPTION', + isDeprecated: false + }, + { + name: 'FIELD', + isDeprecated: false + }, + { + name: 'FRAGMENT_DEFINITION', + isDeprecated: false + }, + { + name: 'FRAGMENT_SPREAD', + isDeprecated: false + }, + { + name: 'INLINE_FRAGMENT', + isDeprecated: false + }, + ], + possibleTypes: null, } ], directives: [ { name: 'include', + locations: [ 'FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT' ], args: [ { defaultValue: null, @@ -732,12 +793,10 @@ describe('Introspection', () => { } } ], - onOperation: false, - onFragment: true, - onField: true }, { name: 'skip', + locations: [ 'FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT' ], args: [ { defaultValue: null, @@ -753,9 +812,6 @@ describe('Introspection', () => { } } ], - onOperation: false, - onFragment: true, - onField: true } ] } diff --git a/src/type/definition.js b/src/type/definition.js index 556963376e..9ef8154094 100644 --- a/src/type/definition.js +++ b/src/type/definition.js @@ -12,6 +12,7 @@ import invariant from '../jsutils/invariant'; import isNullish from '../jsutils/isNullish'; import keyMap from '../jsutils/keyMap'; import { ENUM } from '../language/kinds'; +import { assertValidName } from '../utilities/assertValidName'; import type { OperationDefinition, Field, @@ -1061,13 +1062,3 @@ export class GraphQLNonNull { return this.ofType.toString() + '!'; } } - -const NAME_RX = /^[_a-zA-Z][_a-zA-Z0-9]*$/; - -// Helper to assert that provided names are valid. -function assertValidName(name: string): void { - invariant( - NAME_RX.test(name), - `Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "${name}" does not.` - ); -} diff --git a/src/type/directives.js b/src/type/directives.js index d3aa07096c..b2290604c0 100644 --- a/src/type/directives.js +++ b/src/type/directives.js @@ -11,8 +11,22 @@ import { GraphQLNonNull } from './definition'; import type { GraphQLArgument } from './definition'; import { GraphQLBoolean } from './scalars'; +import invariant from '../jsutils/invariant'; +import { assertValidName } from '../utilities/assertValidName'; +export const DirectiveLocation = { + QUERY: 'QUERY', + MUTATION: 'MUTATION', + SUBSCRIPTION: 'SUBSCRIPTION', + FIELD: 'FIELD', + FRAGMENT_DEFINITION: 'FRAGMENT_DEFINITION', + FRAGMENT_SPREAD: 'FRAGMENT_SPREAD', + INLINE_FRAGMENT: 'INLINE_FRAGMENT', +}; + +export type DirectiveLocationEnum = $Keys; // eslint-disable-line + /** * Directives are used by the GraphQL runtime as a way of modifying execution * behavior. Type system creators will usually not create these directly. @@ -20,28 +34,28 @@ import { GraphQLBoolean } from './scalars'; export class GraphQLDirective { name: string; description: ?string; + locations: Array; args: Array; - onOperation: boolean; - onFragment: boolean; - onField: boolean; constructor(config: GraphQLDirectiveConfig) { + invariant(config.name, 'Directive must be named.'); + assertValidName(config.name); + invariant( + Array.isArray(config.locations), + 'Must provide locations for directive.' + ); this.name = config.name; this.description = config.description; + this.locations = config.locations; this.args = config.args || []; - this.onOperation = Boolean(config.onOperation); - this.onFragment = Boolean(config.onFragment); - this.onField = Boolean(config.onField); } } type GraphQLDirectiveConfig = { name: string; description?: ?string; + locations: Array; args?: ?Array; - onOperation?: ?boolean; - onFragment?: ?boolean; - onField?: ?boolean; } /** @@ -52,14 +66,16 @@ export const GraphQLIncludeDirective = new GraphQLDirective({ description: 'Directs the executor to include this field or fragment only when ' + 'the `if` argument is true.', + locations: [ + DirectiveLocation.FIELD, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ], args: [ { name: 'if', type: new GraphQLNonNull(GraphQLBoolean), description: 'Included when true.' } ], - onOperation: false, - onFragment: true, - onField: true }); /** @@ -70,12 +86,14 @@ export const GraphQLSkipDirective = new GraphQLDirective({ description: 'Directs the executor to skip this field or fragment when the `if` ' + 'argument is true.', + locations: [ + DirectiveLocation.FIELD, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ], args: [ { name: 'if', type: new GraphQLNonNull(GraphQLBoolean), description: 'Skipped when true.' } ], - onOperation: false, - onFragment: true, - onField: true }); diff --git a/src/type/introspection.js b/src/type/introspection.js index b30bf35528..4d067e8f90 100644 --- a/src/type/introspection.js +++ b/src/type/introspection.js @@ -22,6 +22,7 @@ import { GraphQLNonNull, } from './definition'; import { GraphQLString, GraphQLBoolean } from './scalars'; +import { DirectiveLocation } from './directives'; import type { GraphQLFieldDefinition } from './definition'; @@ -78,17 +79,79 @@ const __Directive = new GraphQLObjectType({ fields: () => ({ name: { type: new GraphQLNonNull(GraphQLString) }, description: { type: GraphQLString }, + locations: { + type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull( + __DirectiveLocation + ))) + }, args: { type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(__InputValue))), resolve: directive => directive.args || [] }, - onOperation: { type: new GraphQLNonNull(GraphQLBoolean) }, - onFragment: { type: new GraphQLNonNull(GraphQLBoolean) }, - onField: { type: new GraphQLNonNull(GraphQLBoolean) }, + // NOTE: the following three fields are deprecated and are no longer part + // of the GraphQL specification. + onOperation: { + deprecationReason: 'Use `locations`.', + type: new GraphQLNonNull(GraphQLBoolean), + resolve: d => + d.locations.indexOf(DirectiveLocation.QUERY) !== -1 || + d.locations.indexOf(DirectiveLocation.MUTATION) !== -1 || + d.locations.indexOf(DirectiveLocation.SUBSCRIPTION) !== -1 + }, + onFragment: { + deprecationReason: 'Use `locations`.', + type: new GraphQLNonNull(GraphQLBoolean), + resolve: d => + d.locations.indexOf(DirectiveLocation.FRAGMENT_SPREAD) !== -1 || + d.locations.indexOf(DirectiveLocation.INLINE_FRAGMENT) !== -1 || + d.locations.indexOf(DirectiveLocation.FRAGMENT_DEFINITION) !== -1 + }, + onField: { + deprecationReason: 'Use `locations`.', + type: new GraphQLNonNull(GraphQLBoolean), + resolve: d => d.locations.indexOf(DirectiveLocation.FIELD) !== -1 + }, }), }); +const __DirectiveLocation = new GraphQLEnumType({ + name: '__DirectiveLocation', + description: + 'A Directive can be adjacent to many parts of the GraphQL language, a ' + + '__DirectiveLocation describes one such possible adjacencies.', + values: { + QUERY: { + value: DirectiveLocation.QUERY, + description: 'Location adjacent to a query operation.' + }, + MUTATION: { + value: DirectiveLocation.MUTATION, + description: 'Location adjacent to a mutation operation.' + }, + SUBSCRIPTION: { + value: DirectiveLocation.SUBSCRIPTION, + description: 'Location adjacent to a subscription operation.' + }, + FIELD: { + value: DirectiveLocation.FIELD, + description: 'Location adjacent to a field.' + }, + FRAGMENT_DEFINITION: { + value: DirectiveLocation.FRAGMENT_DEFINITION, + description: 'Location adjacent to a fragment definition.' + }, + FRAGMENT_SPREAD: { + value: DirectiveLocation.FRAGMENT_SPREAD, + description: 'Location adjacent to a fragment spread.' + }, + INLINE_FRAGMENT: { + value: DirectiveLocation.INLINE_FRAGMENT, + description: 'Location adjacent to an inline fragment.' + }, + } +}); + const __Type = new GraphQLObjectType({ name: '__Type', description: diff --git a/src/utilities/__tests__/buildClientSchema.js b/src/utilities/__tests__/buildClientSchema.js index f047e03afe..f1848d86d7 100644 --- a/src/utilities/__tests__/buildClientSchema.js +++ b/src/utilities/__tests__/buildClientSchema.js @@ -500,7 +500,7 @@ describe('Type System: build schema from introspection', () => { new GraphQLDirective({ name: 'customDirective', description: 'This is a custom directive', - onField: true, + locations: [ 'FIELD' ], }) ] }); @@ -508,6 +508,75 @@ describe('Type System: build schema from introspection', () => { await testSchema(schema); }); + it('builds a schema with legacy directives', async () => { + + const oldIntrospection = { + __schema: { + // Minimum required schema. + queryType: { + name: 'Simple' + }, + types: [ { + name: 'Simple', + kind: 'OBJECT', + fields: [ { + name: 'simple', + args: [], + type: { name: 'Simple' } + } ], + interfaces: [] + } ], + // Test old directive introspection results. + directives: [ + { name: 'Old1', args: [], onField: true }, + { name: 'Old2', args: [], onFragment: true }, + { name: 'Old3', args: [], onOperation: true }, + { name: 'Old4', args: [], onField: true, onFragment: true }, + ] + } + }; + + // New introspection produces correct new format. + const newIntrospection = { + __schema: { + directives: [ + { + name: 'Old1', + args: [], + locations: [ 'FIELD' ] + }, + { + name: 'Old2', + args: [], + locations: [ + 'FRAGMENT_DEFINITION', + 'FRAGMENT_SPREAD', + 'INLINE_FRAGMENT' + ] + }, + { + name: 'Old3', + args: [], + locations: [ 'QUERY', 'MUTATION', 'SUBSCRIPTION' ] + }, + { + name: 'Old4', + args: [], + locations: [ + 'FIELD', + 'FRAGMENT_DEFINITION', + 'FRAGMENT_SPREAD', + 'INLINE_FRAGMENT' + ] + }, + ] + } + }; + + const clientSchema = buildClientSchema(oldIntrospection); + const secondIntrospection = await graphql(clientSchema, introspectionQuery); + expect(secondIntrospection.data).to.containSubset(newIntrospection); + }); it('builds a schema aware of deprecation', async () => { diff --git a/src/utilities/__tests__/schemaPrinter.js b/src/utilities/__tests__/schemaPrinter.js index bf32642a72..290cbe83ff 100644 --- a/src/utilities/__tests__/schemaPrinter.js +++ b/src/utilities/__tests__/schemaPrinter.js @@ -511,12 +511,23 @@ type Root { type __Directive { name: String! description: String + locations: [__DirectiveLocation!]! args: [__InputValue!]! onOperation: Boolean! onFragment: Boolean! onField: Boolean! } +enum __DirectiveLocation { + QUERY + MUTATION + SUBSCRIPTION + FIELD + FRAGMENT_DEFINITION + FRAGMENT_SPREAD + INLINE_FRAGMENT +} + type __EnumValue { name: String! description: String diff --git a/src/utilities/assertValidName.js b/src/utilities/assertValidName.js new file mode 100644 index 0000000000..8413767375 --- /dev/null +++ b/src/utilities/assertValidName.js @@ -0,0 +1,22 @@ +/* @flow */ +/** + * Copyright (c) 2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import invariant from '../jsutils/invariant'; + + +const NAME_RX = /^[_a-zA-Z][_a-zA-Z0-9]*$/; + +// Helper to assert that provided names are valid. +export function assertValidName(name: string): void { + invariant( + NAME_RX.test(name), + `Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "${name}" does not.` + ); +} diff --git a/src/utilities/buildClientSchema.js b/src/utilities/buildClientSchema.js index 54960290af..93a07c6697 100644 --- a/src/utilities/buildClientSchema.js +++ b/src/utilities/buildClientSchema.js @@ -36,7 +36,7 @@ import { GraphQLID } from '../type/scalars'; -import { GraphQLDirective } from '../type/directives'; +import { DirectiveLocation, GraphQLDirective } from '../type/directives'; import { TypeKind } from '../type/introspection'; @@ -319,13 +319,30 @@ export function buildClientSchema( } function buildDirective(directiveIntrospection) { + // Support deprecated `on****` fields for building `locations`, as this + // is used by GraphiQL which may need to support outdated servers. + const locations = directiveIntrospection.locations ? + directiveIntrospection.locations.slice() : + [].concat( + !directiveIntrospection.onField ? [] : [ + DirectiveLocation.FIELD, + ], + !directiveIntrospection.onOperation ? [] : [ + DirectiveLocation.QUERY, + DirectiveLocation.MUTATION, + DirectiveLocation.SUBSCRIPTION, + ], + !directiveIntrospection.onFragment ? [] : [ + DirectiveLocation.FRAGMENT_DEFINITION, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ] + ); return new GraphQLDirective({ name: directiveIntrospection.name, description: directiveIntrospection.description, + locations, args: directiveIntrospection.args.map(buildInputValue), - onOperation: directiveIntrospection.onOperation, - onFragment: directiveIntrospection.onFragment, - onField: directiveIntrospection.onField, }); } diff --git a/src/utilities/index.js b/src/utilities/index.js index f51232a57f..18129209c7 100644 --- a/src/utilities/index.js +++ b/src/utilities/index.js @@ -54,3 +54,6 @@ export { isTypeSubTypeOf, doTypesOverlap } from './typeComparators'; + +// Asserts that a string is a valid GraphQL name +export { assertValidName } from './assertValidName'; diff --git a/src/utilities/introspectionQuery.js b/src/utilities/introspectionQuery.js index 0db9df4809..73d548f1b7 100644 --- a/src/utilities/introspectionQuery.js +++ b/src/utilities/introspectionQuery.js @@ -8,6 +8,9 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +import type { DirectiveLocationEnum } from '../type/directives'; + + export const introspectionQuery = ` query IntrospectionQuery { __schema { @@ -20,12 +23,10 @@ export const introspectionQuery = ` directives { name description + locations args { ...InputValue } - onOperation - onFragment - onField } } } @@ -197,8 +198,6 @@ export type IntrospectionEnumValue = { export type IntrospectionDirective = { name: string; description: ?string; + locations: Array; args: Array; - onOperation: boolean; - onFragment: boolean; - onField: boolean; } diff --git a/src/validation/__tests__/KnownDirectives.js b/src/validation/__tests__/KnownDirectives.js index 359789d1e2..e71f5f924b 100644 --- a/src/validation/__tests__/KnownDirectives.js +++ b/src/validation/__tests__/KnownDirectives.js @@ -108,9 +108,9 @@ describe('Validate: Known directives', () => { ...Frag @operationOnly } `, [ - misplacedDirective('include', 'operation', 2, 17), - misplacedDirective('operationOnly', 'field', 3, 14), - misplacedDirective('operationOnly', 'fragment', 4, 17), + misplacedDirective('include', 'QUERY', 2, 17), + misplacedDirective('operationOnly', 'FIELD', 3, 14), + misplacedDirective('operationOnly', 'FRAGMENT_SPREAD', 4, 17), ]); }); diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js index 745bb83d53..a4d2812591 100644 --- a/src/validation/__tests__/harness.js +++ b/src/validation/__tests__/harness.js @@ -307,7 +307,7 @@ export const testSchema = new GraphQLSchema({ directives: [ new GraphQLDirective({ name: 'operationOnly', - onOperation: true + locations: [ 'QUERY' ], }), GraphQLIncludeDirective, GraphQLSkipDirective, diff --git a/src/validation/rules/KnownDirectives.js b/src/validation/rules/KnownDirectives.js index ca35a18f1c..5ed6383868 100644 --- a/src/validation/rules/KnownDirectives.js +++ b/src/validation/rules/KnownDirectives.js @@ -18,6 +18,7 @@ import { INLINE_FRAGMENT, FRAGMENT_DEFINITION } from '../../language/kinds'; +import { DirectiveLocation } from '../../type/directives'; export function unknownDirectiveMessage(directiveName: string): string { @@ -26,9 +27,9 @@ export function unknownDirectiveMessage(directiveName: string): string { export function misplacedDirectiveMessage( directiveName: string, - placement: string + location: string ): string { - return `Directive "${directiveName}" may not be used on "${placement}".`; + return `Directive "${directiveName}" may not be used on ${location}.`; } /** @@ -52,34 +53,34 @@ export function KnownDirectives(context: ValidationContext): any { return; } const appliedTo = ancestors[ancestors.length - 1]; - switch (appliedTo.kind) { - case OPERATION_DEFINITION: - if (!directiveDef.onOperation) { - context.reportError(new GraphQLError( - misplacedDirectiveMessage(node.name.value, 'operation'), - [ node ] - )); - } - break; - case FIELD: - if (!directiveDef.onField) { - context.reportError(new GraphQLError( - misplacedDirectiveMessage(node.name.value, 'field'), - [ node ] - )); - } - break; - case FRAGMENT_SPREAD: - case INLINE_FRAGMENT: - case FRAGMENT_DEFINITION: - if (!directiveDef.onFragment) { - context.reportError(new GraphQLError( - misplacedDirectiveMessage(node.name.value, 'fragment'), - [ node ] - )); - } - break; + const candidateLocation = getLocationForAppliedNode(appliedTo); + if (!candidateLocation) { + context.reportError(new GraphQLError( + misplacedDirectiveMessage(node.name.value, node.type), + [ node ] + )); + } else if (directiveDef.locations.indexOf(candidateLocation) === -1) { + context.reportError(new GraphQLError( + misplacedDirectiveMessage(node.name.value, candidateLocation), + [ node ] + )); } } }; } + +function getLocationForAppliedNode(appliedTo) { + switch (appliedTo.kind) { + case OPERATION_DEFINITION: + switch (appliedTo.operation) { + case 'query': return DirectiveLocation.QUERY; + case 'mutation': return DirectiveLocation.MUTATION; + case 'subscription': return DirectiveLocation.SUBSCRIPTION; + } + break; + case FIELD: return DirectiveLocation.FIELD; + case FRAGMENT_SPREAD: return DirectiveLocation.FRAGMENT_SPREAD; + case INLINE_FRAGMENT: return DirectiveLocation.INLINE_FRAGMENT; + case FRAGMENT_DEFINITION: return DirectiveLocation.FRAGMENT_DEFINITION; + } +}