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

Change MVC's use of System.Text.Json to case-insensitive #10723

Closed
rynowak opened this issue Jun 1, 2019 · 5 comments · Fixed by #10727
Closed

Change MVC's use of System.Text.Json to case-insensitive #10723

rynowak opened this issue Jun 1, 2019 · 5 comments · Fixed by #10727
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@rynowak
Copy link
Member

rynowak commented Jun 1, 2019

Edit: if you arrived here via twitter then Hi! Thanks for coming. I wrote the initial bug report last night pretty late so some adding some information and context.

History and context

MVC gets its existing JSON support through Newtonsoft.Json (thanks @JamesNK). Newtonsoft.Json provides options to configure canonicalization of names and case-sensitivity. Newtonsoft.Json's default settings are to leave your property names as-is and to compare property names case-insensitively.

MVC however configures Newtonsoft.Json to canonicalize property names as camelCase. The effect of this is:

  • JSON output produces camelCase property names
  • JSON input matches property names using case-insensitive comparisons, supporting camelCase or PascalCase.

MVC's choice of a different default than what Newtonsoft.Json users makes sense to us because MVC is for the web (web sites, APIs). If you're using JSON on the web, you're doing to so either:

  • interop with javascript for a web site
  • interop with any other language for an API

In either of these cases, choosing camelCase is the least surprising default.

Enter System.Text.Json

We're adding a new serializer to dotnet/CoreFx - which is a good opportunity to question any and all assumptions about what we're doing. Using a new serializer as the default will mean breaking changes in behavior, so if there are things we want to change in this are it's our one chance.

I originally said we should use camelCase and be case-sensitive because it's the more correct default. JSON is fundamentally a case-sensitive format so it seems like the best default.

The problem

The project is that this breaks webapi client - which uses Newtonsoft.Json's default settings and preserves the case of your C# properties. In fact, for the immediate future, any .NET code that calls your APIs probably is using Newtonsoft.Json and probably is using its default settings (PascalCase).

So they send:

{
    "FirstName": "James",
    "LastName": "Newton-King",
}

But we would only accept:

{
    "firstName": "James",
    "lastName": "Newton-King",
}

This gets worse because we also want to tolerate/ignore unmatched properties by default. The result is that things don't work and you don't know why.

The solution

The solution is opt-in to case-insensitivity. This gives us the same behavior as we had with Newtonsoft.Json.

I'm personally convinced that this is the right default because it's going to just work for most users, and it's also our current behavior. Again, this is a discussion about our default, so it's not about what's the most correct, it's about what's going to work well for most users.

If I were writing an API, I would consider changing this setting, which is easy to do.

services.AddControllers().AddJsonOptions(options => options.JsonSerializerOptions.PropertyNameCaseInsensitive = false);

/cc @anurse @BrennanConroy @pranavkm

@rynowak rynowak added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 1, 2019
@analogrelay
Copy link
Contributor

I think this is also an appropriate default for SignalR (if it isn't our default already). IMO it's rare to want case-sensitivity here and Newtonsoft.Json's default was case-insensitive.

@rynowak
Copy link
Member Author

rynowak commented Jun 1, 2019

I still want it and I think it's the most correct thing we could do 🤣 . If I were writing an API (for example) I would change this to be case-sensitive. I just see this causing some chaos if we make it the default due to existing .NET code like the webapi client that doesn't make it easy to configure (static methods).

@analogrelay
Copy link
Contributor

Scene from the TV show 'Futurama' in which a bureaucrat says "You are technically correct. The best kind of correct"

I get that it feels most correct, but I just think it's highly unlikely to get into a situation where two JSON properties differ only by case. I do agree that the only real case (pun intended) where case is expected to differ is the camelCase/PascalCase transition between JS and C# and we did support that in the past. I guess I'm not worried about making the more compatible (but less technically correct) choice here 🤷‍♂

@rynowak rynowak added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jun 3, 2019
@rynowak rynowak added this to the 3.0.0-preview6 milestone Jun 3, 2019
@rynowak
Copy link
Member Author

rynowak commented Jun 3, 2019

@anurse @BrennanConroy - are you tracking making this change for SignalR?

@analogrelay
Copy link
Contributor

I had thought we already did, but it turns out we do not :). Opening a new issue for that.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants