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 polymorphic serialization to System.Text.Json #54328

Closed

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jun 17, 2021

Fixes #29937. Based on the prototype in #53882 which incorporates #30083. API design pending approval.

Infrastructural Changes

Here is a high-level overview of the infrastructural changes being made:

  • The CanBePolymorphic and CanUseDirectReadOrWrite properties from JsonConverter have been moved to JsonTypeInfo, since the value of these flags can now be influenced by user configuration or attributes.
  • Rewrites the WriteStack struct to support polymorphism:
  • Reworks the ReferenceResolver.IgnoreCycles implementation: the current implementation contains bespoke code to avoid polymorphic converters triggering false negatives. I have refactored the code slightly so that the cycle detection logic is combined with the polymorphic serialization infrastructure.

Potential breaking change

The changes isolated in b14bcdf introduce a deliberate breaking change when it comes to the handling of new object() values. The existing implementation will hardcode the serialization to {} even if the user has subscribed a custom JsonConverter<object> that does something different. I would like to change this behavior for a few reasons:

  • It can be surprising to users: custom object converters cannot be polymorphic with the sole exception of new object() values. Users cannot specify a custom serialization format for new object() values.
  • Keeping it requires maintaining more complex logic in the serialization hot path. The change makes the code simpler by delegating object instance serialization to the relevant converter.
  • It is unlikely that new object() instances are being serialized in production applications.

The breaking change concerns the following scenario:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };
string json = JsonSerializer.Serialize(new object(), options);
Console.WriteLine(json);

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) => writer.WriteNumberValue(42);
    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
}

In .NET 5 the above would print {}, however under the new changes it would print 42. This could break users with custom object converters who rely on the existing hardcoding behavior.

Performance

When benchmarked against main using statistical testing with 5% threshold, performance of the feature branch is largely equivalent. It should be noted however that certain polymorphism-heavy scenaria (e.g. WriteJson<ArrayList>) do record a slight regression (which can be observed when applying thresholds < 3%). After profiling the code I can confirm that this regression is in part attributable to the additional work required by computing the WriteStack.NextValueCanBePolymorphic property.

@eiriktsarpalis eiriktsarpalis added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Text.Json labels Jun 17, 2021
@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Jun 17, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Jun 17, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 17, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #29937. Based on the prototype in #53882 which incorporates #30083. API design pending API review.

Infrastructural Changes

Here is a high-level overview of the infrastructural changes being made:

  • The CanBePolymorphic and CanUseDirectReadOrWrite properties from JsonConverter have been moved to JsonTypeInfo, since the value of these flags can now be influenced by user configuration or attributes.
  • Rewrites the WriteStack struct to support polymorphism:
  • Reworks the ReferenceResolver.IgnoreCycles implementation: the current implementation contains bespoke code to avoid polymorphic converters triggering false negatives. I have refactored the code slightly so that the cycle detection logic is combined with the polymorphic serialization infrastructure.

Potential breaking change

The changes isolated in b14bcdf introduce a deliberate breaking change when it comes to the handling of new object() values. The existing implementation will hardcode the serialization to {} even if the user has subscribed a custom JsonConverter<object> that does something different. I would like to change this behavior for a few reasons:

  • It can be surprising to users: custom object converters cannot be polymorphic with the sole exception of new object() values. Users cannot specify a custom serialization format for new object() values.
  • Keeping it requires maintaining more complex logic in the serialization hot path. The change makes the code simpler by delegating object instance serialization to the relevant converter.
  • It is unlikely that new object() instances are being serialized in production applications.

The breaking change concerns the following scenario:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };
string json = JsonSerializer.Serialize(new object(), options);
Console.WriteLine(json);

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) => writer.WriteNumberValue(42);
    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
}

In .NET 5 the above would print {}, however under the new changes it would print 42. This could break users with custom object converters who rely on the existing hardcoding behavior.

Performance

When benchmarked against main using statistical testing with 5% threshold, performance of the feature branch is largely equivalent. It should be noted however that certain polymorphism-heavy scenaria (e.g. WriteJson<ArrayList>) do record a slight regression (which can be observed when applying thresholds < 3%). After profiling the code I can confirm that this regression is in part attributable to the additional work required by computing the WriteStack.NextValueCanBePolymorphic property.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

* NO MERGE *, area-System.Text.Json

Milestone: 6.0.0

@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support polymorphic serialization through new option
1 participant