Runtime: 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 https://github.com/dotnet/corefx/pull/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: https://github.com/dotnet/corefx/pull/39442#issuecomment-558809698

[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 https://github.com/dotnet/runtime/issues/32197#issue-564328723:

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 https://github.com/dotnet/runtime/issues/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 https://github.com/dotnet/runtime/issues/35388:

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

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

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

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 https://github.com/dotnet/runtime/issues/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 https://github.com/dotnet/runtime/issues/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.

[layomia]

From @CodeBlanch in https://github.com/dotnet/runtime/issues/30688:

Adds a new attribute JsonDeserializeAttribute which turns on a new feature to deserialize into read-only properties.

Proposed API

    /// <summary>
    /// Flags a private property for inclusion during deserialization.
    /// </summary>
    /// <remarks>
    ///   <para>
    ///     By default only properties with public get &amp; set operations defined will be deserialized.
    ///     Use <see cref="JsonDeserializeAttribute"/> to indicate the instance returned by the get method should be used
    ///     to deserialize a property.
    ///   </para>
    ///   <para>
    ///     For collections the instance returned should implement <see cref="System.Collections.IList"/>. For dictionaries
    ///     the instance returned should implement <see cref="System.Collections.IDictionary"/>. In both cases the instance
    ///     should not be read-only or fixed-size (arrays).
    ///   </para>
    ///   <para>
    ///     <see cref="JsonDeserializeAttribute"/> is ignored when a public set operation is defined or property is a value type.
    ///   </para>
    /// </remarks>
    [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
    public sealed class JsonDeserializeAttribute : JsonAttribute
    {
        /// <summary>
        /// Initializes a new instance of <see cref="JsonDeserializeAttribute"/>.
        /// </summary>
        public JsonDeserializeAttribute() { }
    }

Rationale & Usage

Consider this class...

        public class SimpleTestClassUsingJsonDeserialize
        {
            [JsonDeserialize]
            public SimpleTestClass MyClass { get; } = new SimpleTestClass();
        }

...and this json…

{"MyClass":{"MyInt32":18}}

Previously the MyClass property would be ignored during deserialization because it does not have a public setter. What [JsonDeserialize] does is direct the (de)serializer to use the getter to access the instance and then deserialize on to that.

Why do this? Primarily to allow people to use patterns that satisfy CA2227. It also helps people using C#8 nullable reference.

Example of a class expressed precisely:

public class Document
{
        public ChildDoc? Parent { get; set; }

        [JsonDeserialize]
        public IEnumerable<ChildDoc> Children { get; } = new List<ChildDoc>();
}

We can access Children without fear of null.

Same class with trade-offs to make existing API happy:

  public class Document
  {
      public ChildDoc? Parent { get; set; }

#pragma warning disable CA2227 // Collection properties should be read only
      public List<ChildDoc>? Children { get; set; }
#pragma warning restore CA2227 // Collection properties should be read only
  }

Access to Children now needs to have null checking. If we define like this...

public List<ChildDoc> Children { get; set; } = new List<ChildDoc>();

We're newing up an instance that will be thrown away during deserialization, and someone could come along and replace the value leading to inconsistent state. A little less performant and a little more error-prone.

Details

In order to safely deserialize into these properties some strict rules are in place. Collections must be of type IList, not fixed-size, and not read-only. Dictionaries must by of type IDictionary, not fixed-size, and not read-only. Whatever exists before serialization will be replaced by deserialization.

Considerations

Obviously this is meant for reference types. If you use this attribute on a value field it can't really do anything with that. Should it throw an exception when it is building the property info? Current implementation ignores value types.

I went with the attribute primarily for backwards compatibility. People can opt-in to this if they want it.

Implementation

See dotnet/corefx#40517

[layomia]

From offline discussion with @GrabYourPitchforks about JsonSerializer support for init-only properties:

Something you might want to consider: skipping init-only properties when you're patching an existing object. Only allow setting them when you've constructed a fresh instance of the object.

[JulienM28]

Is there any chance to have ths fixed for .Net 5.0 ?

[layomia]

@JulienM28 due to time constraints, this won't come in .NET 5. It will be considered for .NET 6.

[paulodero]

@layomia Do you know when this feature support would be made available?