From 2fc983b23af5515cd1b4a32c47ea8203be92dddf Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 13 May 2015 20:03:23 -0700 Subject: [PATCH] Fix for #2414 - Remove [DefaultValue] support from ModelBinding This part of the change removes default value support from ModelBinding. Updated some unit tests to verify that it does nothing in that case. Deleted a functional test as it was pure duplication of another (supported) case where the property has a pre-initialized value. --- .../ModelBinding/MutableObjectModelBinder.cs | 24 ++------------ .../MutableObjectModelBinderTest.cs | 14 ++++---- .../ModelBindingFromHeaderTest.cs | 33 ------------------- .../Controllers/FromHeader_BlogController.cs | 26 --------------- 4 files changed, 9 insertions(+), 88 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index 0e46b65960..03caa4f61f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -324,21 +324,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding }; } - private static bool TryGetPropertyDefaultValue(PropertyInfo propertyInfo, out object value) - { - var attribute = propertyInfo.GetCustomAttribute(); - if (attribute == null) - { - value = null; - return false; - } - else - { - value = attribute.Value; - return true; - } - } - internal static PropertyValidationInfo GetPropertyValidationInfo(ModelBindingContext bindingContext) { var validationInfo = new PropertyValidationInfo(); @@ -473,16 +458,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return; } - object value; - var hasDefaultValue = false; + object value = null; if (dtoResult.IsModelSet) { value = dtoResult.Model; } - else - { - hasDefaultValue = TryGetPropertyDefaultValue(property, out value); - } // 'Required' validators need to run first so that we can provide useful error messages if // the property setters throw, e.g. if we're setting entity keys to null. @@ -504,7 +484,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } - if (!dtoResult.IsModelSet && !hasDefaultValue) + if (!dtoResult.IsModelSet) { // If we don't have a value, don't set it on the model and trounce a pre-initialized // value. diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs index 5a81dd4281..708db0be61 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -1057,8 +1057,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties); var testableBinder = new TestableMutableObjectModelBinder(); - // ValueTypeRequiredWithDefaultValue value comes from [DefaultValue] when !isModelSet. - var expectedValue = isModelSet ? 0 : 42; + // The [DefaultValue] on ValueTypeRequiredWithDefaultValue is ignored by model binding. + var expectedValue = 0; // Make ValueTypeRequired invalid. var propertyMetadata = dto.PropertyMetadata.Single(p => p.PropertyName == nameof(Person.ValueTypeRequired)); @@ -1160,7 +1160,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Model gets provided values. Assert.Equal(41, model.ValueTypeRequired); Assert.Equal(57, model.ValueTypeRequiredWithDefaultValue); - Assert.Equal(123.456m, model.PropertyWithDefaultValue); // from [DefaultValue] + Assert.Equal(0m, model.PropertyWithDefaultValue); // [DefaultValue] has no effect } [Fact] @@ -1205,7 +1205,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } [Fact] - public void SetProperty_PropertyHasDefaultValue_SetsDefaultValue() + public void SetProperty_PropertyHasDefaultValue_DefaultValueAttributeDoesNothing() { // Arrange var model = new Person(); @@ -1238,7 +1238,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert var person = Assert.IsType(bindingContext.Model); - Assert.Equal(123.456m, person.PropertyWithDefaultValue); + Assert.Equal(0m, person.PropertyWithDefaultValue); Assert.True(bindingContext.ModelState.IsValid); } @@ -1276,7 +1276,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } [Fact] - public void SetProperty_PropertyIsPreinitialized_WithDefaultValue_NoValue_CallsSetter() + public void SetProperty_PropertyIsPreinitialized_DefaultValueAttributeDoesNothing() { // Arrange var model = new Person(); @@ -1304,7 +1304,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert var person = Assert.IsType(bindingContext.Model); - Assert.Equal("default", person.PropertyWithInitializedValueAndDefault); + Assert.Equal("preinitialized", person.PropertyWithInitializedValueAndDefault); Assert.True(bindingContext.ModelState.IsValid); } diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs index 9543545dc4..9867bebd41 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs @@ -325,39 +325,6 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal("Title", error); } - // This model uses [DefaultValueAttribute(...)] for 'Title', and the model binder won't - // trounce it. - // - // There's a validation error because we validate the value in the request (not present). - [Fact] - public async Task FromHeader_BindHeader_ToModel_NoValues_DefaultValueAttribute_ValidationError() - { - // Arrange - var server = TestHelper.CreateServer(_app, SiteName, _configureServices); - var client = server.CreateClient(); - - var request = new HttpRequestMessage( - HttpMethod.Get, - "http://localhost/Blog/BindToModelWithDefaultValue?author=Marvin"); - - // Intentionally not setting a title or tags - - // Act - var response = await client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - - var body = await response.Content.ReadAsStringAsync(); - var result = JsonConvert.DeserializeObject(body); - - Assert.Equal("How to Make Soup", result.HeaderValue); - Assert.Equal(new[] { "Cooking" }, result.HeaderValues); - - var error = Assert.Single(result.ModelStateErrors); - Assert.Equal("Title", error); - } - private class Result { public string HeaderValue { get; set; } diff --git a/test/WebSites/ModelBindingWebSite/Controllers/FromHeader_BlogController.cs b/test/WebSites/ModelBindingWebSite/Controllers/FromHeader_BlogController.cs index 4e056333e9..ff79a70951 100644 --- a/test/WebSites/ModelBindingWebSite/Controllers/FromHeader_BlogController.cs +++ b/test/WebSites/ModelBindingWebSite/Controllers/FromHeader_BlogController.cs @@ -87,18 +87,6 @@ namespace ModelBindingWebSite.Controllers }; } - [HttpGet("BindToModelWithDefaultValue")] - public object BindToModelWithDefaultValue(BlogPostWithDefaultValue blogPost) - { - return new Result() - { - HeaderValue = blogPost.Title, - HeaderValues = blogPost.Tags, - ModelStateErrors = ModelState.Where(kvp => kvp.Value.Errors.Count > 0).Select(kvp => kvp.Key).ToArray(), - }; - } - - private class Result { public string HeaderValue { get; set; } @@ -144,19 +132,5 @@ namespace ModelBindingWebSite.Controllers public string Author { get; set; } } - - public class BlogPostWithDefaultValue - { - [Required] - [FromHeader] - [DefaultValue("How to Make Soup")] - public string Title { get; set; } - - [FromHeader] - [DefaultValue(new string[] { "Cooking" })] - public string[] Tags { get; set; } - - public string Author { get; set; } - } } } \ No newline at end of file