Validation options for schemaDirectives

[reconbot]

I've noticed when you use schema directives that don't exist (eg, a typo) no error is thrown and it's silently ignored. That's not fun to debug and makes using directives harder to use. I propose we have a validation option that ensures a schemaDirectives key exists for every defined directive and that we don't use unknown directives.

[stubailo]

That sounds like a great idea to me - I'd even think that should be the default!

[IvanGoncharov]

@reconbot @stubailo We are working on it for graphql@14.0.0: graphql/graphql-js#1389

[reconbot]

🎉 great! So Apollo tools would just have to assure we have a resolver for the directive?

[hwillson]

We're working on this as part of #945. graphql-tools will be following graphql-js's lead here, so if you attempt to use a directive that doesn't exist, you'll get an error. We're going to be major version bumping graphql-tools on the next release, for this exact reason. Since we're tracking this work in #945, I'll close this off. Thanks!

[reconbot]

Yay!

…

On Fri, Sep 7, 2018, 12:55 PM Hugh Willson ***@***.***> wrote: We're working on this as part of #945 <#945>. graphql-tools will be following graphql-js's lead here, so if you attempt to use a directive that doesn't exist, you'll get an error. We're going to be major version bumping graphql-tools on the next release, for this exact reason. Since we're tracking this work in #945 <#945>, I'll close this off. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#920 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbtPShSHXavRfj-Fim9GRkp3mLgJkks5uYqT3gaJpZM4VuWUo> .

[IvanGoncharov]

@hwillson As a suggestion. In 14.0.0 we now allow extending incomplete schemas (without query type or any type at all) so here is one way you can provide directive during schema building process:
https://github.com/graphql/graphql-js/blob/3595ea922613f3e13a07185407dd70ed45de7a66/src/utilities/__tests__/extendSchema-test.js#L1291-L1306

We added this feature specifically to allow specifying library directives without messing with AST of client SDL.

[eurostar-fennec-cooper]

Should this be reopened? It got delegated to #945, but unfortunately graphql@14.0.0 (and in my case 15.3.0) doesn't seem to have included this feature tracked by graphql/graphql-js#1389.

I can still start a graphql server (apollo, in my case) without defining functions for any of my directives.

[yaacovCR]

Directives have to be defined in SDL in order to be used in the latest versions of GraphQL...

It sounds like you want to make sure that all directive usages have functions that are passed to makeExecutableSchema, but there are lots of other directives that could be used, so perhaps it might be better to add a feature where you check yourself the arguments to makeExecutableSchema before passing a build schema to Apollo server?

[reconbot]

What do you mean by "there are a lot of other directives that would be used"?

The feature request here is to add opt in additional validation for custom directives to help ensure they're properly implemented.

[eurostar-fennec-cooper]

I think, at the end of the day, the issue for me (and probably others) is that I can use directives in my SDL and then refuse to create implementations for them, whether they're never written or accidentally removed etc, but at the moment it seems to fail silently by design.

I don't think directives could be trusted as, for example, auth helpers, since if the implementation is removed for some reason, all your auth checks just disappear. It seems odd that directive in SDL = "wrap the resolver with this function", but that it's acceptable for "this function" to simply...not exist. It would be like using a custom scalar in the SDL and then not providing an implementation for that scalar.

[yaacovCR]

That line should have read that there are lots of other ways directives could be used. Even with GraphQL tools, you can use directive resolvers, class based schema directives, and the new functional schema directives.

Outside graphql-tools, directives are used in all sorts of ways. And after they are used, there is no hard requirement that they are removed/cleaned up....

Schema directives more generally are just a way to extend SDL syntax without writing a new parser, there is no requirement of an implementation at all. It is not invalid to not implement a directive, basically.

That is not to say that some level of validation of arguments to makeExecutableSchema isn't possible. You could write a custom function that introspects a schema for defined custom directives and throws if you forget to implement it.

I think it is fairly straightforward to just remember to provide implementations just like you need to remember to provide Resolvers. The key is to have tests that would fail if you forget something.

By the way, you can definitely define a custom scalar without an implementation, you will just get a scalar that just gets the default parse and serialize functions, which basically pass everything through.

In terms of directives, you can think of it as there being a default in which The graphql you entities that they annotate are just passed through...

[eurostar-fennec-cooper]

Great points. Thank you for the info. I think I'm just going to figure out if I can cover them with tests, etc. Otherwise I'll find an alternative solution to annotating the schema.