API review for JsonExtensionDataAttribute

[steveharter]

Identifies overflow \ extension data attribute to hold JSON values when there is no matching properties, and be able to serialize them to enable round-tripping.

Note the name matches Json.Net's equivalent attribute.

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// When placed on a property of type <see cref="System.Collections.Generic.Dictionary{TKey, TValue}"/>, any
    /// properties that do not have a matching member are added during deserialization and written during serialization.
    /// The TKey value must be <see cref="string"/> and TValue must be <see cref="JsonElement"/> or <see cref="object"/>.
    /// </summary>
    [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
    public sealed partial class JsonExtensionDataAttribute : JsonAttribute 
    { 
        public JsonExtensionDataAttribute() { } 
    } 
}

[steveharter]

cc @rynowak @JamesNK

[rynowak]

Looks right to me 👍

[JamesNK]

Extension data looks like a simple feature on the surface, but there are a lot of interactions with other features you need to think about.

For example, if you configure a serializer to throw an error when there is a JSON property that doesn't match a property on the .NET class, but the .NET class has an extension data property, what should happen?

Or during serialization, what should happen to property keys when a camel case name policy is set? Are keys processed or not.

I will come up with a list of questions to resolve

[JamesNK]

• Can extension data be on a private/internal property? You can do this in Json.NET
• Can extension data be a readonly property? You can do this in Json.NET
• What types can extension data be placed on? Dictionary<TKey, TValue>? IDictionary<TKey, TValue>?
• What does TValue do to incoming data? Are you only going to allow Dictionary<string, JsonDocument/JsonElement>, or can TValue be string/int/object. What would those types do? What happens if you have a TValue of string and there is a JSON array?
• Do ignored null or readonly values end up here? https://github.com/dotnet/corefx/blob/f5bce3ac4ff76b0144571696cd3845d13adfce74/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs#L112-L144
• Is there a name policy that would effect extension data keys during serialization?
• Multiple extension data attributes shouldn't be on one class
• I don't recall whether Json.NET allows extension data metadata to be inherited. Something to think about

[JamesNK]

Should there be flags on extension data attribute to opt-out of one side of round-tripping data, e.g. https://github.com/JamesNK/Newtonsoft.Json/blob/4ab34b0461fb595805d092a46a58f35f66c84d6a/Src/Newtonsoft.Json/JsonExtensionDataAttribute.cs

[JamesNK]

Should note in documentation that extension data will be serialized, even if a property of the same name exists - JamesNK/Newtonsoft.Json#1017

[steveharter]

Thanks @JamesNK.

Open issues to discuss:

• We should probably add a ExtensionPropertyNamingPolicy to the options class. To support this we'd need to pass the option to the JsonDocument and have it use it.
• Do we need CanRead\CanWrite functionality? If so can we extend the existing JsonIgnoreAttribute to allow read- vs write on any type of property (instead of adding to JsonExtensionDataAttribute).
• Should IgnoreNulls work with extension data? Currently it does not (nulls appear are in the extension data). It appears Json.NET doesn't use the setting either. To support this would means we'd need to pass the null option to the JsonDocument.
• Support private\internal property?

if you configure a serializer to throw an error when there is a JSON property that doesn't match a property

Currently throwing is not supported, so we will need to track this until we add that feature.

property keys when a camel case name policy is set

Current they are left as-is. I don't think we want to always use the PropertyNamingPolicy (or DictionaryKeyNamingPolicy) for extension data so I think a new property would be needed: ExtensionPropertyNamingPolicy)

Can extension data be on a private/internal property

Currently no. Just public properties like existing data.

What types can extension data be placed on...

Currently Dictionary<string, object> and Dictionary<string, JsonElement>. This is currently doc'd.

What does TValue do to incoming data...

If not the two supported dictionaries, we throw an InvalidOperationException.

Do ignored null or readonly values end up here?

Currently "yes" for both which is consistent with Json.NET.

Multiple extension data attributes shouldn't be on one class

Currently we throw InvalidOperationException.

[JamesNK]

We should probably add a ExtensionPropertyNamingPolicy to the options class.

This is what I was thinking 👍

Do we need CanRead\CanWrite functionality? If so can we extend the existing JsonIgnoreAttribute to allow read- vs write on any type of property (instead of adding to JsonExtensionDataAttribute).

Hmm, yeah I guess you could do that. I have never had someone ask for Read/Write on JsonIgnore. No matter how it is done, I don't think this is an urgent feature.

Can extension data be on a private/internal property

Requiring extension data to be on a public is fine for now. Support on private could always be added in the future.

Currently Dictionary<string, object> and Dictionary<string, JsonElement>. This is currently doc'd.

👍

Currently "yes" for both which is consistent with Json.NET.

👍

Currently we throw InvalidOperationException. (re: multiple extension data attributes)

👍

[steveharter]

Leaving this issue open while we discuss ExtensionPropertyNamingPolicy and other misc features.

[steveharter]

Closing. No planned support in this release for:

• Naming policy (e.g. ExtensionPropertyNamingPolicy). This would require support from JsonDocument.
• IgnoreNullValues support. This would require support from JsonDocument.
• CanRead\CanWrite (assume true for both)
• Private\internal extension property (must be public)

[steveharter]

Re-open for API review.

[steveharter]

Closing as API approved.