Skip to content

[RFC] Proposed change to directive location introspection #317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 22, 2016
Merged

[RFC] Proposed change to directive location introspection #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/__tests__/starWarsIntrospectionTests.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of locations should also include DirectiveLocation.FRAGMENT_DEFINITION according to the tests and previous behavior, isn't it? (https://github.com/graphql/graphql-js/blob/master/src/execution/__tests__/directives-test.js#L271-L314)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked it in the spec:

The @include directive may be provided for fields, fragment spreads, and inline fragments, and allows for conditional inclusion during execution as described by the if argument.

If I understand correctly DirectiveLocation.FRAGMENT_DEFINITION is not allowed for include and skip directives anymore... Now I wonder why mentioned tests are green.

Is it still a valid query?

query Q {
  a
  ...Frag
}
fragment Frag on TestType @skip(if: true) {
  b
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a bit of research (probably should have done from the beginning :) ) and turns out that this query:

query Q {
  a
  ...Frag
}
fragment Frag on TestType @skip(if: true) {
  b
}

is indeed invalid query, at least it will not pass the query validation. Tests are green because execute (which is used in this test) does not validate the queries and executes them directly. Execution engine on the other hand still supports include and skip directives on fragment definitions, even though they are no longer allowed to be used on fragment definitions.

@leebyron can you please confirm this? Is it intended behavior? I feel that it's not 100% safe to interpret directives if they are used in invalid locations. In my case I decided to completely exclude fragments definitions which use include and skip directives, regardless of if argument (semantically in this case I consider the whole fragment definition to be broken/invalid because of the incorrect usage of the directive):

https://github.com/sangria-graphql/sangria/blob/master/src/test/scala/sangria/execution/DirectivesSpec.scala#L254-L304

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
View file
Open in desktop
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