From 29647fda33ad8a362f6bbf34d577dc1967b70e86 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Tue, 14 Feb 2017 11:31:29 -0800 Subject: [PATCH] [Fixes #5801] Move call to validate constructor in ComplexTypeModelBinder into CreateModel --- .../Binders/ComplexTypeModelBinder.cs | 43 +++++----- .../Binders/ComplexTypeModelBinderTest.cs | 79 ++++++++++++++++--- .../ActionParametersIntegrationTest.cs | 64 ++++++++++++++- 3 files changed, 153 insertions(+), 33 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 6f4d646131..49f48903ba 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -48,28 +48,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext) return TaskCache.CompletedTask; } - // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as - // reflection does not provide information about the implicit parameterless constructor for a struct. - // This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression - // compile fails to construct it. - var modelTypeInfo = bindingContext.ModelType.GetTypeInfo(); - if (bindingContext.Model == null && - (modelTypeInfo.IsAbstract || - modelTypeInfo.GetConstructor(Type.EmptyTypes) == null)) - { - if (bindingContext.IsTopLevelObject) - { - throw new InvalidOperationException( - Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName)); - } - - throw new InvalidOperationException( - Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty( - modelTypeInfo.FullName, - bindingContext.ModelName, - bindingContext.ModelMetadata.ContainerType.FullName)); - } - // Perf: separated to avoid allocating a state machine when we don't // need to go async. return BindModelCoreAsync(bindingContext); @@ -345,13 +323,32 @@ protected virtual object CreateModel(ModelBindingContext bindingContext) // application developer should know that this was an invalid type to try to bind to. if (_modelCreator == null) { + // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as + // reflection does not provide information about the implicit parameterless constructor for a struct. + // This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression + // compile fails to construct it. + var modelTypeInfo = bindingContext.ModelType.GetTypeInfo(); + if (modelTypeInfo.IsAbstract || modelTypeInfo.GetConstructor(Type.EmptyTypes) == null) + { + if (bindingContext.IsTopLevelObject) + { + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName)); + } + + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty( + modelTypeInfo.FullName, + bindingContext.ModelName, + bindingContext.ModelMetadata.ContainerType.FullName)); + } + _modelCreator = Expression .Lambda>(Expression.New(bindingContext.ModelType)) .Compile(); } return _modelCreator(); - } /// diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs index 78fbaa8170..1754f257d1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs @@ -6,12 +6,11 @@ using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Linq; +using System.Reflection; using System.Runtime.Serialization; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Internal; -using Microsoft.AspNetCore.Mvc.ModelBinding.Internal; -using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; using Moq; @@ -349,6 +348,52 @@ public void CreateModel_InstantiatesInstanceOfMetadataType() Assert.IsType(model); } + [Fact] + public void CreateModel_ForStructModelType_AsTopLevelObject_ThrowsException() + { + // Arrange + var bindingContext = new DefaultModelBindingContext + { + ModelMetadata = GetMetadataForType(typeof(PointStruct)), + IsTopLevelObject = true + }; + var binder = CreateBinder(bindingContext.ModelMetadata); + + // Act & Assert + var exception = Assert.Throws(() => binder.CreateModelPublic(bindingContext)); + Assert.Equal( + string.Format( + "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " + + "value types and must have a parameterless constructor.", + typeof(PointStruct).FullName), + exception.Message); + } + + [Fact] + public void CreateModel_ForStructModelType_AsProperty_ThrowsException() + { + // Arrange + var bindingContext = new DefaultModelBindingContext + { + ModelMetadata = GetMetadataForProperty(typeof(Location), nameof(Location.Point)), + ModelName = nameof(Location.Point), + IsTopLevelObject = false + }; + var binder = CreateBinder(bindingContext.ModelMetadata); + + // Act & Assert + var exception = Assert.Throws(() => binder.CreateModelPublic(bindingContext)); + Assert.Equal( + string.Format( + "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " + + "value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" + + " a non-null value in the '{2}' constructor.", + typeof(PointStruct).FullName, + nameof(Location.Point), + typeof(Location).FullName), + exception.Message); + } + [Fact] public async Task BindModelAsync_ModelIsNotNull_DoesNotCallCreateModel() { @@ -758,7 +803,7 @@ public void SetProperty_PropertyHasDefaultValue_DefaultValueAttributeDoesNothing // Arrange var model = new Person(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = metadata.Properties[nameof(model.PropertyWithDefaultValue)]; @@ -780,7 +825,7 @@ public void SetProperty_PropertyIsPreinitialized_NoValue_DoesNothing() // Arrange var model = new Person(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = metadata.Properties[nameof(model.PropertyWithInitializedValue)]; @@ -804,7 +849,7 @@ public void SetProperty_PropertyIsPreinitialized_DefaultValueAttributeDoesNothin // Arrange var model = new Person(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = metadata.Properties[nameof(model.PropertyWithInitializedValueAndDefault)]; @@ -828,7 +873,7 @@ public void SetProperty_PropertyIsReadOnly_DoesNothing() // Arrange var model = new Person(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = metadata.Properties[nameof(model.NonUpdateableProperty)]; @@ -917,7 +962,7 @@ public void SetProperty_PropertyIsSettable_CallsSetter() // Arrange var model = new Person(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.DateOfBirth)]; @@ -943,7 +988,7 @@ public void SetProperty_PropertyIsSettable_SetterThrows_RecordsError() }; var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - + var metadata = GetMetadataForType(typeof(Person)); var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.DateOfDeath)]; @@ -967,7 +1012,7 @@ public void SetProperty_PropertySetterThrows_CapturesException() var model = new ModelWhosePropertySetterThrows(); var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); bindingContext.ModelName = "foo"; - + var metadata = GetMetadataForType(typeof(ModelWhosePropertySetterThrows)); var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.NameNoAttribute)]; @@ -1036,6 +1081,22 @@ private static ModelMetadata GetMetadataForProperty(Type type, string propertyNa return _metadataProvider.GetMetadataForProperty(type, propertyName); } + private class Location + { + public PointStruct Point { get; set; } + } + + private struct PointStruct + { + public PointStruct(double x, double y) + { + X = x; + Y = y; + } + public double X { get; } + public double Y { get; } + } + private class BindingOptionalProperty { [BindingBehavior(BindingBehavior.Optional)] diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index e6470ab6f8..abd9351513 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Binders; using Xunit; namespace Microsoft.AspNetCore.Mvc.IntegrationTests @@ -460,6 +461,34 @@ public async Task ActionParameter_BindingToTypeWithNoParameterlessConstructor_Th exception.Message); } + [Fact] + public async Task ActionParameter_CustomModelBinder_CanCreateModels_ForParameterlessConstructorTypes() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(binderProvider: new CustomComplexTypeModelBinderProvider()); + var parameter = new ParameterDescriptor() + { + Name = "prefix", + ParameterType = typeof(ClassWithNoDefaultConstructor) + }; + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + // Model + Assert.NotNull(modelBindingResult.Model); + var boundModel = Assert.IsType(modelBindingResult.Model); + Assert.Equal(100, boundModel.Id); + + // ModelState + Assert.True(modelState.IsValid); + } + private struct PointStruct { public PointStruct(double x, double y) @@ -479,8 +508,12 @@ private class Class1 private class ClassWithNoDefaultConstructor { - public ClassWithNoDefaultConstructor(int id) { } + public ClassWithNoDefaultConstructor(int id) + { + Id = id; + } public string City { get; set; } + public int Id { get; } } private abstract class AbstractClassWithNoDefaultConstructor @@ -562,5 +595,34 @@ System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() return GetEnumerator(); } } + + // By default the ComplexTypeModelBinder fails to construct models for types with no parameterless constructor, + // but a developer could change this behavior by overridng CreateModel + private class CustomComplexTypeModelBinder : ComplexTypeModelBinder + { + public CustomComplexTypeModelBinder(IDictionary propertyBinders) + : base(propertyBinders) + { + } + + protected override object CreateModel(ModelBindingContext bindingContext) + { + Assert.Equal(typeof(ClassWithNoDefaultConstructor), bindingContext.ModelType); + return new ClassWithNoDefaultConstructor(100); + } + } + + private class CustomComplexTypeModelBinderProvider : IModelBinderProvider + { + public IModelBinder GetBinder(ModelBinderProviderContext context) + { + var propertyBinders = new Dictionary(); + foreach (var property in context.Metadata.Properties) + { + propertyBinders.Add(property, context.CreateBinder(property)); + } + return new CustomComplexTypeModelBinder(propertyBinders); + } + } } } \ No newline at end of file