JsonDictionaryConverter throws when keys begin with escaped '$'

[AE-ajones]

Description

Deserializing JSON with preserved references throws an exception if any dictionary keys begin with an escaped '$' character.

Reproduction Steps

using System;
using System.Collections.Generic;
using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Unicode;
					
public record TestRecord{
	public Dictionary<string, string> TestDict {get; init;} = new();
}

public class Program
{
	public static void Main()
	{
		TestRecord test = new () {
			TestDict = new(){				
				["$break"] = "broken"
			}
		};
		
		TextEncoderSettings encoderSettings = new();
		encoderSettings.AllowRange(UnicodeRanges.BasicLatin);
		encoderSettings.ForbidCharacter('$');
		JsonSerializerOptions options = new(){ReferenceHandler = ReferenceHandler.Preserve, Encoder = JavaScriptEncoder.Create(encoderSettings)};
		
		string json = JsonSerializer.Serialize(test, options);
		TestRecord deserialized = JsonSerializer.Deserialize<TestRecord>(json, options);
	}
}

Expected behavior

The exception should only be thrown if a key begins with an unescaped '$' character.

Actual behavior

The repro steps below compile and execute in dotnet 8 without exception, in dotnet 9 the following exception is thrown:
System.Text.Json.JsonException: Properties that start with '$' are not allowed in types that support metadata. Either escape the character or disable reference preservation and polymorphic deserialization.

Regression?

This worked prior to dotnet 9. It is not a documented breaking change in https://learn.microsoft.com/en-us/dotnet/core/compatibility/9.0

Known Workarounds

Setting AllowOutOfOrderMetadataProperties to true will skip deserialization of the value. This is not a viable workaround for me.

Configuration

No response

Other information

No response

[dotnet-policy-service]

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

[eiriktsarpalis]

The JSON strings "$break" and "\u0024break" are equal under the JSON specification. .NET 9 fixed a bug in the metadata reader that was failing to properly unescape property names; this is why earlier versions of STJ failed to trigger the error mode you are seeing today. We generally don't document bugfixes as breaking changes.

Setting AllowOutOfOrderMetadataProperties to true will skip deserialization of the value.

Can you clarify? I see no difference whether this property is turned on or off.

[eiriktsarpalis]

FWIW we have this API proposal specifically intended to address this issue.

[jeffhandley]

Assigning to @eiriktsarpalis for continued triage.

[mythgarr]

@eiriktsarpalis The difference in behavior seems to be intentional. The error text specifically indicates that this exception is intended to be avoidable by escaping the metadata indicator character:

...Either escape the character or disable reference preservation...

That the JSON specification treats these as equivalent strings is irrelevant. This is a breaking change that was not (and still is not) called out in the documentations with no current mitigation.

Can you clarify? I see no difference whether this property is turned on or off.

I'll admit I did not try this, but based on the code I thought it would skip the key/value pair. However, it just moves the exception into TryReadMetadata.

This undocumented change in behavior has led to crashes in an application that I manage after migrating to dotnet 9. Whether you categorize it as a bug or bug fix, the dotnet foundation has a long-standing practice of disclosing such breaking changes.

[mythgarr]

EDIT: Source generation for JsonSerializerContext doesn't support reference preservation, so that's not an option. Creating a custom Dictionary<string,string> serializer works as a viable workaround. I'll move forward with a fix using this approach.

I still maintain that regardless of whether this change was addressing a bug in previous versions this constitutes a breaking change by any reasonable standard. Bugs (and fixes) happen, but informing developers is important for a framework as widely used as dotnet.

[eiriktsarpalis]

This undocumented change in behavior has led to crashes in an application that I manage after migrating to dotnet 9. Whether you categorize it as a bug or bug fix, the dotnet foundation has a long-standing practice of disclosing such breaking changes.

.NET Core/modern .NET does not guarantee bugward compatibility. The breaking changes that we do document concern departures from the documented contract -- bugfixes as such do not meet the bar for breaking change documentation despite Hyrum's law holding true.

Bugs (and fixes) happen, but informing developers is important for a framework as widely used as dotnet.

Every yearly release pushes thousands of bugfixes that impact observable behavior one way or another. Realistically, documenting everything as a breaking change wouldn't make our breaking change notes particularly helpful (since they would be practically indistinguishable from listing all PRs from github).

[eiriktsarpalis]

EDIT: Source generation for JsonSerializerContext doesn't support reference preservation, so that's not an option.

Related to #107597. It should still be possible to turn it on using a bit of boilerplate:

JsonSerializerOptions options = new(Context.Default.Options)
{
    ReferenceHandler = ReferenceHandler.Preserve
};

JsonSerializer.Serialize(new Foo(), options);

[JsonSerializable(typeof(Foo))]
partial class Context : JsonSerializerContext;

[mythgarr]

The breaking changes that we do document concern departures from the documented contract -- bugfixes as such do not meet the bar for breaking change documentation despite Hyrum's law holding true.

Respectfully, I disagree with your categorization. As previously mentioned this was not a bug but part of the documented behavior. At the risk of repeating myself, the Exception text clearly points to this workaround as an accepted option. The exception text retains this recommendation even in dotnet 9. At a minimum this text is now unequivocally incorrect - escaping the metadata character is not a viable option for addressing the exception.

Regardless of whether this particular change is ultimately added to the list of breaking changes this thread at least provides some documentation of the change, along with potential workarounds for anyone affected. Would you prefer I open a separate issue to report the inaccurate Exception text, or is this report adequate for your needs?

[eiriktsarpalis]

As previously mentioned this was not a bug but part of the documented behavior.

Could you point me to the documented behavior? Thanks.

[mythgarr]

System.Text.Json.JsonException: Properties that start with '$' are not allowed in types that support metadata. Either escape the character or disable reference preservation and polymorphic deserialization.

Emphasis is mine.
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/Resources/Strings.resx#L494

[eiriktsarpalis]

Thanks. On the basis of that exception message I would declare the change a regression and recommend that we backport the fix to .NET 9.

[eiriktsarpalis]

I did a bit of further digging, so I have more context now. The lack of unescaping in the metadata reader has been the source of roundtripping bugs specifically in the case of polymorphism, which allows user-defined metadata properties. The following code prints False in .NET 8, but works in .NET 9:

string json = JsonSerializer.Serialize<Base>(new Derived());
Console.WriteLine(json); // {"categor\u00EDa":"derived"}
Console.WriteLine(JsonSerializer.Deserialize<Base>(json) is Derived); // False

[JsonPolymorphic(TypeDiscriminatorPropertyName = "categoría")]
[JsonDerivedType(typeof(Derived), "derived")]
public record Base;
public record Derived : Base;

Even if escaping the dollar sign was originally documented as a workaround for bypassing validation:

1. It goes against the semantics of JSON strings,
2. It isn't reliable since any intervening process can normalize the string back to the dollar character,
3. It leads to roundtripping issues like the one shown above.

With this I would like to revise the action plan for this issue as follows:

1. Remove mention of the workaround in the exception message and
2. Document the breaking change, like you requested originally.

[AE-ajones]

That sounds like a good compromise. Using a custom converter works for my purposes, there are probably other workarounds that I haven't considered.

Thank you for looking into this - I appreciate the effort!

[eiriktsarpalis]

Addressed by #44748.

[mythgarr]

That PR doesn't seem related - do you mean #112355 ?

[eiriktsarpalis]

Should have been dotnet/docs#44748. Correct issue number but wrong repo :-)