DictionaryKeyPolicy not applied to keys that contain non primitive ty… #41691

marcnet80 · 2019-10-09T20:17:00Z

…pes #41176

https://github.com/dotnet/corefx/issues/41176#event-2697607085

…pes #41176 https://github.com/dotnet/corefx/issues/41176#event-2697607085

maryamariyan · 2019-11-06T21:09:46Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your corefx repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/libraries <path to the patch created in step 1>

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

…itive types https://github.com/dotnet/corefx/issues/41176

…s-fix

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

dotnet#41691

jozkee · 2019-11-12T23:49:54Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -83,7 +84,9 @@ public static partial class JsonSerializer

                    // An object or another enumerator requires a new stack frame.
                    state.Push(elementClassInfo, value);
-                    if (options?.DictionaryKeyPolicy != null && state.Current.ExtensionDataStatus != ExtensionDataWriteStatus.Writing)
+                    var extensionDataAttribute = jsonPropertyInfo.PropertyInfo?.GetCustomAttribute(typeof(JsonExtensionDataAttribute));


Is there a specific reason to check the Attribute?
GetCustomAttribute uses reflection which is very expensive. I was thinking that the sole condition of state.Current.ExtensionDataStatus != ExtensionDataWriteStatus.Writing was enough. did you find any issues while just depending on the condition?

Yes, it is heavy operation, but we need this check for passing test SerializationWithJsonExtensionDataAttribute_IgoneDictionaryKeyPolicy, I'm debugging this test (with extensionDataAttribute) for non-value class types, they were affected by DictionaryKeyPolicy, state.Current.ExtensionDataStatus had NotStarted value.
Without checking attribute, I've got next:

"{""KeyInt"":1000,""KeyString"":""text"",""KeyBool"":true,""keyObject"":{},""keyList"":[],""keyDictionary"":{}}"

I see the problem.
The reason why you get NotStarted is because, as of now, you are checking after one of the values of the JsonExtensionData dictionary is pushed into the stack in order to be written on the next iteration. The state of said value does not carry the parent dictionary's ExtensionDataStatus and therefore you see it as NotStarted.

To check against the proper ExtensionDataStatus try to move the condition before calling state.Push.

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

layomia · 2019-11-13T00:43:22Z

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

+
+        private class TestClassWithDictionary
+        {
+           public Dictionary<string, CustomClass> Data { get; set; }


A test where we serialize a class that has both a dictionary and extension data (both with contents) would be good:

private class TestClassWithDictionaryAndExtensionData { public Dictionary<string, CustomClass> Data { get; set; } [JsonExtensionData] public Dictionary<string, JsonElement> ExtensionData { get; set; } }

Any special reason to use JsonElement?
In order to pass JsonElement values to the ExtensionData I would need to deserialize into a JsonDocument first, thing that I can avoid by using Dictionary<string, object> instead.

layomia · 2019-11-13T02:52:53Z

Formatting issues are causing CI failures: https://github.com/dotnet/corefx/pull/41691/checks?check_run_id=300234753

ahsonkhan

Other than some test nits/suggestions, and @jozkee's feedback, looks good to me.

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

ahsonkhan · 2019-11-13T04:49:46Z

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

+                DictionaryKeyPolicy = JsonNamingPolicy.CamelCase
+            });
+
+            Assert.Equal(JsonCamel, json);


For completeness, can we add another Assert to all these tests where we use both camel case for DictionaryKeyPolicy and PropertyNamingPolicy, just to make sure there isn't some unexpected interaction causing an issue?

So:

json = JsonSerializer.Serialize(obj, new JsonSerializerOptions() { DictionaryKeyPolicy = JsonNamingPolicy.CamelCase, PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); const string JsonCamel = @"{""keyDict"":{""name"":""text"",""number"":1000,""isValid"":true,""values"":[1,2,3]}}"; Assert.Equal(JsonCamel, json);

And then consider renaming the original JsonCamel to something like JsonDictionaryCamel to differentiate between the two cases.

ahsonkhan · 2019-11-13T04:53:14Z

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs

+            {
+                { "KeyDict", CreateCustomObject() }
+            };
+            var json = JsonSerializer.Serialize(obj, new JsonSerializerOptions()


For all these tests, please add an assert for the default JsonSerializer.Serialize(...) output, without any options/naming policy.

jozkee

Please review my suggetions in order to avoid using reflection.
Other than @layomia and @ahsonkhan's feedback, tests looks good to me.

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

jozkee · 2019-11-13T19:35:37Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -83,6 +84,13 @@ public static partial class JsonSerializer

                    // An object or another enumerator requires a new stack frame.
                    state.Push(elementClassInfo, value);
+                    var extensionDataAttribute = jsonPropertyInfo.PropertyInfo?.GetCustomAttribute(typeof(JsonExtensionDataAttribute));


Suggested change

var extensionDataAttribute = jsonPropertyInfo.PropertyInfo?.GetCustomAttribute(typeof(JsonExtensionDataAttribute));

jozkee · 2019-11-13T19:35:58Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -83,6 +84,13 @@ public static partial class JsonSerializer

                    // An object or another enumerator requires a new stack frame.
                    state.Push(elementClassInfo, value);
+                    var extensionDataAttribute = jsonPropertyInfo.PropertyInfo?.GetCustomAttribute(typeof(JsonExtensionDataAttribute));
+                    if (options.DictionaryKeyPolicy != null && extensionDataAttribute == null 


Suggested change

if (options.DictionaryKeyPolicy != null && extensionDataAttribute == null

Move this condition before state.Push and remove extensionDataAttribute

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

jozkee · 2019-11-13T19:39:23Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -83,6 +84,13 @@ public static partial class JsonSerializer

                    // An object or another enumerator requires a new stack frame.
                    state.Push(elementClassInfo, value);
+                    var extensionDataAttribute = jsonPropertyInfo.PropertyInfo?.GetCustomAttribute(typeof(JsonExtensionDataAttribute));
+                    if (options.DictionaryKeyPolicy != null && extensionDataAttribute == null 
+                                                             && state.Current.ExtensionDataStatus != ExtensionDataWriteStatus.Writing)


Suggested change

&& state.Current.ExtensionDataStatus != ExtensionDataWriteStatus.Writing)

Also this condition.

jozkee · 2019-11-13T19:39:44Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -5,6 +5,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Reflection;


Suggested change

using System.Reflection;

ahsonkhan · 2019-11-13T21:40:50Z

@marcnet80 - would you be able to update this PR today before the repo moves? Would it be ok for @jozkee to push to your branch instead and move this forward so we can merge it in now rather than having to wait? It's almost ready. Thanks.

jozkee · 2019-11-14T00:06:41Z

I would like to push the requested test changes, however that is going to restart the CI, and since the repository consolidation is occuring in an hour, I am afraid that having a pending CI would prevent this PR from making it to master before consolidation happens, should I hold off said changes?
@steveharter @ahsonkhan

ahsonkhan · 2019-11-14T00:10:49Z

I am fine with having the test changes be made in a follow up PR.

jozkee · 2019-11-14T01:43:42Z

@marcnet80 great job and thank you for your contribution.

DictionaryKeyPolicy not applied to keys that contain non primitive ty…

02ee2da

…pes #41176 https://github.com/dotnet/corefx/issues/41176#event-2697607085

Dotnet-GitSync-Bot added the area-System.Text.Json label Oct 9, 2019

maryamariyan requested review from ahsonkhan and steveharter November 1, 2019 16:45

jozkee suggested changes Nov 6, 2019

View reviewed changes

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs Outdated Show resolved Hide resolved

marcnet80 requested a review from jozkee November 9, 2019 14:45

marcnet90 and others added 2 commits November 12, 2019 01:43

#41176: DictionaryKeyPolicy not applied to keys that contain non prim…

56c3f4e

…itive types https://github.com/dotnet/corefx/issues/41176

Merge branch 'master' into bug-dictionaryKeyPolicyForNonPrimitiveType…

2e3a394

…s-fix

ahsonkhan requested a review from layomia November 12, 2019 00:07

layomia reviewed Nov 12, 2019

View reviewed changes

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs Outdated Show resolved Hide resolved

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs Show resolved Hide resolved

jozkee reviewed Nov 12, 2019

View reviewed changes

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs Outdated Show resolved Hide resolved

src/System.Text.Json/tests/Serialization/DictionaryTests.KeyPolicy.cs Outdated Show resolved Hide resolved

add unit tests

ff3d86b

dotnet#41691

marcnet80 requested review from jozkee and layomia November 12, 2019 23:44

jozkee reviewed Nov 12, 2019

View reviewed changes

layomia reviewed Nov 13, 2019

View reviewed changes

ahsonkhan added this to the 5.0 milestone Nov 13, 2019

ahsonkhan approved these changes Nov 13, 2019

View reviewed changes

ahsonkhan mentioned this pull request Nov 13, 2019

Added snake case naming policy to the JSON serializer #41354

Closed

jozkee suggested changes Nov 13, 2019

View reviewed changes

Remove check for JsonExtensionDataAttribute to avoid reflection.

3f566b1

steveharter approved these changes Nov 13, 2019

View reviewed changes

ahsonkhan approved these changes Nov 14, 2019

View reviewed changes

jozkee merged commit d9dd0da into dotnet:master Nov 14, 2019

layomia added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Mar 13, 2020

layomia mentioned this pull request Mar 13, 2020

[System.Text.Json] CamelCase DictionaryKeyPolicy for array value or list value dotnet/runtime#33508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DictionaryKeyPolicy not applied to keys that contain non primitive ty… #41691

DictionaryKeyPolicy not applied to keys that contain non primitive ty… #41691

marcnet80 commented Oct 9, 2019

maryamariyan commented Nov 6, 2019 •

edited

Loading

jozkee Nov 12, 2019

marcnet80 Nov 13, 2019

jozkee Nov 13, 2019

layomia Nov 13, 2019 •

edited

Loading

jozkee Nov 13, 2019

layomia commented Nov 13, 2019

ahsonkhan left a comment

ahsonkhan Nov 13, 2019

ahsonkhan Nov 13, 2019

ahsonkhan Nov 13, 2019

jozkee left a comment

jozkee Nov 13, 2019

jozkee Nov 13, 2019

jozkee Nov 13, 2019

jozkee Nov 13, 2019

ahsonkhan commented Nov 13, 2019

jozkee commented Nov 14, 2019

ahsonkhan commented Nov 14, 2019

jozkee commented Nov 14, 2019

DictionaryKeyPolicy not applied to keys that contain non primitive ty… #41691

DictionaryKeyPolicy not applied to keys that contain non primitive ty… #41691

Conversation

marcnet80 commented Oct 9, 2019

maryamariyan commented Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia commented Nov 13, 2019

ahsonkhan 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

jozkee 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

ahsonkhan commented Nov 13, 2019

jozkee commented Nov 14, 2019

ahsonkhan commented Nov 14, 2019

jozkee commented Nov 14, 2019

maryamariyan commented Nov 6, 2019 •

edited

Loading

layomia Nov 13, 2019 •

edited

Loading