Part 3 of aspnet#2776 - revert a6ce9ab and add

rynowak · rynowak · commit 07fabde92ab4 · 2015-08-16T16:20:44.000-07:00
some more tests. This change reverts the behavior change from a6ce9ab and adds more tests around the scneario that was actually broken. The right behavior is that unconvertable values result in a validation error. There's no special behavior around value types and required values.
diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs
@@ -38,11 +38,9 @@ public class BindingMetadata
         /// <summary>
         /// Gets or sets a value indicating whether or not the request must contain a value for the model.
         /// Will be ignored if the model metadata being created does not represent a property.
-        /// See <see cref="ModelMetadata.IsBindingRequired"/>. If <c>null</c>, the value of
-        /// <see cref="ModelMetadata.IsBindingRequired"/> will be computed based on
-        /// <see cref="ModelMetadata.ModelType"/>.
+        /// See <see cref="ModelMetadata.IsBindingRequired"/>.
         /// </summary>
-        public bool? IsBindingRequired { get; set; }
+        public bool IsBindingRequired { get; set; }
 
         /// <summary>
         /// Gets or sets a value indicating whether or not the model is read-only. Will be ignored
diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs
@@ -341,14 +341,9 @@ public override bool IsBindingRequired
                     {
                         _isBindingRequired = false;
                     }
-                    else if (BindingMetadata.IsBindingRequired.HasValue)
-                    {
-                        _isBindingRequired = BindingMetadata.IsBindingRequired;
-                    }
                     else
                     {
-                        // Default to IsBindingRequired = true for value types.
-                        _isBindingRequired = !AllowsNullValue(ModelType);
+                        _isBindingRequired = BindingMetadata.IsBindingRequired;
                     }
                 }
 
diff --git a/src/Microsoft.AspNet.Mvc.Formatters.Xml/DataMemberRequiredBindingMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.Formatters.Xml/DataMemberRequiredBindingMetadataProvider.cs
@@ -22,7 +22,7 @@ public void GetBindingMetadata([NotNull] BindingMetadataProviderContext context)
                 return;
             }
 
-            if (context.BindingMetadata.IsBindingRequired == true)
+            if (context.BindingMetadata.IsBindingRequired)
             {
                 // This value is already required, no need to look at attributes.
                 return;
diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs
@@ -39,7 +39,7 @@ public void DefaultValues()
             Assert.False(metadata.HideSurroundingHtml);
             Assert.True(metadata.HtmlEncode);
             Assert.True(metadata.IsBindingAllowed);
-            Assert.False(metadata.IsBindingRequired); // Defaults to false for reference types
+            Assert.False(metadata.IsBindingRequired);
             Assert.False(metadata.IsComplexType);
             Assert.False(metadata.IsCollectionType);
             Assert.False(metadata.IsEnum);
@@ -223,49 +223,6 @@ public void IsBindingRequired_ReturnsFalse_ForTypes(Type modelType)
             Assert.False(isBindingRequired);
         }
 
-        [Theory]
-        [InlineData(typeof(string))]
-        [InlineData(typeof(IDisposable))]
-        [InlineData(typeof(Nullable<int>))]
-        public void IsBindingRequired_ReturnsFalse_ForNullablePropertyTypes(Type modelType)
-        {
-            // Arrange
-            var provider = new EmptyModelMetadataProvider();
-            var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
-
-            var key = ModelMetadataIdentity.ForProperty(modelType, "Test", typeof(string));
-            var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
-
-            var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
-
-            // Act
-            var isBindingRequired = metadata.IsBindingRequired;
-
-            // Assert
-            Assert.False(isBindingRequired);
-        }
-
-        [Theory]
-        [InlineData(typeof(int))]
-        [InlineData(typeof(DayOfWeek))]
-        public void IsBindingRequired_ReturnsTrue_ForNonNullablePropertyTypes(Type modelType)
-        {
-            // Arrange
-            var provider = new EmptyModelMetadataProvider();
-            var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
-
-            var key = ModelMetadataIdentity.ForProperty(modelType, "Test", typeof(string));
-            var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
-
-            var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
-
-            // Act
-            var isBindingRequired = metadata.IsBindingRequired;
-
-            // Assert
-            Assert.True(isBindingRequired);
-        }
-
         [Theory]
         [InlineData(typeof(string))]
         [InlineData(typeof(IDisposable))]
diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs
@@ -1050,7 +1050,7 @@ public void ProcessDto_ValueTypeProperty_TriesToSetNullModel_CapturesException()
         }
 
         [Fact]
-        public void ProcessDto_ValueTypeProperty_NoValue_Error()
+        public void ProcessDto_ValueTypeProperty_NoValue_NoError()
         {
             // Arrange
             var model = new Person();
@@ -1083,14 +1083,7 @@ public void ProcessDto_ValueTypeProperty_NoValue_Error()
             testableBinder.ProcessDto(bindingContext, dto, modelValidationNode);
 
             // Assert
-            Assert.False(modelState.IsValid);
-
-            var entry = modelState["theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue)];
-            var error = Assert.Single(entry.Errors);
-            Assert.Equal(
-                $"A value for the '{nameof(Person.ValueTypeRequiredWithDefaultValue)}' property was not provided.",
-                error.ErrorMessage);
-            Assert.Null(error.Exception);
+            Assert.True(modelState.IsValid);
         }
 
         [Fact]
diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs
@@ -196,12 +196,11 @@ public async Task ValidationTagHelpers_GeneratesExpectedSpansAndDivs()
             var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/Customer/HtmlGeneration_Customer");
             var nameValueCollection = new List<KeyValuePair<string, string>>
             {
-                new KeyValuePair<string,string>("Number", "0"),
+                new KeyValuePair<string,string>("Number", string.Empty),
                 new KeyValuePair<string,string>("Name", string.Empty),
                 new KeyValuePair<string,string>("Email", string.Empty),
                 new KeyValuePair<string,string>("PhoneNumber", string.Empty),
-                new KeyValuePair<string,string>("Password", string.Empty),
-                new KeyValuePair<string, string>("Gender", "Female"),
+                new KeyValuePair<string,string>("Password", string.Empty)
             };
             request.Content = new FormUrlEncodedContent(nameValueCollection);
 
diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs
@@ -122,12 +122,8 @@ public async Task FromQuery_NonExistingValueAddsValidationErrors_OnProperty_Usin
             // Assert
             var body = await response.Content.ReadAsStringAsync();
             var result = JsonConvert.DeserializeObject<Result>(body);
-
-            Assert.Collection(
-                result.ModelStateErrors,
-                e => Assert.Equal("TestEmployees[0].EmployeeId", e),
-                e => Assert.Equal("TestEmployees[0].EmployeeTaxId", e),
-                e => Assert.Equal("TestEmployees[0].Age", e));
+            var error = Assert.Single(result.ModelStateErrors);
+            Assert.Equal("TestEmployees[0].EmployeeId", error);
         }
     }
 }
diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs
@@ -2218,7 +2218,7 @@ public async Task TryUpdateModel_ClearsModelStateEntries()
             // Arrange
             var server = TestHelper.CreateServer(_app, SiteName, _configureServices);
             var client = server.CreateClient();
-            var url = "http://localhost/TryUpdateModel/TryUpdateModel_ClearsModelStateEntries?id=5&price=1";
+            var url = "http://localhost/TryUpdateModel/TryUpdateModel_ClearsModelStateEntries";
 
             // Act
             var response = await client.GetAsync(url);
diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html
@@ -3,8 +3,8 @@
     <form action="/Customer/HtmlGeneration_Customer" method="post">
         <div>
             <label class="order" for="Number">Number</label>
-            <input class="form-control input-validation-error" type="number" data-val="true" data-val-range="The field Number must be between 1 and 100." data-val-range-max="100" data-val-range-min="1" data-val-required="The Number field is required." id="Number" name="Number" value="0" />
-            <span class="field-validation-error" data-valmsg-for="Number" data-valmsg-replace="true">The field Number must be between 1 and 100.</span>
+            <input class="form-control input-validation-error" type="number" data-val="true" data-val-range="The field Number must be between 1 and 100." data-val-range-max="100" data-val-range-min="1" data-val-required="The Number field is required." id="Number" name="Number" value="" />
+            <span class="field-validation-error" data-valmsg-for="Number" data-valmsg-replace="true">The value &#x27;&#x27; is invalid.</span>
         </div>
         <div>
             <label class="order" for="Name">Name</label>
@@ -27,10 +27,10 @@
         <div>
             <label class="order" for="Gender">Gender</label>
             <input type="radio" value="Male" data-val="true" data-val-required="The Gender field is required." id="Gender" name="Gender" /> Male
-            <input type="radio" value="Female" checked="checked" id="Gender" name="Gender" /> Female
+            <input type="radio" value="Female" id="Gender" name="Gender" /> Female
             <span class="field-validation-valid" data-valmsg-for="Gender" data-valmsg-replace="true"></span>
         </div>
-        <div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The field Number must be between 1 and 100.</li>
+        <div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The value &#x27;&#x27; is invalid.</li>
 <li>The Password field is required.</li>
 </ul></div>
         <div class="order validation-summary-errors"><ul><li style="display:none"></li>
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs
@@ -108,7 +108,7 @@ public async Task FromBodyOnActionParameter_EmptyBody_BindsToNullValue()
         private class Person4
         {
             [FromBody]
-            [BindingBehavior(BindingBehavior.Optional)]
+            [Required]
             public int Address { get; set; }
         }
 
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs
@@ -552,7 +552,6 @@ private class Person4
 
         private class Address4
         {
-            [BindingBehavior(BindingBehavior.Optional)]
             public int Zip { get; set; }
 
             public string Street { get; set; }
@@ -611,7 +610,7 @@ private class Person5
 
         private class Address5
         {
-            public int? Zip { get; set; }
+            public int Zip { get; set; }
 
             [StringLength(3)]
             public string Street { get; set; }
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs
@@ -26,7 +26,6 @@ public class MutableObjectModelBinderIntegrationTest
 
         private class Order1
         {
-            [BindingBehavior(BindingBehavior.Optional)]
             public int ProductId { get; set; }
 
             public Person1 Customer { get; set; }
@@ -262,7 +261,7 @@ public async Task MutableObjectModelBinder_BindsNestedPOCO_WithBodyModelBinder_W
 
         private class Order2
         {
-            public int? ProductId { get; set; }
+            public int ProductId { get; set; }
 
             public Person2 Customer { get; set; }
         }
@@ -436,7 +435,6 @@ public async Task MutableObjectModelBinder_BindsNestedPOCO_WithServicesModelBind
 
         private class Order3
         {
-            [BindingBehavior(BindingBehavior.Optional)]
             public int ProductId { get; set; }
 
             public Person3 Customer { get; set; }
@@ -580,7 +578,7 @@ public async Task MutableObjectModelBinder_BindsNestedPOCO_WithByteArrayModelBin
 
         private class Order4
         {
-            public int? ProductId { get; set; }
+            public int ProductId { get; set; }
 
             public Person4 Customer { get; set; }
         }
@@ -1343,7 +1341,6 @@ private class Order8
         {
             public string Name { get; set; }
 
-            [BindingBehavior(BindingBehavior.Optional)]
             public KeyValuePair<string, int> ProductId { get; set; }
         }
 
@@ -2033,6 +2030,93 @@ public async Task MutableObjectModelBinder_WithRequiredCollectionProperty_WithDa
             Assert.Equal("123", entry.Value.AttemptedValue);
         }
 
+        private class Order14
+        {
+            public int ProductId { get; set; }
+        }
+
+        // This covers the case where a key is present, but has an empty value. The type converter
+        // will report an error.
+        [Fact]
+        public async Task MutableObjectModelBinder_BindsPOCO_TypeConvertedPropertyNonConvertableValue_GetsError()
+        {
+            // Arrange
+            var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
+            var parameter = new ParameterDescriptor()
+            {
+                Name = "parameter",
+                ParameterType = typeof(Order14)
+            };
+
+            // Need to have a key here so that the MutableObjectModelBinder will recurse to bind elements.
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
+            {
+                request.QueryString = new QueryString("?parameter.ProductId=");
+            });
+
+            var modelState = new ModelStateDictionary();
+
+            // Act
+            var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext);
+
+            // Assert
+            Assert.NotNull(modelBindingResult);
+            Assert.True(modelBindingResult.IsModelSet);
+
+            var model = Assert.IsType<Order14>(modelBindingResult.Model);
+            Assert.NotNull(model);
+            Assert.Equal(0, model.ProductId);
+
+            Assert.Equal(1, modelState.Count);
+            Assert.Equal(1, modelState.ErrorCount);
+            Assert.False(modelState.IsValid);
+
+            var entry = Assert.Single(modelState, e => e.Key == "parameter.ProductId").Value;
+            Assert.Equal(string.Empty, entry.Value.AttemptedValue);
+            Assert.Equal(string.Empty, entry.Value.RawValue);
+
+            var error = Assert.Single(entry.Errors);
+            Assert.Equal("The value '' is invalid.", error.ErrorMessage);
+            Assert.Null(error.Exception);
+        }
+
+        // This covers the case where a key is present, but has no value. The type converter
+        // will report an error.
+        [Fact]
+        public async Task MutableObjectModelBinder_BindsPOCO_TypeConvertedPropertyWithNoValue_NoError()
+        {
+            // Arrange
+            var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
+            var parameter = new ParameterDescriptor()
+            {
+                Name = "parameter",
+                ParameterType = typeof(Order14)
+            };
+
+            // Need to have a key here so that the MutableObjectModelBinder will recurse to bind elements.
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
+            {
+                request.QueryString = new QueryString("?parameter.ProductId");
+            });
+
+            var modelState = new ModelStateDictionary();
+
+            // Act
+            var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext);
+
+            // Assert
+            Assert.NotNull(modelBindingResult);
+            Assert.True(modelBindingResult.IsModelSet);
+
+            var model = Assert.IsType<Order14>(modelBindingResult.Model);
+            Assert.NotNull(model);
+            Assert.Equal(0, model.ProductId);
+
+            Assert.Equal(0, modelState.Count);
+            Assert.Equal(0, modelState.ErrorCount);
+            Assert.True(modelState.IsValid);
+        }
+
         private static void SetJsonBodyContent(HttpRequest request, string content)
         {
             var stream = new MemoryStream(new UTF8Encoding(encoderShouldEmitUTF8Identifier: false).GetBytes(content));
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs

Original file line number	Diff line number	Diff line change
`@@ -341,14 +341,9 @@ public override bool IsBindingRequired`
`341`	`341`	`{`
`342`	`342`	`_isBindingRequired = false;`
`343`	`343`	`}`
`344`		`- else if (BindingMetadata.IsBindingRequired.HasValue)`
`345`		`- {`
`346`		`- _isBindingRequired = BindingMetadata.IsBindingRequired;`
`347`		`- }`
`348`	`344`	`else`
`349`	`345`	`{`
`350`		`- // Default to IsBindingRequired = true for value types.`
`351`		`- _isBindingRequired = !AllowsNullValue(ModelType);`
	`346`	`+ _isBindingRequired = BindingMetadata.IsBindingRequired;`
`352`	`347`	`}`
`353`	`348`	`}`
`354`	`349`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ public void GetBindingMetadata([NotNull] BindingMetadataProviderContext context)`
`22`	`22`	`return;`
`23`	`23`	`}`
`24`	`24`
`25`		`- if (context.BindingMetadata.IsBindingRequired == true)`
	`25`	`+ if (context.BindingMetadata.IsBindingRequired)`
`26`	`26`	`{`
`27`	`27`	`// This value is already required, no need to look at attributes.`
`28`	`28`	`return;`
Original file line number	Diff line number	Diff line change
`@@ -122,12 +122,8 @@ public async Task FromQuery_NonExistingValueAddsValidationErrors_OnProperty_Usin`
`122`	`122`	`// Assert`
`123`	`123`	`var body = await response.Content.ReadAsStringAsync();`
`124`	`124`	`var result = JsonConvert.DeserializeObject<Result>(body);`
`125`		`-`
`126`		`- Assert.Collection(`
`127`		`- result.ModelStateErrors,`
`128`		`- e => Assert.Equal("TestEmployees[0].EmployeeId", e),`
`129`		`- e => Assert.Equal("TestEmployees[0].EmployeeTaxId", e),`
`130`		`- e => Assert.Equal("TestEmployees[0].Age", e));`
	`125`	`+ var error = Assert.Single(result.ModelStateErrors);`
	`126`	`+ Assert.Equal("TestEmployees[0].EmployeeId", error);`
`131`	`127`	`}`
`132`	`128`	`}`
`133`	`129`	`}`
Original file line number	Diff line number	Diff line change
`@@ -108,7 +108,7 @@ public async Task FromBodyOnActionParameter_EmptyBody_BindsToNullValue()`
`108`	`108`	`private class Person4`
`109`	`109`	`{`
`110`	`110`	`[FromBody]`
`111`		`- [BindingBehavior(BindingBehavior.Optional)]`
	`111`	`+ [Required]`
`112`	`112`	`public int Address { get; set; }`
`113`	`113`	`}`
`114`	`114`