Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

layomia · 2020-05-14T01:03:31Z

Fixes KeyValuePair is not properly serialized with JsonNamingPolicy.CamelCase - System.Text.Json #1197

This is a breaking change in that we'll potentially serialize the property names differently than in .NET Core 3.x/System.Text.Json 4.7.x (as influenced by the naming policy & encoder). However, roundtripping is not broken as the literals "Key" and "Value" are special-cased to match when deserializing, to accommodate payloads that were serialized with previous versions.
Pushes code coverage for KeyValuePair converter to 100% (previously 70% line, 77% branch)

layomia · 2020-05-14T01:06:25Z

The first commit moves KeyValuePair tests to a dedicated file. I suggest that code review starts from the second commit.

…tions when (de)serializing KeyValuePair instances

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs

steveharter · 2020-05-14T22:03:26Z

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs

+
+        // _keyName and _valueName as JsonEncodedText.
+        private JsonEncodedText _keyNameEncoded;
+        private JsonEncodedText _valueNameEncoded;

        // todo: https://github.com/dotnet/runtime/issues/32352
        // it is possible to cache the underlying converters since this is an internal converter and


This comment pertains to looking up the System.String converter and caching that. This is in case someone changes the converter.

IMO we should either fix it in this PR\pass or close the issue for 5.0 if we don't want to forward to the "proper" converter.

I'll prioritize making a PR to close this.

steveharter

Other than last comment around removing unnecessary Key Value check, LGTM

jozkee · 2020-05-15T02:00:23Z

src/libraries/System.Text.Json/src/Resources/Strings.resx

-  <data name="SerializerDictionaryKeyNull" xml:space="preserve">
-    <value>The dictionary key policy '{0}' cannot return a null key.</value>
+  <data name="NamingPolicyReturnNull" xml:space="preserve">
+    <value>The naming policy '{0}' cannot return null.</value>


Should we include the naming policy in the message for the PropertyNamingPolicy used on CLR propertiesas well? Right now it says The JSON property name for '{0}.{1}' cannot be null..

Yes we could throw a more specific message (in a clean up PR), although right now it shouldn't be hard for a caller to tell what caused an exception with this message.

jozkee · 2020-05-15T02:02:51Z

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs

+
+                if (_keyName == null || _valueName == null)
+                {
+                    ThrowHelper.ThrowInvalidOperationException_NamingPolicyReturnNull(namingPolicy);


Did you consider using ThrowInvalidOperationException_SerializerPropertyNameNull?

I did. It's achievable with some refactoring. We don't create a JsonPropertyInfo for the "Key" and "Value" properties, so we'd have to pass the property names by hand.

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs

...braries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.KeyValuePair.cs

jozkee · 2020-05-15T03:15:41Z

...braries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.KeyValuePair.cs

+        [InlineData(@"{""Value"":1,""Value"":2}")]
+        public static void InvalidJsonFail(string json)
+        {
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<KeyValuePair<int, int>>(json));


Is the JSON Path correct in these cases?

We don't create JsonPropertyInfo instances for the key and value properties, so JSON path will not point directly to them, but to the property name for the KVP instance.

I think this granularity is good enough, but I'll estimate the effort to get the specific path then create an issue.

Keep in mind that there is already an issue for general consistency of JSON Path #31955, but sounds like you want to do something scoping only KVP, so maybe make sure to link them.

layomia · 2020-05-15T04:11:33Z

Will address @jozkee's feedback in a follow up to fix #32148.

danmoseley · 2020-07-24T21:35:29Z

@layomia this is labeled breaking change. Can you please open an issue with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

I don't see an existing one

layomia · 2020-10-03T22:20:24Z

Breaking change doc filed: dotnet/docs#20898.

Move serializer KeyValuePair tests to dedicated file

f876095

layomia added area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels May 14, 2020

layomia added this to the 5.0 milestone May 14, 2020

layomia requested review from jozkee and steveharter as code owners May 14, 2020 01:03

layomia self-assigned this May 14, 2020

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder op…

3b44403

…tions when (de)serializing KeyValuePair instances

layomia force-pushed the kvp_policy branch from 56e7762 to 3b44403 Compare May 14, 2020 02:12

steveharter reviewed May 14, 2020

View reviewed changes

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs Show resolved Hide resolved

steveharter reviewed May 14, 2020

View reviewed changes

steveharter approved these changes May 14, 2020

View reviewed changes

layomia added 2 commits May 14, 2020 15:31

Remove unnessary guard against switched property names

aea91eb

Merge remote-tracking branch 'upstream/master' into kvp_policy

ee45b87

jozkee reviewed May 15, 2020

View reviewed changes

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs Show resolved Hide resolved

jozkee reviewed May 15, 2020

View reviewed changes

...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs Show resolved Hide resolved

jozkee reviewed May 15, 2020

View reviewed changes

...braries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.KeyValuePair.cs Show resolved Hide resolved

jozkee reviewed May 15, 2020

View reviewed changes

layomia merged commit 8dbfb8e into dotnet:master May 15, 2020

danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 30, 2020

ericstj mentioned this pull request Sep 25, 2020

KeyValuePair is not properly serialized with JsonNamingPolicy.CamelCase - System.Text.Json #1197

Closed

layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2020

layomia mentioned this pull request Oct 15, 2020

Honor propery naming policy when (de)serializing KeyValuePair instances #1198

Closed

layomia mentioned this pull request Nov 9, 2020

KeyValuePair serialization doesn't follow camel case option in System.Text.Json #44385

Closed

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

layomia deleted the kvp_policy branch May 1, 2021 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

layomia commented May 14, 2020

layomia commented May 14, 2020

steveharter May 14, 2020

layomia May 14, 2020

steveharter left a comment

jozkee May 15, 2020

layomia May 15, 2020

jozkee May 15, 2020

layomia May 15, 2020

jozkee May 15, 2020

layomia May 15, 2020

jozkee May 15, 2020

layomia commented May 15, 2020 •

edited

Loading

danmoseley commented Jul 24, 2020 •

edited

Loading

layomia commented Oct 3, 2020

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

Honor PropertyNamingPolicy, PropertyNameCaseInsensitive, & Encoder options when (de)serializing KeyValuePair instances #36424

Conversation

layomia commented May 14, 2020

layomia commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia commented May 15, 2020 • edited Loading

danmoseley commented Jul 24, 2020 • edited Loading

layomia commented Oct 3, 2020

layomia commented May 15, 2020 •

edited

Loading

danmoseley commented Jul 24, 2020 •

edited

Loading