Runtime: System.Text.Json read-only property deserialization API enhancement

[CodeBlanch]

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 https://github.com/dotnet/corefx/pull/40517

[larthallor]

How feasible is it to relax the requirement to ICollection of T for generics? It has Clear and Add methods and is also implemented by List of T. Does deserialization require the extra functionality added by IList of T?

[ericstj]

@steveharter mentioned this is feasible for 5.0 in dotnet/corefx#40517. /cc @layomia @ahsonkhan

[markusschaber]

Requiring IList interface disallows Sets. We should just have ICollection here (and you may special case Array if filling fixed-size arrays is an use case.)

[techfan101]

My primary interest here is the ability to deserialize to properties where the type is derived from ICollection (most often IList) and which have only a getter (no setter). This is a common pattern within my pre-existing data models and almost a "must-have" capability for a deserializer.

That said, the ability to deserialize to other read-only types would sometimes be useful as well. However, for me, this is more of a "nice-to-have" capability.

[ericstj]

@layomia this seems out of scope for 5.0.0 at the moment, but I wasn't sure if this scenario might already be covered by other serializer functionality.

[layomia]

@ericstj yes, out of scope for 5.0. There's no existing serializer feature that covers this.

Closing as this is a duplicate of https://github.com/dotnet/runtime/issues/30258.