Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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; } }
Copy link
Member

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 use JsonNamingPolicy.SnakeCaseNewtonsoftCompat or something similar to make that clear? I realize this passed API review, but we should revisit to ensure.

Copy link
Member Author

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.

public abstract string ConvertName(string name);
}
public readonly partial struct JsonProperty
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,15 @@
<Compile Include="System\Text\Json\Serialization\JsonConverterOfT.cs" />
<Compile Include="System\Text\Json\Serialization\JsonDefaultNamingPolicy.cs" />
<Compile Include="System\Text\Json\Serialization\JsonIgnoreCondition.cs" />
<Compile Include="System\Text\Json\Serialization\JsonKebabCaseNamingPolicy.cs" />
<Compile Include="System\Text\Json\Serialization\JsonNamingPolicy.cs" />
<Compile Include="System\Text\Json\Serialization\JsonNumberHandling.cs" />
<Compile Include="System\Text\Json\Serialization\JsonParameterInfo.cs" />
<Compile Include="System\Text\Json\Serialization\JsonParameterInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\JsonPropertyInfo.cs" />
<Compile Include="System\Text\Json\Serialization\JsonPropertyInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\JsonResumableConverterOfT.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSeparatedCaseNamingPolicy.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializer.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.HandleMetadata.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.HandlePropertyName.cs" />
Expand All @@ -161,6 +163,7 @@
<Compile Include="System\Text\Json\Serialization\JsonSerializerDefaults.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializerOptions.Converters.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializerOptions.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSnakeCaseNamingPolicy.cs" />
<Compile Include="System\Text\Json\Serialization\JsonStringEnumConverter.cs" />
<Compile Include="System\Text\Json\Serialization\MemberAccessor.cs" />
<Compile Include="System\Text\Json\Serialization\MetadataPropertyName.cs" />
Expand Down
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
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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>
Expand Down
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
ValueStringBuilder? But we are not in corelib here.
Just maintain a stackalloc buffer here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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."));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about all whitespace input?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
<Compile Include="Serialization\ReferenceHandlerTests.Serialize.cs" />
<Compile Include="Serialization\SampleTestData.OrderPayload.cs" />
<Compile Include="Serialization\SerializationWrapper.cs" />
<Compile Include="Serialization\SnakeCaseUnitTests.cs" />
<Compile Include="Serialization\SpanTests.cs" />
<Compile Include="Serialization\Stream.Collections.cs" />
<Compile Include="Serialization\Stream.ReadTests.cs" />
Expand Down