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

Regression in .NET 9-RC2: Case-insensitive handling of JsonNode #108790

Open
bart-vmware opened this issue Oct 11, 2024 · 6 comments
Open

Regression in .NET 9-RC2: Case-insensitive handling of JsonNode #108790

bart-vmware opened this issue Oct 11, 2024 · 6 comments
Labels
area-System.Collections 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
Milestone

Comments

@bart-vmware
Copy link

Description

While analyzing why a unit test in .NET Aspire fails on .NET 9, I traced it down to a breaking change in System.Text.Json. I suspect the change was introduced in #103645. The regression is that when updating an existing key in a case-insensitive JsonNode with a different key casing, the original casing is preserved. This differs from the behavior in .NET 8, which adapts the existing key casing to the latest.

This Aspire schema generation unit test breaks, after changing the target framework from .NET 8 to .NET 9. Even in the release/9.0-rc1 branch of Aspire, the schema generator unit tests still run on .NET 8, which is probably why this wasn't noticed earlier.

Below is the code for the failing Aspire test (both the test and the implementation haven't changed since Aspire v8):

[Fact]
public void LastUsedCasingOfLogCategoryWins()
{
    var source =
        """
        [assembly: Aspire.LoggingCategories("ONE", "one.TWO.three", "One.Two.Three")]
        """;

    var schema = GenerateSchemaFromCode(source, []);

    AssertIsJson(schema,
        """
        {
          "definitions": {
            "logLevel": {
              "properties": {
                "ONE": {
                  "$ref": "#/definitions/logLevelThreshold"
                },
                "One.Two.Three": {
                  "$ref": "#/definitions/logLevelThreshold"
                }
              }
            }
          }
        }
        """);
}

Which fails with the following output on .NET 9:

Xunit.Sdk.EqualException
Assert.Equal() Failure: Strings differ
                                    ↓ (pos 157)
Expected: ···"       },\r\n        "One.Two.Three": {\r\n  "···
Actual:   ···"       },\r\n        "one.TWO.three": {\r\n  "···
                                    ↑ (pos 157)

Reproduction Steps

  1. Create a new .NET 8 console app and replace Program.cs with the following code:
    using System.Text.Json.Nodes;
    
    var options = new JsonNodeOptions
    {
        PropertyNameCaseInsensitive = true
    };
    
    var obj = new JsonObject(options);
    
    obj["case"] = "example1";
    obj["CASE"] = "example2";
    
    Console.WriteLine(obj.ToString());
    This outputs:
    {
      "CASE": "example2"
    }
    
  2. Add a NuGet package reference to System.Text.Json, version 9.0.0-rc.2.24473.5.
  3. Rerun the program. Output changes to:
    {
      "case": "example2"
    }
    

Expected behavior

When adding an entry to a case-insensitive JsonNode instance, which already contains the same key (but in different casing), then the casing of the new key is used.

Actual behavior

When adding an entry to a case-insensitive JsonNode instance, which already contains the same key (but in different casing), then the casing of the original key is preserved.

Regression?

Yes, the existing behavior of .NET 8 has changed in .NET 9.

Known Workarounds

No response

Configuration

> dotnet --info

.NET SDK:
 Version:           9.0.100-rc.2.24474.11
 Commit:            315e1305db
 Workload version:  9.0.100-manifests.82e6a096
 MSBuild version:   17.12.0-preview-24473-03+fea15fbd1

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22631
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\9.0.100-rc.2.24474.11\

.NET workloads installed:
 [maui-windows]
   Installation Source: VS 17.12.35323.107
   Manifest Version:    9.0.0-rc.1.24453.9/9.0.100-rc.1
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\9.0.100-rc.1\microsoft.net.sdk.maui\9.0.0-rc.1.24453.9\WorkloadManifest.json
   Install Type:              Msi

 [maccatalyst]
   Installation Source: VS 17.12.35323.107
   Manifest Version:    17.5.9270-net9-rc1/9.0.100-rc.1
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\9.0.100-rc.1\microsoft.net.sdk.maccatalyst\17.5.9270-net9-rc1\WorkloadManifest.json
   Install Type:              Msi

 [ios]
   Installation Source: VS 17.12.35323.107
   Manifest Version:    17.5.9270-net9-rc1/9.0.100-rc.1
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\9.0.100-rc.1\microsoft.net.sdk.ios\17.5.9270-net9-rc1\WorkloadManifest.json
   Install Type:              Msi

 [android]
   Installation Source: VS 17.12.35323.107
   Manifest Version:    35.0.0-rc.1.80/9.0.100-rc.1
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\9.0.100-rc.1\microsoft.net.sdk.android\35.0.0-rc.1.80\WorkloadManifest.json
   Install Type:              Msi

 [aspire]
   Installation Source: VS 17.12.35323.107
   Manifest Version:    8.2.0/8.0.100
   Manifest Path:       C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.aspire\8.2.0\WorkloadManifest.json
   Install Type:              Msi

Configured to use loose manifests when installing new manifests.

Host:
  Version:      9.0.0-rc.2.24473.5
  Architecture: x64
  Commit:       990ebf52fc

.NET SDKs installed:
  8.0.403 [C:\Program Files\dotnet\sdk]
  9.0.100-rc.1.24452.12 [C:\Program Files\dotnet\sdk]
  9.0.100-rc.2.24474.11 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-rc.2.24474.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-rc.1.24431.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-rc.2.24473.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 6.0.33 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 9.0.0-rc.1.24452.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 9.0.0-rc.2.24474.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  x86   [C:\Program Files (x86)\dotnet]
    registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables:
  Not set

global.json file:
  Not found

Other information

/cc @eiriktsarpalis @eerhardt

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 11, 2024
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 11, 2024

Thanks for your detailed report. I've been able to trace back the issue to the new System.Collections.Generic.OrderedDictionary type on which JsonNode has been retargeted. Minimal reproduction:

var od = new OrderedDictionary<string, string>(StringComparer.OrdinalIgnoreCase);

od["case"] = "example1";
od["CASE"] = "example2";

foreach (var entry in od)
{
    Console.WriteLine(entry);  // [case, example2]
}

@eiriktsarpalis eiriktsarpalis added area-System.Text.Json area-System.Collections and removed untriaged New issue has not been triaged by the area owner area-System.Text.Json needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 11, 2024
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Oct 11, 2024
Copy link
Contributor

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

@stephentoub
Copy link
Member

I've been able to trace back the issue to the new System.Collections.Generic.OrderedDictionary type on which JsonNode has been retargeted.

Just to be clear, though, this is the expected behavior for OrderedDictionary, and matches what Dictionary does. The comparer is OrdinalIgnoreCase, and so the second write simply update's the existing key's value; it doesn't overwrite the key itself.

@eiriktsarpalis
Copy link
Member

On closer inspection, I don't believe this is a bug. This is common behaviour when overwriting entries in dictionaries with case insensitive comparison. Here's dictionary for example:

var dict = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

dict["case"] = "example1";
dict["CASE"] = "example2";

foreach (var entry in dict)
{
    Console.WriteLine(entry); // [case, example2]
}

The pre .NET 9 for JsonObject used a bespoke implementation of an ordered dictionary which happened to update the key with the operation. This was accidental behaviour and changing it here is well within the parameters of case insensitive lookups. I would recommend removing and then reinserting the entry if case sensitivity of the final JSON output is important.

@bart-vmware
Copy link
Author

Sure, it's easy to work around. But it's still a breaking change in JsonNode that potentially affects users depending on the current behavior, which is why I created this issue.

If this won't be fixed, with the expectation that any affected users update their code, this behavior change should be mentioned in the list of breaking changes.

@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 blocking-release labels Oct 11, 2024
Copy link
Contributor

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Collections 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
Projects
None yet
Development

No branches or pull requests

3 participants