ArgumentsOfTheCorrectType should consiser variables as well as literals

[bbakerman]

Describe the bug

Variables are not considered when checking if the arguments are of the correct type

See https://spectrum.chat/graphql-java/general/validation-rules-while-using-graphql-variables~007a7967-f9e5-4b60-9572-4e0984f7570f

To Reproduce

=============================
schema:
type Query {
   example(id: ID!): Example
}
type Example {
   id: ID!
   name: String!
}
============= fails ArgumentsOfCorrectType rule =============
Query{
   Example(id: true) {
      id
     name
   }
}
============= passes ArgumentsOfCorrectType rule =============
Query($id: ID!){
  Example(id: $id) {
    id
    name
  }
}
"variables": {
"id": true,
}

[dugenkui03]

The target of ID is to represent a unique identifier or cache key. Boolean type is not a suitable type.

The ID scalar type represents a unique identifier, often used to refetch an object or as the key for a cache.

It seems that the best way of coercion ID is customized by server.

[bbakerman]

Looking into this bug I found that the validation code in general does NOT take the variable values into consideration.

graphql.validation.ValidationContext for example does not hold the variable values at all. We would need to refactor this to be able to fix this issue.

The other code challenge is how to turn a complex type (map / list etc ..) into a value that can be checked against the required input. We do this with literals today and we would need to do the same for variable values

[benvenutisw]

Looking at GraphQL specs, the variable values are checked during Execution phase, specifically while trying to coerce the variable values (http://spec.graphql.org/June2018/#sec-Coercing-Variable-Values). The so-called "Validation" works only on the document part, including operations and fragments.

In the example mentioned above, the value true is being coerced to the variable type (ID) and that's why no error is generated.

...
Query($id: ID!){
  Example(id: $id) {
    id
    name
  }
}
"variables": {
"id": true,
}

If we switch the variable type to Int (and do the same with the input id), then the coercion will fail and an error message is expected such as

Variable 'id' has an invalid value : Expected type 'Int' but was 'Boolean'

I saw in the GraphQL reference implementation that the same scenario actually produces the error "ID cannot represent value: true".

But the spec also states that:

A GraphQL service may decide to allow coercing different internal types to the expected return type. For example when coercing a field of type Int a boolean true value may produce 1 or a string value "123" may be parsed as base‐10 123. However if internal type coercion cannot be reasonably performed without losing information, then it must raise a field error. Since this coercion behavior is not observable to clients of the GraphQL server, the precise rules of coercion are left to the implementation. The only requirement is that the server must yield values which adhere to the expected Scalar type.

So, IMHO, the current implementation seems to be compliant with the specs.

As we have multiple alternatives here, it would be great to hear someone else's opinion.

[MaartenTutak]

My team just came across this issue when switching from literals to variables in our testing. Previously passing a String value of "TEST" into a Boolean type would result in an error, now it is being coerced into Boolean value false.

[bbakerman]

There are two factors at play here with booleans and variables

1 is the later validation of variable values - it gets done but not during the validation phase

The other is the treatment of booleans by the scalar when its a variable

See https://github.com/graphql-java/graphql-java/issues/1103

I add this only to put more color into the design discussion

[github-actions[bot]]

Hello, this issue has been inactive for 60 days, so we're marking it as stale. If you would like to continue this discussion, please comment within the next 30 days or we'll close the issue.