Skip to content

[API] RequestDelegateFactory.Create RequestDelegate overload #46024

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

Closed
JamesNK opened this issue Jan 11, 2023 · 5 comments
Closed

[API] RequestDelegateFactory.Create RequestDelegate overload #46024

JamesNK opened this issue Jan 11, 2023 · 5 comments
Assignees
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jan 11, 2023

Background and Motivation

Today RequestDelegateFactory has Create methods that take Delegate. However, there are situations where we also want to wrap a RequestDelegate in a new request delegate. For example, the endpoint has request filters.

RequestDelegateFactory.Create(Delegate, ...) is currently used for both types of delegates. However, it is beneficial to have separate overloads for RequestDelegateFactory.Create(Delegate, ...) and RequestDelegateFactory.Create(RequestDelegate, ...).

The RequestDelegate overload doesn't need to use System.Linq.Expressions to build a request filter pipeline. Apps that only map request delegate endpoints can be statically analyzed not to use expressions and the expressions assembly can be trimmed on publish.

Proposed API

namespace Microsoft.AspNetCore.Http;

public static class RequestDelegateFactory
{
+    public static RequestDelegateResult Create(RequestDelegate handler, RequestDelegateFactoryOptions? options);
}

We don't need InferMetadata method or RequestDelegateMetadataResult overloads because request delegates don't have parameters or return values to gather metadata from.

Usage Examples

if (options?.RouteBuilder.Filters.Count > 0)
{
    // Endpoint is configured with filters. Wrap the request delegate in a new request delegate that calls the filters.
    requestDelegate = RequestDelegateFactory.Create(requestDelegate, options).RequestDelegate;
}

Alternative Designs

The alternative is to build the filter pipeline for a RequestDelegate outside of the delegate factory in the Microsoft.AspNetCore.Routing project. However, because it is a separate project from RequestDelegateFactory, there is some duplication between routing and HTTP extensions projects.

See #46020 for an example of messiness.

Risks

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@JamesNK JamesNK removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 11, 2023
@davidfowl
Copy link
Member

This seems to niche for a public API, whats the scenario for this being a public API?

@JamesNK
Copy link
Member Author

JamesNK commented Jan 11, 2023

The caller is in another project (Microsoft.AspNetCore.Routing) so internal won’t work.

@halter73
Copy link
Member

halter73 commented Jan 11, 2023

It's just the filters that need to be applied, right? It's tempting to just apply the filters manually without RDF.

Edit: I've looked at the PR now. I see the appeal. We also want to start handling serializing the response of Func<HttpContext, Task<T>> too, and this would hopefully help with that.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 24, 2023

No longer required.

@JamesNK JamesNK closed this as completed Jan 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2023
@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

No branches or pull requests

4 participants