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

feat: disable 'did you mean x' during validation #3467

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

n1ru4l
Copy link
Contributor

@n1ru4l n1ru4l commented Jan 28, 2022

This PR introduces a new property didYouMean on the ValidationContext, that can be used by validation rules for determining whether suggestions should be added to the error message.

Example Usage:

import { validate } from "graphql";

const errors = validate(schema, documentAST, undefined, {
  didYouMean: false
});

I did not add the test yet as I first want to start the discussion on whether this is how this should be implemented.


Related issues:

@yaacovCR
Copy link
Contributor

I think this is great!

would suggest option name to be enableSuggestions with default of true, perhaps might eventually default to false

see #2247

in that issue, consensus seems to be around disabling introspection to be one and the same with disabling suggestions, but I think this PR might be about the lower level building blocks that allow that direction

@yaacovCR
Copy link
Contributor

yaacovCR commented Jan 28, 2022

There are also some didYouMean calls in enum parseLiteral, I think we can add an execution option (in parallel to the one in validate) that then has to be passed to parseLiterals?

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Jan 28, 2022

There are also some didYouMean calls in enum parseLiteral, I think we can add an execution option in parallel to the one till validate that then has to be passed to parseLiterals?

I only focused on the validation rules for now. I am happy to also add this to further parts that require it, once there is a consensus on how we should tackle this!

Copy link
Member

@saihaj saihaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like right way forward to me.
Side note: we should make validate also take one argument which is an object.

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Jan 28, 2022

Side note: we should make validate also take one argument which is an object.

I had a discussion with @benjie about this before in the context of adding contextValue to validation rules. One takeaway was "The currently validate signature is useful for the cache use case, [...]". Please correct me if I interpreted this wrong.

validate(a, b, c)

is easier cachable than

validate({
  a,
  b,
  c,
})

For internal reference: https://tortilla-hq.slack.com/archives/CAY2119MX/p1637243621148400

Though I am not against changing the function signature, however, this might be out of the scope of this PR.

@benjie
Copy link
Member

benjie commented Feb 3, 2022

I meant that if your validation result is valid independent of context and variables then you can cache the validation result and the next time you see the query you can know it's valid independent of the new context and variables that it comes along with. The actual signature of the function is unimportant so long as context and variables are not included (caching it based on the list of args vs the extracted list of properties isn't much different).

@benjie
Copy link
Member

benjie commented Feb 3, 2022

(Also the GraphQL spec itself makes it very clear that validation does not factor in context or variables - anything that does require that (e.g. coercing the variable values) is done during ExecuteRequest.)

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree wit Lee's coment here: #2247 (comment)

It should be configured by schema and should disable the execution of introspection and even access to introspection types from GraphQLSchema.
Also, there are other places using didYouMean outside of validation, e.g.

didYouMean(suggestions),

So it should be a solution that can disable introspection leaking all over the library, not only for validation.

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

Successfully merging this pull request may close these issues.

None yet

7 participants