Cleanup InferParameterBindingInfoConvention (#8665)

* Cleanup InferParameterBindingInfoConvention

* Infer BindingSource for collection parameters as Body. Fixes #8536
* Introduce a compat switch to keep 2.1.x LTS behavior for collection parameters
* Do not infer BinderModelName in InferParameterBindingInfoConvention

Author: pranavkm

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

@@ -349,8 +349,9 @@ public virtual ModelMetadata ContainerMetadata
         /// Gets a value indicating whether <see cref="ModelType"/> is a complex type.
         /// </summary>
         /// <remarks>
-        /// A complex type is defined as a <see cref="Type"/> which has a
-        /// <see cref="TypeConverter"/> that can convert from <see cref="string"/>.
+        /// A complex type is defined as a <see cref="Type"/> without a <see cref="TypeConverter"/> that can convert
+        /// from <see cref="string"/>. Most POCO and <see cref="IEnumerable"/> types are therefore complex. Most, if
+        /// not all, BCL value types are simple types.
         /// </remarks>
         public bool IsComplexType { get; private set; }
 

src/Microsoft.AspNetCore.Mvc.Core/ApiBehaviorOptions.cs

@@ -17,6 +17,7 @@ public class ApiBehaviorOptions : IEnumerable<ICompatibilitySwitch>
     {
         private readonly CompatibilitySwitch<bool> _suppressMapClientErrors;
         private readonly CompatibilitySwitch<bool> _suppressUseValidationProblemDetailsForInvalidModelStateResponses;
+        private readonly CompatibilitySwitch<bool> _allowInferringBindingSourceForCollectionTypesAsFromQuery;
         private readonly ICompatibilitySwitch[] _switches;
 
         private Func<ActionContext, IActionResult> _invalidModelStateResponseFactory;
@@ -28,10 +29,12 @@ public ApiBehaviorOptions()
         {
             _suppressMapClientErrors = new CompatibilitySwitch<bool>(nameof(SuppressMapClientErrors));
             _suppressUseValidationProblemDetailsForInvalidModelStateResponses = new CompatibilitySwitch<bool>(nameof(SuppressUseValidationProblemDetailsForInvalidModelStateResponses));
+            _allowInferringBindingSourceForCollectionTypesAsFromQuery = new CompatibilitySwitch<bool>(nameof(AllowInferringBindingSourceForCollectionTypesAsFromQuery));
             _switches = new[]
             {
                 _suppressMapClientErrors,
                 _suppressUseValidationProblemDetailsForInvalidModelStateResponses,
+                _allowInferringBindingSourceForCollectionTypesAsFromQuery
             };
         }
 
@@ -151,6 +154,43 @@ public bool SuppressUseValidationProblemDetailsForInvalidModelStateResponses
             set => _suppressUseValidationProblemDetailsForInvalidModelStateResponses.Value = value;
         }
 
+        /// <summary>
+        /// Gets or sets a value that determines if <see cref="BindingInfo.BindingSource"/> for collection types
+        /// (<see cref="ModelMetadata.IsCollectionType"/>).
+        /// <para>
+        /// When <see langword="true" />, the binding source for collection types is inferred as <see cref="BindingSource.Query"/>.
+        /// Otherwise <see cref="BindingSource.Body"/> is inferred.
+        /// </para>
+        /// </summary>
+        /// <value>
+        /// The default value is <see langword="false"/> if the version is
+        /// <see cref="CompatibilityVersion.Version_2_2"/> or later; <see langword="true"/> otherwise.
+        /// </value>
+        /// <remarks>
+        /// <para>
+        /// This property is associated with a compatibility switch and can provide a different behavior depending on
+        /// the configured compatibility version for the application. See <see cref="CompatibilityVersion"/> for
+        /// guidance and examples of setting the application's compatibility version.
+        /// </para>
+        /// <para>
+        /// Configuring the desired value of the compatibility switch by calling this property's setter takes
+        /// precedence over the value implied by the application's <see cref="CompatibilityVersion"/>.
+        /// </para>
+        /// <para>
+        /// If the application's compatibility version is set to <see cref="CompatibilityVersion.Version_2_1"/> or
+        /// lower then this setting will have the value <see langword="true"/> unless explicitly configured.
+        /// </para>
+        /// <para>
+        /// If the application's compatibility version is set to <see cref="CompatibilityVersion.Version_2_2"/> or
+        /// higher then this setting will have the value <see langword="false"/> unless explicitly configured.
+        /// </para>
+        /// </remarks>
+        public bool AllowInferringBindingSourceForCollectionTypesAsFromQuery
+        {
+            get => _allowInferringBindingSourceForCollectionTypesAsFromQuery.Value;
+            set => _allowInferringBindingSourceForCollectionTypesAsFromQuery.Value = value;
+        }
+
         /// <summary>
         /// Gets a map of HTTP status codes to <see cref="ClientErrorData"/>. Configured values
         /// are used to transform <see cref="IClientErrorActionResult"/> to an <see cref="ObjectResult"/>

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ApiBehaviorApplicationModelProvider.cs

@@ -48,14 +48,15 @@ public ApiBehaviorApplicationModelProvider(
             var defaultErrorTypeAttribute = new ProducesErrorResponseTypeAttribute(defaultErrorType);
             ActionModelConventions.Add(new ApiConventionApplicationModelConvention(defaultErrorTypeAttribute));
 
-            var inferParameterBindingInfoConvention = new InferParameterBindingInfoConvention(modelMetadataProvider)
+            if (!options.SuppressInferBindingSourcesForParameters)
             {
-                SuppressInferBindingSourcesForParameters = options.SuppressInferBindingSourcesForParameters
-            };
-            ControllerModelConventions = new List<IControllerModelConvention>
-            {
-                inferParameterBindingInfoConvention,
-            };
+                var convention = new InferParameterBindingInfoConvention(modelMetadataProvider)
+                {
+                    AllowInferringBindingSourceForCollectionTypesAsFromQuery = options.AllowInferringBindingSourceForCollectionTypesAsFromQuery,
+                };
+
+                ActionModelConventions.Add(convention);
+            }
         }
 
         /// <remarks>
@@ -66,8 +67,6 @@ public ApiBehaviorApplicationModelProvider(
 
         public List<IActionModelConvention> ActionModelConventions { get; }
 
-        public List<IControllerModelConvention> ControllerModelConventions { get; }
-
         public void OnProvidersExecuted(ApplicationModelProviderContext context)
         {
         }
@@ -91,11 +90,6 @@ public void OnProvidersExecuting(ApplicationModelProviderContext context)
                         convention.Apply(action);
                     }
                 }
-
-                foreach (var convention in ControllerModelConventions)
-                {
-                    convention.Apply(controller);
-                }
             }
         }
 

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs

@@ -2,9 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
 using System.Linq;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Internal;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Routing.Template;
@@ -13,13 +11,18 @@
 namespace Microsoft.AspNetCore.Mvc.ApplicationModels
 {
     /// <summary>
-    /// A <see cref="IControllerModelConvention"/> that
-    /// <list type="bullet">
-    /// <item>infers binding sources for parameters</item>
-    /// <item><see cref="BindingInfo.BinderModelName"/> for bound properties and parameters.</item>
-    /// </list>
+    /// An <see cref="IActionModelConvention"/> that infers <see cref="BindingInfo.BindingSource"/> for parameters.
     /// </summary>
-    public class InferParameterBindingInfoConvention : IControllerModelConvention
+    /// <remarks>
+    /// The goal of this covention is to make intuitive and easy to document <see cref="BindingSource"/> inferences. The rules are:
+    /// <list type="number">
+    /// <item>A previously specified <see cref="BindingInfo.BindingSource" /> is never overwritten.</item>
+    /// <item>A complex type parameter (<see cref="ModelMetadata.IsComplexType"/>) is assigned <see cref="BindingSource.Body"/>.</item>
+    /// <item>Parameter with a name that appears as a route value in ANY route template is assigned <see cref="BindingSource.Path"/>.</item>
+    /// <item>All other parameters are <see cref="BindingSource.Query"/>.</item>
+    /// </list>
+    /// </remarks>
+    public class InferParameterBindingInfoConvention : IActionModelConvention
     {
         private readonly IModelMetadataProvider _modelMetadataProvider;
 
@@ -29,41 +32,27 @@ public InferParameterBindingInfoConvention(
             _modelMetadataProvider = modelMetadataProvider ?? throw new ArgumentNullException(nameof(modelMetadataProvider));
         }
 
-        /// <summary>
-        /// Gets or sets a value that determines if model binding sources are inferred for action parameters on controllers is suppressed.
-        /// </summary>
-        public bool SuppressInferBindingSourcesForParameters { get; set; }
+        internal bool AllowInferringBindingSourceForCollectionTypesAsFromQuery { get; set; }
 
-        protected virtual bool ShouldApply(ControllerModel controller) => true;
+        protected virtual bool ShouldApply(ActionModel action) => true;
 
-        public void Apply(ControllerModel controller)
+        public void Apply(ActionModel action)
         {
-            if (controller == null)
+            if (action == null)
             {
-                throw new ArgumentNullException(nameof(controller));
+                throw new ArgumentNullException(nameof(action));
             }
 
-            if (!ShouldApply(controller))
+            if (!ShouldApply(action))
             {
                 return;
             }
 
-            InferBoundPropertyModelPrefixes(controller);
-
-            foreach (var action in controller.Actions)
-            {
-                InferParameterBindingSources(action);
-                InferParameterModelPrefixes(action);
-            }
+            InferParameterBindingSources(action);
         }
 
         internal void InferParameterBindingSources(ActionModel action)
         {
-            if (SuppressInferBindingSourcesForParameters)
-            {
-                return;
-            }
-
             for (var i = 0; i < action.Parameters.Count; i++)
             {
                 var parameter = action.Parameters[i];
@@ -95,56 +84,17 @@ internal void InferParameterBindingSources(ActionModel action)
         // Internal for unit testing.
         internal BindingSource InferBindingSourceForParameter(ParameterModel parameter)
         {
-            if (ParameterExistsInAnyRoute(parameter.Action, parameter.ParameterName))
+            if (IsComplexTypeParameter(parameter))
             {
-                return BindingSource.Path;
+                return BindingSource.Body;
             }
 
-            var bindingSource = IsComplexTypeParameter(parameter) ?
-                BindingSource.Body :
-                BindingSource.Query;
-
-            return bindingSource;
-        }
-
-        // For any complex types that are bound from value providers, set the prefix
-        // to the empty prefix by default. This makes binding much more predictable
-        // and describable via ApiExplorer
-        internal void InferBoundPropertyModelPrefixes(ControllerModel controllerModel)
-        {
-            foreach (var property in controllerModel.ControllerProperties)
+            if (ParameterExistsInAnyRoute(parameter.Action, parameter.ParameterName))
             {
-                if (property.BindingInfo != null &&
-                    property.BindingInfo.BinderModelName == null &&
-                    property.BindingInfo.BindingSource != null &&
-                    !property.BindingInfo.BindingSource.IsGreedy &&
-                    !IsFormFile(property.ParameterType))
-                {
-                    var metadata = _modelMetadataProvider.GetMetadataForProperty(
-                        controllerModel.ControllerType,
-                        property.PropertyInfo.Name);
-                    if (metadata.IsComplexType && !metadata.IsCollectionType)
-                    {
-                        property.BindingInfo.BinderModelName = string.Empty;
-                    }
-                }
+                return BindingSource.Path;
             }
-        }
 
-        internal void InferParameterModelPrefixes(ActionModel action)
-        {
-            foreach (var parameter in action.Parameters)
-            {
-                var bindingInfo = parameter.BindingInfo;
-                if (bindingInfo?.BindingSource != null &&
-                    bindingInfo.BinderModelName == null &&
-                    !bindingInfo.BindingSource.IsGreedy &&
-                    !IsFormFile(parameter.ParameterType) &&
-                    IsComplexTypeParameter(parameter))
-                {
-                    parameter.BindingInfo.BinderModelName = string.Empty;
-                }
-            }
+            return BindingSource.Query;
         }
 
         private bool ParameterExistsInAnyRoute(ActionModel action, string parameterName)
@@ -171,13 +121,13 @@ private bool IsComplexTypeParameter(ParameterModel parameter)
             // No need for information from attributes on the parameter. Just use its type.
             var metadata = _modelMetadataProvider
                 .GetMetadataForType(parameter.ParameterInfo.ParameterType);
-            return metadata.IsComplexType && !metadata.IsCollectionType;
-        }
 
-        private static bool IsFormFile(Type parameterType)
-        {
-            return typeof(IFormFile).IsAssignableFrom(parameterType) ||
-                typeof(IEnumerable<IFormFile>).IsAssignableFrom(parameterType);
+            if (AllowInferringBindingSourceForCollectionTypesAsFromQuery && metadata.IsCollectionType)
+            {
+                return false;
+            }
+
+            return metadata.IsComplexType;
         }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorOptionsSetup.cs

@@ -35,6 +35,7 @@ protected override IReadOnlyDictionary<string, object> DefaultValues
                 {
                     dictionary[nameof(ApiBehaviorOptions.SuppressMapClientErrors)] = true;
                     dictionary[nameof(ApiBehaviorOptions.SuppressUseValidationProblemDetailsForInvalidModelStateResponses)] = true;
+                    dictionary[nameof(ApiBehaviorOptions.AllowInferringBindingSourceForCollectionTypesAsFromQuery)] = true;
                 }
 
                 return dictionary;

test/Microsoft.AspNetCore.Mvc.Core.Test/ApplicationModels/ApiBehaviorApplicationModelProviderTest.cs

@@ -98,15 +98,8 @@ public void Constructor_SetsUpConventions()
                 {
                     var convention = Assert.IsType<ApiConventionApplicationModelConvention>(c);
                     Assert.Equal(typeof(ProblemDetails), convention.DefaultErrorResponseType.Type);
-                });
-
-            Assert.Collection(
-                provider.ControllerModelConventions,
-                c =>
-                {
-                    var convention = Assert.IsType<InferParameterBindingInfoConvention>(c);
-                    Assert.False(convention.SuppressInferBindingSourcesForParameters);
-                });
+                },
+                c => Assert.IsType<InferParameterBindingInfoConvention>(c));
         }
 
         [Fact]
@@ -140,14 +133,13 @@ public void Constructor_DoesNotAddConsumesConstraintForFormFileParameterConventi
         }
 
         [Fact]
-        public void Constructor_SetsSuppressInferBindingSourcesForParametersIsSet()
+        public void Constructor_DoesNotAddInferParameterBindingInfoConvention_IfSuppressInferBindingSourcesForParametersIsSet()
         {
             // Arrange
             var provider = GetProvider(new ApiBehaviorOptions { SuppressInferBindingSourcesForParameters = true });
 
             // Act & Assert
-            var convention = Assert.Single(provider.ControllerModelConventions.OfType<InferParameterBindingInfoConvention>());
-            Assert.True(convention.SuppressInferBindingSourcesForParameters);
+            Assert.Empty(provider.ActionModelConventions.OfType<InferParameterBindingInfoConvention>());
         }
 
         [Fact]