-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add snake_case support for System.Text.Json #42009
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.Text.Json | ||
{ | ||
internal class JsonKebabCaseNamingPolicy : JsonSeparatedCaseNamingPolicy | ||
{ | ||
public override string ConvertName(string name) => ToSeparatedCase(name, '-'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,15 @@ protected JsonNamingPolicy() { } | |
|
||
internal static JsonNamingPolicy Default { get; } = new JsonDefaultNamingPolicy(); | ||
|
||
/// <summary> | ||
/// Returns the naming policy for snake-casing. | ||
/// </summary> | ||
public static JsonNamingPolicy SnakeCase { get; } = new JsonSnakeCaseNamingPolicy(); | ||
|
||
// KebabCase was added together with SnakeCase because sharing implementation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we don't add code in case it might be exposed in future. We would add the implementation when the API is approved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// Once approved it can be made public. | ||
internal static JsonNamingPolicy KebabCase { get; } = new JsonKebabCaseNamingPolicy(); | ||
|
||
/// <summary> | ||
/// When overridden in a derived class, converts the specified name according to the policy. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,82 @@ | ||||||||||
// Licensed to the .NET Foundation under one or more agreements. | ||||||||||
// The .NET Foundation licenses this file to you under the MIT license. | ||||||||||
|
||||||||||
namespace System.Text.Json | ||||||||||
{ | ||||||||||
internal abstract class JsonSeparatedCaseNamingPolicy : JsonNamingPolicy | ||||||||||
{ | ||||||||||
private enum SeparatedCaseState | ||||||||||
{ | ||||||||||
Start, | ||||||||||
NewWord, | ||||||||||
Upper, | ||||||||||
Lower | ||||||||||
} | ||||||||||
|
||||||||||
protected static string ToSeparatedCase(string s, char separator) | ||||||||||
{ | ||||||||||
if (string.IsNullOrEmpty(s)) | ||||||||||
{ | ||||||||||
return s; | ||||||||||
} | ||||||||||
|
||||||||||
StringBuilder sb = new StringBuilder(); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a hot path? If so should it use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied it as-is from Json.NET as a first step. I guess that naming policy only runs once during class initialization? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder what can be used here to reduce the allocation. Unlike camel case, the output length of snake case cannot be predicted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously the team was against FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; |
||||||||||
SeparatedCaseState state = SeparatedCaseState.Start; | ||||||||||
|
||||||||||
for (int i = 0; i < s.Length; i++) | ||||||||||
{ | ||||||||||
if (s[i] == ' ') | ||||||||||
{ | ||||||||||
if (state != SeparatedCaseState.Start) | ||||||||||
{ | ||||||||||
state = SeparatedCaseState.NewWord; | ||||||||||
} | ||||||||||
} | ||||||||||
else if (char.IsUpper(s[i])) | ||||||||||
{ | ||||||||||
switch (state) | ||||||||||
{ | ||||||||||
case SeparatedCaseState.Upper: | ||||||||||
bool hasNext = (i + 1 < s.Length); | ||||||||||
if (i > 0 && hasNext) | ||||||||||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
{ | ||||||||||
char nextChar = s[i + 1]; | ||||||||||
if (!char.IsUpper(nextChar) && nextChar != separator) | ||||||||||
{ | ||||||||||
sb.Append(separator); | ||||||||||
} | ||||||||||
} | ||||||||||
break; | ||||||||||
case SeparatedCaseState.Lower: | ||||||||||
case SeparatedCaseState.NewWord: | ||||||||||
sb.Append(separator); | ||||||||||
break; | ||||||||||
} | ||||||||||
|
||||||||||
char c; | ||||||||||
c = char.ToLowerInvariant(s[i]); | ||||||||||
sb.Append(c); | ||||||||||
Comment on lines
+56
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
state = SeparatedCaseState.Upper; | ||||||||||
} | ||||||||||
else if (s[i] == separator) | ||||||||||
{ | ||||||||||
sb.Append(separator); | ||||||||||
state = SeparatedCaseState.Start; | ||||||||||
} | ||||||||||
else | ||||||||||
{ | ||||||||||
if (state == SeparatedCaseState.NewWord) | ||||||||||
{ | ||||||||||
sb.Append(separator); | ||||||||||
} | ||||||||||
|
||||||||||
sb.Append(s[i]); | ||||||||||
state = SeparatedCaseState.Lower; | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
return sb.ToString(); | ||||||||||
} | ||||||||||
} | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.Text.Json | ||
{ | ||
internal class JsonSnakeCaseNamingPolicy : JsonSeparatedCaseNamingPolicy | ||
{ | ||
public override string ConvertName(string name) => ToSeparatedCase(name, '_'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using Xunit; | ||
|
||
namespace System.Text.Json.Serialization.Tests | ||
{ | ||
public class SnakeCaseUnitTests | ||
{ | ||
[Fact] | ||
public static void ToCamelCaseTest() | ||
{ | ||
// These test cases were copied from Json.NET. | ||
Assert.Equal("url_value", ConvertToSnakeCase("URLValue")); | ||
Assert.Equal("url", ConvertToSnakeCase("URL")); | ||
Assert.Equal("id", ConvertToSnakeCase("ID")); | ||
Assert.Equal("i", ConvertToSnakeCase("I")); | ||
Assert.Equal("", ConvertToSnakeCase("")); | ||
Assert.Null(ConvertToSnakeCase(null)); | ||
Assert.Equal("person", ConvertToSnakeCase("Person")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase("iPhone")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase("IPhone")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase("I Phone")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase("I Phone")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase(" IPhone")); | ||
Assert.Equal("i_phone", ConvertToSnakeCase(" IPhone ")); | ||
Assert.Equal("is_cia", ConvertToSnakeCase("IsCIA")); | ||
Assert.Equal("vm_q", ConvertToSnakeCase("VmQ")); | ||
Assert.Equal("xml2_json", ConvertToSnakeCase("Xml2Json")); | ||
Assert.Equal("sn_ak_ec_as_e", ConvertToSnakeCase("SnAkEcAsE")); | ||
Assert.Equal("sn_a__k_ec_as_e", ConvertToSnakeCase("SnA__kEcAsE")); | ||
Assert.Equal("sn_a__k_ec_as_e", ConvertToSnakeCase("SnA__ kEcAsE")); | ||
Assert.Equal("already_snake_case_", ConvertToSnakeCase("already_snake_case_ ")); | ||
Assert.Equal("is_json_property", ConvertToSnakeCase("IsJSONProperty")); | ||
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.")); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about all whitespace input? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see additional test cases and implementation from an initial PR from @YohDeadfall which was extensively reviewed - dotnet/corefx#41354.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
||
// Use a helper method since the method is not public. | ||
private static string ConvertToSnakeCase(string name) | ||
{ | ||
JsonNamingPolicy policy = JsonNamingPolicy.SnakeCase; | ||
string value = policy.ConvertName(name); | ||
return value; | ||
} | ||
} | ||
} |
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.
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 useJsonNamingPolicy.SnakeCaseNewtonsoftCompat
or something similar to make that clear? I realize this passed API review, but we should revisit to ensure.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.
@layomia I'd require an api design/review from you.