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.

Sign up for GitHub

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

Jump to bottom

Honor naming policy for string enum deserialization #73348

Merged
merged 7 commits into from
Aug 11, 2022
Merged

Honor naming policy for string enum deserialization #73348

merged 7 commits into from
Aug 11, 2022

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Aug 4, 2022 •
edited
Loading

Fixes #31619.

@layomia layomia added this to the 7.0.0 milestone Aug 4, 2022
@layomia layomia self-assigned this Aug 4, 2022
@layomia layomia requested review from krwq and eiriktsarpalis August 4, 2022 00:44
@ghost
Copy link

ghost commented Aug 4, 2022

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

Fixes #31619. Only the first 64 values of a given enum are supported. Flags are not supported.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 7.0.0

eiriktsarpalis
eiriktsarpalis reviewed Aug 5, 2022
View reviewed changes
...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs Outdated
// todo: optimize implementation here by leveraging https://github.com/dotnet/runtime/issues/934.
string[] enumValues = enumString.Split(
#if BUILDING_INBOX_LIBRARY
ValueSeparator
Copy link
Member

Choose a reason for hiding this comment

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

FWIW it is possible for enum identifiers to contain commas in IL, but the good news is that not even the Enum parser is able to handle that:

https://sharplab.io/#v2:DYLgZgzgNALiCWwA+B7ADgUwHYAIDKAnhDBgLYCwAUFQNoA8AYsAIYDmEAfALpUwGY4AsgQCiWAK6kcAXio4cSHAAMlAFQAWGAE4YA5BBzMcAYxSlSzKDni5SBHFmakMKmTgCMchcrWad+wxMzCysMADdsHD8XJTcAJi8qYAwYBycMNwAKYTFJADoVDW09AyNTc0trW3tHZ1ckBqFRCVIC32KAsuDK8MjolQBKPNUUPBgtG1ZMgaoAelmcMOZgNOcQHGIJrFY3ACIi/1KgiqsbHDtVjCsDksDykJxe3GjdqiSUnB0IcWBU6Rxcq0AArMLQQDB0HItDiZWoYGaUeb4IgkVoAQS0rEk2BgIgAHsYMGgYPAUFh1gAlDAAR3EGGIGAAJotlnScLobp1jg8zhc4ddolz7j0Is8OjgAO7MAxYFCpMAocRYRl5OYLeSGVKEBmtQHDLQEEFgjAAIQIADl0pkKUqSc5VPwMthJA7MFYqcxGQB5LDAAh4NDMLBKdws4B0qwmlAoZJB6ysWU6ADC0quOCjMYwcZg6i0KAlPoYzEQ4h0VgAqgBJLAwABsABYAGSfek/GAIpEa5halFkPJ61QGo3g6swADMcUB1tt8HtjseLVdaY93t9/sDwdDS3DadHE/ONmrxnDEHgESse7i52YeKPJ7PaYzsdw8ATKGTqcj0efOBzeYLWBFiWZY4EuSYoIyGR8G6OCXs2Xxth26ryN2yI6v2LT6oaoLgjQqiAlw1pZqufoBkGIZhhG6bflmL5vh+4Jfpm2a5vmhbFuGIH4S08Gtr8SEapqaGohh+SDthxp4QRmRjFsOzblRT60fGiYYCmjHUcxuB/mxgEcaWabcZIvHfPxaqCah2oiXqw4YFJLSEbJkyUY+NFxq+qnqfC5ldj26E2ThdlGaQjnjM5CneYiyFCXQYygjA4hoOBkEACQMHglYAPoAAzZe4ACsHB5GlGU5Xl+V5BYNgAALVdMVQ4EmIAADrluCYLNRg8AwBAoKBs1kERMAA1ylgKTNVoM7OAAtGAWYJToY7NRAWjGM1wDwAARlooLwPSzVWX2qgYHiMB5AAUhAZLLTAjI2CAG1jTgcT5T5KF+SJNo1rOGCCCk6gQQAEkGjLJHk1ZhCgADWv3/RBmReptABWGDGKkMCgqwKRWAAaig8CMgAVIThiYtiNbQPgHndgZGyvkxP7wBA4FYJs4ho++Am+cJfZUmAyRo6SWB5H9OYQRD0PaODWCQzDCPI6jqQoMjF41kC4wk5jlMmjYd3bEwbAGDYssYGiMDjAMQA===

We might want to store a flag indicating that the enum contains commas and fail early, rather than attempt to pass bogus tokens into the naming policy. (FWIW this is a very niche use case)

eiriktsarpalis
eiriktsarpalis approved these changes Aug 5, 2022
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM other than a few pending issues.

krwq
krwq reviewed Aug 7, 2022
View reviewed changes
...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs
/// <summary>
/// Holds a mapping from enum value to text that might be formatted with <see cref="_namingPolicy" />.
/// </summary>
private readonly ConcurrentDictionary<ulong, JsonEncodedText> _nameCacheForWriting;
Copy link
Member

@krwq krwq Aug 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

is converter the right place for this? could converter be potentially re-used in place with different naming policy? Would it make sense to move this to JsonTypeInfo and cache all possible values beforehand (i.e. in Configure method) and then use regular Dictionary since I believe just lookup is thread-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could converter be potentially re-used in place with different naming policy?

I don't believe so.

Would it make sense to move this to JsonTypeInfo and cache all possible values beforehand

The enum converter might be applied to enum types or specific properties. Placing on enum JsonTypeInfos would satisfy only the former.

Copy link
Member

@krwq krwq Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Might be good to still pre-cache all values in the constructor since that won't require to use concurrent dictionary but consider this feedback optional

Copy link
Member

Choose a reason for hiding this comment

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

also another alternative to put the caching both on JsonPropertyInfo and JsonTypeInfo but if JsonPropertyInfo uses JsonTypeInfo's dictionary then you can re-use the same dictionary but assuming JsonConverter is unique per JsonTypeInfo/JsonPropertyInfo consider this optional

Copy link
Member

Choose a reason for hiding this comment

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

hmm.. seems prepopulating might not work due to combinations of flags

@layomia layomia force-pushed the enum branch 2 times, most recently from feeb953 to 7fa3ce9 Compare August 10, 2022 18:49
@layomia layomia mentioned this pull request Aug 10, 2022
Closed
krwq
krwq reviewed Aug 11, 2022
View reviewed changes
...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

private static string[] SplitFlagsEnum(string value)
{
// todo: optimize implementation here by leveraging https://github.com/dotnet/runtime/issues/934.
Copy link
Member

Choose a reason for hiding this comment

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

consider adding [ActiveIssue("https://github.com/dotnet/runtime/issues/934")] in the comment for easier grepping

krwq
krwq reviewed Aug 11, 2022
View reviewed changes
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/EnumConverterTests.cs
""static"": 2
}";

JsonTestHelper.AssertJsonEqual(expected, JsonSerializer.Serialize(dict, options));
Copy link
Member

Choose a reason for hiding this comment

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

you might want to validate deserialization behavior here as well (throw or roundtrip)

krwq
krwq reviewed Aug 11, 2022
View reviewed changes
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/EnumConverterTests.cs
}

[Fact]
public static void EnumDictionaryKeyDeserialization()
Copy link
Member

@krwq krwq Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd just joing those two tests (this and below) into one since they test the same concept and are mostly the same

krwq
krwq reviewed Aug 11, 2022
View reviewed changes
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/EnumConverterTests.cs
}

[Fact]
public static void EnumDictionaryKeySerialization()
Copy link
Member

@krwq krwq Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

add a test where enum converter (set through options) and properties have different naming policy and make sure both scenarios serialize/deserialize correctly when using same options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. It's essentially a test that the converter precedence rules hold (property-level converter wins over type-level), but the redundancy is good.

krwq
krwq reviewed Aug 11, 2022
View reviewed changes
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/EnumConverterTests.cs
};

string expected = @"{
""public, non_public"": 1,
Copy link
Member

@krwq krwq Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

curious is ", " considered something user might want to customize? should we create issue to customize serparator? or possibly some enum specific option on naming policy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an interesting idea but I haven't seen anyone ask or any precedent for it. I wouldn't it pursue without a customer ask.

krwq
krwq reviewed Aug 11, 2022
View reviewed changes
...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs
/// <see cref="ulong"/> is as the key used rather than <typeparamref name="T"/> given measurements that
/// show private memory savings when a single type is used https://github.com/dotnet/runtime/pull/36726#discussion_r428868336.
/// </summary>
private readonly ConcurrentDictionary<ulong, JsonEncodedText> _nameCacheForWriting;
Copy link
Member

Choose a reason for hiding this comment

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

consider renaming:

  • _nameToEnumValueCache
  • _enumValueToNameCache

@layomia
Copy link
Contributor Author

layomia commented Aug 11, 2022

@krwq I'll get this fix in & address test feedback in a follow up.

@layomia layomia merged commit 50fec86 into dotnet:main Aug 11, 2022
@AlgorithmsAreCool
Copy link
Contributor

@layomia Thank you for your hard work on this!

@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krwq krwq krwq left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees

@layomia layomia

Labels
area-System.Text.Json
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

System.Text.Json: JsonStringEnumConverter ignores its JsonNamingPolicy during deserialization.
4 participants
@layomia @AlgorithmsAreCool @krwq @eiriktsarpalis