Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Commit

Permalink
Fixes #2464 - Does not add extra skipped entries for model bound from…
Browse files Browse the repository at this point in the history 
… services.

Also ensures that when a type is marked as skipped, any sub property which is model bound (and hence a modelstate un validated entry),
is marked as skipped (otherwise it would cause the ModelState to be invalid).
Also fixing a bug in model state dictionary FindKeyWithPrefix was not considering [0] & [0][0] as a valid prefix.
  • Loading branch information
@harshgMSFT
harshgMSFT committed May 15, 2015
1 parent 88ac4b9 commit d0927bd
Show file tree
Hide file tree
Showing 15 changed files with 349 additions and 135 deletions.
17 changes: 16 additions & 1 deletion Mvc.sln
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 14
VisualStudioVersion = 14.0.22810.0
VisualStudioVersion = 14.0.22808.1
MinimumVisualStudioVersion = 10.0.40219.1
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "samples", "samples", "{DAAE4C74-D06F-4874-A166-33305D2643CE}"
EndProject
Expand Down Expand Up @@ -166,6 +166,8 @@ Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNet.Mvc.ApiExp
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNet.Mvc.ApiExplorer.Test", "test\Microsoft.AspNet.Mvc.ApiExplorer.Test\Microsoft.AspNet.Mvc.ApiExplorer.Test.xproj", "{4C2AD8AB-8AC0-46C4-80C6-C5577C7255F6}"
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNet.Mvc.Abstractions.Test", "test\Microsoft.AspNet.Mvc.Abstractions.Test\Microsoft.AspNet.Mvc.Abstractions.Test.xproj", "{DA000953-7532-4DF5-8DB9-8143DF98D999}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -996,6 +998,18 @@ Global
{4C2AD8AB-8AC0-46C4-80C6-C5577C7255F6}.Release|Mixed Platforms.Build.0 = Release|Any CPU
{4C2AD8AB-8AC0-46C4-80C6-C5577C7255F6}.Release|x86.ActiveCfg = Release|Any CPU
{4C2AD8AB-8AC0-46C4-80C6-C5577C7255F6}.Release|x86.Build.0 = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|Any CPU.Build.0 = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|x86.ActiveCfg = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Debug|x86.Build.0 = Debug|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|Any CPU.ActiveCfg = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|Any CPU.Build.0 = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|Mixed Platforms.Build.0 = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|x86.ActiveCfg = Release|Any CPU
{DA000953-7532-4DF5-8DB9-8143DF98D999}.Release|x86.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -1077,5 +1091,6 @@ Global
{1154203C-7579-4525-906E-BC55268421C1} = {32285FA4-6B46-4D6B-A840-2B13E4C8B58E}
{A2B72833-5D70-4C42-AE85-E0319926FB8A} = {32285FA4-6B46-4D6B-A840-2B13E4C8B58E}
{4C2AD8AB-8AC0-46C4-80C6-C5577C7255F6} = {3BA657BF-28B1-42DA-B5B0-1C4601FCF7B1}
{DA000953-7532-4DF5-8DB9-8143DF98D999} = {3BA657BF-28B1-42DA-B5B0-1C4601FCF7B1}
EndGlobalSection
EndGlobal
45 changes: 27 additions & 18 deletions src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ public bool TryAddModelError([NotNull] string key, [NotNull] string errorMessage
/// state errors; <see cref="ModelValidationState.Valid"/> otherwise.</returns>
public ModelValidationState GetFieldValidationState([NotNull] string key)
{
var entries = FindKeysWithPrefix(this, key);
var entries = FindKeysWithPrefix(key);
if (!entries.Any())
{
return ModelValidationState.Unvalidated;
Expand Down Expand Up @@ -378,7 +378,7 @@ public void ClearValidationState(string key)
// If key is null or empty, clear all entries in the dictionary
// else just clear the ones that have key as prefix
var entries = (string.IsNullOrEmpty(key)) ?
_innerDictionary : FindKeysWithPrefix(this, key);
_innerDictionary : FindKeysWithPrefix(key);

foreach (var entry in entries)
{
Expand Down Expand Up @@ -502,17 +502,15 @@ IEnumerator IEnumerable.GetEnumerator()
return GetEnumerator();
}

private static IEnumerable<KeyValuePair<string, TValue>> FindKeysWithPrefix<TValue>(
[NotNull] IDictionary<string, TValue> dictionary,
[NotNull] string prefix)
public IEnumerable<KeyValuePair<string, ModelState>> FindKeysWithPrefix([NotNull] string prefix)
{
TValue exactMatchValue;
if (dictionary.TryGetValue(prefix, out exactMatchValue))
ModelState exactMatchValue;
if (_innerDictionary.TryGetValue(prefix, out exactMatchValue))
{
yield return new KeyValuePair<string, TValue>(prefix, exactMatchValue);
yield return new KeyValuePair<string, ModelState>(prefix, exactMatchValue);
}

foreach (var entry in dictionary)
foreach (var entry in _innerDictionary)
{
var key = entry.Key;

Expand All @@ -521,21 +519,32 @@ private static IEnumerable<KeyValuePair<string, TValue>> FindKeysWithPrefix<TVal
continue;
}

if (key.StartsWith("[", StringComparison.OrdinalIgnoreCase))
if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
{
key = key.Substring(key.IndexOf('.') + 1);
if (string.Equals(prefix, key, StringComparison.Ordinal))

if (key.StartsWith("[", StringComparison.OrdinalIgnoreCase))
{
var subKey = key.Substring(key.IndexOf('.') + 1);

if (!subKey.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
{
continue;
}

if (string.Equals(prefix, subKey, StringComparison.Ordinal))
{
yield return entry;
continue;
}

key = subKey;
}
else
{
yield return entry;
continue;
}
}

if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
{
continue;
}

// Everything is prefixed by the empty string
if (prefix.Length == 0)
{
Expand Down
29 changes: 13 additions & 16 deletions src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,10 @@ private bool ValidateNonVisitedNodeAndChildren(
var currentValidationNode = validationContext.ValidationNode;
if (currentValidationNode.SuppressValidation)
{
// Short circuit if the node is marked to be suppressed
var validationState = modelState.GetFieldValidationState(modelKey);
if (validationState == ModelValidationState.Unvalidated)
{
modelValidationContext.ModelState.MarkFieldSkipped(modelKey);
}
// Short circuit if the node is marked to be suppressed.
// If there are any sub entries which were model bound, they need to be marked as skipped,
// Otherwise they will remain as unvalidated and the model state would be Invalid.
MarkChildNodesAsSkipped(modelKey, modelExplorer.Metadata, validationContext);

// For validation purposes this model is valid.
return true;
Expand Down Expand Up @@ -103,7 +101,10 @@ private bool ValidateNonVisitedNodeAndChildren(
if (IsTypeExcludedFromValidation(_excludeFilters, modelType))
{
var result = ShallowValidate(modelKey, modelExplorer, validationContext, validators);
MarkPropertiesAsSkipped(modelKey, modelExplorer.Metadata, validationContext);

// If there are any sub entries which were model bound, they need to be marked as skipped,
// Otherwise they will remain as unvalidated and the model state would be Invalid.
MarkChildNodesAsSkipped(modelKey, modelExplorer.Metadata, validationContext);
return result;
}

Expand All @@ -127,7 +128,7 @@ private bool ValidateNonVisitedNodeAndChildren(
return isValid;
}

private void MarkPropertiesAsSkipped(string currentModelKey, ModelMetadata metadata, ValidationContext validationContext)
private void MarkChildNodesAsSkipped(string currentModelKey, ModelMetadata metadata, ValidationContext validationContext)
{
var modelState = validationContext.ModelValidationContext.ModelState;
var fieldValidationState = modelState.GetFieldValidationState(currentModelKey);
Expand All @@ -140,15 +141,11 @@ private void MarkPropertiesAsSkipped(string currentModelKey, ModelMetadata metad
return;
}

foreach (var childMetadata in metadata.Properties)
// At this point we just want to mark all sub-entries present in the model state as skipped.
var entries = modelState.FindKeysWithPrefix(currentModelKey);
foreach (var entry in entries)
{
var childKey = ModelNames.CreatePropertyModelName(currentModelKey, childMetadata.PropertyName);
var validationState = modelState.GetFieldValidationState(childKey);

if (validationState == ModelValidationState.Unvalidated)
{
validationContext.ModelValidationContext.ModelState.MarkFieldSkipped(childKey);
}
entry.Value.ValidationState = ModelValidationState.Skipped;
}
}

Expand Down
39 changes: 39 additions & 0 deletions test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,45 @@ public void GetFieldValidationState_ReturnsValidIfModelStateDoesNotContainErrors
Assert.Equal(ModelValidationState.Valid, validationState);
}

[Theory]
[InlineData("[0].foo.bar")]
[InlineData("[0].foo.bar[0]")]
public void GetFieldValidationState_IndexedPrefix_ReturnsInvalidIfKeyChildContainsErrors(string key)
{
// Arrange
var msd = new ModelStateDictionary();
msd.AddModelError(key, "error text");

// Act
var validationState = msd.GetFieldValidationState("[0].foo");

// Assert
Assert.Equal(ModelValidationState.Invalid, validationState);
}

[Theory]
[InlineData("[0].foo.bar")]
[InlineData("[0].foo.bar[0]")]
public void GetFieldValidationState_IndexedPrefix_ReturnsValidIfModelStateDoesNotContainErrors(string key)
{
// Arrange
var validState = new ModelState
{
Value = new ValueProviderResult(null, null, null),
ValidationState = ModelValidationState.Valid
};
var msd = new ModelStateDictionary
{
{ key, validState }
};

// Act
var validationState = msd.GetFieldValidationState("[0].foo");

// Assert
Assert.Equal(ModelValidationState.Valid, validationState);
}

[Fact]
public void IsValidPropertyReturnsFalseIfErrors()
{
Expand Down
75 changes: 44 additions & 31 deletions test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Validation/DefaultObjectValidatorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ public void MultipleValidationErrorsOnSameMemberReported()
.Validate(validationContext, topLevelValidationNode);

// Assert
Assert.Equal(1, validationContext.ModelState.Count);
Assert.Contains("Street", validationContext.ModelState.Keys);
var streetState = validationContext.ModelState["Street"];
Assert.Equal(2, streetState.Errors.Count);
Expand Down Expand Up @@ -427,7 +428,7 @@ public void Validation_ShortCircuit_WhenMaxErrorCountIsSet()
validator.Validate(validationContext, topLevelValidationNode);

// Assert
Assert.Equal(new[] { "key1", "user.Password", "", "user.ConfirmPassword" },
Assert.Equal(new[] { "key1", "", "user.ConfirmPassword" },
validationContext.ModelState.Keys.ToArray());
var modelState = validationContext.ModelState["user.ConfirmPassword"];
Assert.Empty(modelState.Errors);
Expand All @@ -438,7 +439,7 @@ public void Validation_ShortCircuit_WhenMaxErrorCountIsSet()
}

[Fact]
public void ForExcludedNonModelBoundType_Properties_NotMarkedAsSkiped()
public void ForExcludedNonModelBoundTypes_NoEntryInModelState()
{
// Arrange
var user = new User()
Expand Down Expand Up @@ -468,11 +469,8 @@ public void ForExcludedNonModelBoundType_Properties_NotMarkedAsSkiped()
validator.Validate(validationContext, topLevelValidationNode);

// Assert
Assert.False(validationContext.ModelState.ContainsKey("user.Password"));
Assert.False(validationContext.ModelState.ContainsKey("user.ConfirmPassword"));
var modelState = validationContext.ModelState["user"];
Assert.Empty(modelState.Errors);
Assert.Equal(modelState.ValidationState, ModelValidationState.Valid);
Assert.True(validationContext.ModelState.IsValid);
Assert.Empty(validationContext.ModelState);
}

[Fact]
Expand Down Expand Up @@ -511,36 +509,44 @@ public void ForExcludedModelBoundTypes_Properties_MarkedAsSkipped()
validator.Validate(validationContext, topLevelValidationNode);

// Assert
var modelState = validationContext.ModelState["user.Password"];
Assert.Empty(modelState.Errors);
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
var entry = Assert.Single(validationContext.ModelState);
Assert.Equal("user.Password", entry.Key);
Assert.Empty(entry.Value.Errors);
Assert.Equal(entry.Value.ValidationState, ModelValidationState.Skipped);
}

modelState = validationContext.ModelState["user.ConfirmPassword"];
Assert.Empty(modelState.Errors);
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
private class Person2
{
public Address Address { get; set; }
}

[Fact]
public void Validate_IfSuppressIsSet_MarkedAsSkipped()
{
// Arrange
var testValidationContext = GetModelValidationContext(
new TestServiceProvider(),
typeof(TestServiceProvider));

new Person2()
{
Address = new Address { Street = "GreaterThan5Characters" }
},
typeof(Person2));
var validationContext = testValidationContext.ModelValidationContext;

// Create an entry like a model binder would.
validationContext.ModelState.Add("person.Address", new ModelState());

var validator = new DefaultObjectValidator(
testValidationContext.ExcludeFilters,
testValidationContext.ModelMetadataProvider);
var modelExplorer = testValidationContext.ModelValidationContext.ModelExplorer;
var topLevelValidationNode = new ModelValidationNode(
"serviceProvider",
"person",
modelExplorer.Metadata,
modelExplorer.Model);

var propertyExplorer = modelExplorer.GetExplorerForProperty("TestService");
var propertyExplorer = modelExplorer.GetExplorerForProperty("Address");
var childNode = new ModelValidationNode(
"serviceProvider.TestService",
"person.Address",
propertyExplorer.Metadata,
propertyExplorer.Model)
{
Expand All @@ -554,8 +560,8 @@ public void Validate_IfSuppressIsSet_MarkedAsSkipped()

// Assert
Assert.True(validationContext.ModelState.IsValid);
var modelState = validationContext.ModelState["serviceProvider.TestService"];
Assert.Empty(modelState.Errors);
Assert.Equal(1, validationContext.ModelState.Count);
var modelState = validationContext.ModelState["person.Address"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
}

Expand Down Expand Up @@ -603,7 +609,14 @@ public void EnumerableType_ValidationSuccessful(object model, Type type)

// Assert
Assert.True(validationContext.ModelState.IsValid);
var modelState = validationContext.ModelState["items"];
Assert.Equal(3, validationContext.ModelState.Count);
var modelState = validationContext.ModelState["items[0]"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Valid);

modelState = validationContext.ModelState["items[1]"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Valid);

modelState = validationContext.ModelState["items[2]"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Valid);
}

Expand Down Expand Up @@ -653,16 +666,15 @@ public void DictionaryType_ValidationSuccessful()

// Assert
Assert.True(validationContext.ModelState.IsValid);
var modelState = validationContext.ModelState["items"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Valid);
modelState = validationContext.ModelState["items[0].Key"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
Assert.Equal(4, validationContext.ModelState.Count);
var modelState = validationContext.ModelState["items[0].Key"];
Assert.Equal(ModelValidationState.Skipped, modelState.ValidationState);
modelState = validationContext.ModelState["items[0].Value"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
Assert.Equal(ModelValidationState.Skipped, modelState.ValidationState);
modelState = validationContext.ModelState["items[1].Key"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
Assert.Equal(ModelValidationState.Skipped, modelState.ValidationState);
modelState = validationContext.ModelState["items[1].Value"];
Assert.Equal(modelState.ValidationState, ModelValidationState.Skipped);
Assert.Equal(ModelValidationState.Skipped, modelState.ValidationState);
}

[Fact]
Expand Down Expand Up @@ -690,8 +702,9 @@ public void Validator_IfValidateAllPropertiesIsNotSet_DoesNotAutoExpand()

// Assert
Assert.True(validationContext.ModelState.IsValid);
var key = Assert.Single(validationContext.ModelState.Keys);
Assert.Equal("person", key);

// Since Person is not IValidatable and we do not look at its properties, the state is empty.
Assert.Empty(validationContext.ModelState.Keys);
}

[Fact]
Expand Down
Loading

0 comments on commit d0927bd

Please sign in to comment.