Improve error message for validation errors in System.Test.Json deserialization

[Peter-B-]

This PR addresses #723 by throwing JsonExceptions, when the creation or validation of a Vogen object fails during System.Text.Jsondeserialization.

All JsonConverter templates were updated to include a method

private static VOTYPE DeserializeJson(VOUNDERLYINGTYPE value)

This method is invoked from both Read and ReadAsPropertyName.

The converters for both DateOnly and TimeOnly directly invoked the constructor before. Please review those changes carefully, since I have the feeling, I broke something there.

I added some unit tests for Json deserialization, adapted existing ones, and updated the snapshots. Please let me know, if I missed something or should adapt the code.

Kind regards
Peter

PS: I grossly underestimated the effort you put into this project. Thanks a lot for sharing this with the community!

[SteveDunn]

Thank you very much @Peter-B- !
One thing that springs to mind with the try/catch approach is that the JIT might not inline the containing method, which could impact performance when deserializing a lot of value objects.

The __Deserialize method generates code which calls ThrowHelper when validation fails. That way, __Deserialize doesn't explicitly declare an Exception, so the method is more likely to be considered for inlining.

I think the approach you've taken with a separate method is the correct one as it removes the STJ dependency for every value object. I think we should explore ways of hooking into the ThrowHelper to follow the same pattern. Perhaps by abstracting the __Deserialize method generation and reusing it. But I understand that Vogen is quite a convoluted codebase, so I can take what you've done and extend it, although if you're happy to continue based on what I've just waffled on about, then feel free!

Thanks again for this, much appreciated - I do think it will make diagnosing serialization issues much easier.

[Peter-B-]

Hi @SteveDunn,

I've seen the ThrowHelper, and I actually considered this. We could directly call the .Validate method and and then use a throw helper to throw a JsonException. However, since Validate is user code, it might throw any exception and we would still have to wrap it into a try - catch. On the other hand, you could argue that Validate should not throw an exception, but return Invalid.

If I understand it correctly, this means we have to generate the complete deserialization, normalization and validation into a placeholder in the template. It might look like this:

private static MyVoString DeserializeJson(System.String value)
{
    if (value is null)
    {
        ThrowHelper.ThrowJsonExceptionWhenCreatedWithNull();
        return default !;
    }

    value = MyVoString.NormalizeInput(value);
    var validation = MyVoString.Validate(value);
    if (validation != Vogen.Validation.Ok)
    {
        ThrowHelper.ThrowJsonExceptionWhenValidationFails(validation);
    }

    return new MyVoString(value);
}

What do you think?

Actually, I'm not sure how I would implement this (reuse and modify the existing code to throw another exception with an inner exception), so if you like to do that, I'd be glad to hand over :-)

Reusing the DeserializeJson method by generating it sounds like a great idea. In fact I felt the urge to wash my hands after changing all the templates. I will try to add this to my branch.

[Peter-B-]

Hello @SteveDunn,

I have extended my implementation and the result looks something like

#if NETCOREAPP3_0_OR_GREATER
            [global::System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
#endif
            private static void ThrowJsonExceptionWhenValidationFails(Vogen.Validation validation)
            {
                var e = ThrowHelper.CreateValidationException(validation);
                throw new global::System.Text.Json.JsonException(null, e);
            }

            private static void ThrowJsonExceptionWhenNull(System.String value)
            {
                if (value == null)
                {
                    var e = ThrowHelper.CreateCannotBeNullException();
                    throw new global::System.Text.Json.JsonException(null, e);
                }
            }

            public static MyVoString_should_not_bypass_validation DeserializeJson(System.String value)
            {
                ThrowJsonExceptionWhenNull(value);
                value = MyVoString_should_not_bypass_validation.NormalizeInput(value);
                var validation = MyVoString_should_not_bypass_validation.validate(value);
                if (validation != Vogen.Validation.Ok)
                {
                    ThrowJsonExceptionWhenValidationFails(validation);
                }

                return new MyVoString_should_not_bypass_validation(value);
            }

The DeserializeJson method does not directly throw exceptions anymore. I extended the Utils to reuse some of the code generation, even though there is still some measure of duplication.

I put the throw helper methods directly into each converter, which is some duplication again, but I didn't know where to put them and how to track, if another converter already created the necessary classes.

I didn't do thorough testing yet. Can you please take a look at it and tell me if this goes into the right direction?

Thank you
Peter

[SteveDunn]

Hello @SteveDunn,

I have extended my implementation and the result looks something like

#if NETCOREAPP3_0_OR_GREATER
            [global::System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
#endif
            private static void ThrowJsonExceptionWhenValidationFails(Vogen.Validation validation)
            {
                var e = ThrowHelper.CreateValidationException(validation);
                throw new global::System.Text.Json.JsonException(null, e);
            }

            private static void ThrowJsonExceptionWhenNull(System.String value)
            {
                if (value == null)
                {
                    var e = ThrowHelper.CreateCannotBeNullException();
                    throw new global::System.Text.Json.JsonException(null, e);
                }
            }

            public static MyVoString_should_not_bypass_validation DeserializeJson(System.String value)
            {
                ThrowJsonExceptionWhenNull(value);
                value = MyVoString_should_not_bypass_validation.NormalizeInput(value);
                var validation = MyVoString_should_not_bypass_validation.validate(value);
                if (validation != Vogen.Validation.Ok)
                {
                    ThrowJsonExceptionWhenValidationFails(validation);
                }

                return new MyVoString_should_not_bypass_validation(value);
            }

The DeserializeJson method does not directly throw exceptions anymore. I extended the Utils to reuse some of the code generation, even though there is still some measure of duplication.

I put the throw helper methods directly into each converter, which is some duplication again, but I didn't know where to put them and how to track, if another converter already created the necessary classes.

I didn't do thorough testing yet. Can you please take a look at it and tell me if this goes into the right direction?

Thank you Peter

Looking great, many thanks @Peter-B- ! I forgot just how confusing that STJ code can be, what with templates and replaceable code etc!

One thing that might be worth considering is if users have configured Vogen to throw a different validation exception. But then again, I guess it would be unusual to deserialise something and rely solely on that exception type to signify errors.

I'm rebasing the snapshot tests now (running by running .\RunSnapshots.ps1 -v "minimal" -reset ) - and will commit any changes.

Thanks again!

[SteveDunn]

... and I've just realised I've pushed to the wrong branch. If you run .\RunSnapshots.ps1 -v "minimal" -reset and push the changes, it should fix the CI build.

Oh no I didn't, it just took a while to come through

[SteveDunn]

LGTM! Thanks very much Peter!

[Peter-B-]

Great! I have to admit that I was confused at times, but the naming and code style is good and so I found my way around.

It is indeed hard to say, if this is a breaking change - of course there is no such thing as a non-breaking change. I think it is definitely an improvement, but you might also want to wait for a major release.

[SteveDunn]

Great! I have to admit that I was confused at times, but the naming and code style is good and so I found my way around.

It is indeed hard to say, if this is a breaking change - of course there is no such thing as a non-breaking change. I think it is definitely an improvement, but you might also want to wait for a major release.

Thank you - yes, I intend to release this as the first beta of 7.0.0 to see if anyone complains. Hopefully it won't impact anyone, and the plus side will be far more detailed exceptions when deserialising.

Thanks for all your efforts with this one!