Support modifying (rather than replacing) already-initialized properties and fields when deserializing

[steveharter]

Per comment in https://github.com/dotnet/corefx/issues/38435#issuecomment-504686914 and the original design before "replace" semantics in dotnet/corefx#39442 was added is to allow a collection to:

• Not have a property setter.
• Previously instantiated from POCO ctor.
• Allow that collection to have elements added to during deserialization.

The existing semantics ignore the property on deserialization.

To turn on this mode, a custom attribute could be created to enable and placed on the property, and\or a global option. Doing this by default would be a breaking change (if added post 3.0).

cc @JamesNK @YohDeadfall

[YohDeadfall]

Doing this by default would be a breaking change (if added post 3.0).

It doesn't sound well. Actually, I don't think that a lot people will use the new serializer due to limitations. But others who will use it, mostly won't see this as a breaking change.

[ahsonkhan]

From @chenyj796 in https://github.com/dotnet/corefx/issues/41433

I have a Config class which contains a readonly property named Items.
I serialize it to json string, and then deserialize the json string back as below.
After run the Run() method, cfg2.Items is empty when cfg3.Items contains one item.

public class Config
{
    public HashSet<string> Items { get; } = new HashSet<string>();
}

public class ConfigTest
{
    public void Run()
    {
        var cfg = new Config();
        cfg.Items.Add("item1");
        var json = System.Text.Json.JsonSerializer.Serialize(cfg);
        var cfg2 = System.Text.Json.JsonSerializer.Deserialize<Config>(json);
        var cfg3 = Newtonsoft.Json.JsonConvert.DeserializeObject<Config>(json);
    }
}

[serhatozgel]

Hi @steveharter, I'm not sure if related to this particular issue but I noted down an issue with array deserialization here: dotnet/corefx#39442 (comment)

[joshlang]

This is a necessary feature for folks wanting to serialize protobuf.

Example:

message Something {
  repeated int32 Values = 1;
}

Protobuf code generator will generate the equivalent of:

public RepeatedField<int> Values { get; } = new RepeatedField<int>();

The expected behavior (and newtonsoft's behavior) would be to call the initializers to populate the data.

For example I would expect { "Values" : [1, 2, 3] } to function essentially like new Something { Values = { 1, 2, 3 } }

It's unexpected, especially when a serializer option IgnoreReadOnlyProperties exists and I set it to false, that the values [1, 2, 3] are just ignored.

Perhaps add a NonoForRealDontIgnoreReadOnlyProperties option :D

Am I correct in understanding you don't intend to fix/enhance this until .net 5.0 is out in a year or so?

[joshlang]

If anyone else is being affected by this, consider a converter in the meantime. Here's one I just made since discovering the issue:

class MagicConverter : JsonConverterFactory
{
    public override bool CanConvert(Type typeToConvert) =>
        !typeToConvert.IsAbstract &&
        typeToConvert.GetConstructor(Type.EmptyTypes) != null &&
        typeToConvert
            .GetProperties()
            .Where(x => !x.CanWrite)
            .Where(x => x.PropertyType.IsGenericType)
            .Select(x => new
            {
                Property = x,
                CollectionInterface = x.PropertyType.GetGenericInterfaces(typeof(ICollection<>)).FirstOrDefault()
            })
            .Where(x => x.CollectionInterface != null)
            .Any();

    public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) => (JsonConverter)Activator.CreateInstance(typeof(SuperMagicConverter<>).MakeGenericType(typeToConvert))!;

    class SuperMagicConverter<T> : JsonConverter<T> where T : new()
    {
        readonly Dictionary<string, (Type PropertyType, Action<T, object>? Setter, Action<T, object>? Adder)> PropertyHandlers;
        public SuperMagicConverter()
        {
            PropertyHandlers = typeof(T)
                .GetProperties()
                .Select(x => new
                {
                    Property = x,
                    CollectionInterface = !x.CanWrite && x.PropertyType.IsGenericType ? x.PropertyType.GetGenericInterfaces(typeof(ICollection<>)).FirstOrDefault() : null
                })
                .Select(x =>
                {
                    var tParam = Expression.Parameter(typeof(T));
                    var objParam = Expression.Parameter(typeof(object));
                    Action<T, object>? setter = null;
                    Action<T, object>? adder = null;
                    Type? propertyType = null;
                    if (x.Property.CanWrite)
                    {
                        propertyType = x.Property.PropertyType;
                        setter = Expression.Lambda<Action<T, object>>(
                            Expression.Assign(
                                Expression.Property(tParam, x.Property),
                                Expression.Convert(objParam, propertyType)),
                            tParam,
                            objParam)
                            .Compile();
                    }
                    else
                    {
                        if (x.CollectionInterface != null)
                        {
                            propertyType = x.CollectionInterface.GetGenericArguments()[0];
                            adder = Expression.Lambda<Action<T, object>>(
                                Expression.Call(
                                    Expression.Property(tParam, x.Property),
                                    x.CollectionInterface.GetMethod("Add"),
                                    Expression.Convert(objParam, propertyType)),
                                tParam,
                                objParam)
                                .Compile();
                        }
                    }
                    return new
                    {
                        x.Property.Name,
                        setter,
                        adder,
                        propertyType
                    };
                })
                .Where(x => x.propertyType != null)
                .ToDictionary(x => x.Name, x => (x.propertyType!, x.setter, x.adder));
        }
        public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) => throw new NotImplementedException();
        public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            var item = new T();
            while (reader.Read())
            {
                if (reader.TokenType == JsonTokenType.EndObject)
                {
                    break;
                }
                if (reader.TokenType == JsonTokenType.PropertyName)
                {
                    if (PropertyHandlers.TryGetValue(reader.GetString(), out var handler))
                    {
                        if (!reader.Read())
                        {
                            throw new JsonException($"Bad JSON");
                        }
                        if (handler.Setter != null)
                        {
                            handler.Setter(item, JsonSerializer.Deserialize(ref reader, handler.PropertyType, options));
                        }
                        else
                        {
                            if (reader.TokenType == JsonTokenType.StartArray)
                            {
                                while (true)
                                {
                                    if (!reader.Read())
                                    {
                                        throw new JsonException($"Bad JSON");
                                    }
                                    if (reader.TokenType == JsonTokenType.EndArray)
                                    {
                                        break;
                                    }
                                    handler.Adder!(item, JsonSerializer.Deserialize(ref reader, handler.PropertyType, options));
                                }
                            }
                            else
                            {
                                reader.Skip();
                            }
                        }
                    }
                    else
                    {
                        reader.Skip();
                    }
                }
            }
            return item;
        }
    }
}

And it's been extensively tested with these industry-standard unit tests featuring breakpoint + mouse-hover debugging assertions:

var options = new JsonSerializerOptions { Converters = { new MagicConverter() } };

var adsfsdf = System.Text.Json.JsonSerializer.Deserialize<Grrrr>("{\"Meow\":[1,2,3]}", options);
var adsfsdf2 = System.Text.Json.JsonSerializer.Deserialize<Grrrr>("{\"Meow\":null}", options);
var adsfsdf3 = System.Text.Json.JsonSerializer.Deserialize<Grrrr>("{\"Meow\":[1,2,3],\"Rawr\":\"asdf\"}", options);
var adsfsdf4 = System.Text.Json.JsonSerializer.Deserialize<Grrrr>("{\"Meow\":[1,2,3],\"Rawr\":null}", options);
var adsfsdf5 = System.Text.Json.JsonSerializer.Deserialize<Grrrr>("{\"Meow\":[1,2,3],\"Rawr\":\"asdf\",\"SubGrr\":{\"Meow\":[1,2,3],\"Rawr\":\"asdf\"}}", options);

It's a read-only converter (it doesn't serialize, just deserialize).

Anyway, just dumping it in case it helps. We'll be implementing and testing ours in earnest in the days to come. Happy to share refinements if anyone needs it, but this is essentially a hacky workaround until a feature shows up, so it'd never be fully featured.

[Sandeep321]

Any updates on the roadmap for this. Any idea on when this could be released?

[steveharter]

This is still a valid issue; it just hadn't been prioritized for 5.0 (yet).

Strawman proposal:

• Add a new attribute to place on properties (JsonObjectCreation).
• Add a new JsonSerializerOptions (JsonObjectCreation) to define the global behavior.

The attribute and option would likely be an enum similar to https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_ObjectCreationHandling.htm (Auto, Reuse, Replace) except that the default would be Replace.

Note this attribute works on objects as well as collections.

For the re-use cases, I don't think we ever want to attempt to call a "Clear" method on collections (we always append).

[gaensebluemchenritter]

With the behaviour as of now we actually need to suppress CA2227 for collections in our DTOs we want to deserialize into. So the out of the box coding standards actually prevent usage of the Deserializer ...

From where I'm standing an additional JsonSerializerOption would probably be the best way to proceed.

[layomia]

From @dbc2 in #32197 (comment):

If I have a get-only, preallocated [JsonExtensionData] property on my model, then the dictionary will be serialized successfully, but will silently fail to be populated on deserialization. This seems wrong. Details as follows.

Say I have the following model:

public class Model
{
    [JsonExtensionData]
    public Dictionary<string, object> ExtensionData { get; } = new Dictionary<string, object>();
}

And the following test method:

public class TestClass
{
    [TestMethod]
    public void TestReadOnlyExtensionDataProperty()
    {
        var model1 = new Model { ExtensionData = { { "item", "item value" } } };

        var json = JsonSerializer.Serialize(model1);

        Debug.WriteLine(json); // Prints {"item":"item value"} as expected

        Assert.AreEqual(json, "{\"item\":\"item value\"}"); // Passes.

        var model2 = JsonSerializer.Deserialize<Model>(json);

        Assert.AreEqual(model1.ExtensionData.Count, model2.ExtensionData.Count, "model2.ExtensionData.Count"); // FAILS!
    }
}

Then the first assert will succeed, because model1 is correctly serialized as {"item":"item value"}. However, the second assert fails, because model2.ExtensionData is empty when deserialized.

Demo fiddle 1 here reproducing the problem.

Conversely, Json.NET is able to round-trip this model successfully, see demo fiddle 2 here.

And if I modify Model.ExtensionData to have a setter that throws an exception, then deserialization also succeeds:

public class Model
{
    readonly Dictionary<string, object> extensionData = new Dictionary<string, object>();

    [JsonExtensionData]
    public Dictionary<string, object> ExtensionData { get => extensionData; set => throw new NotImplementedException(); }
}

Demo fiddle 3 here, which passes.

This seems wrong. Since the dictionary is preallocated it should be populated successfully during deserialization even without a setter -- especially since the setter isn't actually called.

[layomia]

This work should also consider sub-objects without setters, not just collections.

From @johncrim in #3145:

If a parent object doesn't contain setters for all sub-object properties, the sub-objects are not deserialized. This is inconsistent with other JSON serializers, and seems like a major oversight - current behavior requires a large amount of mutability in the deserialized object model.

It's also a normal pattern to provide sub-objects with default values before deserialization.

For example, in this class, the SubObject1 property won't be deserialized, and no exception is thrown.

    public class Container1
    {
        // Not deserialized due to missing setter. No exceptions thrown.
        public SubObject1 SubObject1 { get; } = new SubObject1();
    }

Full test project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <Description>Unit tests demonstrating JsonSerializer bugs</Description>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <Platform>x64</Platform>
    <Platforms>x64</Platforms>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.4.0" />
    <PackageReference Include="xunit" Version="2.4.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" PrivateAssets="All"/>
  </ItemGroup>

</Project>

using System.Text.Json;
using Xunit;

namespace JsonSerializer_SubObjects
{
    public class JsonSerializerTest
    {
        [Fact] // Fails
        public void DeserializesSubObjectsWithoutSetters()
        {
            const string jsonFooValue = "str1";
            var json = "{ \"SubObject1\": { \"Foo\": \"" + jsonFooValue + "\" }}";

            var c1 = JsonSerializer.Deserialize<Container1>(json);

            // BUG dotnet/corefx#1: Value isn't deserialized (this assertion fails)
            Assert.Equal(jsonFooValue, c1.SubObject1.Foo);
            // BUG dotnet/corefx#2: If value can't be deserialized, exception should be thrown
        }

        [Fact] // Passes
        public void DeserializesSubObjectsWithSetters()
        {
            const string jsonFooValue = "str1";
            var json = "{ \"SubObject1\": { \"Foo\": \"" + jsonFooValue + "\" }}";

            var c2 = JsonSerializer.Deserialize<Container2>(json);

            Assert.Equal(jsonFooValue, c2.SubObject1.Foo);
        }

    }

    public class Container1
    {
        // Not deserialized due to no setter. No exceptions thrown.
        public SubObject1 SubObject1 { get; } = new SubObject1();
    }

    public class Container2
    {
        public SubObject1 SubObject1 { get; set; } = new SubObject1();
    }

    public class SubObject1
    {
        public string Foo { get; set; } = "foo";
    }
}

[layomia]

From @componentspace in #35388:

In the following code Newtonsoft.Json correctly deserializes but System.Text.Json doesn't with the dictionary being empty. If the IDictionary<string, string> property has a setter, the deserialization works. However, I don't control the class that requires serialization/deserialization and it includes IDictionary<string, string> properties with getters only.

`
using Newtonsoft.Json;
using System.Collections.Generic;
using System.Text.Json;

namespace TestConsoleApp
{
class Test
{
public IDictionary<string, string> Items { get; } = new Dictionary<string, string>();
}

class Program
{
    static void Main(string[] args)
    {
        var test = new Test();
        test.Items["a"] = "1";
        test.Items["b"] = "2";

        var json = System.Text.Json.JsonSerializer.Serialize(test);

        // Dictionary is empty.
        var test2 = System.Text.Json.JsonSerializer.Deserialize<Test>(json);

        // Correctly deserialized.
        var test3 = JsonConvert.DeserializeObject<Test>(json);
    }
}

}

`

[rrelyea]

Would really love to be able to follow .net design guidelines for collections to not have settable collection properties.

Strongly believe this should be the default behavior.
If it wasn't supported...i would have expected an exception if the property was set in json, but the collection had no setter.

XAML supported save/load of objects like this w/o needing the setter or any extra attribute.
(i was architect of system.xaml)

[layomia]

From @Symbai in #38705:

Description

The pull request #34675 added support for private setters. And the default value for IgnoreReadOnlyProperties is false. So I would have assumed something like: [JsonInclude] public ObservableCollection<int> MyInt { get; } = new ObservableCollection<int>(); would work but it does not. It still requires the private set; to explicit written.

Configuration

• .NET Core 5.0 (5.0.100-preview.5.20279.10)

Other information

    class Program
    {
        public static JsonSerializerOptions Options =>  new JsonSerializerOptions {IgnoreReadOnlyProperties = false};
        static void Main(string[] args)
        {
            var readonlySetter = new MyClass_Readonly();
            readonlySetter.MyInt.Add(1);
            var readonlyJson = JsonSerializer.Serialize(readonlySetter);
            Console.WriteLine("Readonly JSON: " + readonlyJson);
            var newReadonlySetter = JsonSerializer.Deserialize<MyClass_Readonly>(readonlyJson);
            Console.WriteLine("Readonly List Count: " + newReadonlySetter.MyInt.Count);

            var privateSetter = new MyClass_PrivateSetter();
            privateSetter.MyInt.Add(1);
            var privateJson = JsonSerializer.Serialize(privateSetter);
            Console.WriteLine("Private JSON: " + privateJson);
            var newPrivateSetter = JsonSerializer.Deserialize<MyClass_PrivateSetter>(privateJson);
            Console.WriteLine("Private List Count: " + newPrivateSetter.MyInt.Count);
        }
    }
    class MyClass_Readonly
    {
        [JsonInclude] public ObservableCollection<int> MyInt { get;  } = new ObservableCollection<int>();
    }
    class MyClass_PrivateSetter
    {
        [JsonInclude] public ObservableCollection<int> MyInt { get; private set; } = new ObservableCollection<int>();
    }

Output:

Readonly JSON: {"MyInt":[1]}
Readonly List Count: 0
Private JSON: {"MyInt":[1]}
Private List Count: 1

[layomia]

Updating the title here to generalize this to a problem where we need to modify an already initialized property or field. An equivalent to Newtonsoft.Json's ObjectCreationHandling should help solve this issue.

[layomia]

From @mtaylorfsmb in #39630:

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.

[eiriktsarpalis]

This should likely be tackled in conjunction with #63686 cc @krwq

[eiriktsarpalis]

Related to grpc/grpc-dotnet#167

[JamesNK]

Something to consider with this feature is adding Populate methods on JsonSerializer. e.g.

var user = new User();
JsonSerializer.Populate(json, user.Roles);

https://www.newtonsoft.com/json/help/html/PopulateObject.htm

Usage in the real world: https://cs.github.com/?q=jsonconvert.populateobject+language%3AC%23&p=1&scopeName=All+repos

[perason]

Something to consider with this feature is adding Populate methods on JsonSerializer. e.g.

var user = new User();
JsonSerializer.Populate(json, user.Roles);

https://www.newtonsoft.com/json/help/html/PopulateObject.htm

Usage in the real world: https://cs.github.com/?q=jsonconvert.populateobject+language%3AC%23&p=1&scopeName=All+repos

@JamesNK

There's a thread for that already, created just a couple of months before a post I added at StackOverflow

[krwq]

FWIW it will be possible to partially address this with #63686 - you will be able to use Set on the JsonPropertyInfo but it will create an extra allocation (first it will create a collection using default constructor and you will be able to move values into the existing collection) but you will be able to achieve most of the scenarios.

My initial prototype had callback to address this specific scenario exactly but it needs more design as it wouldn't work with async + some other tiny issues so for now at least this will make life slightly easier

[eiriktsarpalis]

but it will create an extra allocation (first it will create a collection using default constructor and you will be able to move values into the existing collection) but you will be able to achieve most of the scenarios.

How so? Wouldn't it be possible to (roughly) do something like the following in the object converter

// inside the property deserialization loop of ObjectConverter
if (jsonPropertyInfo.ReuseInitializedValue && jsonPropertyInfo.Get(parentObject) is object initializedValue)
{
    state.Current.ReturnValue = initializedValue; // signal to the converter of the property type that the type has been initialized.
}

[krwq]

We're moving this to 8.0. We should prioritize this in 8.0 period though. Per my offline conversation with @JamesNK lack of this will make deserializing collections in gRPC inefficient but we're ok with moving this work to next release while we focus on polishing up current contract customization design.

cc: @JamesNK @jeffhandley @eiriktsarpalis

[krwq]

I'm unassigning myself since I don't work on this now and we don't have specific plans for 8.0 yet

[krwq]

I'm closing this issue in favor of #78556 - anyone interested please share your thoughts below so that we can adjust proposal if needed.