From 933a13608f885b284ced937da53caf31d910fb1c Mon Sep 17 00:00:00 2001 From: Mugdha Kulkarni Date: Thu, 2 Jul 2015 17:23:33 -0700 Subject: [PATCH] Adding fix for 2756 and test cases. Skipping the test cases for 2793. --- .../ModelBinding/MutableObjectModelBinder.cs | 22 ++++++- .../MutableObjectModelBinderContext.cs | 2 +- .../MutableObjectModelBinderTest.cs | 64 +++++++++++++++++-- .../ModelBindingTest.cs | 41 ++++++++++++ .../ActionParametersIntegrationTest.cs | 4 +- .../GenericModelBinderIntegrationTest.cs | 4 +- .../TryUpdateModelIntegrationTest.cs | 4 +- .../PropertiesGetCreatedController.cs | 10 +++ .../Models/ArrayOfPersonWithNoProperties.cs | 10 +++ .../Models/PersonWithNoProperties.cs | 10 +++ 10 files changed, 157 insertions(+), 14 deletions(-) create mode 100644 test/WebSites/ModelBindingWebSite/Models/ArrayOfPersonWithNoProperties.cs create mode 100644 test/WebSites/ModelBindingWebSite/Models/PersonWithNoProperties.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index e8eb037768..fb52d0a093 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -32,7 +32,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableObjectBinderContext = new MutableObjectBinderContext() { ModelBindingContext = bindingContext, - PropertyMetadata = GetMetadataForProperties(bindingContext), + PropertyMetadata = GetMetadataForProperties(bindingContext).ToArray(), }; if (!(await CanCreateModel(mutableObjectBinderContext))) @@ -98,7 +98,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return true; } - // 2. Any of the model properties can be bound using a value provider. + // 2. If it is top level object and there are no properties to bind + if (isTopLevelObject && context.PropertyMetadata != null && context.PropertyMetadata.Count == 0) + { + return true; + } + + // 3. Any of the model properties can be bound using a value provider. if (await CanValueBindAnyModelProperties(context)) { return true; @@ -109,6 +115,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private async Task CanValueBindAnyModelProperties(MutableObjectBinderContext context) { + // If there are no properties on the model, there is nothing to bind. We are here means this is not a top + // level object. So we return false. + if (context.PropertyMetadata == null || context.PropertyMetadata.Count == 0) + { + return false; + } + // We want to check to see if any of the properties of the model can be bound using the value providers, // because that's all that MutableObjectModelBinder can handle. // @@ -202,6 +215,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return false; } + if (modelMetadata.IsCollectionType) + { + return false; + } + if (modelMetadata.ModelType == typeof(ComplexModelDto)) { // forbidden type - will cause a stack overflow if we try binding this type diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinderContext.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinderContext.cs index f136e1462d..6d960fc237 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinderContext.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinderContext.cs @@ -9,6 +9,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { public ModelBindingContext ModelBindingContext { get; set; } - public IEnumerable PropertyMetadata { get; set; } + public IReadOnlyList PropertyMetadata { get; set; } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs index c0c44a6d64..09b665058f 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -74,7 +74,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableBinder = new TestableMutableObjectModelBinder(); bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( - bindingContext.ModelBindingContext); + bindingContext.ModelBindingContext).ToArray(); // Act var canCreate = await mutableBinder.CanCreateModel(bindingContext); @@ -167,7 +167,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableBinder = new TestableMutableObjectModelBinder(); bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( - bindingContext.ModelBindingContext); + bindingContext.ModelBindingContext).ToArray(); // Act var retModel = await mutableBinder.CanCreateModel(bindingContext); @@ -217,6 +217,55 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(valueAvailable, result); } + [Fact] + public async Task CanCreateModel_ReturnsFalse_IfNotIsTopLevelObjectAndModelHasNoProperties() + { + // Arrange + var bindingContext = new MutableObjectBinderContext + { + ModelBindingContext = new ModelBindingContext + { + IsTopLevelObject = false, + + ModelMetadata = GetMetadataForType(typeof(PersonWithNoProperties)) + } + }; + + var mutableBinder = new TestableMutableObjectModelBinder(); + bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( + bindingContext.ModelBindingContext).ToArray(); + + // Act + var canCreate = await mutableBinder.CanCreateModel(bindingContext); + + // Assert + Assert.False(canCreate); + } + + [Fact] + public async Task CanCreateModel_ReturnsTrue_IfIsTopLevelObjectAndModelHasNoProperties() + { + // Arrange + var bindingContext = new MutableObjectBinderContext + { + ModelBindingContext = new ModelBindingContext + { + IsTopLevelObject = true, + ModelMetadata = GetMetadataForType(typeof(PersonWithNoProperties)) + }, + }; + + var mutableBinder = new TestableMutableObjectModelBinder(); + bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( + bindingContext.ModelBindingContext).ToArray(); + + // Act + var retModel = await mutableBinder.CanCreateModel(bindingContext); + + // Assert + Assert.True(retModel); + } + [Theory] [InlineData(typeof(TypeWithNoBinderMetadata), false)] [InlineData(typeof(TypeWithNoBinderMetadata), true)] @@ -251,7 +300,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableBinder = new TestableMutableObjectModelBinder(); bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( - bindingContext.ModelBindingContext); + bindingContext.ModelBindingContext).ToArray(); // Act var retModel = await mutableBinder.CanCreateModel(bindingContext); @@ -311,7 +360,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableBinder = new TestableMutableObjectModelBinder(); bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( - bindingContext.ModelBindingContext); + bindingContext.ModelBindingContext).ToArray(); // Act var retModel = await mutableBinder.CanCreateModel(bindingContext); @@ -354,7 +403,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var mutableBinder = new TestableMutableObjectModelBinder(); bindingContext.PropertyMetadata = mutableBinder.GetMetadataForProperties( - bindingContext.ModelBindingContext); + bindingContext.ModelBindingContext).ToArray(); // Act var retModel = await mutableBinder.CanCreateModel(bindingContext); @@ -1753,6 +1802,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public string PropertyWithInitializedValueAndDefault { get; set; } = "preinitialized"; } + private class PersonWithNoProperties + { + public string name; + } + private class PersonWithBindExclusion { [BindNever] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs index 5d795bc1eb..696f366d9d 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingTest.cs @@ -693,6 +693,47 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Null(company.Employees); } + [Fact] + public async Task PocoGetsCreated_IfTopLevelNoProperties() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + // Act + var response = await + client.GetAsync("http://localhost/Properties" + + "/GetPerson"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var person = JsonConvert.DeserializeObject( + await response.Content.ReadAsStringAsync()); + Assert.NotNull(person); + Assert.Null(person.Name); + } + + [Fact] + public async Task ArrayOfPocoGetsCreated_PoCoWithNoProperties() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + // Act + var response = await + client.GetAsync("http://localhost/Properties" + + "/GetPeople?people[0].Name=asdf"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var arrperson = JsonConvert.DeserializeObject( + await response.Content.ReadAsStringAsync()); + Assert.NotNull(arrperson); + Assert.NotNull(arrperson.people); + Assert.Equal(0, arrperson.people.Length); + } + [Theory] [InlineData("http://localhost/Home/ActionWithPersonFromUrlWithPrefix/Javier/26")] [InlineData("http://localhost/Home/ActionWithPersonFromUrlWithoutPrefix/Javier/26")] diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index 007fe6eb7c..6d9af63990 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -77,7 +77,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests public CustomReadOnlyCollection
Address { get; set; } } - [Fact] + [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] public async Task ActionParameter_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound() { // Arrange @@ -252,7 +252,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } - [Fact] + [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] public async Task ActionParameter_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound() { // Arrange diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs index 8a836da386..3395ce7fa6 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs @@ -466,7 +466,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests var parameter = new ParameterDescriptor() { Name = "parameter", - ParameterType = typeof(Collection>) + ParameterType = typeof(ICollection>) }; var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request => @@ -483,7 +483,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.NotNull(modelBindingResult); Assert.True(modelBindingResult.IsModelSet); - var model = Assert.IsType>>(modelBindingResult.Model); + var model = Assert.IsType>>(modelBindingResult.Model); Assert.NotNull(model); Assert.Empty(model); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index fba32dfbed..2b5882c895 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs @@ -173,7 +173,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests public CustomReadOnlyCollection
Address { get; set; } } - [Fact] + [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] public async Task TryUpdateModel_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound() { // Arrange @@ -409,7 +409,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } - [Fact] + [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] public async Task TryUpdateModel_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound() { // Arrange diff --git a/test/WebSites/ModelBindingWebSite/Controllers/PropertiesGetCreatedController.cs b/test/WebSites/ModelBindingWebSite/Controllers/PropertiesGetCreatedController.cs index d2da2e0c1d..dc04bde211 100644 --- a/test/WebSites/ModelBindingWebSite/Controllers/PropertiesGetCreatedController.cs +++ b/test/WebSites/ModelBindingWebSite/Controllers/PropertiesGetCreatedController.cs @@ -12,5 +12,15 @@ namespace ModelBindingWebSite.Controllers { return c; } + + public PersonWithNoProperties GetPerson(PersonWithNoProperties p) + { + return p; + } + + public ArrayOfPersonWithNoProperties GetPeople(ArrayOfPersonWithNoProperties p) + { + return p; + } } } \ No newline at end of file diff --git a/test/WebSites/ModelBindingWebSite/Models/ArrayOfPersonWithNoProperties.cs b/test/WebSites/ModelBindingWebSite/Models/ArrayOfPersonWithNoProperties.cs new file mode 100644 index 0000000000..8e820015f1 --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Models/ArrayOfPersonWithNoProperties.cs @@ -0,0 +1,10 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace ModelBindingWebSite.Models +{ + public class ArrayOfPersonWithNoProperties + { + public PersonWithNoProperties[] people { get; set; } + } +} diff --git a/test/WebSites/ModelBindingWebSite/Models/PersonWithNoProperties.cs b/test/WebSites/ModelBindingWebSite/Models/PersonWithNoProperties.cs new file mode 100644 index 0000000000..787cf9bf72 --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Models/PersonWithNoProperties.cs @@ -0,0 +1,10 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace ModelBindingWebSite.Models +{ + public class PersonWithNoProperties + { + public string Name; + } +}