ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.18k stars 738 forks source link

Strawberry Shake doesn't enumerate errors if return type is Non-nullable #6038

Closed joshslark closed 1 year ago

joshslark commented 1 year ago

Is there an existing issue for this?

Product

Strawberry Shake

Describe the bug

When you make a graphQL call to a mutation and there is a validation error of some sort, you would expect to see the validation error in the IOperationResult. However, the graphQL server is returning an empty Data object which gets processed before the Errors object by Strawberry Shake. If the payload is non-nullable, then the empty Data object causes an ArgumentNullException and the error populated in the IOperationResult is "Value cannot be null" instead of the Validation error.

Steps to reproduce

  1. Create an example mutation in the server that always fails with a descriptive error and returns a non-nullable payload
  2. Use strawberry shake to call the mutation and then call EnsureNoErrors() to raise the exception
  3. It will be "Value cannot be null", instead of the descriptive error message.

Relevant log output

No response

Additional Context?

The issue is caused by the code at [https://github.com/ChilliCream/graphql-platform/blob/bd3f5559d81c5bdabec152bac10e7da154b6e0d2/src/StrawberryShake/Client/src/Core/OperationResultBuilder.cs#L33](). If dataProp is constructed from the Json {}, then the if condition is met and BuildData(dataProp) will result in an ArgumentNullException if the payload should be non-nullable.

To avoid this issue, the code would need to fail the first if condition in order to get the correct error returned. For example, you could add && dataProp.GetRawText() != "{}" and it would skip the first if body. I don't think this is the best solution, but hopefully this helps fully illustrate what the problem is.

Version

13.0.5

joshslark commented 1 year ago

Fixed in 13.1.0-rc.0, thanks!

bennettwork commented 11 months ago

I still have this problem in 13.5.1. In fact it seems worse because it doesn't put the message in the IOperationResult, instead it just throws the exception for me to catch. If I hack the schema to make the payload nullable (by removing the !) then everything works as expected and the validation error shows up in the IOperationResult.