-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Do not use DictionaryKeyPolicy on deserialization #39392
Conversation
|
||
Assert.Equal(2, obj[1].Count); | ||
Assert.Equal(3, obj[1]["Key1"]); | ||
Assert.Equal(4, obj[1]["Key2"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the serialize case to make sure Key1->key1 when the policy is applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here -
public static void CamelCaseSerialize() |
Can you update the docs to indicate that the policy is ignored on deserialization: corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs Lines 95 to 100 in 6005a1e
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a doc update.
} | ||
|
||
[Fact] | ||
public static void KeyConflictDeserialize_LastValueWins() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this still a useful test case? What do we do for this case now since we are no longer using naming policy when deserializing? The behavior should be captured in a test.
ThrowHelper.ThrowInvalidOperationException_SerializerDictionaryKeyNull(options.DictionaryKeyPolicy.GetType()); | ||
} | ||
|
||
keyName = options.DictionaryKeyPolicy.ConvertName(keyName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why did we initially try to get the keyName by calling ConvertName twice?
* Do not use DictionaryKeyPolicy on deserialization * Update policy doc Commit migrated from dotnet/corefx@ba8641a
Fixes https://github.com/dotnet/corefx/issues/39207.
cc @JamesNK