Skip to content
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.

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

Add support for directives applied on IDL & Schema #746

Merged
merged 4 commits into from
Jul 5, 2017

Conversation

IvanGoncharov
Copy link
Member

Before this PR the only way to get directive applied on IDL was working directly with AST.
This PR adds appliedDirectives (would be great to come up with a shorter name?) property to every graphql-js type that should support it according to: graphql/graphql-spec#90
I have also extended buildASTSchema to extract applied directives from AST.

After the initial review, I'd be happy to add documentation comments and some unit tests.

@stubailo
Copy link

stubailo commented Mar 8, 2017

Wow, this would be excellent, we could probably add a lot of great functionality to graphql-tools with this.

@IvanGoncharov
Copy link
Member Author

@leebyron @wincent Can you please review this PR?
After you give green light to these changes in general, I can start working on tests and docs.

The ability to get applied directives from GraphQLSchema or subsequent object is very important for adoption of directives in IDL. At the moment, the only way to get appliedDirectives is to work directly with AST.

We use a fork of graphql-js with this PR in one of our tools. But this approach won't work in our new library as GraphQLSchema object created by fork will conflict with execute from vanilla graphql-js.

@leebyron
Copy link
Contributor

Sorry for the super late review, thanks for the reminder.

I really like this, but I think it's too specific and that is making the PR more complicated than it needs to be. A simpler and more general solution would be to refer to the entire AST node associated with each definition when built with buildASTSchema. That would still allow access to directives for tooling (type.astNode.directives) but would also allow access to the rest of the AST which would be very useful when reporting errors within the schema - allowing error reports that include file:line:column information in a schema definition file.

@OlegIlyenko
Copy link
Contributor

One argument in favor of just a list of directives instead of the full AST node is when schema is not created based on the IDL, but still may include directive information.

For example, if directive information would be exposed via introspection API some day, then the source of the schema would be introspection JSON and it would be beneficial to just have a list of applied directives.

@leebyron
Copy link
Contributor

leebyron commented May 15, 2017

I think that may mix some concerns - we can address these separately.

Right now directives are purely a feature of the GraphQL language and IDL, so a schema not created using IDL definitionally won't contain directives. When building a schema in code, you have the full power of the host programming environment, so directives shouldn't be necessary. Directives are a tool for supplying additional intent to be later interpreted by the host programming environment.

As for exposing more information from introspection - that's a bit out of scope for this particular change, and that RFC may go through more changes. I see type metadata and IDL directives as likely two related but not identical concepts, and don't see a need to prematurely link the two for this particular change.

@OlegIlyenko
Copy link
Contributor

I see, it's a good point 👍

@IvanGoncharov
Copy link
Member Author

@leebyron type.astNode is a simple and effective solution 👍 when you build schema using buildASTSchema. However, I'm not sure how to correctly apply it to extendSchema function.

In one of our tool we need to get directive values from schema extended with IDL, e.g.:

extend type User {
  petName: Pet @fake(type:firstName)
}

What would be the corect value forastNode of User type?

@leebyron
Copy link
Contributor

Very good point, @IvanGoncharov! What if instead of astNode, we have astNodes which is typically an array of length one, however extensions are loaded in as additional nodes in the list?

Also, it could be nice for the GraphQLField definitions to also contain a reference to ast nodes for each field - since otherwise extensions may make finding the field ast for a given field more than a simple lookup

@leebyron
Copy link
Contributor

leebyron commented May 18, 2017

Alternatively, we could have astNode for an initial definition and extensionASTNodes for any applicable extensions. This could be nice in cases where the original schema is not derived from a the schema language, however extensions are - then it would be very clear that astNode provides no value, rather than having to test the kind of astNodes[0]

@bbakerman
Copy link

bbakerman commented May 18, 2017

@leebyron - one question I have regarding IDL and directives capture is whether the directive location will be enforced

Imagine :

type User {
  petName: Pet @metaData(implementation:"some.class.here")
}

The graphql spec says on validation : https://facebook.github.io/graphql/#sec-Validation.Directives

For every directive in a document.
Let directiveName be the name of directive.
Let directiveDefinition be the directive named directiveName.
directiveDefinition must exist.

Its currently specified that this is for query documents and unspecified whether this applies to "IDL schema" documents

One could imagine requiring this to be valid :

type User {
  petName: Pet @metaData(implementation:"some.class.here")
}

directive @metaData(implementation : String) on field | type

That would be very precise. BUT a mistake in my opinion. The use of directives for metadata is really powerful but requiring schema consumers to name where the metadata is valid is TOO PRECISE.

The specific graphql engine decides what metadata it understands and requiring people to name is back to the engine seems low value to me. Precise yes but low value.

I ask this question because we are debating how to handle this in https://github.com/graphql-java/graphql-java and the lack of spec guidance makes it .... interesting.

@leebyron
Copy link
Contributor

one question I have regarding IDL and directives capture is whether the directive location will be enforced

Absolutely it will be enforced. Not doing so invites accidental error.

That would be very precise. BUT a mistake in my opinion. The use of directives for metadata is really powerful but requiring schema consumers to name where the metadata is valid is TOO PRECISE.

I'm not following why being too precise is a bad thing, or a low value thing. Not having this precision would allow people to supply directives in a way that was unexpected or unsupported by the underlying tools that will read those directives. At best this requires GraphQL library authors to essentially reproduce this sort of error checking ad-hoc, at worst this would result in GraphQL users suffering from silent errors where a misspelling or misplacement causes them to accidentally break their services.

@IvanGoncharov
Copy link
Member Author

Alternatively, we could have astNode for an initial definition and extensionASTNodes for any applicable extensions.

@leebyron I have updated this PR according to your suggestion. Also, I added tests. Could you please review it?

@IvanGoncharov IvanGoncharov force-pushed the directives branch 3 times, most recently from 6d3f19b to 047170a Compare June 6, 2017 10:35
@IvanGoncharov
Copy link
Member Author

@leebyron I improved tests and split this PR into smaller commits so it's easier to review.
Could you please review it?

If we add additional fields (e.g. astNode) into values of enum these test
will be broken.
@IvanGoncharov
Copy link
Member Author

@leebyron Updated PR with requested changes.
I agree that undefined suits better 👍 However there is some inconsistency as deprecationReason uses null as a default value, e.g. here:
https://github.com/graphql/graphql-js/blob/master/src/utilities/__tests__/buildClientSchema-test.js#L407

@leebyron
Copy link
Contributor

leebyron commented Jul 5, 2017

However there is some inconsistency as deprecationReason uses null as a default value

Subtle, but that's representing a different concept - null as in it is known that there is no deprecation. This maps to how deprecationReason is represented in introspection json results as well. But for this internal value, undefined (as in - we don't know if a related AST node exists, rather than we know that there is no AST node) is a reasonable default for a concept like this.

@leebyron leebyron merged commit 831598b into graphql:master Jul 5, 2017
@leebyron
Copy link
Contributor

leebyron commented Jul 5, 2017

Great work! Excited to see this as a basis for much better schema validation error messages

@IvanGoncharov IvanGoncharov deleted the directives branch July 6, 2017 08:54
@IvanGoncharov
Copy link
Member Author

@leebyron Can you please release updated NPM package?

@dotansimha
Copy link
Member

Can't wait for a release with this feature!!!

@taion
Copy link
Contributor

taion commented Dec 20, 2017

I'm sorry to ask this here – I usually don't do this sort of thing, but how does one actually define a schema-level directive using GraphQL-js? I can't tell from the tests here, as they only seem to show cases using the schema definition language.

@stubailo
Copy link

@taion this may not be the answer you're looking for but the next big version of graphql-tools will have that capability, in a way very similar to JavaScript decorators.

@taion
Copy link
Contributor

taion commented Dec 20, 2017

@stubailo Thanks! Would the idea there be that those directives would serve as type metadata? I'm a bit confused now on what the resolution is per #746 (comment) on the distinction between type metadata and IDL directives.

@taion
Copy link
Contributor

taion commented Dec 20, 2017

Oh, wait, you can't read the actual directives on a type via introspection anyway, so I guess I'm very badly missing the point here.

@stubailo
Copy link

Yeah, that would be a really huge improvement to the value of directives for sure! I think both are nice use cases :]

@taion
Copy link
Contributor

taion commented Dec 20, 2017

Yeah, sorry... I thought that was already how it worked. My bad for not reading the decorator/directive spec carefully enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants