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

System.Text.Json is unable to deserialize FluentValidation.Results.ValidationResult #1928

Closed
Arclight3 opened this issue Apr 21, 2022 · 12 comments · Fixed by #1947
Closed

System.Text.Json is unable to deserialize FluentValidation.Results.ValidationResult #1928

Arclight3 opened this issue Apr 21, 2022 · 12 comments · Fixed by #1947
Milestone

Comments

@Arclight3
Copy link

FluentValidation version

10.4.0

ASP.NET version

6.0.202

Summary

System.Text.Json is unable to deserialize FluentValidation.Results.ValidationResult, while Newtonsoft.Json is able to do it.

I filled an issue on dotnet runtime repo (dotnet/runtime#68280) but they suggested to open one here because it might be that FluentValidation relies on conventions not supported by System.Text.Json or that it implements a converter only for Json.NET.

Steps to Reproduce

Check this code in .NET 6:

var validationResult = new ValidationResult
{
    Errors = { new ValidationFailure("MyProperty", "Invalid MyProperty") }
};

// System.Text.Json
var serialized = JsonSerializer.Serialize(validationResult);
var deserialized = JsonSerializer.Deserialize<ValidationResult>(serialized);

// Newtonsoft.Json
var serialized2 = JsonConvert.SerializeObject(validationResult, Formatting.Indented);
var deserialized2 = JsonConvert.DeserializeObject<ValidationResult>(serialized2);

The deserialized variable contains a ValidationResult object with IsValid = true, and 0 Errors.
The deserialized2 variable contains a ValidationResult object with IsValid = false, and 1 Error.

The serialization operation produces an identical json string for both libraries, but the System.Text.Json fails to deserialize it back to a ValidationResult object.

@JeremySkinner
Copy link
Member

JeremySkinner commented Apr 21, 2022

I can't really see what the issue is here. We don't do anything special or provide a converter or anything like that for JSON.NET. If it works with JSON.NET then I'd expect it to work with System.Text.Json too...

I would've suggested that you report it to them, but I see you already did and they closed the issue 🤷 I'm not sure what conventions they'd be referring to, and I don't really know why we'd need to provide a JsonConverter for what is a pretty simple type, but I'm probably missing something.

I'm not really able to help further I'm afraid - it really needs someone who knows about System.Text.Json to investigate, which isn't something I have any experience with. I'll tag this as Help Wanted for now and hopefully someone will be able to pick it up.

@crnd
Copy link
Contributor

crnd commented May 3, 2022

This is happening because of how deserialization is implemented in System.Text.Json as it currently doesn't support deserialization for collection properties without a setter (https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to?pivots=dotnet-6-0#add-to-collections-without-setters). The Errors property in ValidationResult is defined without a setter as public List<ValidationFailure> Errors => _errors;. The workarounds for this would be:

  • Define a setter
  • Define a custom converter
  • Use a public field

Having a setter or having a public field would both be changing the public interface so probably not worth it. If someone needs to be able to deserialize ValidationResult, they can always define a custom converter for it.

There is also some work going on with having support for deserializing to collection properties without setters but it has been postponed for three years now so wouldn't hold my breath: dotnet/runtime#30258

@JeremySkinner
Copy link
Member

@crnd thanks for the explanation, that makes sense. Yes it does seem that JSON.NET is fine deserializing without the public setter while STJ requires it. Exposing a setter isn't something we want to do for this property, so I think your suggestion of encouraging users to provide their own custom converter is the way to handle this.

@Vahdanian
Copy link

I noticed that the issue can be fixed by specifying a constructor as [JsonConstructor]. In fact every time "System.Text.Json" calls the default constructor in which the errors list will be initialized by an empty list and this will lead to failure in deserialization.
Screenshot 2022-05-04

Please also note that previous code in ValidationFailure.cs has rather the same issue and trying to deserialize an instance of this class will result in an exception.

exception

@JeremySkinner
Copy link
Member

JeremySkinner commented May 4, 2022

That's very frustrating. I'm really not keen on taking a dependency on STJ to work around their limitations when this has always worked fine with JSON.NET.

@Vahdanian
Copy link

You are right. I'll close the pull request.

@JeremySkinner
Copy link
Member

Ah I hadn’t seen that you’d opened a PR. I’ll take a look at it anyway. Although I’m not keen on taking a dependency on STJ it may be worth it if it is going to benefit a lot of users. I’ll review it when I get some spare time

@JeremySkinner
Copy link
Member

I've had some time to think about this further and I think I've decided to opt for adding public setters to ValidationResult and a public parameterless constructor to ValidationFailure. This seems like the simplest option as it doesn't require adding an explicit reference to System.Text.Json (which I want to avoid), nor does it require end users to define their own type converters.

The setter will preserve the same behaviour as the existing constructor (copies the list of failures and removes nulls).

This will go into 11.0.2 which I'm aiming to release tomorrow.

@JeremySkinner JeremySkinner added this to the 11.0.2 milestone May 26, 2022
@JeremySkinner
Copy link
Member

If anyone has time, I'd appreciate a quick review of #1947

@JeremySkinner
Copy link
Member

Published 11.0.2

@maxime-poulain-smartcoop

Hello @JeremySkinner,

Exposing a setter isn't something we want to do for this property,

I'd say that I am not a huge fan neither to change the modifier to that property to a public setter.

However,
using the modifier as

init

does work; see screenshot:

image

Only issue with this is that you need C# 9 + to have that working.
So the use of a preprocessor is needed for the target frameworks netstandar 2.0/2.1 and netcore 3.1 which are still used by the library.

Dunno if you would consider having the property init therefore.

@JeremySkinner
Copy link
Member

Thanks for the suggestion but will keep it as a setter as changing it again would be a breaking change.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants