Closed jamesfoster closed 8 years ago
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; }
}
}
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?
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?
@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.
@dougbu please discuss with @rynowak
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.
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.
In the context of MVC's calls to ValidationAttribute
's
public ValidationResult GetValidationResult(object, ValidationContext)
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.)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.ValidationResult.MemberNames
is ignored. A ValidationAttribute
cannot identify more than one member as invalid.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.We should not fix (1.) because custom ValidationAttribute
s 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.
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.
The referenced ValidationAttribute
code is
string[] memberNames = validationContext.MemberName != null
? new string[] { validationContext.MemberName }
: null;
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 ValidationAttribute
s.
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.
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.
aecfe77
Thanks @dougbu!
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 aList<string>
to contain only unique entries.For example:
However, when returning the following validation result ...
... 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: https://github.com/aspnet/Mvc/blob/f7a211c09531f145329d76fe71d6676d5c4f3685/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs#L119