Add support for route handler filter factories #40667
Conversation
ghost
added
the
captainsafia
force-pushed
the
ef-factory
branch
from
1c31afa to
0c8e8d7
Compare
captainsafia
added
old-area-web-frameworks-do-not-use
captainsafia
force-pushed
the
ef-factory
branch
from
0c8e8d7 to
39b8721
Compare
Co-authored-by: David Fowler <[email protected]>
captainsafia
requested review from
Tratcher,
halter73 and
javiercn
as code owners
BrennanConroy
left a comment
BrennanConroy
left a comment
There was a problem hiding this comment.
Can a test be added that asserts some endpoint metadata?
| /// <returns>An awaitable result of calling the handler and apply | ||
| /// any modifications made by filters in the pipeline.</returns> | ||
| ValueTask<object?> InvokeAsync(RouteHandlerFilterContext context, Func<RouteHandlerFilterContext, ValueTask<object?>> next); | ||
| ValueTask<object?> InvokeAsync(RouteHandlerInvocationContext context, RouteHandlerFilterDelegate next); |
There was a problem hiding this comment.
How do we feel about adding a RouteHandlerContext parameter? We can try to approve this over email if we like it.
| ValueTask<object?> InvokeAsync(RouteHandlerInvocationContext context, RouteHandlerFilterDelegate next); | |
| ValueTask<object?> InvokeAsync(RouteHandlerContext routeContext, RouteHandlerInvocationContext invocationContext, RouteHandlerFilterDelegate next); |
There was a problem hiding this comment.
Why? This should e a constructor argument, like middleware has the magic next no?
There was a problem hiding this comment.
How does middleware pass the magic next?
The implementation I just pushed passes the RouteHandlerContext as an argument to the object factory generated when the user utilizes the AddFilter<TFilterType>() overload.
src/Http/Http.Abstractions/src/RouteHandlerInvocationContext.cs
Outdated
Show resolved
Hide resolved
| /// provided to a route handler. | ||
| /// </summary> | ||
| public class RouteHandlerInvocationContext | ||
| public sealed class RouteHandlerInvocationContext |
There was a problem hiding this comment.
Don't seal this one. We'll end up un sealing it.
There was a problem hiding this comment.
We discussed this during API review. Unsealing doesn't have a cost, but sealing does so we are taking the more flexible option to start.
If we ship the parameters change in preview4, it shouldn't be too bad.
src/Http/Routing/test/UnitTests/Builder/RouteHandlerEndpointRouteBuilderExtensionsTest.cs
Outdated
Show resolved
Hide resolved
4 participants
Closes #40513