Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(validation): use the existing errors array as context.Result, in… #267

Closed
wants to merge 1 commit into from
Closed

feat(validation): use the existing errors array as context.Result, in… #267

wants to merge 1 commit into from

Conversation

rune-vikestad
Copy link
Contributor

@rune-vikestad rune-vikestad commented Feb 17, 2022

Summary

We found that many of our application errors masked as EXEC_INVALID_LEAF_VALUE with the message Boolean cannot serialize the given value..

I eventually found that for all affected mutations we always hit the validator, but never the resolver. This lead me to look closer at the validation middleware, which I found was swallowing the original errors and replacing them with the magic string "errors".

I eventually found that the root cause of this was one of our AbstractValidator's, where a RuleFor failed. However, the validation middleware currently only replaces the original error with the magic string "errors"

Fixed

In 90.3.4 and earlier you'd find the following line in Dolittle.Vanir.Backend.GraphQL.Validation.ValidationMiddleware.cs

var errors = new List<IError>();
...
context.result = errors;
...

However, in 90.3.5 this suddenly changed to;

var errors = new List<IError>();
....
context.result = "errors";
....

Notice the new value of context.Result.

This is obviously not intended, as you can't magically set a string on a GraphQL result, and expect it to serialize correctly.

…steasd of overriding with a magic "errors" string
@jakhog
Copy link
Contributor

jakhog commented Feb 22, 2022

@rune-vikestad, we merged this fix in #269. I couldn't figure out how to build this PR from a remote repository, and we had some issues with the build pipelines that we needed to sort out. But this change is now released as v12.0.1. (There are no breaking changes in C#, but TS and C# are versioned together).

Thanks a lot for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants