From eefa58206958d90ee915dd398eafc43f381ee40c Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Fri, 5 Jun 2015 16:56:05 -0700 Subject: [PATCH] Address #2526; remove use of a required validator when validating `[BindRequired]` - only use MVC error message when `[BindRequired]` is violated - update that error message to more clearly describe the problem - enable all tests skipped due to dupe bug #2493 - update expectations of a few tests using the old messages nits: - rename `ModelBinding_MissingRequiredMember` to `ModelBinding_MissingBindRequiredMember` - remove `` description of removed `requiredValidator` parameter - remove unused `MutableObjectModelBinderTest.GetRequiredValidator()` --- .../ModelBinding/MutableObjectModelBinder.cs | 40 ++----------- .../Properties/Resources.Designer.cs | 12 ++-- src/Microsoft.AspNet.Mvc.Core/Resources.resx | 58 +++++++++---------- .../MutableObjectModelBinderTest.cs | 25 +++----- .../ModelBindingBindingBehaviorTest.cs | 8 ++- .../ModelBindingDataMemberRequiredTest.cs | 4 +- ...MutableObjectModelBinderIntegrationTest.cs | 32 +++++----- 7 files changed, 74 insertions(+), 105 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index 6cf9707ad1..dcdcbac981 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.ComponentModel; using System.Linq; using System.Reflection; using System.Threading.Tasks; @@ -364,42 +363,19 @@ internal ModelValidationNode ProcessDto( var modelExplorer = metadataProvider.GetModelExplorerForType(bindingContext.ModelType, bindingContext.Model); var validationInfo = GetPropertyValidationInfo(bindingContext); - // Eliminate provided properties from requiredProperties; leaving just *missing* required properties. + // Eliminate provided properties from RequiredProperties; leaving just *missing* required properties. var boundProperties = dto.Results.Where(p => p.Value.IsModelSet).Select(p => p.Key.PropertyName); validationInfo.RequiredProperties.ExceptWith(boundProperties); foreach (var missingRequiredProperty in validationInfo.RequiredProperties) { - var addedError = false; - - // We want to provide the 'null' value, not the value of model.Property, - // so avoiding modelExplorer.GetProperty here which would call the actual getter on the - // model. This avoids issues with value types, or properties with pre-initialized values. - var propertyExplorer = modelExplorer.GetExplorerForProperty(missingRequiredProperty, model: null); - + var propertyExplorer = modelExplorer.GetExplorerForProperty(missingRequiredProperty); var propertyName = propertyExplorer.Metadata.BinderModelName ?? missingRequiredProperty; - var modelStateKey = ModelNames.CreatePropertyModelName( - bindingContext.ModelName, - propertyName); - - // Get the first 'required' validator (if any) to get custom error message. - var validatorProviderContext = new ModelValidatorProviderContext(propertyExplorer.Metadata); - bindingContext.OperationBindingContext.ValidatorProvider.GetValidators(validatorProviderContext); - var validator = validatorProviderContext.Validators.FirstOrDefault(v => v.IsRequired); + var modelStateKey = ModelNames.CreatePropertyModelName(bindingContext.ModelName, propertyName); - if (validator != null) - { - addedError = RunValidator(validator, bindingContext, propertyExplorer, modelStateKey); - } - - // Fall back to default message if BindingBehaviorAttribute required this property and we have no - // actual validator for it. - if (!addedError) - { - bindingContext.ModelState.TryAddModelError( - modelStateKey, - Resources.FormatModelBinding_MissingRequiredMember(propertyName)); - } + bindingContext.ModelState.TryAddModelError( + modelStateKey, + Resources.FormatModelBinding_MissingBindRequiredMember(propertyName)); } // For each property that ComplexModelDtoModelBinder attempted to bind, call the setter, recording @@ -436,10 +412,6 @@ internal ModelValidationNode ProcessDto( /// /// The for the property to set. /// The for the property's new value. - /// - /// The which ensures the value is not null. Or null if no such - /// requirement exists. - /// /// Should succeed in all cases that returns true. protected virtual void SetProperty( [NotNull] ModelBindingContext bindingContext, diff --git a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs index 3313041dfd..cb2fc38c40 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs @@ -1995,19 +1995,19 @@ internal static string FormatModelBinderUtil_ValueInvalidGeneric(object p0) } /// - /// The '{0}' property is required. + /// A value for the '{0}' property was not provided. /// - internal static string ModelBinding_MissingRequiredMember + internal static string ModelBinding_MissingBindRequiredMember { - get { return GetString("ModelBinding_MissingRequiredMember"); } + get { return GetString("ModelBinding_MissingBindRequiredMember"); } } /// - /// The '{0}' property is required. + /// A value for the '{0}' property was not provided. /// - internal static string FormatModelBinding_MissingRequiredMember(object p0) + internal static string FormatModelBinding_MissingBindRequiredMember(object p0) { - return string.Format(CultureInfo.CurrentCulture, GetString("ModelBinding_MissingRequiredMember"), p0); + return string.Format(CultureInfo.CurrentCulture, GetString("ModelBinding_MissingBindRequiredMember"), p0); } /// diff --git a/src/Microsoft.AspNet.Mvc.Core/Resources.resx b/src/Microsoft.AspNet.Mvc.Core/Resources.resx index 7575c2a6b2..1ba3c225fa 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNet.Mvc.Core/Resources.resx @@ -1,17 +1,17 @@  - @@ -499,8 +499,8 @@ The supplied value is invalid for {0}. - - The '{0}' property is required. + + A value for the '{0}' property was not provided. A value is required. diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs index 365407bc18..8fc0a20ded 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -793,7 +793,7 @@ public void ProcessDto_BindRequiredFieldMissing_RaisesModelError() var modelError = Assert.Single(modelState.Errors); Assert.Null(modelError.Exception); Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("The 'Age' property is required.", modelError.ErrorMessage); + Assert.Equal("A value for the 'Age' property was not provided.", modelError.ErrorMessage); } [Fact] @@ -844,7 +844,7 @@ public void ProcessDto_DataMemberIsRequiredFieldMissing_RaisesModelError() var modelError = Assert.Single(modelState.Errors); Assert.Null(modelError.Exception); Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("The 'Age' property is required.", modelError.ErrorMessage); + Assert.Equal("A value for the 'Age' property was not provided.", modelError.ErrorMessage); } [Fact] @@ -1147,9 +1147,9 @@ public void ProcessDto_ProvideRequiredFields_Success() Assert.Equal(0m, model.PropertyWithDefaultValue); // [DefaultValue] has no effect } - // This uses [Required] with [BindRequired] to provide a custom validation messsage. + // [Required] cannot provide a custom validation for [BindRequired] errors. [Fact] - public void ProcessDto_ValueTypePropertyWithBindRequired_CustomValidationMessage() + public void ProcessDto_ValueTypePropertyWithBindRequired_RequiredValidatorIgnored() { // Arrange var model = new ModelWithBindRequiredAndRequiredAttribute(); @@ -1191,16 +1191,16 @@ public void ProcessDto_ValueTypePropertyWithBindRequired_CustomValidationMessage .Value; var error = Assert.Single(entry.Errors); Assert.Null(error.Exception); - Assert.Equal("Custom Message ValueTypeProperty", error.ErrorMessage); + Assert.Equal("A value for the 'ValueTypeProperty' property was not provided.", error.ErrorMessage); // Model gets provided values. Assert.Equal(0, model.ValueTypeProperty); Assert.Equal("value", model.ReferenceTypeProperty); } - // This uses [Required] with [BindRequired] to provide a custom validation messsage. + // [Required] cannot provide a custom validation for [BindRequired] errors. [Fact] - public void ProcessDto_ReferenceTypePropertyWithBindRequired_CustomValidationMessage() + public void ProcessDto_ReferenceTypePropertyWithBindRequired_RequiredValidatorIgnored() { // Arrange var model = new ModelWithBindRequiredAndRequiredAttribute(); @@ -1242,7 +1242,7 @@ public void ProcessDto_ReferenceTypePropertyWithBindRequired_CustomValidationMes .Value; var error = Assert.Single(entry.Errors); Assert.Null(error.Exception); - Assert.Equal("Custom Message ReferenceTypeProperty", error.ErrorMessage); + Assert.Equal("A value for the 'ReferenceTypeProperty' property was not provided.", error.ErrorMessage); // Model gets provided values. Assert.Equal(17, model.ValueTypeProperty); @@ -1715,15 +1715,6 @@ private static ModelBindingContext CreateContext(ModelMetadata metadata, object }; } - private static IModelValidator GetRequiredValidator(ModelBindingContext bindingContext, ModelMetadata propertyMetadata) - { - var validatorProvider = bindingContext.OperationBindingContext.ValidatorProvider; - var validatorProviderContext = new ModelValidatorProviderContext(propertyMetadata); - validatorProvider.GetValidators(validatorProviderContext); - - return validatorProviderContext.Validators.FirstOrDefault(v => v.IsRequired); - } - private static ModelMetadata GetMetadataForCanUpdateProperty(string propertyName) { var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs index 6431fd0754..7a9f8c0e82 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs @@ -49,10 +49,14 @@ public async Task BindingBehavior_MissingRequiredProperties_ValidationErrors() Assert.Equal(2, errors.Count); var error = Assert.Single(errors, kvp => kvp.Key == "model.BehaviourRequiredProperty"); - Assert.Equal("The 'BehaviourRequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + Assert.Equal( + "A value for the 'BehaviourRequiredProperty' property was not provided.", + ((JArray)error.Value)[0].Value()); error = Assert.Single(errors, kvp => kvp.Key == "model.BindRequiredProperty"); - Assert.Equal("The 'BindRequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + Assert.Equal( + "A value for the 'BindRequiredProperty' property was not provided.", + ((JArray)error.Value)[0].Value()); } [Fact] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs index 3132d6778b..881d385079 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs @@ -49,7 +49,9 @@ public async Task DataMember_MissingRequiredProperty_ValidationError() Assert.Equal(1, errors.Count); var error = Assert.Single(errors, kvp => kvp.Key == "model.RequiredProperty"); - Assert.Equal("The 'RequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + Assert.Equal( + "A value for the 'RequiredProperty' property was not provided.", + ((JArray)error.Value)[0].Value()); } [Fact] diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs index e21abd1e8e..363b1c5009 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs @@ -1582,7 +1582,7 @@ private class Person10 public string Name { get; set; } } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithRequiredComplexProperty_NoData_GetsErrors() { // Arrange @@ -1617,7 +1617,7 @@ public async Task MutableObjectModelBinder_WithRequiredComplexProperty_NoData_Ge var entry = Assert.Single(modelState, e => e.Key == "Customer").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["Customer"].Errors); - Assert.Equal("The Customer field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'Customer' property was not provided.", error.ErrorMessage); } private class Order11 @@ -1633,7 +1633,7 @@ private class Person11 public string Name { get; set; } } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithPartialData_GetsErrors() { // Arrange @@ -1676,10 +1676,10 @@ public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithPartia entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["parameter.Customer.Name"].Errors); - Assert.Equal("The Name field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithData_EmptyPrefix_GetsErrors() { // Arrange @@ -1722,10 +1722,10 @@ public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithData_E entry = Assert.Single(modelState, e => e.Key == "Customer.Name").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["Customer.Name"].Errors); - Assert.Equal("The Name field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithData_CustomPrefix_GetsErrors() { // Arrange @@ -1772,7 +1772,7 @@ public async Task MutableObjectModelBinder_WithNestedRequiredProperty_WithData_C entry = Assert.Single(modelState, e => e.Key == "customParameter.Customer.Name").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["customParameter.Customer.Name"].Errors); - Assert.Equal("The Name field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage); } private class Order12 @@ -1781,7 +1781,7 @@ private class Order12 public string ProductName { get; set; } } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithRequiredProperty_NoData_GetsErrors() { // Arrange @@ -1817,10 +1817,10 @@ public async Task MutableObjectModelBinder_WithRequiredProperty_NoData_GetsError var entry = Assert.Single(modelState, e => e.Key == "ProductName").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["ProductName"].Errors); - Assert.Equal("The ProductName field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'ProductName' property was not provided.", error.ErrorMessage); } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithRequiredProperty_NoData_CustomPrefix_GetsErros() { // Arrange @@ -1860,7 +1860,7 @@ public async Task MutableObjectModelBinder_WithRequiredProperty_NoData_CustomPre var entry = Assert.Single(modelState, e => e.Key == "customParameter.ProductName").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["customParameter.ProductName"].Errors); - Assert.Equal("The ProductName field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'ProductName' property was not provided.", error.ErrorMessage); } [Fact] @@ -1908,7 +1908,7 @@ private class Order13 public List OrderIds { get; set; } } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithRequiredCollectionProperty_NoData_GetsErros() { // Arrange @@ -1944,10 +1944,10 @@ public async Task MutableObjectModelBinder_WithRequiredCollectionProperty_NoData var entry = Assert.Single(modelState, e => e.Key == "OrderIds").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["OrderIds"].Errors); - Assert.Equal("The OrderIds field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'OrderIds' property was not provided.", error.ErrorMessage); } - [Fact(Skip = "Error message is incorrect #2493.")] + [Fact] public async Task MutableObjectModelBinder_WithRequiredCollectionProperty_NoData_CustomPrefix_GetsErros() { // Arrange @@ -1987,7 +1987,7 @@ public async Task MutableObjectModelBinder_WithRequiredCollectionProperty_NoData var entry = Assert.Single(modelState, e => e.Key == "customParameter.OrderIds").Value; Assert.Null(entry.Value); var error = Assert.Single(modelState["customParameter.OrderIds"].Errors); - Assert.Equal("The OrderIds field is required.", error.ErrorMessage); + Assert.Equal("A value for the 'OrderIds' property was not provided.", error.ErrorMessage); } [Fact]