Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use options.Encoder when serializing dictionary keys and property names#40787

Merged
steveharter merged 1 commit intodotnet:masterfrom
steveharter:EscapingDict
Sep 5, 2019
Merged

Use options.Encoder when serializing dictionary keys and property names#40787
steveharter merged 1 commit intodotnet:masterfrom
steveharter:EscapingDict

Conversation

@steveharter
Copy link
Contributor

@steveharter steveharter commented Sep 3, 2019

Addresses https://github.com/dotnet/corefx/issues/40704 for master.

These locations were not escaped using the custom encoder specified on options.Encoder:

  • Dictionary key name
  • Property name
  • JsonException.Path (which is a property\dictionary path to an error)

However all values (property, dictionary, etc) correctly used the custom escaper.

This may be ported to 3.0 pending feedback\priority.

@steveharter steveharter added this to the 5.0 milestone Sep 3, 2019
@steveharter steveharter self-assigned this Sep 3, 2019

private static readonly JsonEncodedText _keyName = JsonEncodedText.Encode(KeyName);
private static readonly JsonEncodedText _valueName = JsonEncodedText.Encode(ValueName);
// "encoder: null" is used since the literal values of "Key" and "Value" should not normally be escaped
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to verify that this assumption is OK; without this assumption performance would suffer since the values could no longer be cached (at least can't be cached like below).

@steveharter steveharter force-pushed the EscapingDict branch 2 times, most recently from b5c148b to 778a3eb Compare September 4, 2019 22:09
[Fact]
public static void DeserializePathForObjectFails()
{
const string GoodJson = "{\"Property\u04671\":1}";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. This is the same issue as #40793 (comment) where GitHub gets tripped up by const strings containing characters written out in their "escaped" format, where it starts to consider this as invalid C# (even though it isn't). Note, the string keyword isn't being syntax-highlighted correctly either.

Looks like it's caused by the C# property names that contain escaped characters (from the above class definition ClassWithUnicodePropertyName - line 163 Property\u04671 ), which is throwing off all subsequent highlighting/C# validation up until this semi-colon on line 169.

@steveharter steveharter changed the title Use options.Encoder when serializing dictionary and property names Use options.Encoder when serializing dictionary keys and property names Sep 5, 2019
@steveharter steveharter merged commit 7829d4a into dotnet:master Sep 5, 2019
@steveharter steveharter deleted the EscapingDict branch September 5, 2019 14:41
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Oct 14, 2019
@ericstj
Copy link
Member

ericstj commented Oct 14, 2019

Marking this a breaking since its an observable behavior diff. Let's make sure to document this change.

steveharter added a commit to steveharter/dotnet_corefx that referenced this pull request Oct 14, 2019
steveharter added a commit to steveharter/dotnet_corefx that referenced this pull request Oct 14, 2019
@ahsonkhan
Copy link

Marking this a breaking since its an observable behavior diff. Let's make sure to document this change.

Did we document this change somewhere?

ahsonkhan referenced this pull request in dotnet/samples Dec 27, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants