Add snake_case support for System.Text.Json #42009

huoyaoyuan · 2020-09-09T10:10:59Z

Closes #782
Implementation and tests are both copied from Json.NET

Dotnet-GitSync-Bot · 2020-09-09T10:11:02Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

danmoseley · 2020-09-10T00:56:44Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNamingPolicy.cs

+        /// </summary>
+        public static JsonNamingPolicy SnakeCase { get; } = new JsonSnakeCaseNamingPolicy();
+
+        // KebabCase was added together with SnakeCase because sharing implementation.


Generally we don't add code in case it might be exposed in future. We would add the implementation when the API is approved.

Please create a new API proposal for kebab case if desired: https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.md.

danmoseley · 2020-09-10T01:00:14Z

...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSeparatedCaseNamingPolicy.cs

+                return s;
+            }
+
+            StringBuilder sb = new StringBuilder();


Is this a hot path? If so should it use ValueStringBuilder?

I copied it as-is from Json.NET as a first step. I guess that naming policy only runs once during class initialization?

Naming policies are also used when serializing dictionary keys and the results are not cached (TBD whether it makes sense to). We want to do the most performant thing here.

I wonder what can be used here to reduce the allocation. Unlike camel case, the output length of snake case cannot be predicted.
ValueStringBuilder? But we are not in corelib here.
Just maintain a stackalloc buffer here?

Previously the team was against ValueStringBuilder since it makes libraries to bloat, but this is one of cleanest ways to improve performance. Another one is ArrayPool<char>, but it means that you should repeat whole logic of ValueStringBuilder which isn't a great idea.

FYI StringBuilder has a thread local instance which reused for .NET internals and as I can remember even for string formatting.

Worst-case, isn't the required length appx s.Length * 2 (i.e. a separator between every char)? If so, this can be done simply with:

char[] arr = ArrayPool<char>.Shared.Rent(s.Length * 2 - 1); // worst-case length for a separator between every char int pos = 0; ... arr[pos++] = ...; // every time a char is added ... string result = new string(arr, 0, pos); ArrayPool<char>.Shared.Return(arr); return result;

danmoseley · 2020-09-10T01:00:57Z

src/libraries/System.Text.Json/tests/Serialization/SnakeCaseUnitTests.cs

+            Assert.Equal("shouting_case", ConvertToSnakeCase("SHOUTING_CASE"));
+            Assert.Equal("9999-12-31_t23:59:59.9999999_z", ConvertToSnakeCase("9999-12-31T23:59:59.9999999Z"));
+            Assert.Equal("hi!!_this_is_text._time_to_test.", ConvertToSnakeCase("Hi!! This is text. Time to test."));
+        }


what about all whitespace input?

Json.NET didn't test it. I don't want to compose new test myself, because there are no well-defined correct result for corner cases.

Please see additional test cases and implementation from an initial PR from @YohDeadfall which was extensively reviewed - dotnet/corefx#41354.

there are no well-defined correct result for corner cases.

As much as possible, we need to determine the correct behavior for edge cases. A good first step here is to add tests for as many edge scenarios that we can think of, including all whitespace input as mentioned.

@huoyaoyuan, I suggest you to wait a little bit more and concentrate on other things you're working on. There're a lot of corner cases, but no idea how the API should work. We discovered how external and existing libraries work, but no decision was made, unfortunately. This way you won't change the code after each comment here and would be able to focus on things which could be made right now.

layomia

Thanks for the PR. With this feature, there are a few design decisions to be fleshed out. See dotnet/corefx#41354 (comment). As we're currently wrapping up 5.0, it will probably be a couple weeks before I can take a deep dive here, but feel free to address them in the mean time.

The existing camel-case policy implementation might also need to be updated to use the same splitting mechanism as snake-case so we have a standard way across all built-in policies.

FYI @YohDeadfall

layomia · 2020-09-13T21:44:51Z

...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSeparatedCaseNamingPolicy.cs

+                return s;
+            }
+
+            StringBuilder sb = new StringBuilder();


Naming policies are also used when serializing dictionary keys and the results are not cached (TBD whether it makes sense to). We want to do the most performant thing here.

layomia · 2020-09-13T21:46:07Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNamingPolicy.cs

+        /// </summary>
+        public static JsonNamingPolicy SnakeCase { get; } = new JsonSnakeCaseNamingPolicy();
+
+        // KebabCase was added together with SnakeCase because sharing implementation.


Please create a new API proposal for kebab case if desired: https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.md.

layomia · 2020-09-13T21:49:26Z

src/libraries/System.Text.Json/tests/Serialization/SnakeCaseUnitTests.cs

+            Assert.Equal("shouting_case", ConvertToSnakeCase("SHOUTING_CASE"));
+            Assert.Equal("9999-12-31_t23:59:59.9999999_z", ConvertToSnakeCase("9999-12-31T23:59:59.9999999Z"));
+            Assert.Equal("hi!!_this_is_text._time_to_test.", ConvertToSnakeCase("Hi!! This is text. Time to test."));
+        }


Please see additional test cases and implementation from an initial PR from @YohDeadfall which was extensively reviewed - dotnet/corefx#41354.

there are no well-defined correct result for corner cases.

As much as possible, we need to determine the correct behavior for edge cases. A good first step here is to add tests for as many edge scenarios that we can think of, including all whitespace input as mentioned.

YohDeadfall · 2020-09-14T05:50:48Z

@layomia, the problem here is the same as with mine PR. There is no agreement on compatibility issues with external translators. Otherwise, my PR was ready for merge for a long time ago.

YohDeadfall · 2020-09-14T06:04:06Z

Here is a list of open questions we had before and other information which characters should be omitted and which should stay in the resulting string: dotnet/corefx#41354 (comment)

The whole research was did to not only add the snake case policy, but others too. Also we found that the current camel case policy doesn't work the same way as external.

If you're okay with what I already did I can to resurrect the PR in that repo. Otherwise, we should get consensus first, better in the linked issue, but has no tools to copy all comments from dotnet/corefx#41354.

/cc @stephentoub @jkotas @khellang

stephentoub · 2020-09-14T14:47:16Z

...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSeparatedCaseNamingPolicy.cs

+                    char c;
+                    c = char.ToLowerInvariant(s[i]);
+                    sb.Append(c);


Suggested change

char c;

c = char.ToLowerInvariant(s[i]);

sb.Append(c);

sb.Append(char.ToLowerInvariant(s[i]));

stephentoub · 2020-09-14T14:48:15Z

...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSeparatedCaseNamingPolicy.cs

+                            bool hasNext = (i + 1 < s.Length);
+                            if (i > 0 && hasNext)


Suggested change

bool hasNext = (i + 1 < s.Length);

if (i > 0 && hasNext)

if (i > 0 && i + 1 < s.Length)

steveharter · 2020-09-14T19:25:13Z

From @YohDeadfall:

@huoyaoyuan, I suggest you to wait a little bit more and concentrate on other things you're working on. There're a lot of corner cases, but no idea how the API should work. We discovered how external and existing libraries work, but no decision was made, unfortunately. This way you won't change the code after each comment here and would be able to focus on things which could be made right now.

Yes there are several implementations for how snake case should work, and no standard. The previous attempt from @YohDeadfall at dotnet/corefx#41354 never made it in due to those concerns.

Assuming this PR has the exact same behavior as Newtonsoft (I didn't verify), even with bugs, then the approach aligns to what we did when we added camel case (which was based on Newtonsoft 100%). Even with the built-in implementation matching Newtonsoft, it would still possible in a one-off manner to add a custom naming policy at runtime (as it is possible today) to match Node or Python semantics.

Instead of JsonNamingPolicy.SnakeCase, we could use JsonNamingPolicy.SnakeCaseNewtonsoftCompat or something similar to make that clear.

Once a naming policy is added, any change is breaking since previously serialized JSON may not be able to be deserialized.

steveharter · 2020-09-14T19:29:06Z

src/libraries/System.Text.Json/ref/System.Text.Json.cs

@@ -154,6 +154,7 @@ public abstract partial class JsonNamingPolicy
    {
        protected JsonNamingPolicy() { }
        public static System.Text.Json.JsonNamingPolicy CamelCase { get { throw null; } }
+        public static System.Text.Json.JsonNamingPolicy SnakeCase { get { throw null; } }


Due to the many implementations of snake case, do we want to make it clear what algorithm we're using?

For example, instead of JsonNamingPolicy.SnakeCase, we could use JsonNamingPolicy.SnakeCaseNewtonsoftCompat or something similar to make that clear? I realize this passed API review, but we should revisit to ensure.

@layomia I'd require an api design/review from you.

ericstj · 2021-01-22T18:20:14Z

@layomia it looks like the open question right now is around the naming of the Public API describing this policy. Perhaps we can review that, share thoughts, then bring it to API review for approval?

YohDeadfall · 2021-01-22T18:24:10Z

@ericstj, it would be much better if someone find some time to address questions first in dotnet/corefx#41354 instead of approving/reviewing this. Otherwise, there will be another mindless change which cannot be reverted or changed due to backward compatibility.

@layomia, please, find take a loot at the issue I mentioned. I really would like to implement it properly and update the translator in Npgsql too.

ericstj · 2021-01-22T18:30:52Z

I see, apologies @YohDeadfall I missed that there were still some design issues to resolve. Agreed that we need to close on those. Is dotnet/corefx#41354 (comment) the right thing to look at?

YohDeadfall · 2021-01-22T18:35:58Z

Right, @ericstj. It contains unresolved questions and a research in this area. When all of them get answered, it will be possible to implement a lot of policies including lower and upper cases and so on (take a look at JS translators mentioned in the same PR).

layomia · 2021-03-10T02:57:13Z

This feature needs more research, but isn't being actively worked on right now. It remains important, but we have to circle back a little later in the .NET 6 release. I'll close it for now and ping this thread later on.

huoyaoyuan added 3 commits September 9, 2020 17:24

Copy snake case implementation from Json.NET.

869fa14

Also add KebabCase but keep internal.

02979c9

Add tests for snake case.

cf9a7ee

huoyaoyuan requested review from jozkee, layomia and steveharter as code owners September 9, 2020 10:10

Dotnet-GitSync-Bot added area-System.Text.Json new-api-needs-documentation labels Sep 9, 2020

danmoseley reviewed Sep 10, 2020

View reviewed changes

layomia reviewed Sep 13, 2020

View reviewed changes

stephentoub reviewed Sep 14, 2020

View reviewed changes

steveharter reviewed Sep 14, 2020

View reviewed changes

huoyaoyuan mentioned this pull request Oct 5, 2020

Don't trigger naming style violation for prefixed numbers. dotnet/roslyn#48306

Merged

danmoseley assigned layomia Dec 17, 2020

ahsonkhan mentioned this pull request Jan 22, 2021

System.Text.Json support to System.Runtime.Serialization #29975

Closed

Base automatically changed from master to main March 1, 2021 09:06

ViktorHofer requested a review from eiriktsarpalis as a code owner March 1, 2021 09:06

layomia closed this Mar 10, 2021

ghost locked as resolved and limited conversation to collaborators Apr 9, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

huoyaoyuan deleted the snake-case branch March 4, 2023 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snake_case support for System.Text.Json #42009

Add snake_case support for System.Text.Json #42009

huoyaoyuan commented Sep 9, 2020

Dotnet-GitSync-Bot commented Sep 9, 2020

danmoseley Sep 10, 2020

layomia Sep 13, 2020

danmoseley Sep 10, 2020

huoyaoyuan Sep 10, 2020

layomia Sep 13, 2020

huoyaoyuan Sep 14, 2020

YohDeadfall Sep 14, 2020

stephentoub Sep 14, 2020

danmoseley Sep 10, 2020

huoyaoyuan Sep 10, 2020

layomia Sep 13, 2020

YohDeadfall Sep 14, 2020

layomia left a comment

layomia Sep 13, 2020

layomia Sep 13, 2020

layomia Sep 13, 2020

YohDeadfall commented Sep 14, 2020

YohDeadfall commented Sep 14, 2020

stephentoub Sep 14, 2020

stephentoub Sep 14, 2020

steveharter commented Sep 14, 2020 •

edited

Loading

steveharter Sep 14, 2020

huoyaoyuan Nov 13, 2020

ericstj commented Jan 22, 2021

YohDeadfall commented Jan 22, 2021

ericstj commented Jan 22, 2021

YohDeadfall commented Jan 22, 2021

layomia commented Mar 10, 2021

	bool hasNext = (i + 1 < s.Length);
	if (i > 0 && hasNext)
	if (i > 0 && i + 1 < s.Length)

Add snake_case support for System.Text.Json #42009

Add snake_case support for System.Text.Json #42009

Conversation

huoyaoyuan commented Sep 9, 2020

Dotnet-GitSync-Bot commented Sep 9, 2020

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

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 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

YohDeadfall commented Sep 14, 2020

YohDeadfall commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter commented Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Jan 22, 2021

YohDeadfall commented Jan 22, 2021

ericstj commented Jan 22, 2021

YohDeadfall commented Jan 22, 2021

layomia commented Mar 10, 2021

steveharter commented Sep 14, 2020 •

edited

Loading