Rename filter extension methods and type name to be more agnostic

[captainsafia]

Background and Motivation

With changes introduced in preview6, filters can be registered on both routes and route groups. This proposal recommends renaming the type methods in the route filters area to reflect this.

This proposal also recommends using AddEndpointFilterFactory to disambiguate between extension method overlands that take a filter factory delegate versus a filter delegate.

Proposed API

namespace Microsoft.AspNetCore.Http;

- public static class RouteHandlerFilterExtensions
+ public static class EndpointFilterExtensions
{
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, IRouteHandlerFilter filter);
-  public static TBuilder AddRouteHandlerFilter<TBuilder, TFilterType>(this TBuilder builder);
-  public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder);
-  public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder);
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter);
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory); 

+  public static TBuilder AddEndpointFilter<TBuilder>(this TBuilder builder, IEndpointFilter filter);
+  public static TBuilder AddEndpointFilter<TBuilder, TFilterType>(this TBuilder builder);
+  public static RouteHandlerBuilder AddEndpointFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder);
+  public static RouteGroupBuilder AddEndpointFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder);
+  public static TBuilder AddEndpointFilter<TBuilder>(this TBuilder builder, Func<EndpointExecutionContext, EndpointFilterDelegate, ValueTask<object?>> endpointFilter);
+  public static TBuilder AddEndpointFilterFactory<TBuilder>(this TBuilder builder, Func<EndpointContext, EndpointFilterDelegate, EndpointFilterDelegate> filterFactory); 
}

namespace Microsoft.AspNetCore.Http;

- public interface IRouteHandlerFilter {}
+ public interface IEndpointFilter {}

namespace Microsoft.AspNetCore.Http;

- public delegate ValueTask<object?> RouteHandlerFilterDelegate(RouteHandlerInvocationContext context);
+ public delegate ValueTask<object?> EndpointFilterDelegate(EndpointExecutionContext context);

namespace Microsoft.AspNetCore.Http;

-  public class RouteHandlerContext {}
+  public class EndpointContext {}

namespace Microsoft.AspNetCore.Http;

-  public class DefaultRouteHandlerInvocationContext {}
+  public class DefaultEndpointInvocationContext {}

[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.

[halter73]

API review notes:

There were a few emails (from me included) that probably could have been GitHub comments, so I'm pasting them here.

• @DamianEdwards: "Can we make this name more descriptive? Perhaps EndpointFilterFactoryContext?"
  • @captainsafia: "Good point! EndpointFilterFactoryContext​ seems fine to me. Other thoughts?"
  • @halter73 (me): "I think we missed RouteHandlerInvocationContext (the base type for DefaultRouteHandlerInvocationContext). To align these with the just-proposed EndpointFilterFactoryContext name, I think these should be renamed to EndpointFilterInvocationContext and DefaultEndpoint*Filter(InvocationContext respectively."
• @halter73 (me): "If we decided to properly apply “EndpointFilters” to SignalR Hub method invocations in the future, the word “endpoint” might be a little out of place, but I don’t have a better name. Maybe just “Filter”, but that’s too nondescript."
  • @davidfowl: "I thought about this too, but I think it might be out of place. I was looking at razor pages as well, but it might not work well in all cases. I do think we need to decide on how these apply before we come up with the final name. We know we can't change things after the fact."
  • @captainsafia: "Thoughts on going back to the unmarked "AddFilters"/"FilterDelegate"? On the plus side, it's the least specific and allows us to pick a more specific name at a later time. One the other hand, it's the least specific and is likely vague enough to confuse users. Then again, it's probably prudent to start with a more specific name for the API then expand to a more specific one at a later time then eventually deprecate the old names?"
  • @DamianEdwards: "I still vote for EndpointFilter given that Hubs are the exception (and still invoked with a separate request in the cases of non-WebSocket transports anyway so the mapping isn’t too egregious)."

We want this for preview7 that closes tomorrow, so we can rename EndpointFilter again in an RC if we really need to, but I agree with @DamianEdwards that's what we should go with for now.

API Approved!

namespace Microsoft.AspNetCore.Http;

- public static class RouteHandlerFilterExtensions
+ public static class EndpointFilterExtensions
{
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, IRouteHandlerFilter filter);
-  public static TBuilder AddRouteHandlerFilter<TBuilder, TFilterType>(this TBuilder builder);
-  public static RouteHandlerBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder);
-  public static RouteGroupBuilder AddRouteHandlerFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder);
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerInvocationContext, RouteHandlerFilterDelegate, ValueTask<object?>> routeHandlerFilter);
-  public static TBuilder AddRouteHandlerFilter<TBuilder>(this TBuilder builder, Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate> filterFactory); 

+  public static TBuilder AddEndpointFilter<TBuilder>(this TBuilder builder, IEndpointFilter filter);
+  public static TBuilder AddEndpointFilter<TBuilder, TFilterType>(this TBuilder builder);
+  public static RouteHandlerBuilder AddEndpointFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteHandlerBuilder builder);
+  public static RouteGroupBuilder AddEndpointFilter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TFilterType>(this RouteGroupBuilder builder);
+  public static TBuilder AddEndpointFilter<TBuilder>(this TBuilder builder, Func<EndpointExecutionContext, EndpointFilterDelegate, ValueTask<object?>> endpointFilter);
+  public static TBuilder AddEndpointFilterFactory<TBuilder>(this TBuilder builder, Func<EndpointContext, EndpointFilterDelegate, EndpointFilterDelegate> filterFactory); 
}

- public interface IRouteHandlerFilter {}
+ public interface IEndpointFilter {}

- public delegate ValueTask<object?> RouteHandlerFilterDelegate(RouteHandlerInvocationContext context);
+ public delegate ValueTask<object?> EndpointFilterDelegate(EndpointExecutionContext context);

-  public class RouteHandlerContext {}
+  public class EndpointFilterFactoryContext {}

- public abstract class RouteHandlerInvocationContext {}
+  public class EndpointFilterInvocationContext {}

-  public class DefaultRouteHandlerInvocationContext {}
+  public class DefaultEndpointFilterInvocationContext {}

// EDIT
public sealed class RequestDelegateFactoryOptions
{
-    public IReadOnlyList<Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate>>? RouteHandlerFilterFactories { get; init; }
+    public IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? EndpointFilterFactories { get; init; }
}

[halter73]

While doing the renames, I noticed we missed RequestDelegateFactoryOptions.RouteHandlerFilterFactories in our general RouteHandlerFilter to EndpointFilter rename. I'm considering this already approved in spirit. If anyone objects though, please speak up.

namespace Microsoft.AspNetCore.Http;

public sealed class RequestDelegateFactoryOptions
{
-    public IReadOnlyList<Func<RouteHandlerContext, RouteHandlerFilterDelegate, RouteHandlerFilterDelegate>>? RouteHandlerFilterFactories { get; init; }
+    public IReadOnlyList<Func<EndpointFilterFactoryContext, EndpointFilterDelegate, EndpointFilterDelegate>>? EndpointFilterFactories { get; init; }
}

[davidfowl]

No need for endpoints here? FilterFactories

[captainsafia]

No need for endpoints here? FilterFactories

That seems a little weird to me only because it breaks the consistency that exists elsewhere.

[halter73]

In the interest of time and consistency, I'm sticking with EndpointFilterFactories.

[davidfowl]

I won't die on this hill but Endpoint doesnt have an EndpointMetadata property, it has a Metadata property, the extra Endpoint is redundant.

[halter73]

Not a hill I will die on either (I'd happily approve an API proposal and PR to change it to FilterFactories), but it's funny you mention a lack of an Endpoint.EndpointMetadata property when RequestDelegateFactoryOptions (the type in question) does have an EndpointMetadata property 😆.

[davidfowl]

That one make sense because it's not an endpoint.