From 07fabde92ab4b6dd26016097a0ca297c467fb4ff Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 14 Aug 2015 15:35:04 -0700 Subject: [PATCH] Part 3 of #2776 - revert a6ce9abab1a4a2af91659ba14a317c030f331d92 and add some more tests. This change reverts the behavior change from a6ce9abab1a4a2af91659ba14a317c030f331d92 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. --- .../ModelBinding/Metadata/BindingMetadata.cs | 6 +- .../Metadata/DefaultModelMetadata.cs | 7 +- ...taMemberRequiredBindingMetadataProvider.cs | 2 +- .../Metadata/DefaultModelMetadataTest.cs | 45 +-------- .../MutableObjectModelBinderTest.cs | 11 +-- .../HtmlGenerationTest.cs | 5 +- .../ModelBindingFromQueryTest.cs | 8 +- .../ModelBindingTest.cs | 2 +- ...WebSite.HtmlGeneration_Customer.Index.html | 8 +- .../BodyValidationIntegrationTests.cs | 2 +- .../CollectionModelBinderIntegrationTest.cs | 3 +- ...MutableObjectModelBinderIntegrationTest.cs | 94 ++++++++++++++++++- ...TypeConverterModelBinderIntegrationTest.cs | 52 ++++++++++ .../ValidationIntegrationTests.cs | 6 +- 14 files changed, 162 insertions(+), 89 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs index 6183b883a7..e7540642f2 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/BindingMetadata.cs @@ -38,11 +38,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// /// 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 . If null, the value of - /// will be computed based on - /// . + /// See . /// - public bool? IsBindingRequired { get; set; } + public bool IsBindingRequired { get; set; } /// /// 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 index 4a83480299..28d3e1a695 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -341,14 +341,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { _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 index e4eb241ffd..a9b6884c61 100644 --- a/src/Microsoft.AspNet.Mvc.Formatters.Xml/DataMemberRequiredBindingMetadataProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Formatters.Xml/DataMemberRequiredBindingMetadataProvider.cs @@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata 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 index f55875ebbc..5939d875fb 100644 --- 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 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata 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 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Assert.False(isBindingRequired); } - [Theory] - [InlineData(typeof(string))] - [InlineData(typeof(IDisposable))] - [InlineData(typeof(Nullable))] - 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 index 5fbc79b1f9..a7f82e0424 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -1050,7 +1050,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } [Fact] - public void ProcessDto_ValueTypeProperty_NoValue_Error() + public void ProcessDto_ValueTypeProperty_NoValue_NoError() { // Arrange var model = new Person(); @@ -1083,14 +1083,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding 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 index 2536f0c926..a5d4990fee 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs @@ -196,12 +196,11 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/Customer/HtmlGeneration_Customer"); var nameValueCollection = new List> { - new KeyValuePair("Number", "0"), + new KeyValuePair("Number", string.Empty), new KeyValuePair("Name", string.Empty), new KeyValuePair("Email", string.Empty), new KeyValuePair("PhoneNumber", string.Empty), - new KeyValuePair("Password", string.Empty), - new KeyValuePair("Gender", "Female"), + new KeyValuePair("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 index f512f3240e..dd1496b377 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs @@ -122,12 +122,8 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // Assert var body = await response.Content.ReadAsStringAsync(); var result = JsonConvert.DeserializeObject(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); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs index af7257a2d5..696f366d9d 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs @@ -2218,7 +2218,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // 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 index a32dfd7f63..7d6cf27c87 100644 --- 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 @@
- - The field Number must be between 1 and 100. + + The value '' is invalid.
@@ -27,10 +27,10 @@
Male - Female + Female
-
  • The field Number must be between 1 and 100.
    • +
    • The value '' is invalid.
    • The Password field is required.
    • diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs index db88f551b7..31838694c8 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs @@ -108,7 +108,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests 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 index 8301dcd1d4..1037f03c09 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -552,7 +552,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Address4 { - [BindingBehavior(BindingBehavior.Optional)] public int Zip { get; set; } public string Street { get; set; } @@ -611,7 +610,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests 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 index f107438e27..d3f709ec4c 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs @@ -26,7 +26,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Order1 { - [BindingBehavior(BindingBehavior.Optional)] public int ProductId { get; set; } public Person1 Customer { get; set; } @@ -262,7 +261,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Order2 { - public int? ProductId { get; set; } + public int ProductId { get; set; } public Person2 Customer { get; set; } } @@ -436,7 +435,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Order3 { - [BindingBehavior(BindingBehavior.Optional)] public int ProductId { get; set; } public Person3 Customer { get; set; } @@ -580,7 +578,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Order4 { - public int? ProductId { get; set; } + public int ProductId { get; set; } public Person4 Customer { get; set; } } @@ -1343,7 +1341,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests { public string Name { get; set; } - [BindingBehavior(BindingBehavior.Optional)] public KeyValuePair ProductId { get; set; } } @@ -2033,6 +2030,93 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests 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(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(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 index 8170475234..3b87dbbd12 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs @@ -159,6 +159,58 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } + + [Fact] + public async Task BindParameter_NonConvertableValue_GetsError() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + + ParameterType = typeof(int) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => + { + request.QueryString = QueryString.Create("Parameter1", "abcd"); + }); + + var modelState = new ModelStateDictionary(); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); + + // Assert + + // ModelBindingResult + Assert.NotNull(modelBindingResult); + Assert.False(modelBindingResult.IsModelSet); + + // Model + Assert.Null(modelBindingResult.Model); + + // ModelState + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.Count); + Assert.Equal(1, modelState.ErrorCount); + + var key = Assert.Single(modelState.Keys); + Assert.Equal("Parameter1", key); + + var entry = modelState[key]; + Assert.NotNull(entry.Value); + Assert.Equal("abcd", entry.Value.AttemptedValue); + Assert.Equal("abcd", entry.Value.RawValue); + Assert.Equal(ModelValidationState.Invalid, entry.ValidationState); + + var error = Assert.Single(entry.Errors); + Assert.Null(error.Exception); + Assert.Equal("The value 'abcd' is not valid for Parameter1.", error.ErrorMessage); + } + [Theory] [InlineData(typeof(int))] [InlineData(typeof(bool))] diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs index ebde9dae1a..013aeb8475 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -195,7 +195,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Person3 { - public int? Age { get; set; } + public int Age { get; set; } [Required] public string Name { get; set; } @@ -1018,12 +1018,12 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests private class Address { - public int? Street { get; set; } + public int Street { get; set; } public string State { get; set; } [Range(10000, 99999)] - public int? Zip { get; set; } + public int Zip { get; set; } public Country Country { get; set; } }