[RFC] Proposed change to directive location introspection

[leebyron]

This implements graphql/graphql-spec#152

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.

[OlegIlyenko]

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)

[OlegIlyenko]

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
}

[OlegIlyenko]

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