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

Improve minimal api routing with grouping #36007

Closed
ziaulhasanhamim opened this issue Sep 1, 2021 · 44 comments · Fixed by #41265
Closed

Improve minimal api routing with grouping #36007

ziaulhasanhamim opened this issue Sep 1, 2021 · 44 comments · Fixed by #41265
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 area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing Priority:0 Work that we can't release without
Milestone

Comments

@ziaulhasanhamim
Copy link

ziaulhasanhamim commented Sep 1, 2021

Background and Motivation

The new minimal approach in asp.net core 6 is good. I mean it's great. But I don't this will be useful in a real web application. Because this doesn't have an easy way to integrate route grouping. With route grouping, minimal actions can be separated into multiple files easily with the help of normal or static(since c# doesn't support modules like f#) classes without using MVC or Razor pages.

Proposed API

namespace Microsoft.AspNetCore.Builder;

+ public class RouteGroup : IApplicationBuilder, IEndpointRouteBuilder
+ {
+     // Needed members to implement those interfaces
+ }

public class MinimalActionEndpointRouteBuilderExtensions
{
+     public static void IncludeRouteGroup(this IEndpointRouteBuilder endpoints, string pattern, RouteGroup routeGroup);
}

Usage Examples

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

RouteGroup usersRoutes = new();
usersRoutes.MapGet("{id}", (int id) => GetUserById(id));

app.IncludeRouteGroup("/api/users/", usersRoutes);

You can separate these route actions to multiple files easily. RouteGroup would become more useful if in the future c# gets support for top-level functions(not just for Program.cs).

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

Very express like, I like it, but we're pretty much done with new features for .NET 6, we'll consider it for .NET 7.

@davidfowl davidfowl added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Sep 1, 2021
@davidfowl
Copy link
Member

davidfowl commented Sep 1, 2021

I'm not sure we need to implement IApplicationBuilder. We'd need to think through this a little bit.

cc @khalidabuhakmeh since you implemented something similar.

@davidfowl davidfowl added the feature-minimal-actions Controller-like actions for endpoint routing label Sep 1, 2021
@ziaulhasanhamim
Copy link
Author

ziaulhasanhamim commented Sep 1, 2021

I'm not sure we need to implement IApplicationBuilder. We'd need to think through this a little bit.

Yes. IEndpointRouteBuilder will enable us to do almost all necessary things. So IApplicationBuilder is not really needed in RouteGroup. But IApplicationBuilder can be used a create middleware specific to that route group.

@davidfowl
Copy link
Member

@ziaulhasanhamim right, we have something similar today with IApplicationBuilder.Map, but you don't get an IEndpointRouteBuilder. I could envision something like this:

app.MapGroup("/api/users", group =>
{
    group.UseCors(); // Does this make sense?

    group.MapGet("/", (UsersDbContext db) => db.ToListAsync());
    group.MapGet("/{id}", (int id, UsersDbContext db) => db.Users.Find(id)); 
});

@ziaulhasanhamim
Copy link
Author

@davidfowl yes right you are. That doesn't make sense. So IEndpointRouteBuilder is enough for RouteGroup.

@ziaulhasanhamim
Copy link
Author

ziaulhasanhamim commented Sep 1, 2021

But one overload can be made with a delegate parameter.

public class MinimalActionEndpointRouteBuilderExtensions
{
     public static void IncludeRouteGroup(this IEndpointRouteBuilder endpoints, string pattern, RouteGroup routeGroup);
     public static void IncludeRouteGroup(this IEndpointRouteBuilder endpoints, string pattern, RouteGroup routeGroup, Func<HttpContext, Func<Task>, Task> onMatch);
}

The onMatch will be executed whenever URL for the route group will match.

@davidfowl
Copy link
Member

Do we really need onMatch? What's that for?

@ziaulhasanhamim
Copy link
Author

I won't say we really need that. But there might be a situation where we may want to execute a common piece of code for a group route. Maybe some logging.

@davidfowl
Copy link
Member

public class EndpointRouteBuilderExtensions
{
     public static void MapGroup(this IEndpointRouteBuilder endpoints, string pattern, Action<IApplicationBuilder> configureApp, Action<IEndpointRouteBuilder> configureRoutes);
     public static void MapGroup(this IEndpointRouteBuilder endpoints, string pattern, Action<IEndpointRouteBuilder> configureRoutes);
}
app.MapGroup("/api/users", static app => app.UseBuffering(), routes => routes.MapGet("/", (TodoDb db) => db.ToListAsync()));
app.MapGroup("/api/users", routes => routes.MapGet("/", (TodoDb db) => db.ToListAsync()));

@ziaulhasanhamim
Copy link
Author

MapGroup needs us to give a delegate parameter that will configure the routes. I'm not really convinced with that. I would like if MapGroup returns an IEndpointRouteBuilder instance and we will configure routes on that instance or it will take a parameter of type RouteGroup and we will configure route on that RouteGroup.

public class EndpointRouteBuilderExtensions
{
     public static IEndpointRouteBuilder MapGroup(this IEndpointRouteBuilder endpoints, string pattern);
}
var group = app.MapGroup("/api/users");
group.MapGet("/", (TodoDb db) => db.ToListAsync());

or

public class EndpointRouteBuilderExtensions
{
     public static IEndpointRouteBuilder MapGroup(this IEndpointRouteBuilder endpoints, string pattern, RouteGroup group);
}
RouteGroup group = new();
group.MapGet("/", (TodoDb db) => db.ToListAsync());
app.MapGroup("/api/users", group);

@davidfowl
Copy link
Member

I like the first one:

var group = app.MapGroup("/api/users");
group.MapGet("/", (TodoDb db) => db.ToListAsync());

Can you do things like apply conventions to entire groups? For example:

var group = app.MapGroup("/api/users");
group.RequireAuthentication();
group.MapGet("/", (TodoDb db) => db.ToListAsync());

@ziaulhasanhamim
Copy link
Author

Yes, I think we should be able to do that,

@khalidabuhakmeh
Copy link

@ziaulhasanhamim check out https://github.com/khalidabuhakmeh/branchy

It kind of works like what you're doing. The approach I took was to call MapXXX with each call rather than wait to register them at a later time (https://github.com/khalidabuhakmeh/Branchy/blob/main/src/Branchy/NestedEndpointConventionBuilder.cs).

I also haven't added any functionality to add metadata to groups. While it sounds good in theory, the only metadata I could think of that might be useful is authorization/authentication.

I also recommend checking out the .NET 6 branch of Carter.

@ziaulhasanhamim
Copy link
Author

@khalidabuhakmeh yeah your approach is great. But for me, the router option seems more robust. But I'm not saying your approach is not useful. Your approach is absolutely correct and useful.

@Nerajankc
Copy link

I would like to have that in .net 6. it will make minimal action actually useful to our company and our developers

@davidfowl
Copy link
Member

We're too late in the cycle to design anything that will be a part of .NET 6 now. I'd suggesting using @khalidabuhakmeh's package mentioned above to get a feel for what the API could look like.

@rafikiassumani-msft rafikiassumani-msft added this to the Backlog milestone Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@davidfowl
Copy link
Member

This should be a .NET 7 candidate.

@edsulaiman
Copy link

I would like to have that in .net 6. it will make minimal action actually useful to our company and our developers

I have created a simple code if you want to try it on .NET 6

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

var group = app.MapGroup("/group/{message1}");
group.MapGet("{message2}", (string message1, string message2) => $"{message1} {message2}");

app.Run();

public static class MapGroupExtensions
{
    public static MapGroupBuilder MapGroup(this IEndpointRouteBuilder app, string groupPattern)
    {
        return new MapGroupBuilder(app, groupPattern);
    }
}

public class MapGroupBuilder
{
    private IEndpointRouteBuilder _app;
    private string _groupPattern;

    public MapGroupBuilder(IEndpointRouteBuilder app, string groupPattern)
    {
        _app = app;
        _groupPattern = groupPattern;
    }

    public IEndpointConventionBuilder MapGet(string pattern, Delegate handler)
    {
        string requestPattern = CombinePattern(pattern);
        return _app.MapGet(requestPattern, handler);
    }

    public IEndpointConventionBuilder MapPost(string pattern, Delegate handler)
    {
        string requestPattern = CombinePattern(pattern);
        return _app.MapPost(requestPattern, handler);
    }

    public IEndpointConventionBuilder MapPut(string pattern, Delegate handler)
    {
        string requestPattern = CombinePattern(pattern);
        return _app.MapPut(requestPattern, handler);
    }

    public IEndpointConventionBuilder MapDelete(string pattern, Delegate handler)
    {
        string requestPattern = CombinePattern(pattern);
        return _app.MapDelete(requestPattern, handler);
    }

    private string CombinePattern(string pattern)
    {
        return _groupPattern.TrimEnd('/') + "/" + pattern.TrimStart('/');
    }
}

@davidfowl davidfowl modified the milestones: Backlog, .NET 7 Planning Dec 5, 2021
@ghost
Copy link

ghost commented Dec 5, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@rafikiassumani-msft rafikiassumani-msft added Priority:0 Work that we can't release without Cost:L labels Dec 28, 2021
@rafikiassumani-msft rafikiassumani-msft added the triage-focus Add this label to flag the issue for focus at triage label Jan 11, 2022
@halter73 halter73 added the api-approved API was approved in API review, it can be implemented label Apr 11, 2022
halter73 added a commit that referenced this issue Apr 15, 2022
@rafikiassumani-msft rafikiassumani-msft changed the title Improve minimal api routing with grouping [Epic] Improve minimal api routing with grouping Apr 21, 2022
@rafikiassumani-msft rafikiassumani-msft changed the title [Epic] Improve minimal api routing with grouping [Epic]: Improve minimal api routing with grouping Apr 21, 2022
@rafikiassumani-msft rafikiassumani-msft added the Epic Groups multiple user stories. Can be grouped under a theme. label Apr 21, 2022
@halter73
Copy link
Member

Reopening to track #41367

@halter73 halter73 reopened this Apr 25, 2022
@halter73 halter73 removed Needs: Spec Indicates that a spec defining user experience is required Needs: Design This issue requires design work before implementating. triage-focus Add this label to flag the issue for focus at triage labels Apr 25, 2022
@halter73 halter73 removed this from the 7.0-preview5 milestone Apr 25, 2022
@halter73 halter73 changed the title [Epic]: Improve minimal api routing with grouping Improve minimal api routing with grouping Apr 28, 2022
@halter73 halter73 mentioned this issue Apr 28, 2022
4 tasks
@halter73 halter73 removed the Epic Groups multiple user stories. Can be grouped under a theme. label Apr 28, 2022
@halter73 halter73 added this to the 7.0-preview4 milestone Apr 28, 2022
@halter73
Copy link
Member

The initial work for this was merged into preview4 with #41367, so I'm closing this issue. The remaining work for route groups in .NET 7 is being tracked by the new "Epic" issue at #41433.

@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label 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 area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing Priority:0 Work that we can't release without
Projects
None yet
Development

Successfully merging a pull request may close this issue.