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

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

Open
Peter-B- opened this issue Dec 12, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@Peter-B-
Copy link
Contributor

Describe the feature

Hello,

I use value objects with validation in combination with System.Text.Json. I have a proposal to improve error messages for failed validation.

Current state

When the validation of a value object fails, a ValueObjectValidationException is thrown. However, no information about the context within the Json is provided.

[ValueObject<string>(Conversions.None)]
public partial class IdObject
{
    public static Validation Validate(string value)
    {
        return string.IsNullOrWhiteSpace(value) ? Validation.Invalid("Must not be empty or whitespace") : Validation.Ok;
    }
}

public class Test
{
    public IdObject Id { get; set; }
}
try
{
     var json = """{"Id": " "}""";
     var res =JsonSerializer.Deserialize<Test>(json);
}
catch (Exception ex)
{
    Console.WriteLine(ex);
}

This leads to

Vogen.ValueObjectValidationException: Must not be empty or whitespace
   at IdObject.ThrowHelper.ThrowWhenValidationFails(Validation validation) in D:\projects\temp\VogenIssues\VogenIssues\obj\Debug\net9.0\Vogen\Vogen.ValueObjectGenerator\_IdObject.g.cs:line 369
   at IdObject.__Deserialize(String value) in D:\projects\temp\VogenIssues\VogenIssues\obj\Debug\net9.0\Vogen\Vogen.ValueObjectGenerator\_IdObject.g.cs:line 177
   at IdObject.IdObjectSystemTextJsonConverter.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) in D:\projects\temp\VogenIssues\VogenIssues\Program.cs:line 38
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)
  ...

Within complex Jsons with multiple value objects, the error is hard to pin down.

Proposal

System.Text.Json has special handling for JsonExceptions. If the Message is null, the context within the Json will automatically be provided.

Here is an example with a custom JsonConverter that wraps the ValueObjectValidationException within a JsonException.

[ValueObject<string>(Conversions.None)]
public partial class IdObject
{
    public static Validation Validate(string value)
    {
        return string.IsNullOrWhiteSpace(value) ? Validation.Invalid("Must not be empty or whitespace") : Validation.Ok;
    }

    public class IdObjectSystemTextJsonConverter : JsonConverter<IdObject>
    {
        public override IdObject Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            try
            {
                return __Deserialize(reader.GetString());
            }
            catch (ValueObjectValidationException e)
            {
                throw new JsonException(null, e);
            }
        }

        public override void Write(Utf8JsonWriter writer, IdObject value, JsonSerializerOptions options)
        {
            throw new NotSupportedException();
        }
    }
}

Tested with

try
{
    var jsonSerializerOptions = new JsonSerializerOptions(JsonSerializerDefaults.General);
    jsonSerializerOptions.Converters.Add(new IdObject.IdObjectSystemTextJsonConverter());

    var json = """{"Id": " "}""";
    var res = JsonSerializer.Deserialize<Test>(json, jsonSerializerOptions);
}
catch (Exception ex)
{
    Console.WriteLine(ex);
}

public class Test
{
    public IdObject Id { get; set; }
}

the following exception will be printed:

System.Text.Json.JsonException: The JSON value could not be converted to IdObject. Path: $.Id | LineNumber: 0 | BytePositionInLine: 10.
 ---> Vogen.ValueObjectValidationException: Must not be empty or whitespace
   at IdObject.ThrowHelper.ThrowWhenValidationFails(Validation validation) in D:\projects\temp\VogenIssues\VogenIssues\obj\Debug\net9.0\Vogen\Vogen.ValueObjectGenerator\_IdObject.g.cs:line 369
   at IdObject.__Deserialize(String value) in D:\projects\temp\VogenIssues\VogenIssues\obj\Debug\net9.0\Vogen\Vogen.ValueObjectGenerator\_IdObject.g.cs:line 177
   at IdObject.IdObjectSystemTextJsonConverter.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) in D:\projects\temp\VogenIssues\VogenIssues\Program.cs:line 38

Please note that the path within the Json is printed within the exception:

Path: $.Id | LineNumber: 0 | BytePositionInLine: 10.

Improvements

If this feature is implemented, there are some ways to further improve this:

  • Use some TryParse method to avoid two exceptions being thrown
  • Directly provide a custom message containing both the validation message(s) and the Path context

What do you think?

@Peter-B- Peter-B- added the enhancement New feature or request label Dec 12, 2024
@SteveDunn
Copy link
Owner

Thanks for suggestion @Peter-B- - it would certainly make diagnosing issues easier. The serializer would have to catch all exceptions though as it's possible to specify a different exception type. But either way, a JsonException is a better fit, and nothing will be lost by having the inner exception as the, er, inner exception :)

@Peter-B-
Copy link
Contributor Author

Hello @SteveDunn,

thank you for considering this. I was just looking at how to implement it. I see that all templates invoke VOTYPE.__Deserialize, which does the actual validation (and normalization). But since this method is used for all converters (not just STJ), it cannot throw a JsonException.

My idea is, to add another method

// only called internally from System.Text.Json Converter
private static Dave __DeserializeSystemTextJson(System.String value)
{
    try
    {
        return Dave.__Deserialize(value);
    }
    catch (System.Exception e)
    {
        throw new global::System.Text.Json.JsonException(null, e);
    }
}

All SystemTextJsonConverter-templates can be adapted to use this type.

Do you think this would be a good way to implement this?

I'd also like to create some test cases for that - a JsonException being thrown. Should that go next to ConsumerTests.DeserializationValidationTests.[Int|String]DeserializationValidationTests.Deserialization_systemtextjson_should_not_bypass_validation_pass

@SteveDunn
Copy link
Owner

Hi @Peter-B- thank you. Your suggestion sounds good. Perhaps __DeserializeSystemTextJson should be in each converter, the reason is that there's an upcoming task to have the option of separating out the serializers into a separate project (as some people don't want the System.Text.Json namespace with the domain types).

Re. the test cases - yes, that would be perfect!

Thanks again!

Steve

@Peter-B-
Copy link
Contributor Author

Thank you for the review of this proposal. I will try to implement it.

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

No branches or pull requests

2 participants