graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

feat: disable 'did you mean x' during validation

Open n1ru4l opened this issue 3 years ago • 7 comments

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:

  • https://github.com/graphql/graphql-js/issues/2247
  • https://github.com/redwoodjs/redwood/issues/4275

n1ru4l avatar Jan 28 '22 08:01 n1ru4l

I think this is great!

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

see https://github.com/graphql/graphql-js/issues/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 avatar Jan 28 '22 12:01 yaacovCR

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?

yaacovCR avatar Jan 28 '22 12:01 yaacovCR

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!

n1ru4l avatar Jan 28 '22 12:01 n1ru4l

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.

n1ru4l avatar Jan 28 '22 13:01 n1ru4l

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 avatar Feb 03 '22 09:02 benjie

(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.)

benjie avatar Feb 03 '22 09:02 benjie