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

The JsonIgnore is being ignored causing property collisions with PropertyNameCaseInsensitive set to true #93903

Closed
normj opened this issue Oct 24, 2023 · 13 comments · Fixed by #93933
Assignees
Milestone

Comments

@normj
Copy link

normj commented Oct 24, 2023

Description

I'm updating our JSON serialization code for AWS Lambda to support .NET 8 and ran into a serialization bug that was worked correctly in previous versions. When doing JSON serialization with System.Text.Json and PropertyNameCaseInsensitive set to true properties with JsonIgnore are being included when detecting attribute collisions. For example looking at following type

public class MyPoco
{
    [JsonPropertyName("approximateArrivalTimestamp")]
    public long ApproximateArrivalEpoch { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(ApproximateArrivalEpoch);
        }
    }
}

There are technically 2 properties with the name approximateArrivalTimestamp but since the one that returns DateTime has the JsonIgnore attribute it should be ignored during serialization but instead I get an exception about a conflict between the 2. This example uses attributes to demonstrate because this is what I'm using in my code but I can reproduce this issue without attributes and just using different casing for the property names.

public class MyPoco
{
    public long approximateArrivalTimestamp { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(approximateArrivalTimestamp);
        }
    }
}

Reproduction Steps

Run the following code and you will get a naming collision for approximateArrivalTimestamp. There shouldn't be a collision because the ApproximateArrivalTimestamp property has the JsonIgnore attribute which should exclude it from collisions.

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

namespace JsonAttributeTest;

internal class Program
{
    static void Main(string[] args)
    {
        var options = new JsonSerializerOptions
        {
            DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
            PropertyNameCaseInsensitive = true,
        };

        var poco = new MyPoco { ApproximateArrivalEpoch = 1698106445L };
        
        var json = JsonSerializer.Serialize(poco, options);

        var serializedPoco = JsonSerializer.Deserialize<MyPoco>(json, options);
        Console.WriteLine(serializedPoco.ApproximateArrivalTimestamp);
    }
}

public class MyPoco
{
    [JsonPropertyName("approximateArrivalTimestamp")]
    public long ApproximateArrivalEpoch { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(ApproximateArrivalEpoch);
        }
    }
}

Expected behavior

The MyPoco is successfully serialized to

{
  "approximateArrivalTimestamp": 1698106445
}

and then deserialized back into a new POCO.

Actual behavior

Get a System.InvalidOperationException with the following error message:

The JSON property name for 'JsonAttributeTest.MyPoco.ApproximateArrivalTimestamp' collides with another property.

Regression?

Yes. Same code works .NET 6 and earlier.

Known Workarounds

These are .NET types used to represent AWS Lambda event and our part of our public contract. I can't change the .NET property names to work around the issue because that would be a breaking change. If these were new types then I could change the .NET names to be unique and not rely on the Json attribute.

Configuration

Fails on .NET 8 RC2 but works in earlier versions.

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 24, 2023
@ghost
Copy link

ghost commented Oct 24, 2023

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

Description

I'm updating our JSON serialization code for AWS Lambda to support .NET 8 and ran into a serialization bug that was worked correctly in previous versions. When doing JSON serialization with System.Text.Json and PropertyNameCaseInsensitive set to true properties with JsonIgnore are being included when detecting attribute collisions. For example looking at following type

public class MyPoco
{
    [JsonPropertyName("approximateArrivalTimestamp")]
    public long ApproximateArrivalEpoch { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(ApproximateArrivalEpoch);
        }
    }
}

There are technically 2 properties with the name approximateArrivalTimestamp but since the one that returns DateTime has the JsonIgnore attribute it should be ignored during serialization but instead I get an exception about a conflict between the 2. This example uses attributes to demonstrate because this is what I'm using in my code but I can reproduce this issue without attributes and just using different casing for the property names.

public class MyPoco
{
    public long approximateArrivalTimestamp { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(approximateArrivalTimestamp);
        }
    }
}

Reproduction Steps

Run the following code and you will get a naming collision for approximateArrivalTimestamp. There shouldn't be a collision because the ApproximateArrivalTimestamp property has the JsonIgnore attribute which should exclude it from collisions.

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

namespace JsonAttributeTest;

internal class Program
{
    static void Main(string[] args)
    {
        var options = new JsonSerializerOptions
        {
            DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
            PropertyNameCaseInsensitive = true,
        };

        var poco = new MyPoco { ApproximateArrivalEpoch = 1698106445L };
        
        var json = JsonSerializer.Serialize(poco, options);

        var serializedPoco = JsonSerializer.Deserialize<MyPoco>(json, options);
        Console.WriteLine(serializedPoco.ApproximateArrivalTimestamp);
    }
}

public class MyPoco
{
    [JsonPropertyName("approximateArrivalTimestamp")]
    public long ApproximateArrivalEpoch { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(ApproximateArrivalEpoch);
        }
    }
}

Expected behavior

The MyPoco is successfully serialized to

{
  "approximateArrivalTimestamp": 1698106445
}

and then deserialized back into a new POCO.

Actual behavior

Get a System.InvalidOperationException with the following error message:

The JSON property name for 'JsonAttributeTest.MyPoco.ApproximateArrivalTimestamp' collides with another property.

Regression?

Yes. Same code works .NET 6 and earlier.

Known Workarounds

These are .NET types used to represent AWS Lambda event and our part of our public contract. I can't change the .NET property names to work around the issue because that would be a breaking change. If these were new types then I could change the .NET names to be unique and not rely on the Json attribute.

Configuration

Fails on .NET 8 RC2 but works in earlier versions.

Other information

No response

Author: normj
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@gregsdennis
Copy link
Contributor

What happens if you have PropertyNameCaseInsensitive as false?

What happens if you use PropertyNamingPolicy instead?

@normj
Copy link
Author

normj commented Oct 24, 2023

@gregsdennis Setting PropertyNameCaseInsensitive to false does work even if the name set in the JsonPropertyName is the exact same casing as another property that has the JsonIgnore attribute.

public class MyPoco
{
    [JsonPropertyName("ApproximateArrivalTimestamp")]
    public long ApproximateArrivalEpoch { get; set; }

    [JsonIgnore]
    public DateTime ApproximateArrivalTimestamp
    {
        get
        {
            var epoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
            return epoch.AddMilliseconds(ApproximateArrivalEpoch);
        }
    }
}

Unfortunately in my situation though I have to have PropertyNameCaseInsensitive set to true.

@eiriktsarpalis
Copy link
Member

This appears to be a regression from .NET 7 and is specific to setting PropertyNameCaseInsensitive to true. Minimal repro:

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

var options = new JsonSerializerOptions
{
    PropertyNameCaseInsensitive = true,
};

string json = JsonSerializer.Serialize(new MyPoco(), options);
Console.WriteLine(json);

public class MyPoco
{
    public int foo { get; set; }

    [JsonIgnore]
    public int Foo { get; set; }
}

We should fix this in .NET 8.

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 24, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Oct 24, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 24, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, 8.0.x Oct 24, 2023
@eiriktsarpalis
Copy link
Member

@normj Am I right to assume you're not blocked by this regression?

@normj
Copy link
Author

normj commented Oct 24, 2023

@eiriktsarpalis Actually this is blocking for me.

I discovered this will testing our AWS Lambda .NET code. In Lambda we have a generic serializer based on System.Text.Json that is configured for PropertyNameCaseInsensitive being true to be flexible for user defined .NET types that will be serialized from Lambda's JSON event payload into the .NET type.

I discovered this while testing our AWS provided event objects and we have a few response events that triggered this behavior like this one, ignore the NETCOREAPP3_1 I was in the process of updating that.

So I can't change PropertyNameCaseInsensitive nor can I change the property names where that is trigger the behavior.

Sorry to discover this so late, kicking myself for not testing this sooner.

@eiriktsarpalis
Copy link
Member

I'm asking because a fix will probably not make it for GA, but it will probably be included in an upcoming servicing release. Could you not try renaming the ignored properties to something that doesn't conflict with your JSON contract?

@normj
Copy link
Author

normj commented Oct 24, 2023

Unfortunately I can't rename the property or the JSON field as they are part of the public API contract and would be a breaking change to consumers.

The code linked above was released to NuGet in 2021. This is another one of our NuGet packages that has this same behavior broken in .NET 8 https://www.nuget.org/packages/Amazon.Lambda.KinesisFirehoseEvents

@eiriktsarpalis
Copy link
Member

Do you control the circumstances of how these POCOs get serialized? Is it possible to extract data to a separate DTO type that gets serialized without ambiguity?

@normj
Copy link
Author

normj commented Oct 24, 2023

I don't have control. Users in AWS Lambda say they want to use our serializer and then in their code pick up our NuGet package and send it through the serializer. There is no dependency between serializer package and the package that has the POCO.

@jeffhandley
Copy link
Member

@normj Thank you for reporting this and being clear with us about the impact it has on your scenarios! You connected with us just in time. We really appreciate you trying out the RC2 release, narrowing this problem down to a minimal repro, and sharing with us that you're blocked by this regression. Cheers!!

@normj
Copy link
Author

normj commented Oct 24, 2023

Thanks so much for jumping on this. I felt terrible reporting an issue so late in the release cycle. I really appreciate pushing in the fix so quick.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 24, 2023
@jeffhandley
Copy link
Member

No need to feel terrible for reporting an issue so late--we're very grateful. This is what release candidates are for and you reported it just in time for us to get the fix into .NET 8 GA. Your diagnosis and minimal repro gave us that opportunity. Thanks again!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants