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

Fallback to configurable type for JsonPolymorphic deserialization #111755

Open
rynowak opened this issue Jan 23, 2025 · 3 comments
Open

Fallback to configurable type for JsonPolymorphic deserialization #111755

rynowak opened this issue Jan 23, 2025 · 3 comments
Labels
area-System.Text.Json untriaged New issue has not been triaged by the area owner

Comments

@rynowak
Copy link
Member

rynowak commented Jan 23, 2025

Scenario

My use case is deserializing polymorphic JSON data from an external REST API that's following the well-known discriminator pattern using $type. The challenge is that don't control this API and they continue to add new discriminator values.

This is further complicated because we need to do custom processing on some of the types of data returned by the API. So we want to define C# types for some of the API data but not all of it.

This isn't quite an API proposal per-se because I'm looking for feedback on the approach and overall idea. If this seems like a good idea, I could turn it into that.

The code that I want to write looks something like this:

record struct Response(IReadOnlyList<IWidget>? widgets);

[JsonPolymorphic(TypeDiscriminatorPropertyName = "$type")]
[JsonDerivedType(typeof(A), "A")]
[JsonDerivedType(typeof(B), "B")]
[JsonDerivedType(typeof(Unknown), Fallback = true)] // This feature doesn't exist
interface IWidget;

record struct A([property: JsonRequired] IReadOnlyList<DetailsA> details) : IWidget;
record struct B([property: JsonRequired] IReadOnlyList<DetailsB> details) : IWidget;

record struct DetailsA(/* omitted */);
record struct DetailsB(/* omitted */);

record struct Unknown(
  [property: JsonRequired, JsonPropertyName("$type")] string widgetType,
  [property: JsonRequired] IReadOnlyList<Dictionary<string, JsonElement> details) : IWidget;


// Then later
var widget = JsonSerializer.Deserialize<IWidget>(...);
return widget switch =>
{
    A a => ProcessA(a),
    B b => ProcessB(b),
    Unknown u => ProcessUnknown(u),
    _ => throw new NotSupportedException(),
};

This is good because we can agnostically "pass through" any new widget types returned by the service we're calling. We only need to define new C# types whenever a specific widget type requires some business logic.

Note: This example uses an interface + record structs. I'm not sure if that's optimal from a performance POV, and it could be somewhat flexible. Unless I'm missing something, whether it's classes or structs doesn't really affect my scenario.

What I tried

I can't use IgnoreUnrecognizedTypeDiscriminators because I need to read all the data.

Traditional inheritance isn't a good solution for this because both the child and base class need to define details with different types. Therefore I can't use UnknownDerivedTypeHandling. I'm not sure it applies to deserialization anyway.

I also need to round-trip the data without information loss, so I'd need something like #108885 to be addressed.

I'd really like to avoid duplicating the functionality of [JsonPolymorphic] by writing or generating our own serialization logic.

It's also not possible to use both of JsonConverter and JsonPolymorphic for the same type.

Why this is valuable

As C# has added more language features for immutability and sum types (eg: records, pattern matching) we're using these patterns to great success.

STJ has really excellent support for working with a closed set of polymorphic strong types. This is about a the ability to work with an open set, or partial-typing.

I'm wondering if a lot of the other issues that have been opened on [JsonPolymorphic] are asking about workarounds for this scenario.

An approach that works with drawbacks

I found an approach that works and I'm currently trying to shake out all of the problems with. I'd appreciate any feedback that the experts can share in case this can be improved.

This works by defining two polymorphic types:

  • One with a JsonConverter
  • One with JsonPolymorphic

The job of the converter is to detect whether or not the discriminator value is part of the 'known' set. I can do this by using JsonTypeInfo<> so I have a single source of truth. Then it calls into the serialize for either IStronglyTypedWidget or Unknown.

I think the drawbacks of this are:

  • It's complicated: we need artificial things like a marker interface.
  • It affects performance: we to run a converter and do things inside like copy the reader. This is a performance sensitive code path that runs at scale. The converter code needs to run for every widget, not just the unknown path.

I'm currently working on a benchmark for the approach below in comparison with static approaches like hand-writing the serializer. Happy to share the results if it's interesting. I remember from the design discussions that converters introduce buffering, and that's the part that most concerns me.

record struct Response(IReadOnlyList<IWidget>? widgets);

[JsonConverter(...)]
interface IWidget;

[JsonPolymorphic(TypeDiscriminatorPropertyName = "$type")]
[JsonDerivedType(typeof(A), "A")]
[JsonDerivedType(typeof(B), "B")]
[JsonDerivedType(typeof(Unknown), Fallback = true)] // This feature doesn't exist
interface IStronglyTypedWidget;

record struct A([property: JsonRequired] IReadOnlyList<DetailsA> details) : IStronglyTypedWidget;
record struct B([property: JsonRequired] IReadOnlyList<DetailsB> details) : IStronglyTypedWidget;

record struct DetailsA(/* omitted */);
record struct DetailsB(/* omitted */);

record struct Unknown(
  [property: JsonRequired, JsonPropertyName("$type")] string widgetType,
  [property: JsonRequired] IReadOnlyList<Dictionary<string, JsonElement> details) : IWidget;

// Many details omitted or simplified
class WidgetConverter : JsonConverter<IWidget>
{
    public override bool CanConvert(Type typeToConvert) => typeToConvert.IsAssignableTo(typeof(IWidget));

    public override IWidget Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        // Copy reader so we can advance the token stream separately.
        using var copy = reader;
        copy.Read();

        var discriminatorProperty = copy.GetString();
        copy.Read();
 
        var discriminatorValue = copy.GetString();
        if (IsKnownDiscriminatorValue(discriminatorValue))
        {
          return JsonSerializer.Deserialize<IStronglyTypedWidget>(reader)
        }

        return JsonSerializer.Deserialize<Unknown>(reader)
    }
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 23, 2025
Copy link
Contributor

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

@eiriktsarpalis
Copy link
Member

Hi @rynowak, thanks for the detailed write-up -- this is good feedback. I think it should be possible in principle to implement something that supports your use case (with a few minor tweaks to keep it compatible with the current contract model):

[JsonPolymorphic(TypeDiscriminatorPropertyName = "$type", Fallback = typeof(Unknown))]
[JsonDerivedType(typeof(A), "A")]
[JsonDerivedType(typeof(B), "B")]
interface IWidget;

Judging by your second example, I would infer that you need a way to bind the unrecognized discriminator id to a property of the Unknown type? This is not something that STJ polymorphism supports today, discriminators are specified using type-level mappings to ensure opted in derived types are roundtripped correctly. The same concern shouldn't apply to a general-purpose fallback type though and we might be able to make an exception there. It should be possible to either explicitly bind to the discriminator for a given property like in your example, or perhaps collect all unbound properties via the existing extension data mechanism like so:

public record Unknown : IWidget
{
     [JsonExtensionData]
     public IDictionary<string, object?>? AdditionalProperties { get; set; }
}

which should accumulate all unbound properties (including the type discriminator). This should ensure that values can be safely roundtripped as well. Any such work should be done in conjunction with #72170.

@rynowak
Copy link
Member Author

rynowak commented Jan 24, 2025

Thanks @eiriktsarpalis for taking a look at this.

Judging by your second example, I would infer that you need a way to bind the unrecognized discriminator id to a property of the Unknown type?

Yes that's right. I linked #108885 because it seems relevant.

It should be possible to either explicitly bind to the discriminator for a given property

This is what I ended up doing.

Do you have any feedback on the approach of writing a JsonConverter for this? The one I wrote works, but impacts performance. I'm seeing a 1.6x-2.0x increase in execution time compared to "just" the source-generated serializer (without support for fallback). The profiles also show very little exclusive time spent in my converter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

2 participants