Add the ability to correctly determine if a particular field has been validated.

There are several portions of model validation that attempt to avoid
revalidating if a field has been validated. However the behavior of
ModelStateDictionary makes it difficult to distinguish between an
unvalidated field and a field without validation errors. This change
resolves this issue by letting the caller distinguish between the two
cases.
This commit is contained in:
Pranav K 2014-03-26 09:12:31 -07:00
parent 2de5d57348
commit c72c80c101
10 changed files with 200 additions and 60 deletions

View File

@ -113,7 +113,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var validationNode = (ModelValidationNode)sender;
var modelState = e.ValidationContext.ModelState;
if (modelState.IsValidField(validationNode.ModelStateKey))
if (modelState.IsValidField(validationNode.ModelStateKey) == null)
{
// TODO: Revive ModelBinderConfig
// string errorMessage = ModelBinderConfig.ValueRequiredErrorMessageProvider(e.ValidationContext, modelMetadata, incomingValue);
@ -266,7 +266,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
if (value == null)
{
var modelStateKey = dtoResult.ValidationNode.ModelStateKey;
if (bindingContext.ModelState.IsValidField(modelStateKey))
if (bindingContext.ModelState.IsValidField(modelStateKey) == null)
{
if (requiredValidator != null)
{
@ -295,7 +295,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
ex = targetInvocationException.InnerException;
}
var modelStateKey = dtoResult.ValidationNode.ModelStateKey;
if (bindingContext.ModelState.IsValidField(modelStateKey))
if (bindingContext.ModelState.IsValidField(modelStateKey) == null)
{
bindingContext.ModelState.AddModelError(modelStateKey, ex);
}
@ -305,7 +305,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
// trying to set a non-nullable value type to null, need to make sure there's a message
var modelStateKey = dtoResult.ValidationNode.ModelStateKey;
if (bindingContext.ModelState.IsValidField(modelStateKey))
if (bindingContext.ModelState.IsValidField(modelStateKey) == null)
{
dtoResult.ValidationNode.Validated += CreateNullCheckFailedHandler(propertyMetadata, value);
}
@ -326,6 +326,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
bindingContext.ModelState.AddModelError(modelStateKey, validationResult.Message);
addedError = true;
}
if (!addedError)
{
bindingContext.ModelState.MarkFieldValid(modelStateKey);
}
return addedError;
}

View File

@ -11,5 +11,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
get { return _errors; }
}
public bool? IsValid { get; set; }
}
}

View File

@ -44,9 +44,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
#endregion
public bool IsValid
public bool? IsValid
{
get { return Values.All(modelState => modelState.Errors.Count == 0); }
get { return GetValidity(_innerDictionary); }
}
public ModelState this[[NotNull] string key]
@ -62,25 +62,39 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
public void AddModelError([NotNull] string key, [NotNull] Exception exception)
{
GetModelStateForKey(key).Errors.Add(exception);
var modelState = GetModelStateForKey(key);
modelState.IsValid = false;
modelState.Errors.Add(exception);
}
public void AddModelError([NotNull] string key, [NotNull] string errorMessage)
{
GetModelStateForKey(key).Errors.Add(errorMessage);
var modelState = GetModelStateForKey(key);
modelState.IsValid = false;
modelState.Errors.Add(errorMessage);
}
public bool IsValidField([NotNull] string key)
public bool? IsValidField([NotNull] string key)
{
// if the key is not found in the dictionary, we just say that it's valid (since there are no errors)
foreach (var entry in DictionaryHelper.FindKeysWithPrefix(_innerDictionary, key))
var entries = DictionaryHelper.FindKeysWithPrefix(this, key);
if (!entries.Any())
{
if (entry.Value.Errors.Count != 0)
{
return false;
}
return null;
}
return true;
return GetValidity(entries);
}
public void MarkFieldValid([NotNull] string key)
{
var modelState = GetModelStateForKey(key);
if (modelState.IsValid == false)
{
// TODO We should never end up here from our code
throw new InvalidOperationException(Resources.Validation_InvalidFieldCannotBeReset);
}
modelState.IsValid = true;
}
public void Merge(ModelStateDictionary dictionary)
@ -113,6 +127,25 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
return modelState;
}
private static bool? GetValidity(IEnumerable<KeyValuePair<string, ModelState>> entries)
{
var state = true;
foreach (var entry in entries)
{
var entryState = entry.Value.IsValid;
if (entryState == null)
{
// If any entries of a field is unvalidated, we'll treat the tree as unvalidated.
return null;
}
else if (!entryState.Value)
{
state = false;
}
}
return state;
}
#region IDictionary members
public void Add(KeyValuePair<string, ModelState> item)
{
@ -144,7 +177,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
_innerDictionary.CopyTo(array, arrayIndex);
}
public bool Remove(KeyValuePair<string, ModelState> item)
{
return _innerDictionary.Remove(item);

View File

@ -59,7 +59,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
/// <summary>
/// No encoding found for media type formatter '{0}'. There must be at least one supported encoding registered in order for the media type formatter to read or write content.
/// No encoding found for input formatter '{0}'. There must be at least one supported encoding registered in order for the formatter to read content.
/// </summary>
internal static string MediaTypeFormatterNoEncoding
{
@ -67,12 +67,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
/// <summary>
/// No encoding found for media type formatter '{0}'. There must be at least one supported encoding registered in order for the media type formatter to read or write content.
/// No encoding found for input formatter '{0}'. There must be at least one supported encoding registered in order for the formatter to read content.
/// </summary>
internal static string FormatMediaTypeFormatterNoEncoding(object p0)
{
return string.Format(CultureInfo.CurrentCulture, GetString("MediaTypeFormatterNoEncoding"), p0);
}
/// <summary>
/// Property '{0}' on type '{1}' is invalid. Value-typed properties marked as [Required] must also be marked with [DataMember(IsRequired=true)] to be recognized as required. Consider attributing the declaring type with [DataContract] and the property with [DataMember(IsRequired=true)].
/// </summary>
internal static string MissingDataMemberIsRequired
@ -264,6 +266,22 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
return string.Format(CultureInfo.CurrentCulture, GetString("ValidationAttributeOnNonPublicProperty"), p0, p1);
}
/// <summary>
/// A field previously marked invalid should not be marked valid.
/// </summary>
internal static string Validation_InvalidFieldCannotBeReset
{
get { return GetString("Validation_InvalidFieldCannotBeReset"); }
}
/// <summary>
/// A field previously marked invalid should not be marked valid.
/// </summary>
internal static string FormatValidation_InvalidFieldCannotBeReset()
{
return GetString("Validation_InvalidFieldCannotBeReset");
}
/// <summary>
/// A value is required but was not present in the request.
/// </summary>

View File

@ -165,6 +165,9 @@
<data name="ValidationAttributeOnNonPublicProperty" xml:space="preserve">
<value>Non-public property '{0}' on type '{1}' is attributed with one or more validation attributes. Validation attributes on non-public properties are not supported. Consider using a public property for validation instead.</value>
</data>
<data name="Validation_InvalidFieldCannotBeReset" xml:space="preserve">
<value>A field previously marked invalid should not be marked valid.</value>
</data>
<data name="Validation_ValueNotFound" xml:space="preserve">
<value>A value is required but was not present in the request.</value>
</data>

View File

@ -119,6 +119,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// post-validation steps
var validatedEventArgs = new ModelValidatedEventArgs(validationContext, parentNode);
OnValidated(validatedEventArgs);
var modelState = validationContext.ModelState;
if (modelState.IsValidField(ModelStateKey) != false)
{
// If a node or its subtree were not marked invalid, we can consider it valid at this point.
modelState.MarkFieldValid(ModelStateKey);
}
}
private void ValidateChildren(ModelValidationContext validationContext)
@ -149,7 +156,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// else we could end up with duplicate or irrelevant error messages.
var propertyKeyRoot = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, propertyMetadata.PropertyName);
if (modelState.IsValidField(propertyKeyRoot))
if (modelState.IsValidField(propertyKeyRoot) == null)
{
var propertyValidators = GetValidators(validationContext, propertyMetadata);
var propertyValidationContext = new ModelValidationContext(validationContext, propertyMetadata);
@ -168,18 +175,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
private void ValidateThis(ModelValidationContext validationContext, ModelValidationNode parentNode)
{
var modelState = validationContext.ModelState;
if (!modelState.IsValidField(ModelStateKey))
if (modelState.IsValidField(ModelStateKey) == false)
{
return; // short-circuit
// If any item in the key's subtree has been identified as invalid, short-circuit
return;
}
// If the Model at the current node is null and there is no parent, we cannot validate, and the
// DataAnnotationsModelValidator will throw. So we intercept here to provide a catch-all value-required
// validation error
var modelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, ModelMetadata.GetDisplayName());
if (parentNode == null && ModelMetadata.Model == null)
{
var trueModelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, ModelMetadata.GetDisplayName());
modelState.AddModelError(trueModelStateKey, Resources.Validation_ValueNotFound);
modelState.AddModelError(modelStateKey, Resources.Validation_ValueNotFound);
return;
}
@ -190,8 +198,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var validator = validators[i];
foreach (var validationResult in validator.Validate(validationContext))
{
var trueModelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, validationResult.MemberName);
modelState.AddModelError(trueModelStateKey, validationResult.Message);
var currentModelStateKey = ModelBindingHelper.CreatePropertyModelName(ModelStateKey, validationResult.MemberName);
modelState.AddModelError(currentModelStateKey, validationResult.Message);
}
}
}

View File

@ -54,7 +54,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
Assert.Equal(42, bindingContext.Model);
Assert.True(validationCalled);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
}
[Fact]
@ -106,7 +106,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
Assert.True(isBound);
Assert.Equal(expectedModel, bindingContext.Model);
Assert.True(validationCalled);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
}
[Fact]
@ -132,7 +132,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
// Assert
Assert.False(isBound);
Assert.Null(bindingContext.Model);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
mockListBinder.Verify();
}

View File

@ -319,7 +319,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(1, modelStateDictionary.Count);
// Check Age error.
@ -377,13 +377,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(1, modelStateDictionary.Count);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(2, modelStateDictionary.Count);
// Check Name field
ModelState modelState;
Assert.True(modelStateDictionary.TryGetValue("theModel.Name", out modelState));
Assert.Equal(0, modelState.Errors.Count);
Assert.Equal(true, modelState.IsValid);
// Check Age error.
ModelState modelState;
Assert.True(modelStateDictionary.TryGetValue("theModel.Age", out modelState));
Assert.Equal(1, modelState.Errors.Count);
Assert.Equal(false, modelState.IsValid);
var modelError = modelState.Errors[0];
Assert.Null(modelError.Exception);
@ -409,7 +415,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(2, modelStateDictionary.Count);
// Check Age error.
@ -457,7 +463,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(1, modelStateDictionary.Count);
// Check City error.
@ -488,7 +494,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(1, modelStateDictionary.Count);
// Check ValueTypeRequired error.
@ -524,7 +530,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
ModelStateDictionary modelStateDictionary = bindingContext.ModelState;
Assert.False(modelStateDictionary.IsValid);
Assert.Equal(false, modelStateDictionary.IsValid);
Assert.Equal(1, modelStateDictionary.Count);
// Check ValueTypeRequired error.
@ -568,7 +574,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
Assert.Equal("John", model.FirstName);
Assert.Equal("Doe", model.LastName);
Assert.Equal(dob, model.DateOfBirth);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
}
[Fact]
@ -593,7 +599,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var person = Assert.IsType<Person>(bindingContext.Model);
Assert.Equal(123.456m, person.PropertyWithDefaultValue);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
}
[Fact]
@ -637,7 +643,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
validationNode.Validate(validationContext);
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
Assert.Equal(new DateTime(2001, 1, 1), model.DateOfBirth);
}
@ -684,9 +690,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
testableBinder.SetPropertyPublic(bindingContext, propertyMetadata, dtoResult, requiredValidator);
// Assert
Assert.True(bindingContext.ModelState.IsValid);
Assert.Equal(true, bindingContext.ModelState.IsValid);
validationNode.Validate(validationContext, bindingContext.ValidationNode);
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal(false, bindingContext.ModelState.IsValid);
}
[Fact]
@ -706,7 +712,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
testableBinder.SetPropertyPublic(bindingContext, propertyMetadata, dtoResult, requiredValidator);
// Assert
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal(false, bindingContext.ModelState.IsValid);
Assert.Equal("Sample message", bindingContext.ModelState["foo.ValueTypeRequired"].Errors[0].ErrorMessage);
}
@ -728,7 +734,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
testableBinder.SetPropertyPublic(bindingContext, propertyMetadata, dtoResult, requiredValidator);
// Assert
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal(false, bindingContext.ModelState.IsValid);
Assert.Equal(1, bindingContext.ModelState["foo.NameNoAttribute"].Errors.Count);
Assert.Equal("This is a different exception." + Environment.NewLine
+ "Parameter name: value",
@ -752,7 +758,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
testableBinder.SetPropertyPublic(bindingContext, propertyMetadata, dtoResult, requiredValidator);
// Assert
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal(false, bindingContext.ModelState.IsValid);
Assert.Equal(1, bindingContext.ModelState["foo.Name"].Errors.Count);
Assert.Equal("This message comes from the [Required] attribute.", bindingContext.ModelState["foo.Name"].Errors[0].ErrorMessage);
}

View File

@ -27,7 +27,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
// Assert
Assert.False(retVal);
Assert.Null(bindingContext.Model);
Assert.False(bindingContext.ModelState.IsValid);
Assert.Equal(false, bindingContext.ModelState.IsValid);
Assert.Equal("Input string was not in a correct format.", bindingContext.ModelState["theModelName"].Errors[0].ErrorMessage);
}

View File

@ -60,7 +60,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
[Fact]
public void IsValidFieldReturnsFalseIfDictionaryDoesNotContainKey()
public void IsValidFieldReturnsNullIfDictionaryDoesNotContainKey()
{
// Arrange
var msd = new ModelStateDictionary();
@ -69,7 +69,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var isValid = msd.IsValidField("foo");
// Assert
Assert.True(isValid);
Assert.Null(isValid);
}
[Fact]
@ -83,7 +83,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var isValid = msd.IsValidField("foo");
// Assert
Assert.False(isValid);
Assert.Equal(false, isValid);
}
[Fact]
@ -97,7 +97,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var isValid = msd.IsValidField("foo");
// Assert
Assert.False(isValid);
Assert.Equal(false, isValid);
}
[Fact]
@ -106,25 +106,25 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Arrange
var msd = new ModelStateDictionary()
{
{ "foo", new ModelState() { Value = new ValueProviderResult(null, null, null) } }
{ "foo", new ModelState() { Value = new ValueProviderResult(null, null, null), IsValid = true } }
};
// Act
var isValid = msd.IsValidField("foo");
// Assert
Assert.True(isValid);
Assert.Equal(true, isValid);
}
[Fact]
public void IsValidPropertyReturnsFalseIfErrors()
{
// Arrange
var errorState = new ModelState() { Value = GetValueProviderResult("quux", "quux") };
var errorState = new ModelState() { Value = GetValueProviderResult("quux", "quux"), IsValid = false };
errorState.Errors.Add("some error");
var dictionary = new ModelStateDictionary()
{
{ "foo", new ModelState() { Value = GetValueProviderResult("bar", "bar") } },
{ "foo", new ModelState() { Value = GetValueProviderResult("bar", "bar"), IsValid = true } },
{ "baz", errorState }
};
@ -132,7 +132,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
var isValid = dictionary.IsValid;
// Assert
Assert.False(isValid);
Assert.Equal(false, isValid);
}
[Fact]
@ -141,15 +141,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Arrange
var dictionary = new ModelStateDictionary()
{
{ "foo", new ModelState() { Value = GetValueProviderResult("bar", "bar") } },
{ "baz", new ModelState() { Value = GetValueProviderResult("quux", "bar") } }
{ "foo", new ModelState() { IsValid = true, Value = GetValueProviderResult("bar", "bar") } },
{ "baz", new ModelState() { IsValid = true, Value = GetValueProviderResult("quux", "bar") } }
};
// Act
var isValid = dictionary.IsValid;
// Assert
Assert.True(isValid);
Assert.Equal(true, isValid);
}
[Fact]
@ -218,9 +218,74 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
Assert.Equal("some value", modelState.Value.ConvertTo(typeof(string)));
}
private static ValueProviderResult GetValueProviderResult(object rawValue, string attemptedValue)
[Fact]
public void GetFieldValidity_ReturnsUnvalidated_IfNoEntryExistsForKey()
{
return new ValueProviderResult(rawValue, attemptedValue, CultureInfo.InvariantCulture);
// Arrange
var dictionary = new ModelStateDictionary();
dictionary.SetModelValue("user.Name", GetValueProviderResult());
// Act
var isValidField = dictionary.IsValidField("not-user");
// Assert
Assert.Equal(null, isValidField);
}
[Fact]
public void GetFieldValidity_ReturnsUnvalidated_IfAnyItemInSubtreeIsInvalid()
{
// Arrange
var dictionary = new ModelStateDictionary();
dictionary["user.Address"] = new ModelState { IsValid = true };
dictionary.SetModelValue("user.Name", GetValueProviderResult());
dictionary.AddModelError("user.Age", "Age is not a valid int");
// Act
var isValidField = dictionary.IsValidField("user");
// Assert
Assert.Equal(null, isValidField);
}
[Theory]
[InlineData("user")]
[InlineData("user.Age")]
public void GetFieldValidity_ReturnsInvalid_IfAllKeysAreValidatedAndAnyEntryIsInvalid(string key)
{
// Arrange
var dictionary = new ModelStateDictionary();
dictionary["user.Address"] = new ModelState { IsValid = true };
dictionary["user.Name"] = new ModelState { IsValid = true };
dictionary.AddModelError("user.Age", "Age is not a valid int");
// Act
var isValidField = dictionary.IsValidField(key);
// Assert
Assert.Equal(false, isValidField);
}
[Fact]
public void GetFieldValidity_ReturnsValid_IfAllKeysAreValid()
{
// Arrange
var dictionary = new ModelStateDictionary();
dictionary["user.Address"] = new ModelState { IsValid = true };
dictionary["user.Name"] = new ModelState { IsValid = true };
// Act
var isValidField = dictionary.IsValidField("user");
// Assert
Assert.Equal(true, isValidField);
}
private static ValueProviderResult GetValueProviderResult(object rawValue = null, string attemptedValue = null)
{
return new ValueProviderResult(rawValue ?? "some value",
attemptedValue ?? "some value",
CultureInfo.InvariantCulture);
}
}
}