fix(validation): incorrect validation errors when variable descriptions are used

[phryneas]

I initially reported this as an LSP error (graphql/graphiql#4138), but turns out it's an error in graphql-js.

[vercel]

@phryneas is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[yaacovCR]

This fixes the current error and maintains the spec comment on the executable definitions PR (https://github.com/graphql/graphql-spec/pull/1170/changes#diff-0f02d73330245629f776bb875e5ca2b30978a716732abca136afdd028d5cd33cR38) where we say that descriptions should have no effect on execution/validation/response.

This seems like the correct change to me, passing a non-string literal will fail parsing, but should have no effect on validation.

[yaacovCR]

i threw out #4519 as an alternative approach to this PR, not sure if it's better or worse, basically it encodes within our code that we are skipping validation of all descriptions at a higher level such that we never recurse into these keys.

although I have to say I do find it odd that this means that tools may find (with hand-crafted ASTs) to have a description of non-string-type. A different approach would be to allow validation of descriptions, although the spec change as linked above explicitly says not to do this. So tools cannot always rely on the description not being malformed... Perhaps this issue requires a link to the reasoning behind which we are not validating that descriptions on executable definitions are not....valid....

[phryneas]

I think you're interpreting it the wrong way round.
Up until now, an Int variable definition could only have properties like value or defaultValue - all of type Int.

So until now this check would naively just check all properties of an Int variable to be of type Int.
Now there is a new property, description, which is of type String.

So the check fails because it finds a String property on an Int variable.

The error message doesn't really explain that nicely, but to be fair the error message was never meant to explain this case.

You can verify this: descriptions on String-Type variables are fine, but descriptions on any other type of variable trigger this error.

(I can't really look at the other PR right now, I'm on the road. Just wanted to leave a short explanation here)

[yaacovCR]

So the check fails because it finds a String property on an Int variable.

You can verify this: descriptions on String-Type variables are fine, but descriptions on any other type of variable trigger this error.

I might be understanding it correctly, but just more concerned about how for broken validation of descriptions doesn't "fix" the validation of descriptions within operations, but rather "skips" the validation entirely. A "fix" for description validation, for example, might be to hardcode the validation as expecting type String, rather than being based on the type of the variable. Alternatively, I may be hopelessly confused. :)

But assuming I am understanding this right, at first blush, I found "skipping" rather than "fixing" the validation desirable, as it matches a comment on the spec PR that introduced these descriptions, and I noted that if one attempts to parse an operation with an invalid description, it would fail anyway.

And then after some reflection, I found it odd that we would be ok allowing hand-rolled ASTs with invalid descriptions to pass validation. Of course, the spec PR says that we must! So then I wondered as to the motivation of that...

[Note: the linked PR basically does the same thing as this PR does, skipping the validation of descriptions, but instead of visiting the description and then skipping it, it never visits it, by subtracting description from the AST keys that are visited.]

[phryneas]

I don't think the point of this rule is to validate the AST, but to validate if the user specified operation was correct.

It just makes some assumptions about the AST that were correct in the past without descriptions, but are not correct anymore with descriptions in the AST now.

So it would error if you did something like

query Foo(bar: Int = "SomeString"){}

Which would parse correctly but would be semantically wrong.

But I might be wrong here. As for which PR to merge, I don't really care. I can take a look at your PR at some time after the holidays, I don't have the chance right now.