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

System.Text.Json uses inconsistent polymorphism semantics with custom object converters #72681

Closed
eiriktsarpalis opened this issue Jul 22, 2022 · 3 comments
Assignees
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. enhancement Product code improvement that does NOT require public API changes/additions needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 22, 2022

System.Text.Json hardcodes polymorphic serialization when serializing root-level values of type object, regardless of whether the registered JsonConverter<object> supports polymorphism. While this is consistent with the semantics of the built-in converter for object, it can result in inconsistent serialization contracts when used in conjunction with custom converters, depending on whether the value is serialized at the root level or as a nested node:

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

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };

Console.WriteLine(JsonSerializer.Serialize<object>(0, options)); // Prints 0, custom converter not honored
Console.WriteLine(JsonSerializer.Serialize<object[]>(new object[] { 0 }, options)); // Prints [42], custom converter honored

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 NotImplementedException();
}

This issue proposes that we change the root-level behavior for custom object converters so that it no longer defaults to polymorphism (unless a polymorphic converter is being used). This will make serialization contracts consistent, regardless of whether the instance is serialized as a root-level value or as a nested value.

This change might be a breaking change for users that rely on the current behavior. As a workaround, users can get back the existing polymorphic behavior by calling into one of the untyped JsonSerializer APIs, explicitly passing the runtime type of the value as the inputType parameter:

-JsonSerializer.Serialize<object?>(value, options);
+Type runtimeType = value?.GetType() ?? typeof(object);
+JsonSerializer.Serialize(value, runtimeType, options);

Related to #54436 and #72187.

@eiriktsarpalis eiriktsarpalis self-assigned this Jul 22, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jul 22, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 22, 2022
@eiriktsarpalis eiriktsarpalis added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet and removed area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jul 22, 2022
@ghost
Copy link

ghost commented Jul 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

System.Text.Json hardcodes polymorphic serialization when serializing root-level values of type object, regardless of whether the registered JsonConverter<object> supports polymorphism. While this is consistent with the semantics of the built-in converter for object, it can result in inconsistent serialization contracts when used in conjunction with custom converters, depending on whether the value is serialized at the root level or as a nested node:

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

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };

Console.WriteLine(JsonSerializer.Serialize<object>(0, options)); // Prints 0, custom converter not honored
Console.WriteLine(JsonSerializer.Serialize<object[]>(new object[] { 0 }, options)); // Prints [42], custom converter honored

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options)
    {
        if (value is int)
        {
            writer.WriteNumberValue(42);
        }
        else
        {
            JsonSerializer.Serialize(writer, value, value.GetType(), options);
        }
    }

    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => throw new NotImplementedException();
}

This issue proposes that we change the root-level behavior for custom object converters so that it no longer defaults to polymorphism (unless a polymorphic converter is being used). This will make serialization contracts consistent, regardless of whether the instance is serialized as a root-level value or as a nested value.

This change might be a breaking change for users that rely on the current behavior. As a workaround, users can get back the existing polymorphic behavior by calling into one of the untyped JsonSerializer APIs, explicitly passing the runtime type of the value as the inputType parameter:

-JsonSerializer.Serialize<object?>(value, options);
+Type runtimeType = value?.GetType() ?? typeof(object);
+JsonSerializer.Serialize(value, runtimeType, options);

Related to #54436 and #72187.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@ghost
Copy link

ghost commented Jul 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

System.Text.Json hardcodes polymorphic serialization when serializing root-level values of type object, regardless of whether the registered JsonConverter<object> supports polymorphism. While this is consistent with the semantics of the built-in converter for object, it can result in inconsistent serialization contracts when used in conjunction with custom converters, depending on whether the value is serialized at the root level or as a nested node:

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

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };

Console.WriteLine(JsonSerializer.Serialize<object>(0, options)); // Prints 0, custom converter not honored
Console.WriteLine(JsonSerializer.Serialize<object[]>(new object[] { 0 }, options)); // Prints [42], custom converter honored

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options)
    {
        if (value is int)
        {
            writer.WriteNumberValue(42);
        }
        else
        {
            JsonSerializer.Serialize(writer, value, value.GetType(), options);
        }
    }

    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => throw new NotImplementedException();
}

This issue proposes that we change the root-level behavior for custom object converters so that it no longer defaults to polymorphism (unless a polymorphic converter is being used). This will make serialization contracts consistent, regardless of whether the instance is serialized as a root-level value or as a nested value.

This change might be a breaking change for users that rely on the current behavior. As a workaround, users can get back the existing polymorphic behavior by calling into one of the untyped JsonSerializer APIs, explicitly passing the runtime type of the value as the inputType parameter:

-JsonSerializer.Serialize<object?>(value, options);
+Type runtimeType = value?.GetType() ?? typeof(object);
+JsonSerializer.Serialize(value, runtimeType, options);

Related to #54436 and #72187.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, breaking-change, needs-breaking-change-doc-created

Milestone: 7.0.0

@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Jul 22, 2022
@eiriktsarpalis
Copy link
Member Author

Fixed in #72789.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. enhancement Product code improvement that does NOT require public API changes/additions needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

No branches or pull requests

1 participant