[Fixes #5698] Regression in 1.1 model binding for model types without default constructor

- Also reverts "Check for default constructor in ComplexTypeModelBinderProvider" commit d09e921c4a.

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);

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 &&
+            if (context.Metadata.IsComplexType && !context.Metadata.IsCollectionType)
-                !context.Metadata.IsCollectionType &&
-                HasDefaultConstructor(context.Metadata.ModelType.GetTypeInfo()))
             {
                 var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
                 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;
-        }
     }
 }

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);
         }
 
-        /// <summary>
-        /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.
-        /// </summary>
-        internal static string MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType
-        {
-            get { return GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"); }
-        }
-
-        /// <summary>
-        /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.
-        /// </summary>
-        internal static string FormatMiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType(object p0, object p1)
-        {
-            return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"), p0, p1);
-        }
-
         /// <summary>
         /// An {0} cannot be created without a valid instance of {1}.
         /// </summary>
@@ -1355,7 +1339,7 @@ namespace Microsoft.AspNetCore.Mvc.Core
         }
 
         /// <summary>
-        /// VaryByQueryKeys requires the response cache middleware.
+        /// '{0}' requires the response cache middleware.
         /// </summary>
         internal static string VaryByQueryKeys_Requires_ResponseCachingMiddleware
         {
@@ -1363,7 +1347,7 @@ namespace Microsoft.AspNetCore.Mvc.Core
         }
 
         /// <summary>
-        /// VaryByQueryKeys requires the response cache middleware.
+        /// '{0}' requires the response cache middleware.
         /// </summary>
         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);
         }
 
+        /// <summary>
+        /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.
+        /// </summary>
+        internal static string MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType
+        {
+            get { return GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"); }
+        }
+
+        /// <summary>
+        /// Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.
+        /// </summary>
+        internal static string FormatMiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType(object p0, object p1)
+        {
+            return string.Format(CultureInfo.CurrentCulture, GetString("MiddlewareFilterConfigurationProvider_CreateConfigureDelegate_CannotCreateType"), p0, p1);
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        internal static string ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject
+        {
+            get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"); }
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(object p0)
+        {
+            return string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"), p0);
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        internal static string ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty
+        {
+            get { return GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty"); }
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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);

src/Microsoft.AspNetCore.Mvc.Core/Resources.resx

@@ -387,4 +387,10 @@
     <value>Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.</value>
     <comment>0 is the type to configure. 1 is the name of the parameter, configurationType.</comment>
   </data>
+  <data name="ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject" xml:space="preserve">
+    <value>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.</value>
+  </data>
+  <data name="ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty" xml:space="preserve">
+    <value>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.</value>
+  </data>
 </root>

src/Microsoft.AspNetCore.Mvc.TagHelpers/Properties/Resources.Designer.cs

@@ -139,7 +139,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers
         }
 
         /// <summary>
-        /// 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.
         /// </summary>
         internal static string FormTagHelper_CannotDetermineActionWithRouteAndActionOrControllerSpecified
         {
@@ -171,7 +171,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers
         }
 
         /// <summary>
-        /// Cannot override the '{6}' attribute for &lt;{0}&gt;. &lt;{0}&gt; 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 &lt;{0}&gt;. &lt;{0}&gt; elements with a specified '{7}' must not have attributes starting with '{6}' or an '{1}', '{2}', '{3}', '{4}', or '{5}' attribute.
         /// </summary>
         internal static string FormActionTagHelper_CannotOverrideFormAction
         {
@@ -179,7 +179,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers
         }
 
         /// <summary>
-        /// Cannot override the '{6}' attribute for &lt;{0}&gt;. &lt;{0}&gt; 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 &lt;{0}&gt;. &lt;{0}&gt; elements with a specified '{7}' must not have attributes starting with '{6}' or an '{1}', '{2}', '{3}', '{4}', or '{5}' attribute.
         /// </summary>
         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
         }
 
         /// <summary>
-        /// Cannot determine a '{4}' attribute for &lt;{0}&gt;. &lt;{0}&gt; elements with a specified '{1}' must not have an '{2}' or '{3}' attribute.
+        /// Cannot determine a '{4}' attribute for &lt;{0}&gt;. &lt;{0}&gt; elements with a specified '{1}' must not have an '{2}', '{3}', or '{5}' attribute.
         /// </summary>
         internal static string FormActionTagHelper_CannotDetermineFormActionRouteActionOrControllerSpecified
         {

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderProviderTest.cs

@@ -55,107 +55,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
             Assert.IsType<ComplexTypeModelBinder>(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;
-            }
-        }
     }
 }

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
+            // Act & Assert
-            var result = await argumentBinder.BindModelAsync(parameter, testContext);
+            var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => argumentBinder.BindModelAsync(parameter, testContext));
-
+            Assert.Equal(
-            // Assert
+                string.Format(
-            Assert.True(result.IsModelSet);
+                    "Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " +
-            Assert.True(modelState.IsValid);
+                    "value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" +
-            var model = Assert.IsType<Class1>(result.Model);
+                    " a non-null value in the '{2}' constructor.",
-            Assert.Null(model.Property1);
+                    typeof(ClassWithNoDefaultConstructor).FullName,
-            var keyValuePair = Assert.Single(modelState);
+                    nameof(Class1.Property1),
-            Assert.Equal("Name", keyValuePair.Key);
+                    typeof(Class1).FullName),
-            Assert.Equal("James", keyValuePair.Value.AttemptedValue);
+                exception.Message);
-            Assert.Equal("James", keyValuePair.Value.RawValue);
         }
 
         [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<InvalidOperationException>(
+            var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => argumentBinder.BindModelAsync(parameter, testContext));
-                () => 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<InvalidOperationException>(
+            var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => argumentBinder.BindModelAsync(parameter, testContext));
-                () => 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);
         }
 

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";