Skip to content
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

Add API to turn delegate or MethodInfo into a RequestDelegate #31181

Closed
davidfowl opened this issue Mar 24, 2021 · 9 comments
Closed

Add API to turn delegate or MethodInfo into a RequestDelegate #31181

davidfowl opened this issue Mar 24, 2021 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Mar 24, 2021

Background and Motivation

MVC Controllers have this magic power baked inside of them to allow dispatching HTTP requests to an arbitrarily shaped method on a controller (called actions). As part of the work to make this more accessible, we want to expose a zero dependency building block from lower in the stack that enables the same thing with arbitrary methods and delegates. This lets library authors build convenient and complex request handling logic without having to take big dependencies on controllers or mvc.

Proposed API

namespace Microsoft.AspNetCore.Http
{
+    public sealed class RequestDelegateFactory
+    {
+        public static RequestDelegate Create(Delegate requestHandler);
+        public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, object> targetFactory);
+        public static RequestDelegate Create(MethodInfo methodInfo);
+    }
}

Usage Examples

There are 2 scenarios

  1. Wrapping a fixed Delegate
  2. MethodInfo with or without "this"

Enhanced routing (MapAction) with is number 1, MVC controllers are examples of number 2.

  1. Dispatch with delegate
Func<string> hello = () => "Hello World";
RequestDelegate rd1 = RequestDelegateFa.Build(hello);
app.Run(rd1);

RequestDelegate rd2 = RequestDelegateFactory.Create(Func<string>)MyClass.Hello);
app.Run(rd2);

public class MyClass
{
    public static string Hello() => "Hello World";
}

2a. Class dispatch with instance

MethodInfo methodInfo = typeof(MyClass).GetMethod(nameof(MyClass.Hello));

// This will create an instance of MyClass per request, giving the factory a chance to construct
// MyClass using per request scoped objects
RequestDelegate rd1 = RequestDelegateFactory.Create(methodInfo, context => new MyClass());

// Example with DI
RequestDelegate rd2 = RequestDelegateFactory.Create(methodInfo, context => context.RequestServices.GetRequiredService<MyClass>());

app.Run(rd1);
app.Run(rd2);

public class MyClass
{
    public string Hello() => "Hello World";
}

2a. Class dispatch with static

MethodInfo methodInfo = typeof(MyClass).GetMethod(nameof(MyClass.Hello));

RequestDelegate rd = RequestDelegateFactory.Create(methodInfo);

app.Run(rd);

public class MyClass
{
    public static string Hello() => "Hello World";
}

Alternatives

Don't expose this building block and expose it on more places. Middleware, Routing, IApplicationBuilder etc.

PS: We might also need to consider options that control behavior in the future.

cc @pranavkm

@davidfowl davidfowl added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews feature-minimal-actions Controller-like actions for endpoint routing labels Mar 24, 2021
@davidfowl davidfowl added this to the 6.0-preview4 milestone Mar 24, 2021
@davidfowl davidfowl added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 24, 2021
@halter73
Copy link
Member

Don't expose this building block and expose it on more places. Middleware, Routing, IApplicationBuilder etc.

We should expose this building block. I see no reason to be overly prescriptive and make it difficult for developers to get at this RequestDelegate. We shouldn't assume we can think of all the ways developers might possibly want to use it. If we don't expose this, people will do hacks like build a whole middleware pipeline just to get at a RequestDelegate.

We don't technically need all of these overloads. Build(MethodInfo methodInfo, Func<HttpContext, object> targetFactory) can be implemented by calling Build(Delegate action) or vice-versa. Efficiently using the Build(Delegate action) overload if you have a nontrivial targetFactory would likely require expression tree compilation or something similarly complicated, so I'm completely fine keeping the other overloads for convenience.

If we do decide to reduce the overloads, Build(Delegate action) is the one I most want to keep though. I think it will see the most usage outside of framework code.

@davidfowl
Copy link
Member Author

We don't technically need all of these overloads. Build(MethodInfo methodInfo, Func<HttpContext, object> targetFactory) can be implemented by calling Build(Delegate action) or vice-versa. Efficiently using the Build(Delegate action) overload if you have a nontrivial targetFactory would likely require expression tree compilation or something similarly complicated, so I'm completely fine keeping the other overloads for convenience.

I think it's too difficult. I tried it but happy to be proven wrong here.

@halter73
Copy link
Member

I think it's too difficult. I tried it but happy to be proven wrong here.

I haven't tried it yet, but I believe you. Adding the attributes back to the generated Delegate's MethodInfo might be a blocker.

I do like all 3 overloads. I'm adding the MethodInfo overloads to #31171 now.

@mumby0168
Copy link

Will there be a way to have the framework pick these up via reflection? A bit like services.AddControllers() detects all controllers in the assembly. Maybe marking the "special" methods with an attribute?

Thanks,
Billy

@davidfowl
Copy link
Member Author

@mumby0168 explicitly no. No scanning will be done by default to discover these, we already have that framework (mvc). This building blocking can be used to build that sort of thing if you want but we won’t be building that again.

@halter73 halter73 added this to In Progress in ASP.NET Core Blazor & MVC 6.0 Mar 25, 2021
@davidfowl davidfowl added area-servers and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Mar 25, 2021
@JamesNK
Copy link
Member

JamesNK commented Mar 26, 2021

Is there a way to get access to the HttpContext with an abritary method?

@halter73
Copy link
Member

Yep. Just add an HttpContext parameter:

else if (parameter.ParameterType == typeof(HttpContext))
{
paramterExpression = HttpContextParameter;
}

@davidfowl
Copy link
Member Author

Part of me is wondering if this is more of a factory than a builder?

@halter73
Copy link
Member

That's a good point. I'll change it to RequestDelegateFactory in the PR.

@mkArtakMSFT mkArtakMSFT added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 29, 2021
@mkArtakMSFT mkArtakMSFT removed this from In Progress in ASP.NET Core Blazor & MVC 6.0 Mar 30, 2021
@ghost ghost added Done This issue has been fixed and removed Working labels Mar 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2021
@danroth27 danroth27 added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 14, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

No branches or pull requests

7 participants