Model binder doesn't validate decimals with thousands separator

[frankabbruzzese]

While client side validation hamdles properly decimals with thousands separators, model binder consider them invalid. So client validation submit the form, but then one gets a server side error.
This is an old problem (Mvc5, Mvc4...) and it is due to the ModelBinder using TypeDescriptor.GetConverter(type) to get a converter for strings to decimals. So the point is the "strange" behavior of the decimal TypeConverter.
Now it seems that the dotnet core team decided not to fix this behavior (I understand that this might cause unacceptable compatibilty issues).
So maybe the best place to fix this issue is the SimpleTypeModelBinder class, by using a different conversion method just for numbers.
It is worth pointing out that not only the jquery validation plugin, but substantially all client side globalization packages(like for instance both old and new version of the Globalize package) accept thousand separators when parsing/recognizing well formed numbers.

[dougbu]

@frankabbruzzese the SimpleTypeModelBinder passes a CultureInfo into the TypeConverter when doing the conversion. But the chosen culture is specific to the source value provider: By default, the FormValueProvider and JQueryFormValueProvider use the current culture but the QueryStringValueProvider and RouteValueProvider use CultureInfo.InvariantCulture (because that data is less likely to come from humans). Are you getting decimal data from a query string or route value?

If yes, suggest replacing the QueryStringValueProviderFactory in MvcOptions.ValueProviderFactories with one that instantiates the QueryStringValueProvider with CultureInfo.CurrentCulture. Unfortunately you'd need to subclass RouteValueProvider to do something similar with it (see #5336).

Another possibility is the server isn't changing the culture in each request. You may need to add services.AddLocalization() or call one of the MVC AddSomethingLocalization() methods in your Startup class.

[frankabbruzzese]

@dougbu , unluckly I am sure 99% the problem exists (100% is impossible with software :)).
Data comes from a form submit and I locked culture in startup.cs to en-US. Anyway, also when working with several cultures decimal separator works properly. This means culture are being recognized properly. The problem is just thousands separator that doesn't work in any culture.

Problem may be reproduced in 10 minutes:

1. Create a new project with Individual user accounts (just to have some form scaffolded)
   2 Add localization and lock cultue to en-US (just to be sure of the culture being used)
2. Add a decimal field to the Login view model
3. Render this field in the login view
4. Run, clck the login link, and try insert a decimal with a thousands separator, say: 10,000.02, with some other fake data just to pass client validation.

Just to be sure I debugged the SimpleTypeModelBunder and verified the culture is passed properly to:

model = _typeConverter.ConvertFrom(
                        context: null,
                        culture: valueProviderResult.Culture,
                        value: value);

[dougbu]

@frankabbruzzese you're correct this behaviour is intrinsic to MVC's use of TypeConverters. Behaves about the same without adding localization. And, decimal isn't that unique e.g. double properties are similar.

ModelState contains an error (e.g. "The value '23,000.34' is not valid for Number.") in this case.

MVC can't stop using TypeConverters for back-compatibility reasons. But we might be able to fall back to something else when we catch a FormatException in SimpleTypeModelBinder.

[frankabbruzzese]

@dougbu ,
may be this behavior is correctt for doubles but for sure it is not for decimals. However, on the client side there are no decimals or doubles but just numbers, so thousands separator should be allowed for all numbers. Fallback on FormatException is a good compromise with compatibility.
Just for curiosity, tracking back how decimal TypeConverter is implemented we find the Convert class and then the decimal parsing method. So the culprit seems to be decimal parser that adopt a "strict" validation policy by accepting thousands separators only if some standard formats are specified(like "N" format). Unluckly, down the chain in the TypeDescriptor one may specify just culture and not format string.

That said there seems to be just two options for the fallback, either calling parsing with adequate format string, or simply removing all thousands separators from the string and then trying again with the TypeConverter.

[wc-matteo]

Any updates on this?

It's very easy to encounter this issue. Even using the asp-format tag helper to add the group separator makes a field invalid on post (with values that contains that group separator).

[dougbu]

@frankabbruzzese is correct options are limited here. Would take some reflection and a generic method to fall back to decimal.Parse(..., NumberStyles.Float | NumberStyles.AllowThousands, ...); not pretty or efficient. Determining when NumberFormatInfo is relevant and removing NumberFormatInfo.NumberGroupSeparator substrings is also perilous; could miss errors e.g. 1,,,0.

I recommend creating a DecimalConverter subclass that overrides the object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) method. (Unfortunately the problematic object FromString(string, NumberFormatInfo) method is internal.) Then associate your converter with your property with a [TypeConverter].

MVC could create a handful of TypeConverter classes similar to the workaround above and use them when the system-provided one is exactly DecimalConverter, Int32Converter, et cetera. Would be a lot of code but could be done efficiently.

/cc @danroth27 @Eilon

[frankabbruzzese]

I wrote a custom SimpleTypeBinder and I followed the "remove separator " path. It is efficient and its impact is negligible as compared with the overall operations required by the binder.

Yes, it accepts wrong inputs, substantially correcting th error, but ...in my opinion a cooperative error correcting behavior is a feature, not a problem.

The TypeConverter approach appears too invasive to me since it would spread the effects of the problem all over the application ViewModels...like a virus. :)

[frankabbruzzese]

I like the idea "MVC could create a handful of TypeConverter..." . However, I would define an abstraction on top of TypeConverter.Something, like IToStringConvrter<T> whose unique method should be: Convert(T value, CultureInfo). Now if Model binder finds an IToStringConvrter<T> implementation in DI just use it.
Otherwise it reverts to using IToStringConvrter<object> whose default implementation should use TypeDescriptor.GetConverter(type).

This way one might have a framework to customize string conversion of any type.

[rynowak]

I like the idea "MVC could create a handful ... This way one might have a framework to customize string conversion of any type.

That really is just IModelBinder isn't it?

[frankabbruzzese]

@rynowak , not exactly, IModelBinder performs other tasks too (handling error in model state for instance), moreover each model binder needs the logics to select it. While, my idea was just abstracting type conversion from strings. However, I understand that, at least for what concerns simple types, IModelBinder is quite close to my abstraction, so that may be it is not worth to implements it: as a matter of fact, the SimpleTypeModelBinder is just a few lines of code around a TypeConverter.

So what is an acceptable solution, simply the "remove separator" path? Forcing the user to specify a custom TypeConverter on each decimal property may be acceptable as a temporary patch, but not as a final solution. Frankly speaking, too invasive, for a temporary patch, too.

[frankabbruzzese]

It is worth out pointing out that maybe the problem is not just type conversion for decimals that is bugged. Maybe, using standard type converters for numbers is not a good idea since Html and JavaScript don't know anything about decimals, they know just about numbers. Thus, probably model binders should accept all number formats not just thousands separators for decimals. The only constraints being that the converted value is an acceptable value for the target type. Thus for instance a decimal should accept also an exponential notation if the number fits in the decimal representation (exactly as JavaScript would do), and an integer should accept also something like 1.00, but not 1.0.1.

[Eilon]

It seems reasonable for us to add a new model binder that is specific to float/double/decimal that allows the numerical separators. If anyone wants to go back to the "old" (1.x) behavior, they can remove this new model binder from the default list.

This feature would be specific to the float/double/decimal types, and not apply to the various integer types. There are also no plans to build a new type conversion infrastructure here - just a simple fix.

[wc-matteo]

Wouldn't be better to use a type formatter instead of a type converter as outlined here?

If you want to convert a formatted value to another type you'll have to use the parse method(s). If you just want to convert between types (without formatting) then use a type converter.

[frankabbruzzese]

@Eilon thousand separators for float/double/decimal is perfect. It is the same convention used by most of client side JavaScript number handling libraries.

@wc-matteo the problem exists just for numbers, since dates work properly. There, is a conceptual bug in the way numbers are parsed: they need information on the format. On the contrary dates are able to parse any allowed format with no information on the format used. Moreover, also the default assumed for decimals is wrong, in that they assume an "N" format if no format indication is passed, and the "N" format doen't allow thousands separators while as a default decimals should allow the separator.

The reason to use TypeConverter is that a TypeConverter may be created for any type, while not all types have a parse method. While there exists an IFormattable interface, an analogous interface for parsing do not exists. Thus, the only acceptable way to deal with all type that may be converted from string is the TypeFormatter.

Moreover, since numbers come form Html/JavaScript that makes no difference between floats and decimals, thousands separator should be supported for any not integer type. This means, that also standard parser would fail. An ad hoc soulution is needed.

[wc-matteo]

Thanks @frankabbruzzese for the reasonable explanation.

[dougbu]

c351712