Skip to content

Use request culture to deserialize JSON body #31331

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

Merged

Conversation

martincostello
Copy link
Member

PR Title

Add support for reading localized parsing of dates etc. in JSON model binding.

PR Description

Adds a new ReadJsonWithRequestCulture option to support deserializing JSON using the current HTTP request's culture when using the Newtonsoft.Json input formatter with MVC as suggested by #9994 (comment).

Addresses #9994

Add new ReadJsonWithRequestCulture option to support
deserializing JSON using the current HTTP request's culture
when using the Newtonsoft.Json input formatter with MVC.
Addresses dotnet#9994.
@ghost ghost added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member labels Mar 28, 2021
@davidfowl
Copy link
Member

Needs an API proposal.

@martincostello
Copy link
Member Author

#31334

@pranavkm pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 28, 2021
@ghost
Copy link

ghost commented Mar 28, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

/// <value>
/// The default value is <see langword="false"/>.
/// </value>
public bool ReadJsonWithRequestCulture { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this might be a better name if this worked for both input and output formatting?

Suggested change
public bool ReadJsonWithRequestCulture { get; set; }
public bool UseCurrentCulture { get; set; }

@martincostello can this also use the culture for output formatting? Seems weird it only works for reading, but not for writing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that makes sense. Initially I just went down the route outlined in the initial issue, but I agree it would be better to have it be bi-directional.

I'll go with that suggested name for now and look at having the output formatter use it too.

Copy link
Member Author

Choose a reason for hiding this comment

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

So from playing around with this locally, the serializer always writes in ISO format for DateTime values (or whatever is configured by the JsonSerializerSettings, e.g. the DateFormatString property).

Setting the writer/serializer to use the current culture has no effect on this and I'm not sure there's a sensible default to overwrite it for or that it's a good idea to special-case for DateTime/DateTimeOffset.

I'd also have to introduce a new constructor (or make a breaking change) to allow the MvcNewtonsoftJsonOptions to be passed into the output formatter to observe the value of the option to know if to set the culture or not.

Thoughts on best route forward?

Copy link
Member

Choose a reason for hiding this comment

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

cc @JamesNK in case he has thoughts

Copy link
Member

Choose a reason for hiding this comment

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

I think the main place culture is used with Newtonsoft.Json is when deserializing strings into .NET types.

For example:

{"Amount": "1.0"}
public class Product
{
    public decimal Amount { get; set; }
}

By default the culture is InvariantCulture. Changing the culture can be used in apps that want to allow strings like "1,0" (seems to be a European thing) to successfully convert to a decimal/double/float.

Copy link
Member

Choose a reason for hiding this comment

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

Semi-related to this is how we want to handle culture for simple minimal action parameters (#32377). First we need to default to InvariantCulture, but I think we might want a way to configure this.

@pranavkm pranavkm added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 12, 2021
@pranavkm pranavkm added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 4, 2021
@ghost
Copy link

ghost commented Jun 4, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

Add missing summary tag from merge with main.
@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 8, 2021
@pranavkm pranavkm enabled auto-merge (squash) June 10, 2021 16:46
@pranavkm
Copy link
Contributor

Thanks for the PR!

@pranavkm pranavkm merged commit 44bcd96 into dotnet:main Jun 10, 2021
@ghost ghost added this to the 6.0-preview6 milestone Jun 10, 2021
@martincostello martincostello deleted the Read-Json-With-Request-Culture-9994 branch June 10, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add property to MvcNewtonsoftJsonOptions to support culture-aware deserialization
5 participants