Add support for string based properties decorated with the RangeAttribute

[jcasale]

Consider the following:

public class MyModel
{
    [Required]
    [Range(1, 65535)]
    public string Port { get; set; }
}
var fixture = new Fixture();
var model = fixture.Create<MyModel>();

In 3.51.0 the first call the Create() worked, however subsequent calls threw, now in 4.0.0-rc1 (the version I am using), the first call throws ObjectCreationException whereas the attribute itself offers support for the use case.

[ploeh]

Originated from https://stackoverflow.com/q/47184400/126014

[zvirja]

@jcasale Thanks for raising the issue.

Well, as far as I noticed, AutoFixture wasn't designed to handle gracefully the Range attribute applied to non-numeric members. Therefore, I'm basically not surprised as we already have the same issue for enums. They should be fixed together and I've already started to investigate that.

I don't have too much experience with annotation attributes and at the first glance it looks a bit weird that you apply numeric range attribute to a property of string type. Could you please point to some documentation describing that such usage is perfectly valid (and what is the expected behavior)? Just want to ensure that we cover valid scenarios 😊

P.S. 👍 for using AutoFixture v4 😉

[jcasale]

To be honest, it's a convention I am used to from the ASP arena and I don't know if it's an abuse of the facility or intended. I will check out the docs and if not, the reference source for logic that indicates it's valid and expected and not simply a fragile coincidence.

[jcasale]

Reference source implies it will handle casting a string (which it does) and the docs illustrate an example where the the value being applied is coerced and in that case, boxed. I have a workaround and I admit the use case is rare, however it does appear to be valid but certainly low priority if any.

[zvirja]

@jcasale Thanks for the confirmation. Well, it's indeed seems that string could be a valid type. Also it looks like that Range attribute could support even other types, like DateTime. The question is whether we are going to support that in AutoFixture out-of-the-box as we clearly cannot support all those types.

Probably, it makes sense to follow the way, when we support this feature partially - recognize the Range attribute and wrap that to the RangedRequest. Later clients could add customization to handle the RangedRequest for the custom types they have. The benefit is that you will not need to handle the Range attribute directly. Also we as a project will not need to support the variety of different possible types as they might be quite rare.

@jcasale What would you say about that plan?
@moodmosaic Your opinion is also appreciated as the topic is quite tricky.

[moodmosaic]

The purpose of supporting data annotations in AutoFixture is to provide a more fine-grained control over the scope of generated values.

However, some of those data annotations have a totally weird API, where you can easily do the wrong thing, as with the RangeAttribute, which has 3 constructor overloads:

• RangeAttribute(Double, Double)
• RangeAttribute(Int32, Int32)
• RangeAttribute(Type, String, String)

And because of that pesky 3rd constructor overload accepting a Type, it is possible to do this:

[Range(1, long.MaxValue)]
public long SomeProperty { get; set; }

• 1 gets casted into a Double
• long.MaxValue gets casted into a Double resulting to an arithmetic overflow.

So, IMHO, and AFAICT, in this case:

[Range(1, 65535)]
public string Port { get; set; }

we should throw an error. In the error message we should probably tell to the user that the right way of controlling the scope of generated strings is by doing this:

[Range(typeof(string), "1", "65535")]
public string Port { get; set; }

And then, we'd have to make sure we support not only strings, but chars, dates, and so on:

[Range(typeof(DateTime),"14-Dec-1984", "14-Dec-2099")]

[Range(typeof(char), "n", "s")]

---

This is one of the reasons that F# Hedgehog makes this concept explicit, and easier, through the Range type and combinators.

[jcasale]

In the error message we should probably tell to the user that the right way of controlling the scope of generated strings is by doing this:

[Range(typeof(string), "1", "65535")]
public string Port { get; set; }

That is incorrect, it works coincidentally for values where none of the leading place values exceed the leading place values of the max string. For example, 1 through 6 pass, however 7 through 9 fail, 10 through 65 pass, 66 to 99 fail, 100 through to 655 pass then 656 fails etc.

A contrived example:

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Reflection;

internal class Program
{
    [Range(typeof(string), "1", "65535")]
    // [Range(1, 65535)]
    public string Port { get; set; }

    private static void Main()
    {
        for (int i = 1; i < 65536; i++)
        {
            Program program = new Program
            {
                Port = i.ToString()
            };

            foreach (var error in program.Validate())
            {
                Console.WriteLine($"{i}, {error.ErrorMessage}");
            }
        }
    }
}

public static class Extensions
{
    public static IEnumerable<ValidationResult> Validate<T>(this T model)
        where T : class
    {
        if (model == null)
        {
            throw new ArgumentNullException(nameof(model));
        }

        foreach (PropertyInfo propertyInfo in model.GetType().GetProperties())
        {
            object[] attributes = propertyInfo.GetCustomAttributes(typeof(ValidationAttribute), false);

            if (attributes.Length == 0)
            {
                continue;
            }

            ValidationContext validationContext = new ValidationContext(model)
            {
                DisplayName = propertyInfo.Name
            };

            if (attributes.OfType<RequiredAttribute>().FirstOrDefault() is RequiredAttribute required)
            {
                ValidationResult result = required.GetValidationResult(propertyInfo.GetValue(model), validationContext);

                if (result != null)
                {
                    yield return result;

                    yield break;
                }
            }

            foreach (ValidationAttribute attribute in attributes)
            {
                ValidationResult result = attribute.GetValidationResult(propertyInfo.GetValue(model), validationContext);

                if (result == null)
                {
                    continue;
                }

                yield return result;
            }
        }
    }
}

[zvirja]

@jcasale Thanks for the sample.

That's why I'd suggest to not include this feature to the AutoFixture for now as there might be a whole set of different options depending on the OperandType/MemberType combinations.

In the #920 I've introduced the generic RangedRequest in a way that later any specimen builder could decide what to do. Request offers both the OperandType (this is how the Ranged attribute refers to the type you specified) and MemberData - type of the member you applied this property to.

Later @jcasale could register it's own RangedRequest builder to handle strings in a way he wants, without a need to deal with the RangedAttribute directly.

For me that looks like a good trade-off.

[jcasale]

@zvirja No objections, I have a workaround and realized some good takeaways from this. In reference to registering my own RangedRequest, off the top of your head do you know of any code exercising the concept that I could review to see how this is accomplished?

[zvirja]

any code exercising the concept that I could review

Probably, not for now, as #920 is still under the review by @moodmosaic. Only after we merge the PR, we'll know its shape, so I'll be able to show you a demo.

However, the best place would be to look at the NumericRangedRequestRelay implementation (if it doesn't changes during the review) as it's a sample of builder for numeric types.

[zvirja]

@jcasale Feel free to use the NumericRangedRequestRelay or EnumRangedRequestRelay as a sample of such customization.

This API will be available since our next release that should happen in the nearby future.

Closing this one as no further action is required so far. Feel free to ask more questions if you have 😉