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

Consider adding a JsonSerializerOptions.CultureInfo option #1366

Open
ollie10 opened this issue Jan 5, 2020 · 25 comments
Open

Consider adding a JsonSerializerOptions.CultureInfo option #1366

ollie10 opened this issue Jan 5, 2020 · 25 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@ollie10
Copy link

ollie10 commented Jan 5, 2020

Hi, I'm stuck on this problem
.Net Core 3.1 Web App with a page that uses Google Maps Autocomplete and I need to save the address and coordinates (Latitude / Longitude as decimals).

Creating an object in javascript and sending it to the controller via jQuery Ajax.

The problem is the following:

  • I'm using the default System.Text.Json serializer / deserializer
  • The decimal values returned by google Js API are in this format: 55.7539619 (for example)
  • I use a route parameter to set statically the Culture like site.com/spain/ for es-ES site.com/russia/ for ru-RU

In the controller all the other properties arrive correctly except the latitude and longitude which are decimals

The problem arises when a culture is being used and the number is not in the format of that culture in the way that the separator is comma (for Spain for example) and not the point (which is standard for javascript)

So i looked for some solutions but none of them seems solving completely the problem:

  • i could replace the dot with comma before sending but it won't work for US culture for example, and besides that I would lose precision

In Newtonsoft Json there was the possibility to specify the Culture as options of the serializer but System.Text.Json doesn't have a property like that.
I think that System.Text.Json should deserialize Json in the correct format more than 'interpreting' the culture

This is the code I'm using:

propertyToSave.OtherProp = 'text' // arrives correctly as all the other properties
propertyToSave.Latitude = selectedPlace.geometry.location.lat(); //val: 55.7539619
propertyToSave.Longitude = selectedPlace.geometry.location.lng();// val: 37.60819619999995

$.ajax({
	url: "/" + countryInfo.EnglishName.toLowerCase() + "/submit",
	dataType: "json",
	method: "POST",
	data: { property: propertyToSave, __RequestVerificationToken: $("input[name=__RequestVerificationToken]").val() },
	....

I specify that if i replace dots with commas the values arrive correctly to the controller, in any other case the properties are always 0

Any suggestion? Is a known problem? I suppose the Web API should suffer of this problem too

@ollie10
Copy link
Author

ollie10 commented Jan 5, 2020

Values sent by the browser:

Sent

Values received by the controller:

Received

@Clockwork-Muse
Copy link
Contributor

The decimal values returned by google Js API are in this format: 55.7539619 (for example)

JavaScript, and thus most use of JSON, doesn't do decimal (base-10 floating point), it uses double (base-2 floating point). This (probably) won't solve your problem, but it does mean that values that are transferred may not always be what you expect.

I use a route parameter to set statically the Culture like site.com/spain/ for es-ES site.com/russia/ for ru-RU

There's multiple problems with this, besides the one you're experiencing. The biggest one is that individual countries can have multiple major languages; Spain, for example, has Catalan (ca-ES), Canada has Quebec-French (fr-CA - there can be some pretty big penalties for ignoring this one, by the way), etc. It also ignores things like user preferences for display language. It's use in a route may be dubious (it's difficult to tell what else you're doing with it) - if I'm looking at houses/apartments/whatever in Russia, does that mean the page suddenly renders in Russian? That won't be helpful if I'm trying to book a vacation there...

If the user is logged in, respect whatever language they have selected in their profile. If they're not, use the Accept-Language header, if set. Otherwise, take a guess based on IP range.


I specify that if i replace dots with commas the values arrive correctly to the controller, in any other case the properties are always 0

Simply replacing the period with a comma would actually make it invalid JSON (since the comma is the attribute separator), so I'm suspicious that it works at all. Your client-side code isn't what we'd be interested in - we'd need to see your controller code. As it is, in a trivial example I cannot reproduce your issue.

(personally, I've always disliked relying on CurrentCulture, and would rather explicitly use a culture object for just those items that require it)

@ollie10
Copy link
Author

ollie10 commented Jan 6, 2020

Hi @Clockwork-Muse thanks for reply, the use case of the route is to select a specific area more than a language, even if I see what you mean anyway for now I'm not planning to accept multiple languages for each country (not planning to publish in those countries right now).

So the question is another and very simple: how to pass the value 55.7539619 passed me from google selectedPlace.geometry.location.lat(); to a controller via ajax in a website perhaps that (not caring all the discourse of routing etc etc) has a culture fixed on whatever European country (or that accept the culture from the Accept-Language) where the decimal separator is the comma?
Many thanks for your time

@ollie10
Copy link
Author

ollie10 commented Jan 6, 2020

Just to verify your thesis I made another test, I added all the configuration to support the culture en-US just adding the route site.com/usa/...

Nothing else added, no code changed on the client side, and with that everything works like a charm

alt text

About my controller code is a very normal controller accepting a ViewModel which has all the properties that I'm passing:

[Authorize]
[ValidateAntiForgeryToken]
[HttpPost]
public async Task<JsonResult> Submit(PropertyViewModel property)
{
...
]
public class PropertyViewModel
{
...
   [Required]
   public decimal Latitude { get; set; }

   [Required]
   public decimal Longitude { get; set; }

   [Required]
   public string LatLng { get; set; }
...
}

@ollie10
Copy link
Author

ollie10 commented Jan 6, 2020

Just made another test to be sure: changing the type of the property to double instead of decimal solves the problem, so there should be some problem specific with decimal and the serialization

@Clockwork-Muse
Copy link
Contributor

Just to verify your thesis I made another test, I added all the configuration to support the culture en-US just adding the route site.com/usa/...

... if you actually want the route to drive localization, actually use the locale or language codes, like a number of sites already do. If you're just getting resources about a specific country, don't do localization, because it's unrelated.

@ollie10
Copy link
Author

ollie10 commented Jan 6, 2020

Hum, you seems not understanding correctly the problem... There websites that have a localization set in the configuration statically or taking localization with the header, at the end it doesn't matter, the problem is that when the CurrentCulture is for any reason (settings, header, querystring etc etc) set to something different than en-US the decimal number are not readed

That's the problem, what there is on top of it it doesn't matter, I can make a simple web app to show you without any code just with a Page and a Controller and using using the default culture of my PC and sending an object (with decimals) in JSON to the controller with ajax and the problem will arise anyway...

@danmoseley
Copy link
Member

I can make a simple web app to show you

@ollie10 perhaps you could make such a repro and push it up as a github repo? That's what many people do. Best of all, though, is a repro short enough (say up to 50 lines) that you can paste it in here and run it in a console app.

@ollie10
Copy link
Author

ollie10 commented Jan 7, 2020

Hello @danmosemsft, @Clockwork-Muse I just pushed the repo with the example, you can find it here:

https://github.com/ollie10/test.json.decimals/tree/master/Json.Test

Here the view:
https://github.com/ollie10/test.json.decimals/blob/master/Json.Test/Json.Test/Views/Home/Index.cshtml

Here the controller:
https://github.com/ollie10/test.json.decimals/blob/master/Json.Test/Json.Test/Controllers/HomeController.cs

I publish also the results now I have another problem, the number is interpreted but the period is not taken into account if the Culture is not en-US, i show you the results:

The culture is taken in this case only from the header, nothing to do with the case I told you at the begging, standard .net Core code without any customization in CultureProviders and so on and the decimal is still misinterpeted.

en-US Culture:
alt text

it-IT Culture:
alt text

In this example I just created a new Web Application with nothing inside except the page that sends the data via ajax, I added the code in the startup for allowing the localization

CultureInfo[] supportedCultures = new[] { new CultureInfo("en-US"), new CultureInfo("it-IT"), new CultureInfo("es-ES") };

app.UseRequestLocalization(new RequestLocalizationOptions() { SupportedCultures = supportedCultures, SupportedUICultures = supportedCultures }).UseEndpoints(endpoints =>
   	   {
   		   endpoints.MapControllerRoute(
   			   name: "default",
   			   pattern: "{controller=Home}/{action=Index}/{id?}");
   	   });

@danmoseley
Copy link
Member

Moving to dotnet/runtime where new issues go.

@danmoseley danmoseley transferred this issue from dotnet/corefx Jan 7, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jan 7, 2020
@layomia layomia self-assigned this Feb 20, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Feb 20, 2020
@layomia layomia added this to the 5.0 milestone Feb 20, 2020
@bCamba
Copy link

bCamba commented Mar 23, 2020

I am also having problems with this issue. @layomia Will this only be fixed in 5.0?

@layomia layomia added this to Backlog in System.Text.Json - 6.0 via automation Apr 7, 2020
@layomia
Copy link
Contributor

layomia commented Aug 6, 2020

Moving this to future - adding built-in logic for culture-aware number handling is currently not on the roadmap for System.Text.Json. It looks like OP had a workaround which involved changing the property type from decimal to double. Another workaround here is to write a custom converter to provide handling needed for each specific scenario.

Any support that involves allowing commas in number payloads would need to build atop support for deserializing quoted numbers. The numbers would need to be quoted, otherwise the JSON payload would be malformed.

@eiriktsarpalis
Copy link
Member

If I'm understanding the OP correctly, the issue boils to the following reproduction:

const double number = 3.1415926;
string numberStr = number.ToString(System.Globalization.CultureInfo.GetCultureInfo("de-DE")); // 3,1415926
JsonSerializer.Deserialize<double>(numberStr); // throws JsonException: ',' is invalid after a single JSON value. Expected end of data.

We might want to consider adding adding a CultureInfo property to JsonSerializerOptions like Json.NET is doing.

@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 20, 2021
@ghost
Copy link

ghost commented Oct 20, 2021

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

@eiriktsarpalis eiriktsarpalis changed the title System.Text.Json Can't deserialize decimal properties (possibly a bug) Consider adding a JsonSerializerOptions.CultureInfo option Oct 20, 2021
@eiriktsarpalis eiriktsarpalis removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 16, 2021
@ollie10
Copy link
Author

ollie10 commented Apr 22, 2022

Hello @eiriktsarpalis @layomia @danmoseley , any news on this topic? I just posted a question on StackOverflow becuase it's still happening, 2 years after has been reported...

Nothing to do? Many thanks

@eiriktsarpalis
Copy link
Member

@ollie10 we haven't been able to prioritize this issue yet. Have you considered trying some of the suggested workarounds, like using a custom converter for decimal?

@ollie10
Copy link
Author

ollie10 commented Apr 22, 2022

At the moment i'm passing it as a string as another parameter and then deserializing it manually but it's as said a workaround...

@eiriktsarpalis
Copy link
Member

That should not be necessary if you register a custom converter for your type, see https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to?pivots=dotnet-6-0

@ollie10
Copy link
Author

ollie10 commented Apr 22, 2022

many thanks @eiriktsarpalis I used this solution which is more elegant even if it remains a workaround and it's weird it hasn't been covered yet. I mean, these are numbers coming from javascript, javascript doesn't allow culture when serializing / deserializing, why the default behaviour of a controller should?
Anyway if it is useful for someone this is my code and I added the DataAnnotation the the properties I want to deserialize

public class DoubleJsonConverter : JsonConverter<double>
{
	public override double Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
	{
		return double.Parse(reader.GetString(), CultureInfo.InvariantCulture);
	}

	public override void Write(Utf8JsonWriter writer, double doubleValue, JsonSerializerOptions options)
	{
		writer.WriteStringValue(doubleValue.ToString(CultureInfo.InvariantCulture));
	}
}

And on the property

[JsonConverter(typeof(DoubleJsonConverter))]
public double Latitude { get; set; }

@eiriktsarpalis
Copy link
Member

FWIW it should be possible to avoid annotating every single double property by adding the converter to the JsonSerializerOptions.Converters list.

@ollie10
Copy link
Author

ollie10 commented Apr 22, 2022

Thanks but those two were the only two in the whole application

@ollie10
Copy link
Author

ollie10 commented Apr 28, 2022

Hello again @eiriktsarpalis @danmoseley @layomia, I was double-checking the issue and seems is not solved.
I mean the solution of the converter works properly, but I just realized I was terribly wrong, I'm not sending data posting a JSON from jQuery, but it's been sent via a normal post form via jQuery so the deserializer doesn't enter the equation, in fact the method Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) is never been called when the controller needs to bind the posted form to the object

So I further investigated to send the object as a JSON but without any success: the [FromBody] annotation in the controller it doesn't work in my case (don't know if I'm doing something wrong) but even if it would work then I would have another problem: sending the object as JSON and sending the __RequestVerificationToken as a normal POST parameter to pass the check [ValidateAntiForgeryToken]
As far as I know, I cannot send some objects as JSON and some as normal POST parameters in the same ajax call, so I don't see it an option.

The curious thing is that I realized that a normal post would work if the thread culture is set in en-US which is not my case as I mentioned you before, I have a custom RequestCultureProvider that map the route /panama/ to es-PA, /dominicana/ to es-DO and so on.

But the big problem which I think is causing a lot of headaches is that I don't have a way to change the culture to interpret the values in a specified culture, what I can eventually do is passing the Accept-Language header in the call specifying the culture and in this way the way numbers are red.

$.ajax({
	url: 'url',
	data: { property: propertyToSave, __RequestVerificationToken: $('input[name=__RequestVerificationToken]').first().val() },
	timeout: DEFAULT_AJAX_TIMEOUT,
	dataType: 'json', // this is what I'm expecting from the server not the format is sent
	method: 'POST',
	beforeSend: function (request) {
		request.setRequestHeader('Accept-Language', 'en-US');
	},
...

I did it so but is not working, my question is: the framework shouldn't take this into account in order to bind objects instead of other things? This can cause a lot of troubles, for example, imagine if you have an application which doesn't set any culture on an American server which provides content to a browser which doesn't have the same culture.

In my case I'm setting it manually from the routing but if you don't do it so, it can take the one of the server or the browser depending on how it's configured and can cause a lot of troubles...
Don't know if you see my point

@eiriktsarpalis
Copy link
Member

It would seem like this is an issue related to aspnetcore MVC model binding? I would suggest filing filing a separate issue on the aspnetcore repo, and possibly incorporate an application with a minimal reproduction in your report.

@ArthurMa1978
Copy link

ArthurMa1978 commented Apr 11, 2023

The ask is adding a CultureInfo option into the JsonDocumentOptions like NewtonJson does.
We got similar issue, all serialize / deserialize operations will fail if set culture to some special region, for example Russian (ru-RU).

@ollie10
Copy link
Author

ollie10 commented Jun 29, 2023

Problem notified 3,5 years ago, still not solved. Probably, MS thinks the world is only the United States and the whole world uses points instead of commas, which is actually the other way around: the whole world uses commas instead of points. Maybe they will make us a grace for .NET 15...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

8 participants