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

Validate variables are known types #3282

Closed
wants to merge 2 commits into from

Conversation

spawnia
Copy link
Member

@spawnia spawnia commented Sep 30, 2021

Adds a check to the validation rule VariablesAreInputTypes to ensure
that referenced types exist at all before checking they are input types.

This check is not explicitly described in the specification for validating
variables: http://spec.graphql.org/draft/#sec-Validation.Variables
I am assuming this is simply an oversight and the rule is implicit.
Still, it is valuable to check for it in order to provide a proper error to clients.

@spawnia
Copy link
Member Author

spawnia commented Sep 30, 2021

Is this rule the correct place to add this validation? Should we modify the spec to make this validation rule explicit?

Adds a check to the validation rule VariablesAreInputTypes to ensure
that referenced types exist at all before checking they are input types.

This check is not explicitly described in the specification for validating
variables: http://spec.graphql.org/draft/#sec-Validation.Variables
I am assuming this is simply an oversight and the rule is implicit.
Still, it is valuable to check for it in order to provide a proper error to clients.
@spawnia spawnia force-pushed the validate-unknown-variable-types branch from e21ef92 to 2eeafec Compare September 30, 2021 12:14
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Oct 1, 2021
@IvanGoncharov
Copy link
Member

@spawnia Thanks for opening PR 👍
I simplified it a bit in d41e2b6 and almost merged, but notice it's duplicated in "KnownTypeNames" rules.
I used this opportunity to make it more explicit in #3284

Since code changes are not relevant anymore I'm closing this PR.

Is this rule the correct place to add this validation? Should we modify the spec to make this validation rule explicit?

I did a quick cross-check and we have a bunch of discrepancies with the spec, so I opened a separate issue for that: #3286
If you are interested I can assign it to you?

@spawnia spawnia deleted the validate-unknown-variable-types branch October 6, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants