Skip to content

Added new overloads to RequestDelegateFactory #34375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/Http/Http.Extensions/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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!
Expand Down
77 changes: 44 additions & 33 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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

Expand All @@ -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 ?
Expand Down Expand Up @@ -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.");
Copy link
Member Author

@davidfowl davidfowl Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is worth a look. I added some tests for this scenario as well. TL;DR If you specify [FromRoute] and but didn't specify a route parameter in the template, this will throw. In theory, you could add route values dynamically via middleware so maybe it's too aggressive but I think that's pretty rare.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not identical, but Blazor does this today and it helps alleviate the class of issues. We can always change it to be less restrictive if we users run in to in the future (particularly given that we have an option type for configure this).

}

return BindParameterFromProperty(parameter, RouteValuesExpr, routeAttribute.Name ?? parameter.Name, factoryContext);
}
else if (parameterCustomAttributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this even attempt to bind from RouteValues anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only scenario is if the list of routes is null. But maybe if it's not null and it's not a route, the we fallback directly to query? I can add that in rc1

}
else
Expand Down Expand Up @@ -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();
Expand Down
21 changes: 21 additions & 0 deletions src/Http/Http.Extensions/src/RequestDelegateFactoryOptions.cs
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Options for controlling the behavior
/// Options for controlling the behavior of <see cref="RequestDelegate" /> when created using <see cref="RequestDelegateFactory" />.

/// </summary>
public class RequestDelegateFactoryOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class RequestDelegateFactoryOptions
public sealed 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; }
}
}
Loading