From 842d661ac2f5bbe365feca96b8171785cb4f7bf2 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Thu, 9 Feb 2017 10:09:55 -0800 Subject: [PATCH] [Fixes #5698] Regression in 1.1 model binding for model types without default constructor - Also reverts "Check for default constructor in ComplexTypeModelBinderProvider" commit d09e921c4a896865b0bd81bd99e8e69dc19460b7. --- .../Binders/ComplexTypeModelBinder.cs | 23 +++++ .../Binders/ComplexTypeModelBinderProvider.cs | 14 +-- .../Properties/Resources.Designer.cs | 68 +++++++++---- .../Resources.resx | 6 ++ .../Properties/Resources.Designer.cs | 8 +- .../ComplexTypeModelBinderProviderTest.cs | 96 ------------------- .../ActionParametersIntegrationTest.cs | 53 +++++----- .../TryUpdateModelIntegrationTest.cs | 51 ++++++++++ 8 files changed, 163 insertions(+), 156 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 3109b1e0e7..6f4d646131 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Linq.Expressions; using System.Reflection; using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Internal; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders @@ -47,6 +48,28 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders 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); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs index bcdff753ff..580b45f8fe 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinderProvider.cs @@ -2,7 +2,6 @@ // 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 @@ -20,9 +19,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders throw new ArgumentNullException(nameof(context)); } - if (context.Metadata.IsComplexType && - !context.Metadata.IsCollectionType && - HasDefaultConstructor(context.Metadata.ModelType.GetTypeInfo())) + if (context.Metadata.IsComplexType && !context.Metadata.IsCollectionType) { var propertyBinders = new Dictionary(); foreach (var property in context.Metadata.Properties) @@ -35,14 +32,5 @@ 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/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index 618765631f..0c0a0b6324 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1306,22 +1306,6 @@ namespace Microsoft.AspNetCore.Mvc.Core return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilter_ServiceResolutionFail"), p0, p1, p2, p3); } - /// - /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor. - /// - internal static string MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType - { - get { return GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"); } - } - - /// - /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor. - /// - internal static string FormatMiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType(object p0, object p1) - { - return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"), p0, p1); - } - /// /// An {0} cannot be created without a valid instance of {1}. /// @@ -1355,7 +1339,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// VaryByQueryKeys requires the response cache middleware. + /// '{0}' requires the response cache middleware. /// internal static string VaryByQueryKeys_Requires_ResponseCachingMiddleware { @@ -1363,7 +1347,7 @@ namespace Microsoft.AspNetCore.Mvc.Core } /// - /// VaryByQueryKeys requires the response cache middleware. + /// '{0}' requires the response cache middleware. /// internal static string FormatVaryByQueryKeys_Requires_ResponseCachingMiddleware(object p0) { @@ -1386,6 +1370,54 @@ namespace Microsoft.AspNetCore.Mvc.Core return string.Format(CultureInfo.CurrentCulture, GetString("CandidateResolver_DifferentCasedReference"), p0); } + /// + /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor. + /// + internal static string MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType + { + get { return GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"); } + } + + /// + /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor. + /// + internal static string FormatMiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType(object p0, object p1) + { + return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"), p0, p1); + } + + /// + /// 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. + /// + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject + { + get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"); } + } + + /// + /// 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. + /// + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"), p0); + } + + /// + /// 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. + /// + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty + { + get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty"); } + } + + /// + /// 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. + /// + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty(object p0, object p1, object p2) + { + return string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty"), p0, p1, p2); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 8a35cabd50..db8178fd62 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -387,4 +387,10 @@ Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor. 0 is the type to configure. 1 is the name of the parameter, configurationType. + + 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. + + + 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. + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.TagHelpers/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.TagHelpers/Properties/Resources.Designer.cs index d2d8502c6f..5ef947f86e 100644 --- a/src/Microsoft.AspNetCore.Mvc.TagHelpers/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.TagHelpers/Properties/Resources.Designer.cs @@ -139,7 +139,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers } /// - /// Cannot determine an '{4}' attribute for {0}. A {0} with a specified '{1}' must not have an '{2}' or '{3}' attribute. + /// Cannot determine an '{4}' attribute for {0}. A {0} with a specified '{1}' must not have an '{2}', '{3}', or '{5}' attribute. /// internal static string FormTagHelper_CannotDetermineActionWithRouteAndActionOrControllerSpecified { @@ -171,7 +171,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers } /// - /// Cannot override the '{6}' attribute for <{0}>. <{0}> elements with a specified '{6}' must not have attributes starting with '{5}' or an '{1}', '{2}', '{3}', or '{4}' attribute. + /// Cannot override the '{7}' attribute for <{0}>. <{0}> elements with a specified '{7}' must not have attributes starting with '{6}' or an '{1}', '{2}', '{3}', '{4}', or '{5}' attribute. /// internal static string FormActionTagHelper_CannotOverrideFormAction { @@ -179,7 +179,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers } /// - /// Cannot override the '{6}' attribute for <{0}>. <{0}> elements with a specified '{7}' must not have attributes starting with '{6}' or an '{1}', '{2}', '{3}', '{4}', or '{5}' attribute. + /// Cannot override the '{7}' attribute for <{0}>. <{0}> elements with a specified '{7}' must not have attributes starting with '{6}' or an '{1}', '{2}', '{3}', '{4}', or '{5}' attribute. /// internal static string FormatFormActionTagHelper_CannotOverrideFormAction(object p0, object p1, object p2, object p3, object p4, object p5, object p6, object p7) { @@ -187,7 +187,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers } /// - /// Cannot determine a '{4}' attribute for <{0}>. <{0}> elements with a specified '{1}' must not have an '{2}' or '{3}' attribute. + /// Cannot determine a '{4}' attribute for <{0}>. <{0}> elements with a specified '{1}' must not have an '{2}', '{3}', or '{5}' attribute. /// internal static string FormActionTagHelper_CannotDetermineFormActionRouteActionOrControllerSpecified { 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 ba9f5c1b18..9dc044f814 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs @@ -55,107 +55,11 @@ 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 bf9eb4ffc1..e6470ab6f8 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -384,7 +384,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests } [Fact] - public async Task ActionParameter_ModelPropertyTypeWithNoDefaultConstructor_NoOps() + public async Task ActionParameter_ModelPropertyTypeWithNoParameterlessConstructor_ThrowsException() { // Arrange var parameterType = typeof(Class1); @@ -400,60 +400,63 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests }); 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); + // Act & Assert + var exception = await Assert.ThrowsAsync(() => argumentBinder.BindModelAsync(parameter, testContext)); + 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(ClassWithNoDefaultConstructor).FullName, + nameof(Class1.Property1), + typeof(Class1).FullName), + exception.Message); } [Fact] - public async Task ActionParameter_BindingToStructModel_Fails() + public async Task ActionParameter_BindingToStructModel_ThrowsException() { // Arrange var parameterType = typeof(PointStruct); var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); var parameter = new ParameterDescriptor() { - ParameterType = parameterType + ParameterType = parameterType, + Name = "p" }; var testContext = ModelBindingTestHelper.GetTestContext(); // Act & Assert - var exception = await Assert.ThrowsAsync( - () => argumentBinder.BindModelAsync(parameter, testContext)); - + 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), + 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); } [Theory] [InlineData(typeof(ClassWithNoDefaultConstructor))] [InlineData(typeof(AbstractClassWithNoDefaultConstructor))] - public async Task ActionParameter_NoDefaultConstructor_Fails(Type parameterType) + public async Task ActionParameter_BindingToTypeWithNoParameterlessConstructor_ThrowsException(Type parameterType) { // Arrange var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); var parameter = new ParameterDescriptor() { - ParameterType = parameterType + ParameterType = parameterType, + Name = "p" }; var testContext = ModelBindingTestHelper.GetTestContext(); // Act & Assert - var exception = await Assert.ThrowsAsync( - () => argumentBinder.BindModelAsync(parameter, testContext)); - + 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), + 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.", + parameterType.FullName), exception.Message); } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index 982879aa23..47b8ac9945 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs @@ -1091,6 +1091,57 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(modelStateEntry.RawValue); } + private class AddressWithNoParameterlessConstructor + { + private readonly int _id; + public AddressWithNoParameterlessConstructor(int id) + { + _id = id; + } + public string Street { get; set; } + public string City { get; set; } + } + + [Fact] + public async Task TryUpdateModel_ExistingModelWithNoParameterlessConstructor_OverwritesBoundValues() + { + // Arrange + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = QueryString.Create("Street", "SomeStreet"); + }); + + var modelState = testContext.ModelState; + var model = new AddressWithNoParameterlessConstructor(10) + { + Street = "DefaultStreet", + City = "Toronto", + }; + var oldModel = model; + + // Act + var result = await TryUpdateModelAsync(model, string.Empty, testContext); + + // Assert + Assert.True(result); + + // Model + Assert.Same(oldModel, model); + Assert.Equal("SomeStreet", model.Street); + Assert.Equal("Toronto", model.City); + + // ModelState + Assert.True(modelState.IsValid); + + var entry = Assert.Single(modelState); + Assert.Equal("Street", entry.Key); + var state = entry.Value; + Assert.Equal("SomeStreet", state.AttemptedValue); + Assert.Equal("SomeStreet", state.RawValue); + Assert.Empty(state.Errors); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + } + private void UpdateRequest(HttpRequest request, string data, string name) { const string fileName = "text.txt";