diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs index d9c7881c22..4d97e7d8eb 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs @@ -683,49 +683,30 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding if (prefix.Length == 0) { - // Everything is prefixed by the empty string + // Everything is prefixed by the empty string. return true; } - if (key.Length < prefix.Length) + if (prefix.Length > key.Length) + { + return false; // Not long enough. + } + + if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) { return false; } - - var subKeyIndex = 0; - if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + + if (key.Length == prefix.Length) { - if (key[0] == '[') - { - subKeyIndex = key.IndexOf('.') + 1; - - if (string.Compare(key, subKeyIndex, prefix, 0, prefix.Length, StringComparison.OrdinalIgnoreCase) != 0) - { - return false; - } - else if (prefix.Length == (key.Length - subKeyIndex)) - { - // prefix == subKey - return true; - } - } - else - { - return false; - } - } - else if (key.Length == prefix.Length) - { - // key == prefix + // Exact match return true; } - var charAfterPrefix = key[subKeyIndex + prefix.Length]; - switch (charAfterPrefix) + var charAfterPrefix = key[prefix.Length]; + if (charAfterPrefix == '.' || charAfterPrefix == '[') { - case '[': - case '.': - return true; + return true; } return false; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs b/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs index 89a171d257..c55f49e80f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs @@ -1429,21 +1429,12 @@ namespace Microsoft.AspNetCore.Mvc { throw new ArgumentNullException(nameof(model)); } - - var modelName = prefix ?? string.Empty; - - // Clear ModelStateDictionary entries for the model so that it will be re-validated. - ModelBindingHelper.ClearValidationStateForModel( - model.GetType(), - ModelState, - MetadataProvider, - modelName); - + ObjectValidator.Validate( ControllerContext, new CompositeModelValidatorProvider(ControllerContext.ValidatorProviders), validationState: null, - prefix: prefix, + prefix: prefix ?? string.Empty, model: model); return ModelState.IsValid; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ElementalValueProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ElementalValueProvider.cs index a021b6642a..83bfeed66a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ElementalValueProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ElementalValueProvider.cs @@ -24,7 +24,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal public bool ContainsPrefix(string prefix) { - return PrefixContainer.IsPrefixMatch(prefix, Key); + return ModelStateDictionary.StartsWithPrefix(prefix, Key); } public ValueProviderResult GetValue(string key) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/PrefixContainer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/PrefixContainer.cs index 4d26b0a1f4..1bdb186b88 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/PrefixContainer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/PrefixContainer.cs @@ -173,45 +173,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - public static bool IsPrefixMatch(string prefix, string testString) - { - if (testString == null) - { - return false; - } - - if (prefix.Length == 0) - { - return true; // shortcut - non-null testString matches empty prefix - } - - if (prefix.Length > testString.Length) - { - return false; // not long enough - } - - if (!testString.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) - { - return false; // prefix doesn't match - } - - if (testString.Length == prefix.Length) - { - return true; // exact match - } - - // invariant: testString.Length > prefix.Length - switch (testString[prefix.Length]) - { - case '.': - case '[': - return true; // known delimiters - - default: - return false; // not known delimiter - } - } - + // This is tightly coupled to the definition at ModelStateDictionary.StartsWithPrefix private int BinarySearch(string prefix) { var start = 0; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBindingHelper.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBindingHelper.cs index b5d018418e..5b120d9f05 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBindingHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBindingHelper.cs @@ -513,10 +513,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } var modelMetadata = metadataProvider.GetMetadataForType(modelType); - - // Clear ModelStateDictionary entries for the model so that it will be re-validated. var modelState = actionContext.ModelState; - ClearValidationStateForModel(modelType, modelState, metadataProvider, prefix); var operationBindingContext = new OperationBindingContext { @@ -659,27 +656,76 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(metadataProvider)); } - // If modelkey is empty, we need to iterate through properties (obtained from ModelMetadata) and - // clear validation state for all entries in ModelStateDictionary that start with each property name. - // If modelkey is non-empty, clear validation state for all entries in ModelStateDictionary - // that start with modelKey + ClearValidationStateForModel(metadataProvider.GetMetadataForType(modelType), modelstate, modelKey); + } + + /// + /// Clears entries for . + /// + /// The . + /// The entry to clear. + /// The . + public static void ClearValidationStateForModel( + ModelMetadata modelMetadata, + ModelStateDictionary modelstate, + string modelKey) + { + if (modelMetadata == null) + { + throw new ArgumentNullException(nameof(modelMetadata)); + } + + if (modelstate == null) + { + throw new ArgumentNullException(nameof(modelstate)); + } + if (string.IsNullOrEmpty(modelKey)) { - var modelMetadata = metadataProvider.GetMetadataForType(modelType); - var elementMetadata = modelMetadata.ElementMetadata; - if (elementMetadata != null) + // If model key is empty, we have to do a best guess to try and clear the appropriate + // keys. Clearing the empty prefix would clear the state of ALL entries, which might wipe out + // data from other models. + if (modelMetadata.IsEnumerableType) { - modelMetadata = elementMetadata; + // We expect that any key beginning with '[' is an index. We can't just infer the indexes + // used, so we clear all keys that look like index>. + // + // In the unlikely case that multiple top-level collections where bound to the empty prefix, + // you're just out of luck. + foreach (var kvp in modelstate) + { + if (kvp.Key.Length > 0 && kvp.Key[0] == '[') + { + // Starts with an indexer + kvp.Value.Errors.Clear(); + kvp.Value.ValidationState = ModelValidationState.Unvalidated; + } + } } - - foreach (var property in modelMetadata.Properties) + else if (modelMetadata.IsComplexType) { - var childKey = property.BinderModelName ?? property.PropertyName; - modelstate.ClearValidationState(childKey); + foreach (var property in modelMetadata.Properties) + { + modelstate.ClearValidationState(property.BinderModelName ?? property.PropertyName); + } + } + else + { + // Simple types bind to a single entry. So clear the entry with the empty-key, in the + // unlikely event that it has errors. + var entry = modelstate[string.Empty]; + if (entry != null) + { + entry.Errors.Clear(); + entry.ValidationState = ModelValidationState.Unvalidated; + } } } else { + // If model key is non-empty, we just want to clear all keys with that prefix. We expect + // model binding to have only used this key (and suffixes) for all entries related to + // this model. modelstate.ClearValidationState(modelKey); } } diff --git a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs index c507471ca7..3d6c01778b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs @@ -245,8 +245,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [Theory] [InlineData("foo")] [InlineData("foo.bar")] - [InlineData("[0].foo.bar")] - [InlineData("[0].foo.bar[0]")] + [InlineData("foo[bar]")] public void GetFieldValidationState_ReturnsInvalidIfKeyChildContainsErrors(string key) { // Arrange @@ -263,8 +262,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [Theory] [InlineData("foo")] [InlineData("foo.bar")] - [InlineData("[0].foo.bar")] - [InlineData("[0].foo.bar[0]")] + [InlineData("foo[bar]")] public void GetFieldValidationState_ReturnsValidIfModelStateDoesNotContainErrors(string key) { // Arrange @@ -505,9 +503,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } [Theory] + [InlineData("")] [InlineData("user")] [InlineData("user.Age")] - [InlineData("product")] public void GetFieldValidity_ReturnsInvalid_IfAllKeysAreValidatedAndAnyEntryIsInvalid(string key) { // Arrange @@ -515,9 +513,26 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding dictionary["user.Address"] = new ModelStateEntry { ValidationState = ModelValidationState.Valid }; dictionary["user.Name"] = new ModelStateEntry { ValidationState = ModelValidationState.Valid }; dictionary.AddModelError("user.Age", "Age is not a valid int"); + + // Act + var validationState = dictionary.GetFieldValidationState(key); + + // Assert + Assert.Equal(ModelValidationState.Invalid, validationState); + } + + [Theory] + [InlineData("")] + [InlineData("[0]")] + [InlineData("[0].product")] + public void GetFieldValidity_ReturnsInvalid_IfAllKeysAreValidatedAndAnyEntryIsInvalid_Collection(string key) + { + // Arrange + var dictionary = new ModelStateDictionary(); + dictionary["[0].product.Name"] = new ModelStateEntry { ValidationState = ModelValidationState.Valid }; dictionary["[0].product.Age[0]"] = new ModelStateEntry { ValidationState = ModelValidationState.Valid }; - dictionary.AddModelError("[1].product.Name", "Name is invalid"); + dictionary.AddModelError("[0].product.Name", "Name is invalid"); // Act var validationState = dictionary.GetFieldValidationState(key); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs index 443a74819e..24f1033e1c 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs @@ -687,10 +687,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [Theory] [InlineData("")] [InlineData(null)] - public void ClearValidationStateForModel_EmtpyModelKey(string modelKey) + public void ClearValidationState_ForComplexTypeModel_EmptyModelKey(string modelKey) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); + var modelMetadata = metadataProvider.GetMetadataForType(typeof(Product)); + var dictionary = new ModelStateDictionary(); dictionary["Name"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; dictionary.AddModelError("Name", "MyProperty invalid."); @@ -699,12 +701,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding dictionary.AddModelError("Id", "Id is required."); dictionary["Category"] = new ModelStateEntry { ValidationState = ModelValidationState.Valid }; + dictionary["Unrelated"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; + dictionary.AddModelError("Unrelated", "Unrelated is required."); + // Act - ModelBindingHelper.ClearValidationStateForModel( - typeof(Product), - dictionary, - metadataProvider, - modelKey); + ModelBindingHelper.ClearValidationStateForModel(modelMetadata, dictionary, modelKey); // Assert Assert.Equal(0, dictionary["Name"].Errors.Count); @@ -713,15 +714,48 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal(ModelValidationState.Unvalidated, dictionary["Id"].ValidationState); Assert.Equal(0, dictionary["Category"].Errors.Count); Assert.Equal(ModelValidationState.Unvalidated, dictionary["Category"].ValidationState); + + Assert.Equal(1, dictionary["Unrelated"].Errors.Count); + Assert.Equal(ModelValidationState.Invalid, dictionary["Unrelated"].ValidationState); + } + + // Not a wholly realistic scenario, but testing it regardless. + [Theory] + [InlineData("")] + [InlineData(null)] + public void ClearValidationState_ForSimpleTypeModel_EmptyModelKey(string modelKey) + { + // Arrange + var metadataProvider = new EmptyModelMetadataProvider(); + var modelMetadata = metadataProvider.GetMetadataForType(typeof(string)); + + var dictionary = new ModelStateDictionary(); + dictionary[string.Empty] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; + dictionary.AddModelError(string.Empty, "MyProperty invalid."); + + dictionary["Unrelated"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; + dictionary.AddModelError("Unrelated", "Unrelated is required."); + + // Act + ModelBindingHelper.ClearValidationStateForModel(modelMetadata, dictionary, modelKey); + + // Assert + Assert.Equal(0, dictionary[string.Empty].Errors.Count); + Assert.Equal(ModelValidationState.Unvalidated, dictionary[string.Empty].ValidationState); + + Assert.Equal(1, dictionary["Unrelated"].Errors.Count); + Assert.Equal(ModelValidationState.Invalid, dictionary["Unrelated"].ValidationState); } [Theory] [InlineData("")] [InlineData(null)] - public void ClearValidationStateForCollectionsModel_EmtpyModelKey(string modelKey) + public void ClearValidationState_ForCollectionsModel_EmptyModelKey(string modelKey) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); + var modelMetadata = metadataProvider.GetMetadataForType(typeof(List)); + var dictionary = new ModelStateDictionary(); dictionary["[0].Name"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; dictionary.AddModelError("[0].Name", "Name invalid."); @@ -735,12 +769,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding dictionary["[1].Category"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; dictionary.AddModelError("[1].Category", "Category invalid."); + dictionary["Unrelated"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; + dictionary.AddModelError("Unrelated", "Unrelated is required."); + // Act - ModelBindingHelper.ClearValidationStateForModel( - typeof(List), - dictionary, - metadataProvider, - modelKey); + ModelBindingHelper.ClearValidationStateForModel(modelMetadata, dictionary, modelKey); // Assert Assert.Equal(0, dictionary["[0].Name"].Errors.Count); @@ -755,6 +788,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal(ModelValidationState.Unvalidated, dictionary["[1].Id"].ValidationState); Assert.Equal(0, dictionary["[1].Category"].Errors.Count); Assert.Equal(ModelValidationState.Unvalidated, dictionary["[1].Category"].ValidationState); + + Assert.Equal(1, dictionary["Unrelated"].Errors.Count); + Assert.Equal(ModelValidationState.Invalid, dictionary["Unrelated"].ValidationState); } [Theory] @@ -764,10 +800,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData("product.Order[0].Address.Street")] [InlineData("product.Category.Name")] [InlineData("product.Order")] - public void ClearValidationStateForModel_NonEmtpyModelKey(string prefix) + public void ClearValidationState_ForComplexModel_NonEmptyModelKey(string prefix) { // Arrange - var metadataProvider = new TestModelMetadataProvider(); + var metadataProvider = new EmptyModelMetadataProvider(); + var modelMetadata = metadataProvider.GetMetadataForType(typeof(Product)); var dictionary = new ModelStateDictionary(); dictionary["product.Name"] = new ModelStateEntry { ValidationState = ModelValidationState.Invalid }; @@ -787,11 +824,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding dictionary.AddModelError("product.Order[0]", "Order invalid."); // Act - ModelBindingHelper.ClearValidationStateForModel( - typeof(Product), - dictionary, - metadataProvider, - prefix); + ModelBindingHelper.ClearValidationStateForModel(modelMetadata, dictionary, prefix); // Assert foreach (var entry in dictionary.Keys) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/SimpleValueProvider.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/SimpleValueProvider.cs index c8e2712749..0fc396d21e 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/SimpleValueProvider.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/SimpleValueProvider.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Globalization; -using Microsoft.AspNetCore.Mvc.Internal; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test { @@ -27,7 +26,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test { foreach (string key in Keys) { - if (PrefixContainer.IsPrefixMatch(prefix, key)) + if (ModelStateDictionary.StartsWithPrefix(prefix, key)) { return true; }