RangeAttribute with string constructor fails on various locales

[MrJul]

RangeAttribute(Type, string, string) can be used to validate various types implementing IComparable. However, it does the conversion from the string parameters using the current culture, which is wrong.

Simple repro:

Thread.CurrentThread.CurrentCulture = new CultureInfo("fr");
new RangeAttribute(typeof(decimal), "0.1", "0.9").IsValid(123m)

fails with a FormatException: 0.1 is not a valid value for Decimal.

While in this sample the strings could be adjusted at runtime to represent numbers using the current culture, the typical usage of an attribute is

[Range(typeof(decimal), "0.1", "0.9")]
public decimal Value { get; set; }

where the values must be constants, and can't change depending on the culture.

Culprit line:

https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/RangeAttribute.cs#L203

The fix can't be to simply change CurrentCulture to InvariantCulture here, since this will alter the runtime validation behavior: InvariantCulture should be used to convert the min and max parameters (maybe falling back to CurrentCulture for back-compat, if the constructor was used directly), but CurrentCulture should be kept to validate the user-entered value.

If accepted, I can add some unit tests and a fix for this issue.

[joshfree]

@steveharter

[steveharter]

Moving to rc1. Will likely assign to a mef contact as well, as this is an issue with the RangeAttribute, and not with globalization. The suggested fix of having the attribute values as Invariant makes sense because the use of commas, decimal, etc literals can vary between cultures and should thus be stored as an invariant format.

[dsplaisted]

Fixing this looks like it will have a compat impact, so we will probably need to quirk this.

[karelz]

@weshaggard what are the options for quirks here? We want portable libraries targeting .NET Standard 2.0 have the same behavior on .NET Core and Desktop I assume.

Another option is to add a new member opting into the new behavior. We need to consider all other attributes - they should be consistent.

cc: @divega

[karelz]

Ping @weshaggard?

[weshaggard]

Sorry I don't know the space in question much here so it is hard to say. Assuming it is just code as opposed to attribute discovery then we have the usual AppContext switches that can be considered to enable folks to opt into the same behavior. I would defer to @divega about whether we feel a quirk would be warranted here or not.

[divega]

Data Annotations Triage: We discussed this in triage today and we believe the issue is worth addressing. The functionality is broken if you need to run an application on locales that use different decimal separators from where the code was written. The correct thing would be to change it to use InvariantCulture.

That said we are having a hard time figuring out what kind of compatibility bar to apply to this and in general to Data Annotation issues. @rowanmiller will follow up on this internally.

Assuming we need to have a hard bar for compatibility with .NET Framework, we are not sure whether having a quirk or a new member is a better option:

• It seems both approaches would require work in .NET Core and .NET Framework for the issue to be completely addressed
• A quirk would be a little less discoverable, but it would keep the surface on the attribute simpler.

[lajones]

We would consider a PR which provided either a new constructor (taking an extra bool indicating whether to use InvariantCulture) or a new member which did the same thing.