Improve handling of custom `ValidationAttribute`s and their `ValidationResult`s

[jamesfoster]

Hi

I'm trying to move some validation logic into a ValidationAttribute and I've come to a dead end. The idea is to mark a List<string> to contain only unique entries.

For example:

public class MyModel
{
  [UniqueCollection]
  public List<string> Values { get; set; }
}

However, when returning the following validation result ...

  return new ValidationResult(message, new[]{ "[1]", "[4]" });

... I would expect to see ModelState errors with the keys "Values[1]" and "Values[4]". Instead there is only one error with the key "Values.[1]". (Note the .)

I could put the attribute on the class and return a result like ...., new[] { "Values[1]" }); but that would require a bit of reflection and it breaks when you rename the property.

Is there another (nice) way to achieve the same result??

I've traced it down to here:

Mvc/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs

Line 119 in f7a211c

var key = ModelNames.CreatePropertyModelName(_key, result.MemberName);

[tuespetre]

I would have to recommend implementing IValidatableObject on your model class and using the new nameof operator to guard against property renames.

Edit 2016-01-14:

I needed this functionality myself today. Here is my example in case anyone else needs it for reference:

public class LoginIndexModel : IValidatableObject
{
    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        var duplicates =
        (
            from g in Logins.GroupBy(l => new { l.Username, l.Password })
            where g.Count() > 1
            select new ValidationResult
            (
                errorMessage:
                    $"The username/password combination of '{g.Key.Username}' " +
                    $"and '{g.Key.Password}' appears more than once.",
                memberNames: 
                    from l in g
                    select $"{nameof(Logins)}[{Logins.IndexOf(l)}]"
            )
        );

        foreach (var result in duplicates)
        {
            yield return result;
        }
    }

    public IList<Row> Logins { get; set; } = new List<Row>();

    public class Row
    {
        [Required]
        public string Username { get; set; }

        public string Password { get; set; }
    }
}

[jamesfoster]

Thanks, That works, and its better than putting the validation in the controller. However, I prefer the DataAnnotations approach as it doesn't pollute to the view model so much.

Is this something that should be fixed in MVC?

[tuespetre]

The CreatePropertyModelName implementation:

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ModelNames.cs#L20

Maybe that method could be modified to check if the property name starts with the opening square bracket and concatenate appropriately?

[dougbu]

@tuespetre is correct. This looks like a straightforward bug. We should also scan the MVC repo for similar over-simplifications.

FYI we have code in TemplateInfo.GetFullHtmlFieldName() that does the Right Thing:tm: But that's not available in this project. Probably need to move some of that (not the HtmlFieldPrefix property) down into the Abstractions or Core project or duplicate the code.

[danroth27]

@dougbu please discuss with @rynowak

[jamesfoster]

I found out why I was only getting one ModelState error.

The DataAnnotationsModelValidator seems to only return a single result for the first member in ValidationResult.MemberNames.

It should return one for each member name specified.