Added new overloads to RequestDelegateFactory (#34375)

- Throw exceptions if route parameters are specified via attributes but not declared in the list of route parameters.
- Don't fall back to the query string if the the route parameter is specified.
- Added tests

Author: davidfowl

src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt

@@ -157,6 +157,12 @@ Microsoft.AspNetCore.Http.Headers.ResponseHeaders.SetCookie.get -> System.Collec
 Microsoft.AspNetCore.Http.Headers.ResponseHeaders.SetCookie.set -> void
 Microsoft.AspNetCore.Http.Headers.ResponseHeaders.SetList<T>(string! name, System.Collections.Generic.IList<T>? values) -> void
 Microsoft.AspNetCore.Http.RequestDelegateFactory
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RequestDelegateFactoryOptions() -> void
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteParameterNames.get -> System.Collections.Generic.IEnumerable<string!>?
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.RouteParameterNames.init -> void
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.ServiceProvider.get -> System.IServiceProvider?
+Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions.ServiceProvider.init -> void
 Microsoft.AspNetCore.Mvc.ProblemDetails
 Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.get -> string?
 Microsoft.AspNetCore.Mvc.ProblemDetails.Detail.set -> void
@@ -186,9 +192,8 @@ static Microsoft.AspNetCore.Http.HeaderDictionaryTypeExtensions.AppendList<T>(th
 static Microsoft.AspNetCore.Http.HeaderDictionaryTypeExtensions.GetTypedHeaders(this Microsoft.AspNetCore.Http.HttpRequest! request) -> Microsoft.AspNetCore.Http.Headers.RequestHeaders!
 static Microsoft.AspNetCore.Http.HeaderDictionaryTypeExtensions.GetTypedHeaders(this Microsoft.AspNetCore.Http.HttpResponse! response) -> Microsoft.AspNetCore.Http.Headers.ResponseHeaders!
 static Microsoft.AspNetCore.Http.HttpContextServerVariableExtensions.GetServerVariable(this Microsoft.AspNetCore.Http.HttpContext! context, string! variableName) -> string?
-static Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(System.Delegate! action, System.IServiceProvider? serviceProvider) -> Microsoft.AspNetCore.Http.RequestDelegate!
-static Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(System.Reflection.MethodInfo! methodInfo, System.IServiceProvider? serviceProvider) -> Microsoft.AspNetCore.Http.RequestDelegate!
-static Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(System.Reflection.MethodInfo! methodInfo, System.IServiceProvider? serviceProvider, System.Func<Microsoft.AspNetCore.Http.HttpContext!, object!>! targetFactory) -> Microsoft.AspNetCore.Http.RequestDelegate!
+static Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(System.Delegate! action, Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions? options = null) -> Microsoft.AspNetCore.Http.RequestDelegate!
+static Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(System.Reflection.MethodInfo! methodInfo, System.Func<Microsoft.AspNetCore.Http.HttpContext!, object!>? targetFactory = null, Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions? options = null) -> Microsoft.AspNetCore.Http.RequestDelegate!
 static Microsoft.AspNetCore.Http.ResponseExtensions.Clear(this Microsoft.AspNetCore.Http.HttpResponse! response) -> void
 static Microsoft.AspNetCore.Http.ResponseExtensions.Redirect(this Microsoft.AspNetCore.Http.HttpResponse! response, string! location, bool permanent, bool preserveMethod) -> void
 static Microsoft.AspNetCore.Http.SendFileResponseExtensions.SendFileAsync(this Microsoft.AspNetCore.Http.HttpResponse! response, Microsoft.Extensions.FileProviders.IFileInfo! file, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task!

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

@@ -60,9 +60,11 @@ public static partial class RequestDelegateFactory
         /// Creates a <see cref="RequestDelegate"/> implementation for <paramref name="action"/>.
         /// </summary>
         /// <param name="action">A request handler with any number of custom parameters that often produces a response with its return value.</param>
-        /// <param name="serviceProvider">The <see cref="IServiceProvider"/> instance used to detect which parameters are services.</param>
+        /// <param name="options">The <see cref="RequestDelegateFactoryOptions"/> used to configure the behavior of the handler.</param>
         /// <returns>The <see cref="RequestDelegate"/>.</returns>
-        public static RequestDelegate Create(Delegate action, IServiceProvider? serviceProvider)
+#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
+        public static RequestDelegate Create(Delegate action, RequestDelegateFactoryOptions? options = null)
+#pragma warning restore RS0026 // Do not add multiple public overloads with optional parameters
         {
             if (action is null)
             {
@@ -75,69 +77,60 @@ public static RequestDelegate Create(Delegate action, IServiceProvider? serviceP
                 null => null,
             };
 
-            var targetableRequestDelegate = CreateTargetableRequestDelegate(action.Method, serviceProvider, targetExpression);
+            var targetableRequestDelegate = CreateTargetableRequestDelegate(action.Method, options, targetExpression);
 
             return httpContext =>
             {
                 return targetableRequestDelegate(action.Target, httpContext);
             };
         }
 
-        /// <summary>
-        /// Creates a <see cref="RequestDelegate"/> implementation for <paramref name="methodInfo"/>.
-        /// </summary>
-        /// <param name="methodInfo">A static request handler with any number of custom parameters that often produces a response with its return value.</param>
-        /// <param name="serviceProvider">The <see cref="IServiceProvider"/> instance used to detect which parameters are services.</param>
-        /// <returns>The <see cref="RequestDelegate"/>.</returns>
-        public static RequestDelegate Create(MethodInfo methodInfo, IServiceProvider? serviceProvider)
-        {
-            if (methodInfo is null)
-            {
-                throw new ArgumentNullException(nameof(methodInfo));
-            }
-
-            var targetableRequestDelegate = CreateTargetableRequestDelegate(methodInfo, serviceProvider, targetExpression: null);
-
-            return httpContext =>
-            {
-                return targetableRequestDelegate(null, httpContext);
-            };
-        }
-
         /// <summary>
         /// Creates a <see cref="RequestDelegate"/> implementation for <paramref name="methodInfo"/>.
         /// </summary>
         /// <param name="methodInfo">A request handler with any number of custom parameters that often produces a response with its return value.</param>
-        /// <param name="serviceProvider">The <see cref="IServiceProvider"/> instance used to detect which parameters are services.</param>
         /// <param name="targetFactory">Creates the <see langword="this"/> for the non-static method.</param>
+        /// <param name="options">The <see cref="RequestDelegateFactoryOptions"/> used to configure the behavior of the handler.</param>
         /// <returns>The <see cref="RequestDelegate"/>.</returns>
-        public static RequestDelegate Create(MethodInfo methodInfo, IServiceProvider? serviceProvider, Func<HttpContext, object> targetFactory)
+#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
+        public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, RequestDelegateFactoryOptions? options = null)
+#pragma warning restore RS0026 // Do not add multiple public overloads with optional parameters
         {
             if (methodInfo is null)
             {
                 throw new ArgumentNullException(nameof(methodInfo));
             }
 
-            if (targetFactory is null)
+            if (methodInfo.DeclaringType is null)
             {
-                throw new ArgumentNullException(nameof(targetFactory));
+                throw new ArgumentException($"{nameof(methodInfo)} does not have a declaring type.");
             }
 
-            if (methodInfo.DeclaringType is null)
+            if (targetFactory is null)
             {
-                throw new ArgumentException($"A {nameof(targetFactory)} was provided, but {nameof(methodInfo)} does not have a Declaring type.");
+                if (methodInfo.IsStatic)
+                {
+                    var untargetableRequestDelegate = CreateTargetableRequestDelegate(methodInfo, options, targetExpression: null);
+
+                    return httpContext =>
+                    {
+                        return untargetableRequestDelegate(null, httpContext);
+                    };
+                }
+
+                targetFactory = context => Activator.CreateInstance(methodInfo.DeclaringType)!;
             }
 
             var targetExpression = Expression.Convert(TargetExpr, methodInfo.DeclaringType);
-            var targetableRequestDelegate = CreateTargetableRequestDelegate(methodInfo, serviceProvider, targetExpression);
+            var targetableRequestDelegate = CreateTargetableRequestDelegate(methodInfo, options, targetExpression);
 
             return httpContext =>
             {
                 return targetableRequestDelegate(targetFactory(httpContext), httpContext);
             };
         }
 
-        private static Func<object?, HttpContext, Task> CreateTargetableRequestDelegate(MethodInfo methodInfo, IServiceProvider? serviceProvider, Expression? targetExpression)
+        private static Func<object?, HttpContext, Task> CreateTargetableRequestDelegate(MethodInfo methodInfo, RequestDelegateFactoryOptions? options, Expression? targetExpression)
         {
             // Non void return type
 
@@ -157,9 +150,14 @@ public static RequestDelegate Create(MethodInfo methodInfo, IServiceProvider? se
 
             var factoryContext = new FactoryContext()
             {
-                ServiceProviderIsService = serviceProvider?.GetService<IServiceProviderIsService>()
+                ServiceProviderIsService = options?.ServiceProvider?.GetService<IServiceProviderIsService>()
             };
 
+            if (options?.RouteParameterNames is { } routeParameterNames)
+            {
+                factoryContext.RouteParameters = new(routeParameterNames);
+            }
+
             var arguments = CreateArguments(methodInfo.GetParameters(), factoryContext);
 
             var responseWritingMethodCall = factoryContext.TryParseParams.Count > 0 ?
@@ -202,6 +200,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
 
             if (parameterCustomAttributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute)
             {
+                if (factoryContext.RouteParameters is { } routeParams && !routeParams.Contains(parameter.Name))
+                {
+                    throw new InvalidOperationException($"{parameter.Name} is not a route paramter.");
+                }
+
                 return BindParameterFromProperty(parameter, RouteValuesExpr, routeAttribute.Name ?? parameter.Name, factoryContext);
             }
             else if (parameterCustomAttributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
@@ -242,6 +245,13 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
             }
             else if (parameter.ParameterType == typeof(string) || TryParseMethodCache.HasTryParseMethod(parameter))
             {
+                // We're in the fallback case and we have a parameter and route parameter match so don't fallback
+                // to query string in this case
+                if (factoryContext.RouteParameters is { } routeParams && routeParams.Contains(parameter.Name))
+                {
+                    return BindParameterFromProperty(parameter, RouteValuesExpr, parameter.Name, factoryContext);
+                }
+
                 return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext);
             }
             else
@@ -814,6 +824,7 @@ private class FactoryContext
             public Type? JsonRequestBodyType { get; set; }
             public bool AllowEmptyRequestBody { get; set; }
             public IServiceProviderIsService? ServiceProviderIsService { get; init; }
+            public List<string>? RouteParameters { get; set; }
 
             public bool UsingTempSourceString { get; set; }
             public List<(ParameterExpression, Expression)> TryParseParams { get; } = new();

src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs

@@ -0,0 +1,21 @@
+// 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.
+
+namespace Microsoft.AspNetCore.Http
+{
+    /// <summary>
+    /// Options for controlling the behavior 
+    /// </summary>
+    public class RequestDelegateFactoryOptions
+    {
+        /// <summary>
+        /// The <see cref="IServiceProvider"/> instance used to detect if handler parameters are services.
+        /// </summary>
+        public IServiceProvider? ServiceProvider { get; init; }
+
+        /// <summary>
+        /// The list of route parameter names that are specified for this handler.
+        /// </summary>
+        public IEnumerable<string>? RouteParameterNames { get; init; }
+    }
+}