Skip to content

Commit

Permalink
[RFC] Proposed change to directive location introspection
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
leebyron committed Mar 17, 2016
1 parent 57d71e1 commit e89c19d
Show file tree
Hide file tree
Showing 15 changed files with 340 additions and 84 deletions.
3 changes: 3 additions & 0 deletions src/__tests__/starWarsIntrospectionTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ describe('Star Wars Introspection Tests', () => {
},
{
name: '__Directive'
},
{
name: '__DirectiveLocation'
}
]
}
Expand Down
3 changes: 3 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,7 @@ export {
isEqualType,
isTypeSubTypeOf,
doTypesOverlap,

// Asserts a string is a valid GraphQL name.
assertValidName,
} from './utilities';
80 changes: 68 additions & 12 deletions src/type/__tests__/introspection.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
Expand Down Expand Up @@ -674,8 +696,8 @@ describe('Introspection', () => {
ofType: null,
},
},
isDeprecated: false,
deprecationReason: null
isDeprecated: true,
deprecationReason: 'Use `locations`.'
},
{
name: 'onFragment',
Expand All @@ -689,8 +711,8 @@ describe('Introspection', () => {
ofType: null,
},
},
isDeprecated: false,
deprecationReason: null
isDeprecated: true,
deprecationReason: 'Use `locations`.'
},
{
name: 'onField',
Expand All @@ -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,
Expand All @@ -732,12 +793,10 @@ describe('Introspection', () => {
}
}
],
onOperation: false,
onFragment: true,
onField: true
},
{
name: 'skip',
locations: [ 'FIELD', 'FRAGMENT_SPREAD', 'INLINE_FRAGMENT' ],
args: [
{
defaultValue: null,
Expand All @@ -753,9 +812,6 @@ describe('Introspection', () => {
}
}
],
onOperation: false,
onFragment: true,
onField: true
}
]
}
Expand Down
11 changes: 1 addition & 10 deletions src/type/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1061,13 +1062,3 @@ export class GraphQLNonNull<T: GraphQLNullableType> {
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.`
);
}
48 changes: 33 additions & 15 deletions src/type/directives.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,37 +11,51 @@
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<typeof DirectiveLocation>; // 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.
*/
export class GraphQLDirective {
name: string;
description: ?string;
locations: Array<DirectiveLocationEnum>;
args: Array<GraphQLArgument>;
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<DirectiveLocationEnum>;
args?: ?Array<GraphQLArgument>;
onOperation?: ?boolean;
onFragment?: ?boolean;
onField?: ?boolean;
}

/**
Expand All @@ -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
});

/**
Expand All @@ -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
});
69 changes: 66 additions & 3 deletions src/type/introspection.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
GraphQLNonNull,
} from './definition';
import { GraphQLString, GraphQLBoolean } from './scalars';
import { DirectiveLocation } from './directives';
import type { GraphQLFieldDefinition } from './definition';


Expand Down Expand Up @@ -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:
Expand Down
Loading

0 comments on commit e89c19d

Please sign in to comment.