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

Closed
Tracked by #71967 ...
hez2010 opened this issue Jul 17, 2019 · 88 comments · Fixed by #69613
Closed
Tracked by #71967 ...

Add snake_case support for System.Text.Json #782

hez2010 opened this issue Jul 17, 2019 · 88 comments · Fixed by #69613
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors Priority:3 Work that is nice to have Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@hez2010
Copy link
Contributor

hez2010 commented Jul 17, 2019

Contents below were taken from @khellang, thanks!

API Proposal

Add additional casing support for System.Text.Json, based on this comment.

namespace System.Text.Json;

public partial class JsonNamingPolicy
{
    public static JsonNamingPolicy CamelCase { get; }
+    public static JsonNamingPolicy SnakeLowerCase { get; }
+    public static JsonNamingPolicy SnakeUpperCase { get; }
+    public static JsonNamingPolicy KebabLowerCase { get; }
+    public static JsonNamingPolicy KebabUpperCase { get; }
}

public enum JsonKnownNamingPolicy
{
    Unspecified = 0,
    CamelCase = 1,
+    SnakeLowerCase = 2,
+    SnakeUpperCase = 3,
+    KebabLowerCase = 4,
+    KebabUpperCase = 5,
}

Behavior Proposal

I propose the same behavior as Newtonsoft.Json, just like the existing camel case behavior. The implementation is here and the tests are here.

Other Comments

I think snake case naming is pretty common, especially in the Ruby world, and should be supported out of the box. GitHub's API is probably the most popular example using this naming convention off the top of my head.

Implements

See: dotnet/corefx#40003

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 4, 2019

If you have any designs in mind, could you update the top post with what the proposed APIs would look like / how commonly used snake case is / etc? Here's what I think is a good example: https://github.com/dotnet/corefx/issues/28933#issue-312415055. This should help people better understand the problem/importance of this issue, and may come up with improvements.

@khellang
Copy link
Member

khellang commented Aug 5, 2019

API Proposal

namespace System.Text.Json
{
    public abstract class JsonNamingPolicy
    {
        protected JsonNamingPolicy() { }
        public static JsonNamingPolicy CamelCase { get { throw null; } }
+       public static JsonNamingPolicy SnakeCase { get { throw null; } }
        internal static JsonNamingPolicy Default { get { throw null; } }
        public abstract string ConvertName(string name);
    }

+   internal sealed class JsonSnakeCaseNamingPolicy : JsonNamingPolicy
+   {
+       public override string ConvertName(string name) { throw null; }
+   }
}

Behavior Proposal

I propose the same behavior as Newtonsoft.Json, just like the existing camel case behavior. The implementation is here and the tests are here.

Other Comments

I think snake case naming is pretty common, especially in the Ruby world, and should be supported out of the box. GitHub's API is probably the most popular example using this naming convention off the top of my head.

UPDATE: Looks like dotnet/corefx#40003 implemented more or less this exact proposal.

@hez2010
Copy link
Contributor Author

hez2010 commented Aug 5, 2019

@khellang I've pasted your proposal contents to the top post, thanks!

@xsoheilalizadeh
Copy link

I've prepared a benchmark that compares other implementation with dotnet/corefx#40003 PR.

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.18362
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview8-013656
  [Host]     : .NET Core 3.0.0-preview8-28405-07 (CoreCLR 4.700.19.37902, CoreFX 4.700.19.40503), 64bit RyuJIT
  DefaultJob : .NET Core 3.0.0-preview8-28405-07 (CoreCLR 4.700.19.37902, CoreFX 4.700.19.40503), 64bit RyuJIT

|                          Method |        Mean |      Error |     StdDev | Rank |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------- |------------:|-----------:|-----------:|-----:|-------:|------:|------:|----------:|
|  ToSnakeCaseStringBuilderBySpan |    85.97 ns |  1.0500 ns |  0.9821 ns |    1 | 0.0637 |     - |     - |     200 B |
| ToSnakeCaseNewtonsoftJsonBySpan |    86.35 ns |  1.7816 ns |  1.4877 ns |    1 | 0.0484 |     - |     - |     152 B |
|       ToSnakeCaseNewtonsoftJson |    88.89 ns |  0.8417 ns |  0.7461 ns |    2 | 0.0484 |     - |     - |     152 B |
|               ToSnakeCaseBySpan |   122.01 ns |  1.9066 ns |  1.6902 ns |    3 | 0.0253 |     - |     - |      80 B |
|                 ToSnakeCaseLinq |   365.75 ns |  4.4839 ns |  3.5008 ns |    4 | 0.1450 |     - |     - |     456 B |
|                ToSnakeCaseRegex | 2,108.71 ns | 40.6829 ns | 31.7625 ns |    5 | 

@xsoheilalizadeh
Copy link

I investigated a couple of weeks about this case and tried other approaches + dotnet/corefx#40003 that have better performance to having snake case naming policy. Finally, I got the best result and it have less allocation and fast execution. My last implementation is ToSnakeCaseBySpan . Shoud I open a new PR for this?

|                          Method |        Mean |      Error |     StdDev | Rank |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------- |------------:|-----------:|-----------:|-----:|-------:|------:|------:|----------:|
|               ToSnakeCaseBySpan |    27.47 ns |  0.4629 ns |  0.4330 ns |    1 | 0.0153 |     - |     - |      48 B |
|  ToSnakeCaseStringBuilderBySpan |    85.23 ns |  1.6495 ns |  1.3774 ns |    2 | 0.0637 |     - |     - |     200 B |
|       ToSnakeCaseNewtonsoftJson |    85.72 ns |  1.6418 ns |  1.4554 ns |    2 | 0.0484 |     - |     - |     152 B |
| ToSnakeCaseNewtonsoftJsonBySpan |    86.96 ns |  1.7060 ns |  1.5958 ns |    2 | 0.0484 |     - |     - |     152 B |
|                 ToSnakeCaseLinq |   353.42 ns |  3.9670 ns |  3.7108 ns |    3 | 0.1450 |     - |     - |     456 B |
|                ToSnakeCaseRegex | 2,056.69 ns | 29.5694 ns | 26.2125 ns |    4 | 0.1526 |     - |     - |     496 B |

\cc @karelz

@hez2010
Copy link
Contributor Author

hez2010 commented Sep 13, 2019

@xsoheilalizadeh I think the name of a property is usually not too long, so maybe you can use stackalloc to reduce memory allocation on heap to reduce GC pressure? (It may achieve zero allocation)

public unsafe ReadOnlySpan<char> ToSnakeCaseBySpan()
{
    ...
    Span<char> buffer = stackalloc char[bufferSize];
    ...
}

@xsoheilalizadeh
Copy link

Nice suggestion, but here is a question how I should return buffer, I've got following:

Cannot use local 'buffer' in this context because it may expose referenced variables outside of their declaration scope

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Sep 13, 2019

Should I open a new PR for this?

Nope, not yet 😅 not until the API is approved.

@hez2010
Copy link
Contributor Author

hez2010 commented Sep 13, 2019 via email

@karelz
Copy link
Member

karelz commented Sep 13, 2019

@ahsonkhan are we ready to review the API? @xsoheilalizadeh is eager to implement it ...

@xsoheilalizadeh
Copy link

I've figured out that the ToSnakeCaseBySpan can't pass these tests, it's just compatible with pascal case properties, I've tried to make it compatible with other formats but unfortunately, I've failed and it'd be unreadable. I was hurrying to contribute and got a bad result, sorry.

@ahsonkhan
Copy link
Member

ahsonkhan commented Sep 16, 2019

This looks like a reasonable API addition. I was considering how common such formats are to warrant adding support built-in (rather than folks creating their own naming policy to support it), and looks like quite a few places use snake_case including Facebook, AWS, and Twitter APIs (aside from the fact that Json.NET supports it):
https://stackoverflow.com/questions/5543490/json-naming-convention

And of course, what @khellang said:

I think snake case naming is pretty common, especially in the Ruby world, and should be supported out of the box. GitHub's API is probably the most popular example using this naming convention off the top of my head.

cc @steveharter

@terrajobst
Copy link
Member

terrajobst commented Sep 17, 2019

Video

Makes sense. We also considered others (such as kebap casing) but it seems we should defer that until someone actually needs that.

@Gnbrkm41
Copy link
Contributor

@xsoheilalizadeh, are you still working on it? :D you should be able to open PRs for it.

@xsoheilalizadeh
Copy link

Unfortunately no till I pass tests, I tried to pass tests but my implementation only support pascal-case naming for example FirstName will be first_name but in other cases like IsCIA it will be Is_c_i_a that isn't expected.

@hez2010
Copy link
Contributor Author

hez2010 commented Sep 22, 2019

@Gnbrkm41 How about the existing PR dotnet/corefx#40003

@Gnbrkm41
Copy link
Contributor

I mean... sure, why not, as long as it has all the tests and it passes :D

@YohDeadfall
Copy link
Contributor

@ahsonkhan Did the same thing already, so taking this.

@VassilAtanasov
Copy link

Makes sense. We also considered others (such as kebap casing) but it seems we should defer that until someone actually needs that.

The lack of PascalCase option is a show stopper for our migration.
I believe there should be a "preserve case" option which will also work for us.

@khellang
Copy link
Member

The lack of PascalCase option is a show stopper for our migration.
I believe there should be a "preserve case" option which will also work for us.

Isn't that the default? 🤔

https://github.com/dotnet/corefx/blob/296c0e71ddcc4885d30af18dea829c1923cfdc5c/src/System.Text.Json/src/System/Text/Json/Serialization/JsonDefaultNamingPolicy.cs#L9

@VassilAtanasov
Copy link

VassilAtanasov commented Sep 27, 2019

The lack of PascalCase option is a show stopper for our migration.
I believe there should be a "preserve case" option which will also work for us.

Isn't that the default? 🤔

https://github.com/dotnet/corefx/blob/296c0e71ddcc4885d30af18dea829c1923cfdc5c/src/System.Text.Json/src/System/Text/Json/Serialization/JsonDefaultNamingPolicy.cs#L9

Yes - that worked ... but not easily!
You can not select the Default !!!
JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.Default
and also PropertyNamingPolicy gets initialised in constructor to JsonCamelCaseNamingPolicy
new JsonOptions().JsonSerializerOptions.PropertyNamingPolicy

However you can trick it with :-)
JsonSerializerOptions.PropertyNamingPolicy = null;
and PascalCase goes through unobstructed!

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Oct 1, 2019

the default is to use exact match IIRC. setting it to null also does that as well.

@niemyjski
Copy link

I'd prefer if we could just define our own naming strategies. We use our own naming method for snake_case because it occurred before Json.net came out with it and it's slightly different. So perhaps just allow us to set a naming policy or use the default one.

@khellang
Copy link
Member

khellang commented Oct 4, 2019

@niemyjski Of course you can. Just implement JsonNamingPolicy like the built-in ones and set that. Almost everything about the new serializer is configurable and/or extensible, these issues are usually just about including more stuff in the box.

@sveinfid
Copy link

Please could KebabCase also be added?

It should be a very minor addition - basically the same as SnakeCase but with - in place of _

E.g. requests sent to Apple Push Notification Service require kebab-case JSON fields: https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification#2943360

@ahsonkhan
Copy link
Member

@sveinfid - There was precedence for snake casing (e.g. in newtonsoft json) and enough user need to provide built-in support for it. I am not sure that's the case for KebabCase. Please file a separate issue (with the API proposal) and we can consider it, but my hunch is, this is something that the caller would have to implement (like any other custom casing/naming strategy) and doesn't seem common enough to require built-in support.

Like you said, the impl could be relatively straight forward (take existing snake case impl and replace the delimiter), which means there is also little value in providing one built-in.

@layomia
Copy link
Contributor

layomia commented Dec 12, 2019

From @YohDeadfall in #673:

Originally proposed by @hez2010 in dotnet/corefx#39564.

@ahsonkhan, could you add the original issue here and important comments from dotnet/corefx#41354?

@karelz We need your opinion here, since @khellang asked a question about altering the existing behavior of the camel case policy too follow common translator rules for the JS world as splitting a string by words and concatenating it back using some separator and applying a policy to change casing for each word. It's a breaking change and @stephentoub likes it. Me too. See dotnet/corefx#41354 (comment).

I still would like to drive this feature.

@steveharter @layomia

@layomia layomia transferred this issue from dotnet/corefx Dec 12, 2019
@terrajobst
Copy link
Member

terrajobst commented May 20, 2022

@dotnet/area-system-text-json based on the comments from @teo-tsirpanis and the latest change request from @YohDeadfall I have pushed the API back to api-ready-for-review.

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels May 20, 2022
@teo-tsirpanis
Copy link
Contributor

Any particular questions you're trying to answer?

My question was what was the approved API shape was Well, forget it, I found #782 (comment) (😳), but it might still be useful to re-review it for the additional casings.

JsonKnownNamingPolicy might also need to be updated.

@eiriktsarpalis eiriktsarpalis self-assigned this May 21, 2022
@terrajobst
Copy link
Member

terrajobst commented May 23, 2022

Well, forget it, I found #782 (comment) (😳),

Ah yes, that's why didn't find the notes -- we approved it but didn't apply the label. Oops. But yeah, we'll take another quick look tomorrow.

@OnamChilwan
Copy link

Is there any update on this, when can we expect this to be added? I am targeting .NET 6 and can only see CamelCase as an option..

@YohDeadfall
Copy link
Contributor

@OnamChilwan, you can currently use a package mentioned in #782 (comment). The pull request I made uses it's code with style changes only.

@terrajobst, till what date pull request can be merged and appear in .NET 7? If it's too close and no time left for an approval of public API changes we can go with option 2. It allows to trim the PR only to SnakeCase and add other options later, but with Shouting (less preferred naming in my opinion).

@J0rgeSerran0
Copy link

@OnamChilwan you can use this code too
I did 3 years ago, is in some threads about this issue, and I shared it for the people that is looking for a solution with this

https://github.com/J0rgeSerran0/JsonNamingPolicy

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 6, 2022
@jeffhandley jeffhandley modified the milestones: Future, 8.0.0 Jul 6, 2022
radical pushed a commit to radical/runtime that referenced this issue Jul 7, 2022
@wardboumans
Copy link

Incredible this is still not in .NET7. This is why people still stick to Newtonsoft.

@GiorgioG
Copy link

GiorgioG commented Sep 1, 2022

This is pathetic. I can't advocate for System.Text.Json anymore if it's going to remain a worse option than Newtonsoft.

@georgiosd
Copy link

Really @wardboumans @GiorgioG? Using a custom naming policy that already exists out there is that despicable that you'd drop the library? Wow.

@bartonjs
Copy link
Member

bartonjs commented Sep 20, 2022

Video

  • We discussed some other options, like PascalCase (forcing a field or non-conforming name from camelCase to PascalCase)
  • We felt that SnakeCaseUpper and friends were better than SnakeUpperCase and friends.
namespace System.Text.Json;

public partial class JsonNamingPolicy
{
    public static JsonNamingPolicy SnakeCaseLower { get; }
    public static JsonNamingPolicy SnakeCaseUpper { get; }
    public static JsonNamingPolicy KebabCaseLower { get; }
    public static JsonNamingPolicy KebabCaseUpper { get; }
}

public enum JsonKnownNamingPolicy
{
    SnakeCaseLower = 2,
    SnakeCaseUpper = 3,
    KebabCaseLower = 4,
    KebabCaseUpper = 5,
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 20, 2022
@YohDeadfall
Copy link
Contributor

Cool, thanks! Then I will align the pull request according to it and address requests.

@GiorgioG
Copy link

@georgiosd Yes really. I can't tell you how many times I've advocated to use System.Text.Json and then have to defend these basic kind of omissions. JSON.NET has had this since at least 2016: https://github.com/JamesNK/Newtonsoft.Json/commits/master/Src/Newtonsoft.Json/Serialization/SnakeCaseNamingStrategy.cs

@georgiosd
Copy link

Well maybe they should not have optimized the framework and include a snake case naming strategy.

Really, if you're deciding on the basis of having to write a naming policy implementation (that is what, 5 lines?) and not the overall improvements perhaps you have bigger problems in the organization.

@J0rgeSerran0
Copy link

Totally agree with @georgiosd

I thought that my implementation would be up and running for 6 months (more or less)?
3 years later... I see the end of this discussion is near

Speaking of naming and in my case, the classes JsonKebabCaseNamingPolicy and JsonSnakeCaseNamingPolicy (KebabCasexxx) (SnakeCasexxx)

As you can see, what Georgios says is a fact

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors Priority:3 Work that is nice to have Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet