Hotchocolate: Type validation doesn't work for string arguments when using variables

[gregsdennis]

There seems to be a problem with the type validation when an argument is of GQL type String and you're passing the value in from a variable.

To Reproduce
I have attached a solution that replicates the issue: HotChocolateTypeValidationBug.zip

Run this solution and then execute the following queries:

This correctly fails, reporting that the stringData argument is invalid.

query {
  test(stringData: false)
}

This incorrectly executes without error.

QUERY:

query Test($stringData: String!){
  test(stringData: $stringData)
}

VARIABLES:

{"stringData":false}

Expected behavior
I expect that both versions would produce the error.

Desktop (please complete the following information):

• OS: Windows
• Version 10

Additional context
Debugging into my query, it appears that the false value is being converted into a string containing "false" then passed to my method.

Interestingly, if I use {"stringData":5}, I get a "5" in the method parameter, but if I use {"stringData":{"test":"string"}}, a NullReferenceException is thrown from within HotChocolate:

at System.Object.GetType()
   at HotChocolate.Execution.VariableValueBuilder.EnsureClrTypeIsCorrect(IHasClrType type, Object value) in C:\\hc\\src\\Core\\Core\\Execution\\Utilities\\VariableValueBuilder.cs:line 169
   at HotChocolate.Execution.VariableValueBuilder.Normalize(VariableDefinitionNode variableDefinition, Variable variable, Object rawValue) in C:\\hc\\src\\Core\\Core\\Execution\\Utilities\\VariableValueBuilder.cs:line 139
   at HotChocolate.Execution.VariableValueBuilder.CoerceVariableValue(VariableDefinitionNode variableDefinition, IReadOnlyDictionary`2 variableValues, Variable variable) in C:\\hc\\src\\Core\\Core\\Execution\\Utilities\\VariableValueBuilder.cs:line 0
   at HotChocolate.Execution.VariableValueBuilder.CreateValues(IReadOnlyDictionary`2 variableValues) in C:\\hc\\src\\Core\\Core\\Execution\\Utilities\\VariableValueBuilder.cs:line 57
   at HotChocolate.Execution.ResolveOperationMiddleware.InvokeAsync(IQueryContext context) in C:\\hc\\src\\Core\\Core\\Execution\\Middleware\\ResolveOperationMiddleware.cs:line 49
   at HotChocolate.Execution.ClassMiddlewareFactory.<>c__DisplayClass2_0`1.<CreateDelegate>b__0(IQueryContext context) in C:\\hc\\src\\Core\\Core\\Execution\\Middleware\\ClassMiddlewareFactory.cs:line 53
   at HotChocolate.Execution.ValidateQueryMiddleware.InvokeAsync(IQueryContext context) in C:\\hc\\src\\Core\\Core\\Execution\\Middleware\\ValidateQueryMiddleware.cs:line 68
   at HotChocolate.Execution.ParseQueryMiddleware.InvokeAsync(IQueryContext context) in C:\\hc\\src\\Core\\Core\\Execution\\Middleware\\ParseQueryMiddleware.cs:line 67
   at HotChocolate.Execution.ExceptionMiddleware.InvokeAsync(IQueryContext context) in C:\\hc\\src\\Core\\Core\\Execution\\Middleware\\ExceptionMiddleware.cs:line 26

I'm using schema-first here. I'm not sure if this problem exists for code-first. I expect it probably does.

[gregsdennis]

@michaelstaib any way we can get a fix for this in 0.8.x? We're experiencing this issue in a production environment.

[michaelstaib]

Yes we could create 0.8.3. I will have a look at it tonight.

[michaelstaib]

I moved this now to a hotfix release. We have another issue that we want to fix quickly and I think we can do that quickly. Version 9 is still two weeks out.

[michaelstaib]

The issue is now partially fixed with 9.0.0-preview.23.

So, with this build lists and objects are no longer allowed and will lead to an exception.

With scalars it is more difficult and I am still figuring out how we should go about it. The problem here is that we convert scalars when they do not match. This is useful if we have something like a short and we want to add that as a value for an int.

When we have this completely fixed in the V9 branch then we will port it back to the V8 branch.

[gregsdennis]

I can see the conversion being useful for long support since GQL doesn't natively support more than 32-bit integers and suggests to string-encode them. It's less useful in the reported case where you're expecting a string and get a number.

[michaelstaib]

@gregsdennis I totally agree on the with you... I am just evaluation how we fix it without breaking custom scalars.

[gregsdennis]

@michaelstaib any updates on this?

[michaelstaib]

Not yet, but we will fix it with version 9.1. We will start on bug fixing at the end of 9.1.

[michaelstaib]

We are now starting on bug fixing .... this one will be on 9.1.0-preview.25.

[michaelstaib]

I have fixed this one now.... I will write a couple more tests and then merge it next week into 10.0.0-rc.1.