From b557ca55d9f66e361b05e788cae7521c9ae72542 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 10 Feb 2016 13:11:13 -0800 Subject: [PATCH] Fix behavior of StartsWithPrefix This undoes a behavior change introduced in 7b18d1d3f1f04ba8952b13fd2b0f903e684bf78d. The intent was to have ClearValidationState do the right thing for a case where a collection was bound to the empty prefix, and then used again with TryUpdateModel. This change was implemented by saying that a key like "[0].Foo" is a match for the prefix of "Foo". This isn't really right, and it's only interesting for the ClearValidationState case. The problem is that we don't know what the keys look like for a collection. We can assume that they start with [0] but that's not really a guarantee, it's a guess. This change fixes the behavior of StartsWithModel, and move the responsibility for this case back into ClearValidationState. This change also removes the call to ClearValidationState from TryUpdateModel. If you need this behavior, then call ClearValidationState manually. Trying to bind and then re-bind a model object isn't really what we intend. --- .../ModelBinding/ModelStateDictionary.cs | 39 +++------- .../ControllerBase.cs | 13 +--- .../Internal/ElementalValueProvider.cs | 2 +- .../Internal/PrefixContainer.cs | 40 +--------- .../ModelBinding/ModelBindingHelper.cs | 76 +++++++++++++++---- .../ModelBinding/ModelStateDictionaryTest.cs | 27 +++++-- .../ModelBinding/ModelBindingHelperTest.cs | 71 ++++++++++++----- .../ModelBinding/SimpleValueProvider.cs | 3 +- 8 files changed, 149 insertions(+), 122 deletions(-) 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 @@ public static bool StartsWithPrefix(string prefix, string key) 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; + return false; // Not long enough. } - var subKeyIndex = 0; if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) { - 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; - } + return false; } - else if (key.Length == prefix.Length) + + 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 @@ public virtual bool TryValidateModel( { 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 @@ public ElementalValueProvider(string key, string value, CultureInfo culture) 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 @@ private static void GetKeyFromNonEmptyPrefix(string prefix, string entry, IDicti } } - 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 @@ public static async Task TryUpdateModelAsync( } 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 @@ public static void ClearValidationStateForModel( 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 @@ public void GetValidationState_ReturnsValidationStateForKey_IgnoresChildren() [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 @@ public void GetFieldValidationState_ReturnsInvalidIfKeyChildContainsErrors(strin [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 @@ public void GetFieldValidity_ReturnsUnvalidated_IfAnyItemInSubtreeIsInvalid() } [Theory] + [InlineData("")] [InlineData("user")] [InlineData("user.Age")] - [InlineData("product")] public void GetFieldValidity_ReturnsInvalid_IfAllKeysAreValidatedAndAnyEntryIsInvalid(string key) { // Arrange @@ -515,9 +513,26 @@ public void GetFieldValidity_ReturnsInvalid_IfAllKeysAreValidatedAndAnyEntryIsIn 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 @@ public async Task TryUpdataModel_ModelTypeDifferentFromModel_Throws() [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 @@ public void ClearValidationStateForModel_EmtpyModelKey(string modelKey) 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 @@ public void ClearValidationStateForModel_EmtpyModelKey(string modelKey) 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 @@ public void ClearValidationStateForCollectionsModel_EmtpyModelKey(string modelKe 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 @@ public void ClearValidationStateForCollectionsModel_EmtpyModelKey(string modelKe 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 @@ public void ClearValidationStateForCollectionsModel_EmtpyModelKey(string modelKe [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 @@ public void ClearValidationStateForModel_NonEmtpyModelKey(string prefix) 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 @@ public bool ContainsPrefix(string prefix) { foreach (string key in Keys) { - if (PrefixContainer.IsPrefixMatch(prefix, key)) + if (ModelStateDictionary.StartsWithPrefix(prefix, key)) { return true; }