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 should honor fields that are already initialized during deserialization #39630

Closed
mtaylorfsmb opened this issue Jul 20, 2020 · 1 comment

Comments

@mtaylorfsmb
Copy link

I couldn't find this written up anywhere in here so if it is a dup then feel free to close this one.

As documented System.Text.Json always creates a new object and assigns it to the public settable fields. This is different than Json.NET and introduces problems that may not be easily solvable. Here is a simple example.

public class Template
{
    public Dictionary<string, string> DataFields { get; set; } = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
}

The Template class is used in a REST API wrapper around a third party library. It allows a series of key-value data fields to be provided. This information is sent to a lower level API that is case sensitive on keys. The caller of the API knows the case for the fields in its template and sets them accordingly.

There are 2 problems with how this works with System.Text.Json.

  1. The field has to be settable otherwise the deserializer won't set it. This makes it so that calling code can mess this up as well but we could isolate the client and server side types to protect this somewhat. Ideally we shouldn't need a setter to set dictionaries unless the property wasn't initialized.
  2. The deserializer will create a case sensitive comparer irrelevant. We have no control over this easily.

Note: This is just one example. There could be any # of other types with other scenarios.

The recommended workarounds, while doable, create more work that shouldn't be necessary.

  1. Creating a value converter would work if all dictionaries in your code work the same way or if you used separate serializer settings for each one but that is inefficient and would require a separate converter for each combination (plus the serializer options).
  2. Create a derived type from Dictionary that calls the base constructor. This is recommended for types that don't have public default constructors. But this may introduce additional fields in the serializer/deserializer JSON that are unexpected depending upon how the serializer treats dictionaries.

The proposal I make is that the deserializer should, out of the box without the need for any additional code, allow fields to be initialized by the owning type AND not overwrite the object if it is not null. This would save the allocation if the property is already initialized, allow for the owning type to ensure the object is initialized properly and eliminate the need for a setter on a field that shouldn't be settable anyway. The current workarounds add undue complexity and potential risk with no real value. If performance is a concern (although I cannot imagine a null check being expensive) then make it an opt-in option in the serializer.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jul 20, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2020
@layomia layomia added this to the 5.0.0 milestone Jul 21, 2020
@layomia
Copy link
Contributor

layomia commented Jul 21, 2020

#30258 addresses the ask here. This feature cannot be enabled by default (without additional configuration), as this would be a behavioral breaking change in the serializer.

I'll close this as a duplicate.

@layomia layomia closed this as completed Jul 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants