Improve handling of custom ValidationAttributes and their ValidationResults

[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.

[dougbu]

Though the relevant code has changed significantly, the symptoms remain in 1.0.0-rc2-final and the current dev branch. The current behaviour is also almost unchanged from MVC 5 (see Note 1).

But the picture looks a bit wonky, especially when validating collection elements. This includes the original case of a ValidationAttribute on a collection type.

Problems

In the context of MVC's calls to ValidationAttribute's

    public ValidationResult GetValidationResult(object, ValidationContext)

1. Though .NET's ValidationAttribute (see Note 2) and MVC's DataAnnotationsModelValidator clearly create and use member names relative to the containing object, any ValidationResult.MemberNames entry other than null and validationContext.MemberName are interpreted relative to the property value or collection element. This for example means custom validators cannot add errors for sibling properties; if a CompareAttribute on Property1 returned a ValidationResult with MemberNames == new[] { "Property2" }, MVC would interpret the error as relevant for the (likely nonexistent) Property1.Property2. (See also Note 3.)
2. An initial ValidationResult.MemberNames entry intended to refer to an element of a collection is not correctly concatenated with the property name (or container name if we make a switch wr.t. (1.)). Names like Property.[2] are never correct and MVC always includes the dot.
3. All but the first entry in ValidationResult.MemberNames is ignored. A ValidationAttribute cannot identify more than one member as invalid.
4. In the specific case (see Note 4) of a ValidationAttribute on a top-level model type or collection element type, we set validationContext.MemberName to the type's Name. This goes against the limited documentation (see note 3 again) I can find for this property. The general sentiment is MemberName should be null in these cases.

Recommendations

We should not fix (1.) because custom ValidationAttributes overriding ValidationResult IsValid(object, ValidationContext) is uncommon to start and using anything but the default ValidationResult.MemberNames is even less common. Those who have done this work are likely expecting the current MVC interpretation of the MemberNames value.

We should fix (2.) through (4.) because the current behaviour is confusing and users have no workarounds.

Details

Note 1

The main MVC 5 -> ASP.NET Core MVC difference is that MVC 5 uselessly passes the same instance for value and validationContext.ObjectInstance when validating an element in a collection. ASP.NET Core MVC passes the container (the collection), consistent with how properties are validated.

Note 2

The referenced ValidationAttribute code is

    string[] memberNames = validationContext.MemberName != null
        ? new string[] { validationContext.MemberName }
        : null;

Note 3

The available information about ValidationResult.MemberNames values other than null and new[] { validationContext.MemberName } is almost non-existent. For example, the .NET Validator does not interpret this property at all. (This is in part because Validator neither handles collection elements nor recurses to validate within a property.) The code in Note 2 is also about it for built-in ValidationAttributes.

Note 4

An example of a ValidationAttribute on a collection element type would be:

public class MyModel
{
    public ElementModel[] UniqueElements { get; set; }
}

[IsNotValid]
public class ElementModel
{
    public string Name { get; set; }
}

public class IsNotValidAttribute : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        return new ValidationResult(...);
    }
}

In the above IsNotValidAttribute, validationContext.MemberName == "ElementModel". That's useful to neither the attribute (which has validationContext.ObjectType) nor the calling code if it comes back in ValidationResult.MemberNames (because it can't be used in a property path). Perhaps it would be "nice" to pass in "[0]" or "[23]" but that works only when the collection is a dictionary or list. And, as I said earlier, the general sentiment is null is expected in this case.

[dougbu]

Following recommendations above, #4828 does not fix my "Problem 1". Most ValidationAttributes deal with lower-level values, not sibling properties. So "fix" would be debatable there.

[dougbu]

aecfe77

[tuespetre]

Thanks @dougbu!