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

Do not use DictionaryKeyPolicy on deserialization #30142

Closed
JamesNK opened this issue Jul 5, 2019 · 2 comments · Fixed by dotnet/corefx#39392
Closed

Do not use DictionaryKeyPolicy on deserialization #30142

JamesNK opened this issue Jul 5, 2019 · 2 comments · Fixed by dotnet/corefx#39392
Assignees
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 5, 2019

re: https://github.com/dotnet/corefx/issues/38840#issuecomment-506888027

DictionaryKeyPolicy is being used for deserialization as well as serialization. I'm not sure when this decision was made, but I think this has a number of problems:

  1. JsonNamingPolicy (and JsonNamingStrategy in Json.NET) are designed as a one way operation. There is only ConvertName on the type. There is no pair of ConvertTo and ConvertFrom methods. That is because you can't reliably undo PascalCase to camelCase or underscore_case.

  2. It is common for people to reuse the same serializer options for both serializing and deserializing. When using the policy is used in both directions, calling the same JsonNamingPolicy on an incoming dictionary key will at best do nothing, or at worst double convert. For example, a naming policy that HTML encodes a string will now HTML encode it in both directions: "J & NK" -> "J & NK" -> "J & NK"

  3. It is inconsistent. DictionaryKeyPolicy is called during deserialization but PropertyNamePolicy is not.

  4. What is the user scenario for this? The naming policy with serialization is useful for people who want dictionary keys to be camel cased, but what purpose is there to run the policy on deserialization? You can't undo camel case back to pascal case so it's not that. I ask because this is a feature that has never been requested in Json.NET.

@layomia
Copy link
Contributor

layomia commented Jul 8, 2019

@steveharter
Copy link
Member

Offline discussion recap: do not use naming policy on deserialization. The reason is primarily for performance since the scenario where this is useful (normalizing incoming dictionary keys that have mixed casing) is an edge case with workarounds (creating a custom converter for dictionary or instantiating collection case-insensitive from POCO ctor).

On (1) and (2) above -- the naming policy is one-way and is expected to be idempotent, so running it on deserialization and again on serialization should be fine.
On (3) above - consistency not really a factor since the two cases (property names and dict keys) are different; the property name policy does actually run during serialization and deserialization in a way -- the first time the POCO type is serialized or deserialized the policy executes against the POCO property (not the JSON) and then caches the value which is used during deserialization to match up against the JSON and during serialization to write the property name.
On (4) above - the scenario is normalizing incoming dictionary keys that have mixed casing; again an edge case with workarounds.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants