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

RequestDelegateFactory should take an optional set of route pattern names or route parameter names #33700

Closed
davidfowl opened this issue Jun 21, 2021 · 4 comments · Fixed by #34375
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 feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Jun 21, 2021

Background and Motivation

This would allow unattributed methods to disambiguate between route and query string when the method parameters are unattributed. Today we generate code that prefers route over query string and we could avoid that in certain cases. It would also allow us to better fail if the user used [FromRoute] and there was no route parameter with that name defined.

Proposed API

Since our overloads are getting a bit crazy with this change, I suggest we add an options object to handle the overflow.

public static class RequestDelegateFactory
{
-    public static RequestDelegate Create(Delegate action, IServiceProvider? serviceProvider)
+    public static RequestDelegate Create(Delegate action, Func<HttpContext, object>? targetFactory = null, RequestDelegateFactoryOptions? options = null)
-    public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, IServiceProvider? serviceProvider)
+    public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, RequestDelegateFactoryOptions? options = null)
}

+ public class RequestDelegateFactoryOptions
+ {
+     public IServiceProvider? ServiceProvider { get; init; }
+     public IReadOnlyList<string>? RouteParameterNames { get; init; }
+ }

Usage Examples

var options = new RequestDelegateFactoryOptions 
{ 
    ServiceProvider  = serviceProvider, 
    RouteParameterNames  = new[] { "id" }
};

var rd = RequestDelegateFactory.Create((int id) => id, options: options);

Alternative Designs

A single static method with optional parameters.

public static class RequestDelegateFactory
{
+    public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, IServiceProvider? serviceProvider = null, IReadOnlyList<string>? routeParameterNames = null);
}

Risks

None

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 21, 2021
@davidfowl
Copy link
Member Author

MVC skips binding sources that aren't from the request and that solves this problem, see

if (!context.Results[i].Source.IsFromRequest)

public static readonly BindingSource Services = new BindingSource(

cc @pranavkm

@BrennanConroy BrennanConroy added the feature-minimal-actions Controller-like actions for endpoint routing label Jun 21, 2021
@BrennanConroy BrennanConroy added this to the 6.0-preview7 milestone Jun 21, 2021
@davidfowl davidfowl changed the title RequestDelegateFactory should take an optional RoutePattern or route parameter names RequestDelegateFactory should take an optional set of route pattern names or route parameter names Jun 22, 2021
@halter73
Copy link
Member

I almost used IsFromRequest instead of specifically checking for BindingSource.Services in #33727, but I didn't want to depend too much on BindingSource logic/conventions. The only other default non-IsFromRequest is BindingSource.Special.

I guess we could use BindingSource.Special to represent CancellationToken, HttpContext and things like that. Now those are just "services" as far as the EndpointMetadataApiDescriptionProvider is concerned but I don't think this matters much since it's all internal anyway.

@davidfowl
Copy link
Member Author

I put this text on the wrong issue..

@davidfowl davidfowl added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 7, 2021
@halter73 halter73 added this to To do in Minimal APIs 6.0 Jul 8, 2021
@pranavkm
Copy link
Contributor

+ public class RequestDelegateFactoryOptions
+ {
+     public IServiceProvider? ServiceProvider { get; init; }
-     public IReadOnlyList<string>? RouteParameterNames { get; init; }
+    public IEnumerable<string>? RouteParameterNames { get; init; }
+ }

@pranavkm pranavkm 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 Jul 12, 2021
@halter73 halter73 removed their assignment Jul 12, 2021
Minimal APIs 6.0 automation moved this from To do to Done Jul 15, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 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 feature-minimal-actions Controller-like actions for endpoint routing
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants