PR changes

Author: pranavkm

src/Mvc/Mvc.Abstractions/src/ModelBinding/IPropertyFilterProvider.cs

@@ -13,7 +13,7 @@ public interface IPropertyFilterProvider
         /// <summary>
         /// Gets a predicate which can determines which model properties should be bound by model binding.
         /// <para>
-        /// This predicates are also applied to determine which parameters are bound when a model's constructor is bound.
+        /// This predicate is also used to determine which parameters are bound when a model's constructor is bound.
         /// </para>
         /// </summary>
         Func<ModelMetadata, bool> PropertyFilter { get; }

src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

@@ -118,13 +118,13 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
 
                 if (_boundProperties is null)
                 {
-                    var boundParameters = BoundConstructor.Parameters;
+                    var boundParameters = BoundConstructor.Parameters!;
                     var boundProperties = new List<ModelMetadata>();
 
                     foreach (var metadata in Properties)
                     {
                         if (!boundParameters.Any(p =>
-                            string.Equals(p.ParameterName, metadata.PropertyName, StringComparison.OrdinalIgnoreCase)
+                            string.Equals(p.ParameterName, metadata.PropertyName, StringComparison.Ordinal)
                             && p.ModelType == metadata.ModelType))
                         {
                             boundProperties.Add(metadata);
@@ -153,13 +153,13 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
                     return _parameterMapping;
                 }
 
-                var boundParameters = BoundConstructor.Parameters;
+                var boundParameters = BoundConstructor.Parameters!;
                 var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
 
                 foreach (var parameter in boundParameters)
                 {
                     var property = Properties.FirstOrDefault(p =>
-                        string.Equals(p.Name, parameter.ParameterName, StringComparison.OrdinalIgnoreCase) &&
+                        string.Equals(p.Name, parameter.ParameterName, StringComparison.Ordinal) &&
                         p.ModelType == parameter.ModelType);
 
                     if (property != null)
@@ -175,19 +175,14 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
 
         /// <summary>
         /// Gets <see cref="ModelMetadata"/> instance for a constructor that is used during binding and validation.
-        /// <para>
-        /// A constructor is used during model binding and validation if it is the only constructor on the type,
-        /// is a parameterless constructor on a type with multiple constructors, or is a constructor with the
-        /// <c>ModelBindingConstructorAttribute</c>.
-        /// </para>
         /// </summary>
         public virtual ModelMetadata? BoundConstructor { get; }
 
         /// <summary>
         /// Gets the collection of <see cref="ModelMetadata"/> instances for a constructor's parameters.
         /// This is only available when <see cref="MetadataKind"/> is <see cref="ModelMetadataKind.Constructor"/>.
         /// </summary>
-        public abstract IReadOnlyList<ModelMetadata> Parameters { get; }
+        public virtual IReadOnlyList<ModelMetadata>? Parameters { get; }
 
         /// <summary>
         /// Gets the name of a model if specified explicitly using <see cref="IModelNameProvider"/>.

src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadataProvider.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.
 
 using System;
@@ -56,7 +56,7 @@ public virtual ModelMetadata GetMetadataForProperty(PropertyInfo propertyInfo, T
         }
 
         /// <summary>
-        /// Supplies metadata describing a property.
+        /// Supplies metadata describing a constructor.
         /// </summary>
         /// <param name="constructor">The <see cref="ConstructorInfo"/>.</param>
         /// <param name="modelType">The type declaring the constructor.</param>

src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs

@@ -300,8 +300,7 @@ public bool TryAddModelError(string key, Exception exception, ModelMetadata meta
                 // "The supplied value is invalid for Int32." (when error is for an element or parameter).
                 var messageProvider = metadata.ModelBindingMessageProvider;
 
-                var name = metadata.DisplayName ?? 
-                    ((metadata.MetadataKind == ModelMetadataKind.Parameter) ? metadata.ParameterName : metadata.PropertyName);
+                var name = metadata.DisplayName ?? metadata.PropertyName;
                 string errorMessage;
                 if (entry == null && name == null)
                 {

src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs

@@ -1025,8 +1025,8 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_Wit
             var bindingMetadataProvider = new DefaultBindingMetadataProvider();
             var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider });
             var optionsAccessor = new OptionsAccessor();
-            optionsAccessor.Value.ModelBindingMessageProvider.SetUnknownValueIsInvalidAccessor(
-                _ => "Hmm, the supplied value is not valid.");
+            optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyUnknownValueIsInvalidAccessor(
+                () => "Hmm, the supplied value is not valid.");
 
             var method = typeof(string).GetMethod(nameof(string.Copy));
             var parameter = method.GetParameters()[0]; // Copy(string str)
@@ -1141,8 +1141,8 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithPa
             var bindingMetadataProvider = new DefaultBindingMetadataProvider();
             var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider });
             var optionsAccessor = new OptionsAccessor();
-            optionsAccessor.Value.ModelBindingMessageProvider.SetAttemptedValueIsInvalidAccessor(
-                (value, _) => $"Hmm, the value '{ value }' is not valid.");
+            optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyAttemptedValueIsInvalidAccessor(
+                value => $"Hmm, the value '{ value }' is not valid.");
 
             var method = typeof(string).GetMethod(nameof(string.Copy));
             var parameter = method.GetParameters()[0]; // Copy(string str)

src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs

@@ -2,13 +2,12 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Buffers;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Linq.Expressions;
 using System.Reflection;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Mvc.Core;
-using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 using Microsoft.Extensions.Logging;
 
@@ -17,7 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
     /// <summary>
     /// <see cref="IModelBinder"/> implementation for binding complex types.
     /// </summary>
-    public class ComplexObjectModelBinder : IModelBinder
+    public sealed class ComplexObjectModelBinder : IModelBinder
     {
         // Don't want a new public enum because communication between the private and internal methods of this class
         // should not be exposed. Can't use an internal enum because types of [TheoryData] values must be public.
@@ -35,6 +34,8 @@ public class ComplexObjectModelBinder : IModelBinder
         private readonly IDictionary<ModelMetadata, IModelBinder> _propertyBinders;
         private readonly IReadOnlyList<IModelBinder> _parameterBinders;
         private readonly ILogger _logger;
+        private Func<object> _modelCreator;
+
 
         internal ComplexObjectModelBinder(
             IDictionary<ModelMetadata, IModelBinder> propertyBinders,
@@ -79,15 +80,9 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
             if (bindingContext.Model == null)
             {
                 var boundConstructor = modelMetadata.BoundConstructor;
-                if (boundConstructor is null)
-                {
-                    ThrowObjectCannotBeCreated(modelMetadata);
-                }
-
-                object[] values;
-                if (boundConstructor.Parameters.Count > 0)
+                if (boundConstructor != null)
                 {
-                    values = new object[boundConstructor.Parameters.Count];
+                    var values = new object[boundConstructor.Parameters.Count];
                     var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParameters(
                         bindingContext,
                         propertyData,
@@ -96,15 +91,15 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
 
                     attemptedBinding |= attemptedParameterBinding;
                     bindingSucceeded |= parameterBindingSucceeded;
+
+                    if (!CreateModel(bindingContext, boundConstructor, values))
+                    {
+                        return;
+                    }
                 }
                 else
                 {
-                    values = Array.Empty<object>();
-                }
-
-                if (!CreateModel(bindingContext, boundConstructor, values))
-                {
-                    return;
+                    CreateModel(bindingContext);
                 }
             }
 
@@ -178,6 +173,58 @@ internal static bool CreateModel(ModelBindingContext bindingContext, ModelMetada
             }
         }
 
+        /// <summary>
+        /// Creates suitable <see cref="object"/> for given <paramref name="bindingContext"/>.
+        /// </summary>
+        /// <param name="bindingContext">The <see cref="ModelBindingContext"/>.</param>
+        /// <returns>An <see cref="object"/> compatible with <see cref="ModelBindingContext.ModelType"/>.</returns>
+        internal void CreateModel(ModelBindingContext bindingContext)
+        {
+            if (bindingContext == null)
+            {
+                throw new ArgumentNullException(nameof(bindingContext));
+            }
+
+            // If model creator throws an exception, we want to propagate it back up the call stack, since the
+            // 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)
+                {
+                    var metadata = bindingContext.ModelMetadata;
+                    switch (metadata.MetadataKind)
+                    {
+                        case ModelMetadataKind.Parameter:
+                            throw new InvalidOperationException(
+                                Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForParameter(
+                                    modelTypeInfo.FullName,
+                                    metadata.ParameterName));
+                        case ModelMetadataKind.Property:
+                            throw new InvalidOperationException(
+                                Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForProperty(
+                                    modelTypeInfo.FullName,
+                                    metadata.PropertyName,
+                                    bindingContext.ModelMetadata.ContainerType.FullName));
+                        case ModelMetadataKind.Type:
+                            throw new InvalidOperationException(
+                                Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForType(
+                                    modelTypeInfo.FullName));
+                    }
+                }
+
+                _modelCreator = Expression
+                    .Lambda<Func<object>>(Expression.New(bindingContext.ModelType))
+                    .Compile();
+            }
+
+            bindingContext.Model = _modelCreator();
+        }
+
         private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindParameters(
             ModelBindingContext bindingContext,
             int propertyData,
@@ -671,32 +718,6 @@ internal void SetProperty(
             }
         }
 
-        private void ThrowObjectCannotBeCreated(ModelMetadata metadata)
-        {
-            var modelType = metadata.ModelType;
-            switch (metadata.MetadataKind)
-            {
-                case ModelMetadataKind.Parameter:
-                    throw new InvalidOperationException(
-                        Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForParameter(
-                            modelType.FullName,
-                            metadata.ParameterName));
-                case ModelMetadataKind.Property:
-                    throw new InvalidOperationException(
-                        Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForProperty(
-                            modelType.FullName,
-                            metadata.PropertyName,
-                            metadata.ContainerType.FullName));
-                case ModelMetadataKind.Type:
-                    throw new InvalidOperationException(
-                        Resources.FormatComplexObjectModelBinder_NoSuitableConstructor_ForType(
-                            modelType.FullName));
-                case ModelMetadataKind.Constructor:
-                    Debug.Fail("We do not expect constructors here.");
-                    break;
-            }
-        }
-
         private static void AddModelError(
             Exception exception,
             string modelName,

src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs

@@ -93,8 +93,7 @@ internal static ConstructorInfo GetBoundConstructor(Type type)
                 return null;
             }
 
-            var parameterlessConstructor = constructors.FirstOrDefault(f => f.GetParameters().Length == 0);
-            return GetRecordTypeConstructor(type, constructors) ?? parameterlessConstructor;
+            return GetRecordTypeConstructor(type, constructors);
         }
 
         private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorInfo[] constructors)
@@ -121,21 +120,15 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
             for (var i = 0; i < parameters.Length; i++)
             {
                 var parameter = parameters[i];
-                var mappedProperties = properties.Where(property =>
-                    string.Equals(property.Name, parameter.Name, StringComparison.OrdinalIgnoreCase) &&
-                    property.Property.PropertyType == parameter.ParameterType)
-                    .ToList();
+                var mappedProperty = properties.FirstOrDefault(property =>
+                    string.Equals(property.Name, parameter.Name, StringComparison.Ordinal) &&
+                    property.Property.PropertyType == parameter.ParameterType);
 
-                if (mappedProperties.Count == 0)
+                if (mappedProperty is null)
                 {
                     // No property found, this is not a primary constructor.
                     return null;
                 }
-                else if (mappedProperties.Count > 1)
-                {
-                    // More than one property found that maps to a constructor parameter. This is ambigious.
-                    return null;
-                }
             }
 
             return constructor;

src/Mvc/Mvc.Core/src/Resources.resx

@@ -523,13 +523,13 @@
     <value>Transformer '{0}' was retrieved from dependency injection with a state value. State can only be specified when the dynamic route is mapped using MapDynamicControllerRoute's state argument together with transient lifetime transformer. Ensure that '{0}' doesn't set its own state and that the transformer is registered with a transient lifetime in dependency injection.</value>
   </data>
   <data name="ComplexObjectModelBinder_NoSuitableConstructor_ForParameter" 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. Record types must have a single constructor. Alternatively, give the '{1}' parameter a non-null default value.</value>
+    <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. Record types must have a single primary constructor. Alternatively, give the '{1}' parameter a non-null default value.</value>
   </data>
   <data name="ComplexObjectModelBinder_NoSuitableConstructor_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. Record types must have a single constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor.</value>
+    <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. Record types must have a single primary constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor.</value>
   </data>
   <data name="ComplexObjectModelBinder_NoSuitableConstructor_ForType" 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. Record types must have a single constructor.</value>
+    <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. Record types must have a single primary constructor.</value>
   </data>
   <data name="ValidationStrategy_MappedPropertyNotFound" xml:space="preserve">
     <value>No property found that maps to constructor parameter '{0}' for type '{1}'. Validation requires that each bound parameter must have a property to read the value.</value>