Global endpoint filter (edit: and other conventions?)

[WeihanLi]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Since we introduced the endpoint filter feature, is there also a global endpoint filter available?

So that we could avoid registering the endpoint filter for each endpoint.

Describe the solution you'd like

Maybe likes MVC global filters, and maybe group endpoint filter for a Group (MapGroup) likes the filter for controller

Additional context

No response

[davidfowl]

The way to do this is with groups. Make a global group and stick all of the endpoints in it. In the future, we may consider a default convention builder on the WebApplication object.

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

var routes = app.MapGroup("");

routes.AddEndpointFilter((context, next) =>
{
    return next(context);
});

routes.MapGet("/", () => "Hello World!");

app.Run();

[WeihanLi]

Thanks @davidfowl

[halter73]

I love using an empty group prefix for this! Support for gobal metadata and filters was the main reason I figured it was worth allowing empty group prefixes. I think we can do better though. What do we think about having WebApplication implement IEndpointConventionBuilder.

Background and Motivation

See above.

Proposed API

namespace Microsoft.AspNetCore.Builder;

namespace Microsoft.AspNetCore.Http;

- public sealed class WebApplication : IHost, IApplicationBuilder, IEndpointRouteBuilder, IAsyncDisposable
+ public sealed class WebApplication : IHost, IApplicationBuilder, IEndpointRouteBuilder, IEndpointConventionBuilder, IAsyncDisposable

Usage Examples

var app = WebApplication.Create(args);

app.AddEndpointFilter((context, next) =>
{
    return next(context);
});

app.MapGet("/", () => "Hello World!");

app.Run();

Alternative Designs

Leave it as is. You can always create an route group with an empty prefix if you want this functionality as @davidfowl suggests:

var app = WebApplication.Create(args);
var routes = app.MapGroup("");

routes.AddEndpointFilter((context, next) =>
{
    return next(context);
});

routes.MapGet("/", () => "Hello World!");

app.Run();

Risks

It would be too noisy to have too many extension methods that all work on WebApplication.

[ghost]

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.

[WeihanLi]

The MatchGroup with empty had to update the existing code, if there are a lot of endpoints it may need a big effort to update.
Personally, I prefer the proposal

[davidfowl]

I don't like this proposal I think top level intellisense will be too noisy. Routing methods are usually Map*, middleware methods are Use*, these conventions could don't have a consistent naming scheme. We use With*, Add*, Require*. I'm worried about jumbled intellisense and naming conflicts.

[davidfowl]

Here's an alternative proposal:

namespace Microsoft.AspNetCore.Http;

public sealed class WebApplication 
{
+    public RouteGroupBuilder Routes { get; }
}

We expose a default top level RouteGroup that you can hang your routes on. Internally, we'd create a default route group and would use it for both top level routes hanging off the WebApplication itself and for the Routes property.

app.Routes.MapGet("/", () => "Hello");

is the same as:

app.MapGet("/", () => "Hello");

Adding global conventions is then:

app.Routes.AddEndpointFilter((context, next) =>
{
    return next(context);
});

It also solves some of the complaints we've had in this area:

• https://andrewlock.net/exploring-dotnet-6-part-4-building-a-middleware-pipeline-with-webapplication/ (scroll to the last section)
• https://khalidabuhakmeh.com/adding-clarity-to-dotnet-minimal-hosting#proposed-minimal-enhancement

[halter73]

Another alternative is adding a Conventions property that's just an IEndpointConventionBuilder. Under the covers, we would use groups to implement this, but we don't need to expose that implementation detail if we don't want to.

namespace Microsoft.AspNetCore.Http;

public sealed class WebApplication 
{
+    public IEndpointConventionBuilder Conventions { get; }
}

Usage:

app.MapGet("/", () => "Hello");

app.Conventions.AddEndpointFilter((context, next) =>
{
    return next(context);
});

We've said that we don't want APIs targeting RouteGroupBuilder directly. It's a sealed type. Any single API should target either IEndpointRouteBuilder or IEndpointConventionBuilder, but not both. If later on, we feel this is a mistake, we could create a new interface that implements both. We could then have RouteGroupBuilder implement that and tell people to target the new combined interface, but I don't think we're going to need that.

[halter73]

Or we could do a hybrid approach, so we still allow you to delineate between the IApplicationBuilder and the IEndpointConventionBuilder if you want to.

namespace Microsoft.AspNetCore.Http;

public sealed class WebApplication 
{
+    public IEndpointRouteBuilder Routes { get; }
+    public IEndpointConventionBuilder Conventions { get; }
}

Usage:

app.Routes.MapGet("/", () => "Hello");

app.Conventions.AddEndpointFilter((context, next) =>
{
    return next(context);
});

[davidfowl]

I considered this as well but I figured 2 birds with one stone (see the blog posts). Is there any reason why we can't expose a sealed type? Are you predicting extension methods targeting this?

[halter73]

I considered this as well but I figured 2 birds with one stone (see the blog posts).

I just read them before I suggested the hybrid proposal that addresses the concern of both blogposts. I think it's better because it's not adding yet a new property that arguably implements too many interfaces on a single type.

Are you predicting extension methods targeting this?

I hope not. If we were willing to make a major change to the route group APIs in the 11th hour of .NET 7 we could change RouteGroupBuilder itself to not implement two very distinct interfaces. Arguably, this would avoid repeating the mistake we made with WebApplication:

namespace Microsoft.AspNetCore.Routing;

- public sealed class RouteGroupBuilder : IEndpointRouteBuilder, IEndpointConventionBuilder
+ public sealed class RouteGroupBuilder
{
+     public IEndpointRouteBuilder Routes { get; }
+     public IEndpointConventionBuilder Conventions { get; }
}

With this API change, the current workaround for global convetions goes from being like:

var app = WebApplication.Create(args);
var routes = app.MapGroup("");

routes.AddEndpointFilter((context, next) =>
{
    return next(context);
});

routes.MapGet("/", () => "Hello World!");

app.Run();

To something like this:

var app = WebApplication.Create(args);
var group = app.MapGroup("");

group.Conventions.AddEndpointFilter((context, next) =>
{
    return next(context);
});

group.Routes.MapGet("/", () => "Hello World!");

app.Run();

@andrewlock and @khalidabuhakmeh do either of you have any thoughts on this?

[halter73]

API Review Notes:

• Having to type the extra .Routes or .Conventions characters is annoying.
• Can we use destructors?
  • This enables you to delineate between the IApplicationBuilder and the IEndpointConventionBuilder if you want to.
• Can we use it for RouteGroupBuilder?
• Can we add an empty overload to MapGroup without the prefix parameter? If this is the preferred way to add conventions to a group of endpoints, it might make sense.

We feel we're going to need customer feedback to see if we really want to do any of the above proposals.

We do not think we want RouteGroupBuilder to stop implementing both IEndpointRouteBuilder and IEndpointConventionBuilder. That's one of the few decisions we won't be able to take back after .NET 7.

Most of the other suggestions are additive and could be done in a later release.

[andrewlock]

do either of you have any thoughts on this?

I mean, I have definite thoughts if we could turn back time and have WebApplication not directly implement IEndpointRouteBuilder (as I mentioned in the blog post), but I assume that's not really on the cards 😄

For the hybrid approach:
I think having WebApplication directly implement IEndpointRouteBuilder and having a Routes property may be worse than one or the other, as it adds confusion/ambiguity (i.e. the difference between app.MapGet() and app.Routes.MapGet() is not obvious)

RE splitting RouteBuilder:
I'm not sure if it adds a lot splitting IEndpointRouteBuilder and IEndpointConventionBuilder other than make things a bit more verbose. I quite like that I didn't even appreciate that there were two interfaces involved here previously, as everything just flows quite nicely.

For me the top-level Conventions property makes most sense probably, though I'm not a fan of the name (I don't think of filters etc as being "conventions", and nor do any of the docs, even if that's now they're implemented).

Personally, I think the original workaround, of creating a "top-level route" is the best option given the existing constraints

[khalidabuhakmeh]

I agree with @andrewlock in the sense that Conventions doesn't make a lot of sense here. I would consider it as part of the request pipeline, so possibly Pipeline or Filters (to stay in line with Razor Pages and MVC) would make the most sense.

The thing I like about ASP.NET Core is the shared model, and that includes the shared mental model across all the approaches. Trying to be too different might drive people to "pick sides", rather than realizing the strength is in flowing between Razor Pages, Minimal API endpoints, MVC, and SignalR.