Restore polymorphism for JSON serialization

[martincostello]

JSON serialization polymorphism was removed in 83eb76f because it isn't supported by System.Text.Json.

It should be supported in .NET 9 preview 2, so once that's available in March(?) can try it out and see if it does the job.

See dotnet/runtime#72604 (comment).

[martincostello]

@eiriktsarpalis I just tried this out 1152660 but it didn't work.

Should this work, or is it just that the name of the property doesn't describe that it only works in the root of the JSON document?

In essence I'm trying to do polymorphism of a property that is a child of the root:

{
  "foo": "bar",
  "payload": {
    "id": "whatever",
    "type": "a type",
    "...": "..."
  }
}

If this scenario isn't supported I'll just revert the changes, but if it should let me know and I'll open an issue in dotnet/runtime.

[martincostello]

Forgot the exception message from the logs:

Amazon.Lambda.Serialization.SystemTextJson.JsonSerializerException: Error converting the Lambda event JSON payload to type MartinCostello.LondonTravel.Skill.Models.SkillRequest: The metadata property is either not supported by the type or is not the first property in the deserialized JSON object. Path: $.request.type | LineNumber: 16 | BytePositionInLine: 11.
---> System.Text.Json.JsonException: The metadata property is either not supported by the type or is not the first property in the deserialized JSON object. Path: $.request.type | LineNumber: 16 | BytePositionInLine: 11.

[eiriktsarpalis]

You're right that the discriminator should be present at the same level as the type being serialized. Nested properties are not supported currently, sorry!

[eiriktsarpalis]

or is not the first property in the deserialized JSON object

I'm surprised that this message appears when out-of-order reads are enabled. Can you share a repro?

[martincostello]

The code in commit 1152660 should do it, but I could only find the issue by deploying it to AWS Lambda and then running my end-to-end test suite. The application is compiled with AoT, so that might be making the difference.

I do have some tests here that should have found the issue before I even pushed the commit, but clearly I'm missing something here as they pass locally and in CI (where they aren't AOT'd).

[eiriktsarpalis]

The application is compiled with AoT, so that might be making the difference.

That is surprising. Would it be possible to file a minimal repro in the runtime repo? Thanks!

[martincostello]

Sure - I'll try and come up with something.

[martincostello]

Haven't started on the repro yet, but I did have a thought - if this isn't supported, why does my unit test work... 🤔

[martincostello]

Right, I think I've worked out what's going on here:

1. The AWS Lambda-specific serializer uses reflection to pass its own JsonSerializerOptions to the constructor of my custom JsonSerializerContext that accepts one.
2. That then conflicts internally somehow with the generated options being AllowOutOfOrderMetadataProperties=true but theirs being AllowOutOfOrderMetadataProperties=false, causing the polymorphism to fail as the type discriminator isn't the first property and I guess it then uses their options rather than those from the source generator, so it throws.

If I change my code to provide a customization delegate for their options, then the polymorphism works as expected:

- var serializer = new SourceGeneratorLambdaJsonSerializer<AppJsonSerializerContext>();
+ var serializer = new SourceGeneratorLambdaJsonSerializer<AppJsonSerializerContext>((p) => p.AllowOutOfOrderMetadataProperties = true);

My end-to-end tests still fail, but that's because now I'm seeing a different AoT/trimmer related issue to do with Polly and/or HTTP resilience I need to investigate further, but it's resolved the JSON deserialization issue.

My hunch as to why this discrepancy isn't being found and surfaced in an intuitive way ("I've set the thing but it's saying I haven't set the thing") is because IBuiltInJsonTypeInfoResolver.IsCompatibleWithOptions() doesn't appear to be checking that options.AllowOutOfOrderMetadataProperties == generatedSerializerOptions.AllowOutOfOrderMetadataProperties.

This seems to raise the following questions:

1. Should the AllowOutOfOrderMetadataProperties comparison be added to IsCompatibleWithOptions()? I'm not familiar with how the result of being false influences later behaviour in the (de)serialization.
2. Is there a bug in the AWS Lambda serializer and/or S.T.J that's creating this incompatible behaviour if the root cause isn't the missing AllowOutOfOrderMetadataProperties comparison?
3. Why do I only find this at runtime in AWS Lambda with the native AoT'd application (runs on Amazon Linux 2023 with arm64)? I don't seem to be able to replicate this behaviour in my tests, nor if I try to force the payloads to be deserialized in a native AoT'd console app on my Windows laptop.
4. You asserted that polymorphism isn't supported below the JSON document root, yet it appears that it does - is this a mistake, or a happy accident that it actually does, or it happens to work but it isn't supported?

[eiriktsarpalis]

The AWS Lambda-specific serializer uses reflection to pass its own JsonSerializerOptions to the constructor of my custom JsonSerializerContext that accepts one.

That then conflicts internally somehow with the generated options being AllowOutOfOrderMetadataProperties=true but theirs being AllowOutOfOrderMetadataProperties=false,

Settings specified on the JsonSourceGenerationOptions attribute determine the configuration of the generated Default instance, but that will be overridden by whatever run time JsonSerializerOptions configuration is passed to the context constructor.

My hunch as to why this discrepancy isn't being found and surfaced in an intuitive way ("I've set the thing but it's saying I haven't set the thing") is because IBuiltInJsonTypeInfoResolver.IsCompatibleWithOptions() doesn't appear to be checking that options.AllowOutOfOrderMetadataProperties == generatedSerializerOptions.AllowOutOfOrderMetadataProperties.

I wouldn't say that's related. What this method does effectively is check if the particular run time configuration ends up invalidating the fast-path methods that were generated at compile time. Polymorphism isn't supported in fast path, so adding that check isn't necessary.

[martincostello]

• Why do I only find this at runtime in AWS Lambda with the native AoT'd application (runs on Amazon Linux 2023 with arm64)? I don't seem to be able to replicate this behaviour in my tests, nor if I try to force the payloads to be deserialized in a native AoT'd console app on my Windows laptop.
• You asserted that polymorphism isn't supported below the JSON document root, yet it appears that it does - is this a mistake, or a happy accident that it actually does, or it happens to work but it isn't supported?

Any suggestions on these two points?

Ideally I'd have found this issue locally in my tests, rather than needing to wait for deployment to prod with AoT.

Also if this is working "by accident", I'd likely revert the changes in case it suddenly breaks.

[eiriktsarpalis]

Why do I only find this at runtime in AWS Lambda with the native AoT'd application (runs on Amazon Linux 2023 with arm64)?

I don't know. I would suggest creating a minimal repro, submitting it to the AWS team and we can take it from there.

You asserted that polymorphism isn't supported below the JSON document root, yet it appears that it does

Polymorphism will work for any value declared as polymorphic within the object graph. What isn't supported is nesting the type discriminator beneath the depth of the JSON object currently being serialized.

[martincostello]

What isn't supported is nesting the type discriminator beneath the depth of the JSON object currently being serialized.

That's the bit I'm still not getting - the type discriminator is at $.request.type, so that's beneath the depth so shouldn't be working, right? Yet it is working.

alexa-london-travel/test/LondonTravel.Skill.Tests/Samples/IntentRequest.json

Line 24 in 7dc8243

"type": "IntentRequest",

[eiriktsarpalis]

Something else must be at play in that case. Here's how a vanilla example behaves:

Console.WriteLine(JsonSerializer.Deserialize<MyPoco>("""{"type":"derived"}""") is DerivedPoco); // True
Console.WriteLine(JsonSerializer.Deserialize<MyPoco>("""{"request":{"type":"derived"}}""") is DerivedPoco); // False

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(DerivedPoco), "derived")]
class MyPoco;
class DerivedPoco : MyPoco;

Unless of course by working in this case you mean that it's deserializing successfully even though it is an instance of the base type? That's by design, possible workarounds are to either specify a type discriminator for the base type itself or just mark the base type as abstract:

Console.WriteLine(JsonSerializer.Deserialize<MyPoco>("""{"type":"derived"}""") is DerivedPoco); // True
Console.WriteLine(JsonSerializer.Deserialize<MyPoco>("""{"request":{"type":"derived"}}""") is DerivedPoco); // NotSupportedException

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(DerivedPoco), "derived")]
abstract class MyPoco;
class DerivedPoco : MyPoco;

[martincostello]

Ah I understand now - I misunderstood that what you meant was that polymorphism only works at the root, but what you're actually saying is that you can't have an object be polymorphic based on a property in a child property of that element.

That's not what I'm doing in my case. I have something like this:

public class Root
{
    public Child Child { get; set; }
}

[JsonDerivedType(typeof(ChildA), "a")]
[JsonDerivedType(typeof(ChildB), "b")]
[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
public abstract class Child
{
    public string Name { get; set; }
}

public class ChildA : Child
{
    public string SomeProperty { get; set; }
}

public class ChildB : Child
{
    public string OtherProperty { get; set; }
}

{
  "child": {
    "name": "some-name",
    "type": "a",
    "someProperty": "foo"
  }
}

{
  "child": {
    "name": "other-name",
    "type": "b",
    "otherProperty": "bar"
  }
}

Essentially I'm being polymorphic below the root, but for the child properties that are polymorphic, the type discriminator is at the same level as that property's properties.

This is working as expected for me with the out-of-order support as I can deserialize the JSON document string to a Root with a Child property of the appropriate ChildA or ChildB type at runtime.

That means now that I just need to figure out why I can't replicate the issue I was having that I fixed in 3bd0d0e locally.

[martincostello]

That means now that I just need to figure out why I can't replicate the issue I was having that I fixed in 3bd0d0e locally.

Mystery solved - my test payloads have the type discriminator as the first property (I think I just copied them from another source originally and modified them to be project specific), hence they never needed the out-of-order support and worked as-is. The payloads at runtime in AWS Lambda don't have it as the first (in the one I looked at just now it was third), hence the original blocker. 🤦

Migration successful then!