From d09e921c4a896865b0bd81bd99e8e69dc19460b7 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Wed, 5 Oct 2016 15:25:56 -0700 Subject: [PATCH] Check for default constructor in ComplexTypeModelBinderProvider --- .../Binders/ComplexTypeModelBinderProvider.cs | 14 ++- .../ComplexTypeModelBinderProviderTest.cs | 96 +++++++++++++++ .../ActionParametersIntegrationTest.cs | 114 ++++++++++++++++++ 3 files changed, 223 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs index 580b45f8fe..bcdff753ff 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Reflection; using System.Collections.Generic; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders @@ -19,7 +20,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders throw new ArgumentNullException(nameof(context)); } - if (context.Metadata.IsComplexType && !context.Metadata.IsCollectionType) + if (context.Metadata.IsComplexType && + !context.Metadata.IsCollectionType && + HasDefaultConstructor(context.Metadata.ModelType.GetTypeInfo())) { var propertyBinders = new Dictionary(); foreach (var property in context.Metadata.Properties) @@ -32,5 +35,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return null; } + + private bool HasDefaultConstructor(TypeInfo modelTypeInfo) + { + // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs. + // - Reflection does not provide information about the implicit parameterless constructor for a struct. + // - Also this binder would eventually fail to construct an instance of the struct as the Linq's + // NewExpression compile fails to construct it. + return !modelTypeInfo.IsAbstract && modelTypeInfo.GetConstructor(Type.EmptyTypes) != null; + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs index 9dc044f814..ba9f5c1b18 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs @@ -55,11 +55,107 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.IsType(result); } + [Theory] + [InlineData(typeof(PointStructWithExplicitConstructor))] + [InlineData(typeof(PointStructWithNoExplicitConstructor))] + public void Create_ForStructModel_ReturnsNull(Type modelType) + { + // Arrange + var provider = new ComplexTypeModelBinderProvider(); + var context = new TestModelBinderProviderContext(modelType); + + // Act + var result = provider.GetBinder(context); + + // Assert + Assert.Null(result); + } + + [Theory] + [InlineData(typeof(ClassWithNoDefaultConstructor))] + [InlineData(typeof(ClassWithStaticConstructor))] + [InlineData(typeof(ClassWithInternalDefaultConstructor))] + public void Create_ForModelTypeWithNoDefaultPublicConstructor_ReturnsNull(Type modelType) + { + // Arrange + var provider = new ComplexTypeModelBinderProvider(); + var context = new TestModelBinderProviderContext(modelType); + + // Act + var result = provider.GetBinder(context); + + // Assert + Assert.Null(result); + } + + [Fact] + public void Create_ForAbstractModelTypeWithDefaultPublicConstructor_ReturnsNull() + { + // Arrange + var provider = new ComplexTypeModelBinderProvider(); + var context = new TestModelBinderProviderContext(typeof(AbstractClassWithDefaultConstructor)); + + // Act + var result = provider.GetBinder(context); + + // Assert + Assert.Null(result); + } + + private struct PointStructWithNoExplicitConstructor + { + public double X { get; set; } + public double Y { get; set; } + } + + private struct PointStructWithExplicitConstructor + { + public PointStructWithExplicitConstructor(double x, double y) + { + X = x; + Y = y; + } + public double X { get; } + public double Y { get; } + } + private class Person { public string Name { get; set; } public int Age { get; set; } } + + private class ClassWithNoDefaultConstructor + { + public ClassWithNoDefaultConstructor(int id) { } + } + + private class ClassWithInternalDefaultConstructor + { + internal ClassWithInternalDefaultConstructor() { } + } + + private class ClassWithStaticConstructor + { + static ClassWithStaticConstructor() { } + + public ClassWithStaticConstructor(int id) { } + } + + private abstract class AbstractClassWithDefaultConstructor + { + private readonly string _name; + + public AbstractClassWithDefaultConstructor() + : this("James") + { + } + + public AbstractClassWithDefaultConstructor(string name) + { + _name = name; + } + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index 09ab787188..bf9eb4ffc1 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -383,6 +383,120 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Empty(modelState.Keys); } + [Fact] + public async Task ActionParameter_ModelPropertyTypeWithNoDefaultConstructor_NoOps() + { + // Arrange + var parameterType = typeof(Class1); + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + Name = "p", + ParameterType = parameterType + }; + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = QueryString.Create("Name", "James").Add("Property1.City", "Seattle"); + }); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + Assert.True(modelState.IsValid); + var model = Assert.IsType(result.Model); + Assert.Null(model.Property1); + var keyValuePair = Assert.Single(modelState); + Assert.Equal("Name", keyValuePair.Key); + Assert.Equal("James", keyValuePair.Value.AttemptedValue); + Assert.Equal("James", keyValuePair.Value.RawValue); + } + + [Fact] + public async Task ActionParameter_BindingToStructModel_Fails() + { + // Arrange + var parameterType = typeof(PointStruct); + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + ParameterType = parameterType + }; + var testContext = ModelBindingTestHelper.GetTestContext(); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => argumentBinder.BindModelAsync(parameter, testContext)); + + Assert.Equal( + string.Format("Could not create a model binder for model object of type '{0}'.", parameterType.FullName), + exception.Message); + } + + [Theory] + [InlineData(typeof(ClassWithNoDefaultConstructor))] + [InlineData(typeof(AbstractClassWithNoDefaultConstructor))] + public async Task ActionParameter_NoDefaultConstructor_Fails(Type parameterType) + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var parameter = new ParameterDescriptor() + { + ParameterType = parameterType + }; + var testContext = ModelBindingTestHelper.GetTestContext(); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => argumentBinder.BindModelAsync(parameter, testContext)); + + Assert.Equal( + string.Format("Could not create a model binder for model object of type '{0}'.", parameterType.FullName), + exception.Message); + } + + private struct PointStruct + { + public PointStruct(double x, double y) + { + X = x; + Y = y; + } + public double X { get; } + public double Y { get; } + } + + private class Class1 + { + public ClassWithNoDefaultConstructor Property1 { get; set; } + public string Name { get; set; } + } + + private class ClassWithNoDefaultConstructor + { + public ClassWithNoDefaultConstructor(int id) { } + public string City { get; set; } + } + + private abstract class AbstractClassWithNoDefaultConstructor + { + private readonly string _name; + + public AbstractClassWithNoDefaultConstructor() + : this("James") + { + } + + public AbstractClassWithNoDefaultConstructor(string name) + { + _name = name; + } + + public string Name { get; set; } + } + private class CustomReadOnlyCollection : ICollection { private ICollection _original;