views:

79

answers:

2

I've been working on an application that uses xVal's server side validation with data annotations. We recently ran into errors where the validation messages have been unpredictable for fields that have multiple validations that could fail if the field is empty (e.g., an email address is required, but also fails a validity check).

Assuming that I needed to just return the first validation error, I added a method to our validation runner to achieve that goal (UPDATE: see edit at the bottom for the exact method):

public static IEnumerable<ErrorInfo> GetFirstErrors<T>(object instance) where T : ValidationAttribute
    {
        return from prop in TypeDescriptor.GetProperties(instance).Cast<PropertyDescriptor>()
               from attribute in prop.Attributes.OfType<T>().Take(1)
               where !attribute.IsValid(prop.GetValue(instance))
               select new ErrorInfo(prop.Name, attribute.FormatErrorMessage(string.Empty), instance);
    }

I also set up a simple test method to verify in NUnit:

private class FirstErrorValidationTest
    {
        [RequiredValueValidator(ErrorMessage = "This field is required"), StringLength(50)]
        public string FirstName { get; set; }

        [RequiredValueValidator(ErrorMessage = "This field is required"), StringLength(50)]
        public string LastName { get; set; }

        [RequiredValueValidator(ErrorMessage = "This field is required"), EmailAddressValidator, StringLength(50)]
        public string EmailAddress { get; set; }
    }

    [Test]
    public void Assert_GetFirstErrors_Gets_First_Listed_Validation_Attribute_Error_Messages()
    {
        FirstErrorValidationTest test = new FirstErrorValidationTest()
        {
            FirstName = "",
            LastName = "",
            EmailAddress = ""
        };

        var errors = DataAnnotationsValidationRunner.GetFirstErrors(test);

        Assert.AreEqual(3, errors.Count());

        foreach (var error in errors)
            Assert.IsTrue(error.ErrorMessage.Contains("required"));
    }

The problem is that this test's output is highly unpredictable. Sometimes it passes, sometimes it returns just one or two of the errors, and sometimes none at all. Is the issue here with my LINQ query, my test, or both?

EDIT: Great point on pasting in a slightly different method; here's the one actually being hit:

    public static IEnumerable<ErrorInfo> GetFirstErrors(object instance)
    {
        return from prop in TypeDescriptor.GetProperties(instance).Cast<PropertyDescriptor>()
               from attribute in prop.Attributes.OfType<ValidationAttribute>().Take(1)
               where !attribute.IsValid(prop.GetValue(instance))
               select new ErrorInfo(prop.Name, attribute.FormatErrorMessage(string.Empty), instance);
    }
A: 

Try to use

var errors = DataAnnotationsValidationRunner.GetFirstErrors(test).ToArray();
zihotki
I gave that a try and still get unpredictable results. I noticed though that when I leave NUnit up, clean/recompile and then run it again is when it's most prone to change.
Brandon Linton
+2  A: 

Get rid of the Take(1). I suspect the empty string is passing the Required test. If you happen to get that instead of the length validator, the test passes.

Craig Stuntz
The Take(1) call is definitely the culprit. Is there no way to have any sort of implied ordering or priority on the attribute selection (i.e., check required field attributes above any of the others)?
Brandon Linton
It's possible, but since this is a unit test, why? Shouldn't you check all attributes on the test data?
Craig Stuntz
I definitely agree, but what would be a better way of enforcing the global rule of displaying one validation error per field (and giving favor to required-field failures)? It looks like the model state errors are rolled up in a dictionary for the view to use anyway, so if the selection was arbitrary it seems like we couldn't predict what error would be shown first.
Brandon Linton
You could `orderby attribute.GetType() == typeof(RequiredAttribute) ? 0 : 1` if you really had to. But ISTM this is a display issue, not a testing issue. I'd rewrite the `Html.ValidationMessage()` helper, instead.
Craig Stuntz
Great point. Thanks for all the help!
Brandon Linton