Compat bug in System.ComponentModel.Annotations tests for invalid RangeAttribute params

[hughbe]

Example test:

[Theory]
[InlineData(typeof(int), "1", "3")]
[InlineData(typeof(double), "1", "3")]
public static void Validate_CantConvertValueToTargetType_ThrowsException(Type type, string minimum, string maximum)
{
    var attribute = new RangeAttribute(type, minimum, maximum);
    Assert.Throws<Exception>(() => attribute.Validate("abc", new ValidationContext(new object())));
    Assert.Throws<Exception>(() => attribute.IsValid("abc"));
}

On netfx this test passes - Exception is thrown. On netcore this test fails - ValidationException is thrown.

This was introduced when porting the library to .NET core, as TypeDescriptor did not exist.

From the reference source (https://github.com/Microsoft/referencesource/blob/4fe4349175f4c5091d972a7e56ea12012f1e7170/System.ComponentModel.DataAnnotations/DataAnnotations/RangeAttribute.cs#L178-L182):

TypeConverter converter = TypeDescriptor.GetConverter(type);
IComparable min = (IComparable)converter.ConvertFromString((string)minimum);
IComparable max = (IComparable)converter.ConvertFromString((string)maximum);

Func<object, object> conversion = value => (value != null && value.GetType() == type) ? value : converter.ConvertFrom(value);

From the .NET core source (https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/RangeAttribute.cs#L200-L207):

Func<object, object> conversion =
    value =>
        (value != null && value.GetType() == type)
            ? value
            : Convert.ChangeType(value, type, CultureInfo.CurrentCulture);
var min = (IComparable)conversion(minimum);
var max = (IComparable)conversion(maximum);
Initialize(min, max, conversion);

I suggest this is triaged as a .NET core bug. I'm submitting a fix to align it with netfx behaviour.

[karelz]

@divega @lajones @ajcvickers can you please comment?

[karelz]

While the exception in .NET Core seems "better", we should consider the impact of the breaking change vs. its benefit. I have a gut feel that compat will win in this case.

[lajones]

Agreed. TypeDescriptor didn't exist at the time this was ported and we did "best efforts". Also we were fine with small incompatibilities back then and that has now changed. Since TypeDescriptor now exists it seems like the best solution to go back to the old way of doing it (even if that does mean a slightly less useful exception). Will decide in triage tomorrow.