search

graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.04k stars 2.02k forks source link

add validation of directives in SDL #1389

Open leebyron opened 6 years ago

leebyron commented 6 years ago

From #1383:

It's very critical to add validation of directives in SDL since as noted here no one validates them correctly at the moment. That will lead to a situation very similar to the recent Allow interfaces to have no implementors. Since spec defines set of validation rules but no one implements them. Probably we shouldn't enforce Directives Are Unique Per Location until https://github.com/facebook/graphql/issues/429 resolved but all other validation should be implemented e.g. require the schema to contain applied directives, correct directive arguments, etc.

mjmahone commented 6 years ago

Currently working on a PR for this issue. I am going to make it more strict, for now. So we will validate all of:

My plan is to add this within the validateSchema function, as we already are doing a minor bit of directive validation there.

IvanGoncharov commented 6 years ago

I'm also working on it for a couple of weeks and I tried a few different approaches. Currently, I'm working on creating validateSDL and an accompanying set of rules. Idea is to share the majority of code with validate function and reuse some of the existing rules. For example:

directives are only used once per location in the schema

https://github.com/graphql/graphql-js/blob/master/src/validation/rules/UniqueDirectivesPerLocation.js

A few benefits:

@mjmahone What do you think about this aproach? I will try to publish WIP PR tomorrow.

mjmahone commented 6 years ago

@IvanGoncharov that sounds like you're further along than me. I want basically everything you described, but I am willing to take a shortcut and just extend validateSchema to get this out sooner. I've got all the validation (I think) working as-is (putting up a PR imminently).

On thing is that I really want to have us push 14.0.0 as soon as possible. I keep reaching for the "schema directive" tool and then remembering "oh right, that won't be available until we release 14." I don't think my work would be difficult to undo, though, so we could always publish a 14.1 where we switch validateSchema to be run purely on the schema AST nodes.

IvanGoncharov commented 6 years ago

I don't think my work would be difficult to undo, though, so we could always publish a 14.1 where we switch validateSchema to be run purely on the schema AST nodes.

validateSchema is part of public API and people using it: https://github.com/search?q=graphql+validateSchema+-filename%3AvalidateSchema&type=Code Also some 3rd-party implementations synchronise their codebase with graphql-js after each release, e.g. graphql-php. It's not a super big problem but still moving functionality from one public function to another is a breaking change.

Can you wait until monday morning your time? I will limit scope of my PR to two checks you implemented in your PR and it should take me 1-2 days together with tests.

mjmahone commented 6 years ago

Yup I will wait. I would much rather have validation that can be run on the SDL AST than on an already-created schema, and like that this validation would mirror what we already do with validate.

@IvanGoncharov if there's a branch I could work on top of, or if you want me to create that branch, I might be able to add more validation, or move more validation from validateSchema to individual SDL validation rules.

IvanGoncharov commented 6 years ago

if there's a branch I could work on top of, or if you want me to create that branch, I might be able to add more validation, or move more validation from validateSchema to individual SDL validation rules.

I'm trying to figure out if it's possible to reuse the same context for buildSchema and extendSchema since they are totally different use cases so I'm mostly figuring API for ValidationContext. I will reuse tests from your PR and I will try to push WIP PR after I have stable API to get feedback from you.

IvanGoncharov commented 6 years ago

@mjmahone Update: I finally figured out how to solve the last major problem. I will open a couple of PRs implementing this feature in the next few hours.

mjmahone commented 6 years ago

@IvanGoncharov is there more SDL validation you think we need prior to the 14.0 release? If so, anything I can do to help push it forward? I'd like us to put up at least an RC release by Monday if possible.

IvanGoncharov commented 6 years ago

@mjmahone Sorry for the silence. I'm working on a PR that will add a few more rules. I will push hard to publish it until Sunday morning your time so we can discuss the next steps.

I'd like us to put up at least an RC release by Monday if possible.

Monday seems reasonable due date for RC release 👍

Hendrixer commented 6 years ago

@IvanGoncharov not sure what other validations you have in mind. But validating input types on Directive args would be nice. Down to help with this if needed. Thanks for the hard work. 💯

mjmahone commented 6 years ago

@IvanGoncharov I think with #1463, this release is ready for at least a public RC? If so, I can put up a PR to bump the version tomorrow. I should also be able to do the upgrade across Relay's packages as well, although we might wait to actually bump the Relay version (@kassens would know that process better than me).

IvanGoncharov commented 6 years ago

But validating input types on Directive args would be nice.

@Hendrixer You absolutely right, we still don't validate directives fully since this rule doesn't support SDL: https://github.com/graphql/graphql-js/blob/master/src/validation/rules/ValuesOfCorrectType.js

But I think it less important and we could add this support later minor release. Since breaking change is not about fully validating directives but about forcing developers to actually define them. At the moment we validating

Down to help with this if needed.

It would be great if you try current master: https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge And provide feedback on it. Here is an example of how directives could be specified: https://github.com/graphql/graphql-js/blob/463174438d913bc0a2bf16a8b36b27d2fc3a66a1/src/utilities/__tests__/extendSchema-test.js#L1292-L1303

@mjmahone In the last few weeks, I also discover a couple of pretty small issues that technically a breaking change. They all about removing a few obscure features so I will try to make PRs in a next few hours.

eurostar-fennec-cooper commented 3 years ago

Is this feature still being considered? I ask because it's mentioned in https://github.com/ardatan/graphql-tools/issues/920. It's hard to trust/rely on directives when a graphql server can be started without directive functions and the directives in the SDL get skipped over/ignored.

yaacovCR commented 3 years ago

I believe directive usage is validated now. Your issue over at graphql-tools https://github.com/ardatan/graphql-tools/issues/920#issuecomment-762265595 suggests that you have a slightly different ask, see my response over there

yaacovCR commented 3 years ago

Well, validated in a sense that the directives have to exist, I don't know if the arguments are validated / coerced yet...

eurostar-fennec-cooper commented 3 years ago

Thanks for the help, all I want is to ensure that the directive functions exist, although yes I believe that the original author of the issue on graphql-tools wants to verify that the implementation is also correct.

I've been trying this with 15.3.0 and 14.5.8 and finding that the directive functions don't have to exist, but I'll give it another go.

yuchenshi commented 1 year ago

Any updates on validating the argument types?

https://spec.graphql.org/October2021/#sec-Values-of-Correct-Type seems to suggest that all literals should be validated, not just in executable. Shall we extend ValuesOfCorrectTypeRule.ts to SDL?