[Route Groups] Support AddFilter, WithOpenApi and other additive conventions

[halter73]

The nice thing about this fix is that more local "conventions" can see and modify metadata added by outer groups. At first, I wasn't sure this was going to be possible for groups, but I found the solution by adding the new virtual EndpointDataSource.GetGroupedEndpoints(RouteGroupContext) method, and then creating a custom RouteEndointDataSource that takes over a lot of the complicated logic that used to exist in EndpointRouteBuilderExtensions.

The upside of moving this logic into RouteEndointDataSource other than general cleanliness, is that as a custom EndpointDataSource, it can override EndpointDataSource.GetGroupedEndpoints(GroupContext) and inspect the full group prefix and all the group metadata before calling RequestDelegateFactory.Create() or running any filters.

Even though RequestDelegateFactory now runs after any conventions are added to the endpoint (aside from conventions added by the RequestDelegateFactory), WithOpenApi is fixed by having the RouteEndointDataSource add MethodInfo before running any conventions. RouteEndointDataSource also adds any attributes as metadata. This is more similar to the previous behavior of RequestDelegateFactory in .NET 6 where it did not add this metadata.

The new-to-.NET-7 RequestDelegateFactoryOptions.InitialEndpointMetadata is now just RequestDelegateFactoryOptions.EndpointMetadata because now it's mutable and is equivalent to the RouteEndpointBuilder.Metadata at this very late stage of building. This gives the absolute most flexibility to filters and metadata providers because they can add metadata wherever they want, but they have to be careful not to override metadata they don't want to by simply adding to the end without looking if anything has already been configured. I think this is fine because the filter and metadata provider APIs are new .NET 7 so there aren't expectations that metadata added will have a low precedence yet. @captainsafia @DamianEdwards

You can see how this works by looking at some of the tests. For example, WithOpenApi_GroupMetadataCanBeSeenByAndOverriddenByMoreLocalMetadata:

string GetString() => "Foo";

static void WithLocalSummary(RouteHandlerBuilder builder)
{
    builder.WithOpenApi(operation =>
    {
        operation.Summary += $" | Local Summary | 200 Status Response Content-Type: {operation.Responses["200"].Content.Keys.Single()}";
        return operation;
    });
}

WithLocalSummary(builder.MapDelete("/root", GetString));

var outerGroup = builder.MapGroup("/outer");
var innerGroup = outerGroup.MapGroup("/inner");

WithLocalSummary(outerGroup.MapDelete("/outer-a", GetString));

// The order WithOpenApi() is relative to the MapDelete() methods does not matter.
outerGroup.WithOpenApi(operation =>
{
    operation.Summary = "Outer Group Summary";
    return operation;
});

WithLocalSummary(outerGroup.MapDelete("/outer-b", GetString));
WithLocalSummary(innerGroup.MapDelete("/inner-a", GetString));

innerGroup.WithOpenApi(operation =>
{
    operation.Summary += " | Inner Group Summary";
    return operation;
});

WithLocalSummary(innerGroup.MapDelete("/inner-b", GetString));

var summaries = builder.DataSources.SelectMany(ds => ds.Endpoints).Select(e => e.Metadata.GetMetadata<OpenApiOperation>().Summary).ToArray();

Assert.Equal(5, summaries.Length);
Assert.Contains(" | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);
Assert.Contains("Outer Group Summary | Inner Group Summary | Local Summary | 200 Status Response Content-Type: text/plain", summaries);

And AddRouteHandlerFilterMethods_WorkWithMapGroup.

string PrintId(int id) => $"ID: {id}";

static void AddParamIncrementingFilter(IEndpointConventionBuilder builder)
{
    builder.AddRouteHandlerFilter(async (context, next) =>
    {
        context.Arguments[0] = ((int)context.Arguments[0]!) + 1;
        return await next(context);
    });
}

AddParamIncrementingFilter(builder.Map("/{id}", PrintId));

var outerGroup = builder.MapGroup("/outer");
AddParamIncrementingFilter(outerGroup);
AddParamIncrementingFilter(outerGroup.Map("/{id}", PrintId));

var innerGroup = outerGroup.MapGroup("/inner");
AddParamIncrementingFilter(innerGroup);
AddParamIncrementingFilter(innerGroup.Map("/{id}", PrintId));

var endpoints = builder.DataSources
    .SelectMany(ds => ds.Endpoints)
    .ToDictionary(e => ((RouteEndpoint)e).RoutePattern.RawText!);

Assert.Equal(3, endpoints.Count);

// For each layer of grouping, another filter is applies which increments the expectedId by 1 each time.
await AssertIdAsync(endpoints["/{id}"], expectedPattern: "/{id}", expectedId: 3);
await AssertIdAsync(endpoints["/outer/{id}"], expectedPattern: "/outer/{id}", expectedId: 4);
await AssertIdAsync(endpoints["/outer/inner/{id}"], expectedPattern: "/outer/inner/{id}", expectedId: 5);

Fixes #41427
Fixes #42137
Fixes #41722

[davidfowl]

So thinking about this more I think I'd like to see one change to clean up how we're handling filters currently. Instead of treating it like metadata that we have to hunt from the collection, create a RouteHandlerEndpointBuilder that has the list of filters instead (we would unseal RouteEndpointBuilder).

public sealed class RouteHandlerEndpointBuilder : RouteEndpointBuilder
{
         public IReadOnlyList<Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate>>? RouteHandlerFilterFactories { get; init; }
  
     public RouteHandlerEndpointBuilder(
       RequestDelegate requestDelegate,
       RoutePattern routePattern,
       int order) : base(requestDelegate, routePattern, order) { }
}

I like this for a couple reasons:

• It's a pattern for identifying these endpoints specifically so we can choose to skip applying metadata to endpoints based on this type.
• No aggregation of RouteHandlerFilterMetadata entries is required.
• We don't need to expose the RouteHandlerFilterMetadata.
• Conventions can mutate the list.

[halter73]

That's a really good idea. We'd have to unseal RouteEndpointBuilder, but no big deal. I think we can do one better though.

We could consider adding RouteHandlerFilterFactories directly to RouteEndpointBuilder.

RequestDelegate is just a special case of our Delegate handling from a functional perspective. We should still keep things as they are today if there are no filters to avoid any possible startup penalty and trimming issues. But if someone does apply a filter to a RequestDelegate call to MapPost(...), etc..., they should get to have their filter run on it if they want to.

I know this also means if someone calls something MapControllers(), MapHub<Hub>() in a filtered group, the filters will run on those endpoints too, but I think most developers will actually really enjoy seeing the all registered routes and stuff in their filters if they filter an entire group. It's easy to separate Controllers and Hubs from other routes in the group so they don't get filtered if you want. It's also easy enough for a filter to skip over route handlers Delegates that are really just RequestDelegates if you want to incur no per-request performance penalty.

This would involve calling RDF.Create() in RouteEndpointBuilder.Build() if and only if there are filters set. I know it might sound like going too far, but I really think it's super powerful and worth doing if we make it clear this is how filters work, and make it very easy to choose to no-op in your filter for plain RequestDelegate's if you want to and incur no per request performance penalty. People writing their own debugging and diagnostic tools are going to love it. I might write a few now to show it off.

[halter73]

I've done this. See this commit for most of the implementation. Absolutely zero cost per-request unless a filter is added directly to the RouteEndpointBuilder and the filter actually modifies the per-invocation RouteHandlerFilterDelegate. If the filter does modify RouteHandlerFilterDelegate for the RequestDelegate endpoint, the modified RouteHandlerFilterDelegate does run of course, but that's what you asked for. A RequestDelegate is just one kind a Delegate after all. And aside from applying filters and handling Task<T>-returning RequestDelegates (for more than just MapGet after that commit I might add), RDF does not change the behavior of the RequestDelegate compared to just running it directly.

This is all new API in .NET 7. No one is forcing anyone to AddRouteHandlerFilters to everyday plain-RequestDelegate backed RouteEndpointBuilders, but if you do, it's' very nice to have them run. What's really nice is this allows filters to run on practically any endpoint in existence. MapHub, MapHealthChecks, MapControllers, MapRazorPages, etc... But again, only if you ask for it. This is the first time people can even try doing this now that AddRouteHandlerFilter can be applied to any IEndpointConventionBuilder.

[davidfowl]

I think this should be decoupled from this PR and the implementation shouldn't use RDF.

[captainsafia]

Throwing in my two cents here now that I've reviewed the change. While I can recognize that there might be compelling scenarios for it, I'm a little cautious about:

• Changing our direction on whether or not route handler-filters could be applied to non-route handlers. We were committed enough to that stance to rename the API from AddFilter to AddRouteHandlerFilter. Side note: if we do want to pursue this direction then we should consider changing it back to AddFilter.
• Invoking the RDF in all route endpoint builders. This change accounts invoking RDF from non-route handler scenarios when there are filters involved but have we considered other changes in the RDF that need to be made so that it can be invoked in more places.

In general, this seems high-risk/medium-reward and would be more sensible to approach as an independent change.

[davidfowl]

I'm not sure I like this coupling. RDF exists to be decoupled from routing.

[davidfowl]

While I think this is cool and I now understand it more, can we do this in a later change after we discuss the implications? If we enable this it might make sense to rename RouteHandlerFilterInvocationContext as it makes things more general purpose. I also am not sure about the coupling to the request delegate factory. That seems unnecessary. I would just rewrite the logic here since there's no need for codegen.

[captainsafia]

Suggested change

	internal sealed class RouteEndpointDataSource : EndpointDataSource
	internal sealed class RouteHandlerEndpointDataSource : EndpointDataSource

Nit for consistency.

[captainsafia]

Are you trying to validate that the return type wasn't modified by the logic above here? I'm not sure what value this check is adding.

[davidfowl]

This syntax.... @jaredpar have you seen this pattern much?

[jaredpar]

Seen it a few times but it's pretty rare. The == and != for primitive values is still more common.