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

Add APIs to JSON Contract customization #71123

Closed
6 of 16 tasks
krwq opened this issue Jun 22, 2022 · 6 comments · Fixed by #71797
Closed
6 of 16 tasks

Add APIs to JSON Contract customization #71123

krwq opened this issue Jun 22, 2022 · 6 comments · Fixed by #71797
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@krwq
Copy link
Member

krwq commented Jun 22, 2022

Continuation of #63686 - some API additions we wanted to make in this release.

API changes

namespace System.Text.Json.Serialization.Metadata;

public class JsonTypeInfo
{
    // Maps to IJsonOnSerializing
    public Action<object>? OnSerializing { get; set; } = null;
    // Maps to IJsonOnSerialized
    public Action<object>? OnSerialized { get; set; } = null;
    // Maps to IJsonOnDeserializing
    public Action<object>? OnDeserializing { get; set; } = null;
    // Maps to IJsonOnDeserialized
    public Action<object>? OnDeserialized { get; set; } = null;
}

public abstract partial class JsonPropertyInfo
{
    // Abstracts backing member, DefaultJsonTypeInfoResolver will return instance of PropertyInfo/FieldInfo
    public System.Reflection.ICustomAttributeProvider? AttributeProvider { get; set; } = null;

    // Maps to JsonPropertyOrderAttribute
    public int Order { get; set; } = 0;

    // Maps to JsonExtensionDataAttribute
    // Alternatively this could be a nullable JsonPropertyInfo on JsonTypeInfo.
    public bool IsExtensionData { get; set; } = false;
}

[EditorBrowsable(EditorBrowsableState.Never)]
public static partial class JsonMetadataServices
{
    public static JsonConverter<T?> GetNullableConverter<T>(JsonSerializerOptions options) where T : struct { throw null; }
}

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
     // Existing GetConverter API
     public JsonConverter GetConverter(Type type);

     // Gets metadata that has been cached by this options instance
     // Can be used to plug metadata directly into JsonSerializer overloads that accept JsonTypeInfo<T>
     public JsonTypeInfo GetTypeInfo(Type type);
}

Sample

public static void DataContractAttributeModifier(JsonTypeInfo jsonTypeInfo)
{
    if (jsonTypeInfo.Kind == JsonTypeInfoKind.Object &&
        type.GetCustomAttribute<DataContractAttribute>() is not null)
    {
        foreach (JsonPropertyInfo propertyInfo in jsonTypeInfo.Properties)
        {
            DataMemberAttribute? dataMemberAttribute = (DataMemberAttribute?)propertyInfo.AttributeProvider.GetCustomAttributes(typeof(DataMemberAttribute), inherit: false).FirstOrDefault()
            if (dataMemberAttribute is not null)
            {
                propertyInfo.Name = dataMemberAttribute.Name;
                propertyInfo.Order = dataMemberAttribute.Order;
            }
        }
    }
}

Remaining work

  • Remaining comments in JSON contract customization #70435
  • Adjust polymorphism APIs to match approved API shape
  • check trimmed app size before and after contract customization changes
  • JsonTypeInfo instances should never encapsulate mutable JsonSerializerOptions instances.
  • Investigate all logic consuming the JsonSerializerOptions.SerializerContext property as combined resolvers might invalidate their intentions.
  • More JsonSerializeContext + JsonTypeInfoResolver.Combine tests
  • More functional tests, review issues and implement scenarios
  • Re-iterate on the behavior in StaticInitialization_DeserializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows and StaticInitialization_SerializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows
  • once everything above is done ensure no regressions are made in performance suite (we've done checkpoint run already for JSON contract customization #70435 and no regressions)
  • XML docs in public APIs need improvement, e.g. <summary> sections should be one-liners, should use <see /> elements where applicable, should use <remarks> sections where applicable, etc. Wording seems to deviate with how we document elsewhere.
  • Regressions in System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties> #71392
  • Unify options/sourcegen root-level serialization helpers to use JsonTypeInfo<T>.
  • TODOs in the code/tests
  • Tests with RemoteExecute, make sure static initialization works correctly
  • Examine nullability/linkability of JsonSerializerOptions.TypeInfoResolver
  • Walk through all existing APIs and double check they will cooperate well with the design
@krwq krwq added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json labels Jun 22, 2022
@krwq krwq self-assigned this Jun 22, 2022
@krwq krwq added this to the 7.0.0 milestone Jun 22, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 22, 2022
@krwq krwq removed the untriaged New issue has not been triaged by the area owner label Jun 22, 2022
@ghost
Copy link

ghost commented Jun 22, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Main issue #63686 will be fixed soon with #70435
There is some remaining work we plan to finish for this release.

API changes

namespace System.Text.Json.Serialization.Metadata {
public static partial class JsonMetadataServices
{
    public static System.Text.Json.Serialization.JsonConverter<T?> GetNullableConverter<T>(System.Text.Json.JsonSerializerOptions options) where T : struct { throw null; }
}
public abstract partial class JsonTypeInfo<T> : System.Text.Json.Serialization.Metadata.JsonTypeInfo
{
    [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
    public System.Action<System.Text.Json.Utf8JsonWriter, T>? SerializeHandler { get { throw null; } } // existing API, previously entire class was EditorBrowsable.Never
}
// TODO: add JsonPropertyInfo.UnderlyingMemberInfo
}

Remaining work

  • Remaining comments in JSON contract customization #70435
  • More functional tests, review issues and implement scenarios
  • More JsonSerializeContext + JsonTypeInfoResolver.Combine tests
  • Walk through all existing APIs and double check they will cooperate well with the design
  • Tests with RemoteExecute
  • Adjust polymorphism APIs to match approved API shape
  • Examine nullability/linkability of JsonSerializerOptions.TypeInfoResolver
  • TODOs in the code/tests
  • once everything above is done ensure no regressions are made in performance suite (we've done checkpoint run already for JSON contract customization #70435 and no regressions)
Author: krwq
Assignees: krwq
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation blocked Issue/PR is blocked on something - see comments and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 28, 2022
@krwq krwq changed the title JSON Contract customization remaining work JSON Contract customization added APIs Jun 28, 2022
@krwq krwq added blocking Marks issues that we want to fast track in order to unblock other important work and removed blocked Issue/PR is blocked on something - see comments labels Jun 28, 2022
@krwq krwq changed the title JSON Contract customization added APIs Add APIs to JSON Contract customization Jul 5, 2022
@JamesNK
Copy link
Member

JamesNK commented Jul 6, 2022

    // Maps to JsonExtensionDataAttribute
    // Alternatively this could be a nullable property on JsonTypeInfo property.
    public bool IsExtensionData { get; set; } = false;

IsExtensionData on a type doesn't make sense to me. It's metadata for a property.

How does IsExtensionData work? I'm guessing that the property getter is called to fetch the dictionary, and if one doesn't exist then a new instance is set. And then S.T.J sets items into the dictionary when deserializing extension data and uses the dictionary's enumerator to get extension data to the serializer.

@krwq
Copy link
Member Author

krwq commented Jul 6, 2022

@JamesNK the alternative proposal is:

public class JsonTypeInfo
{
    public JsonPropertyInfo? ExtensionDataProperty { get; set; }
}

basically that would mean you cannot accidentally set two properties with IsExtensionData. It would be treated as a special property and not show up on the list of Properties. If there are two attributes on two properties that would cause the exception for DefaultJsonTypeInfoResolver but you will still be allowed to construct the JsonTypeInfo by hand (but without chance to fix user error).

One interesting case is the JsonExtensionDataAttribute on single property on base and derived type which we have discussed - possible solution could be ignoring the extension property on the base when it's on the derived

@eiriktsarpalis
Copy link
Member

How does IsExtensionData work?

There can be at most one properties marked ExtensionData per type. On serialization it will serialize key/value pairs as inlined properties of the underlying JSON object and conversely, it will be populated with deserialized properties not matching with any of the other properties. See also https://docs.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonextensiondataattribute?view=net-6.0#remarks

@bartonjs
Copy link
Member

bartonjs commented Jul 7, 2022

Video

Looks good as proposed.

  • We discussed JsonPropertyInfo.IsExtensionData vs JsonTypeInfo.ExtensionDataProperty and felt that the JsonPropertyInfo version better reflects any existing attribute state during generation.
  • We discussed the necessity of the Order property, and felt it made sense for the target scenarios.
namespace System.Text.Json.Serialization.Metadata
{
    public class JsonTypeInfo
    {
        // Maps to IJsonOnSerializing
        public Action<object>? OnSerializing { get; set; } = null;
        // Maps to IJsonOnSerialized
        public Action<object>? OnSerialized { get; set; } = null;
        // Maps to IJsonOnDeserializing
        public Action<object>? OnDeserializing { get; set; } = null;
        // Maps to IJsonOnDeserialized
        public Action<object>? OnDeserialized { get; set; } = null;
    }

    public abstract partial class JsonPropertyInfo
    {
        // Abstracts backing member, DefaultJsonTypeInfoResolver will return instance of PropertyInfo/FieldInfo
        public System.Reflection.ICustomAttributeProvider? AttributeProvider { get; set; } = null;

        // Maps to JsonPropertyOrderAttribute
        public int Order { get; set; } = 0;

        // Maps to JsonExtensionDataAttribute
        // Alternatively this could be a nullable JsonPropertyInfo on JsonTypeInfo.
        public bool IsExtensionData { get; set; } = false;
    }

    [EditorBrowsable(EditorBrowsableState.Never)]
    public static partial class JsonMetadataServices
    {
        public static JsonConverter<T?> GetNullableConverter<T>(JsonSerializerOptions options) where T : struct { throw null; }
    }
}

namespace System.Text.Json
{
    public partial class JsonSerializerOptions
    {
        // Existing GetConverter API
        public JsonConverter GetConverter(Type type);

        // Gets metadata that has been cached by this options instance
        // Can be used to plug metadata directly into JsonSerializer overloads that accept JsonTypeInfo<T>
        public JsonTypeInfo GetTypeInfo(Type type);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 7, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@eiriktsarpalis eiriktsarpalis reopened this Jul 8, 2022
@eiriktsarpalis
Copy link
Member

Closing in favor of #63686, I moved part of the checklist to the "Progress" section there for more visibility.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants