Skip to content

Make route handler filters work for MVC controllers #42557

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 11, 2022
Merged

Make route handler filters work for MVC controllers #42557

merged 7 commits into from
Aug 11, 2022

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jul 4, 2022 •
edited
Loading

  • This change makes route handler filters work better with MVC controller actions by wrapping the action delegate directly. This means that run after action filters.
  • To do so we need to expose the RouteHandlerFilters on the RouteEndpointBuilder and use them in MVC

Fixes #42799
Fixes #42593

@davidfowl davidfowl requested a review from a team July 4, 2022 03:09
davidfowl
davidfowl commented Jul 4, 2022
View reviewed changes
@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 5, 2022
@halter73 halter73 mentioned this pull request Jul 13, 2022
Closed
@davidfowl davidfowl force-pushed the davidfowl/controller-rhfilters branch 3 times, most recently from 242a80c to 84cffde Compare July 14, 2022 12:41
davidfowl
davidfowl commented Jul 18, 2022
View reviewed changes
Kahbazi
Kahbazi reviewed Jul 19, 2022
View reviewed changes
@halter73 halter73 mentioned this pull request Jul 19, 2022
Merged
@captainsafia captainsafia self-assigned this Jul 19, 2022
davidfowl
davidfowl commented Jul 20, 2022
View reviewed changes
@captainsafia captainsafia force-pushed the davidfowl/controller-rhfilters branch from fb67fc4 to 7fde485 Compare July 27, 2022 15:47
davidfowl
davidfowl commented Jul 27, 2022
View reviewed changes
@captainsafia captainsafia force-pushed the davidfowl/controller-rhfilters branch 2 times, most recently from 676f5ac to 27f98b7 Compare July 27, 2022 20:59
@captainsafia captainsafia marked this pull request as ready for review July 27, 2022 23:32
halter73
halter73 reviewed Aug 3, 2022
View reviewed changes
@davidfowl
Make route handler filters work for MVC controllers
088e709 
    - This change makes route handler filters work better with MVC controller actions by wrapping the action delegate directly.
    - To do so we need to expose the RouteHandlerFilters on the RouteEndpointBuilder and use them in MVC
    - Implement GetEndpointGroup on ActionEndpointDataSource
    - Reuse the action method executor logic to pick the best invoker even in the filter scenario.
    - Added tests
@davidfowl
Naming updates
60ab760
@davidfowl davidfowl force-pushed the davidfowl/controller-rhfilters branch from 27f98b7 to 60ab760 Compare August 8, 2022 18:19
@davidfowl
Stephen optimization
24a8ea3 
- If the initial delegate is the same as the final delegate then skip the filter pipeline.
@davidfowl
Copy link
Member Author

OK this is ready for another pass.

captainsafia
captainsafia approved these changes Aug 11, 2022
View reviewed changes
src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs
IReadOnlyList<Action<EndpointBuilder>> conventions,
IReadOnlyList<Action<EndpointBuilder>> perRouteConventions)
{
// REVIEW: The RouteEndpointDataSource adds HttpMethodMetadata before running group conventions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's goodness to give access to this metadata to group conventions. I don't anticipate that it'll be as required here as it is in REDS, where it's needed for OpenAPI support. IMO, nice to have but not groundbreaking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's goodness to give access to this metadata to group conventions. I don't anticipate that it'll be as required here as it is in REDS, where it's needed for OpenAPI support. IMO, nice to have but not groundbreaking.

Open API fixes it with Finally 😄

Copy link
Member

@halter73 halter73 Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely fix this inconsistency. RouteEndpointDataSource has the nicer, more convenient behavior. Maybe we can sneak this into #43225.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 Sure -- I can adjust. Do any other conventions rely on this particular metadata? OpenAPI was the only one that came to mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we’d need a scenario where it matters. This code needs to be re-worked to do that I think. We don’t want group conventions to override endpoint metadata in attributes and metadata set from the action descriptor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code needs to be re-worked to do that I think. We don’t want group conventions to override endpoint metadata in attributes and metadata set from the action descriptor

I agree with this part.

I think we’d need a scenario where it matters.

I think consistency is enough of a reason to say we ought to do this. I can agree that fixing this isn't the highest priority thing, and if it's too complicated it might not be worth doing now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, WithOpenApi is the only thing I know of that already depends on this and it is fixed by Finally.

src/Mvc/test/Mvc.FunctionalTests/RoutingGroupsTest.cs
@@ -4,7 +4,9 @@
using System.Net;
using System.Net.Http;
using System.Net.Http.Json;
using System.Text.Json;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needed?

Suggested change
using System.Text.Json;

@halter73 halter73 mentioned this pull request Aug 11, 2022
Merged
10 tasks
@davidfowl davidfowl merged commit 7f8e473 into main Aug 11, 2022
@davidfowl davidfowl deleted the davidfowl/controller-rhfilters branch August 11, 2022 23:30
@ghost ghost added this to the 7.0-rc1 milestone Aug 11, 2022
@halter73 halter73 mentioned this pull request Aug 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 left review comments

@captainsafia captainsafia captainsafia approved these changes

@Kahbazi Kahbazi Kahbazi left review comments

@javiercn javiercn Awaiting requested review from javiercn

@Tratcher Tratcher Awaiting requested review from Tratcher

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

Assignees

@captainsafia captainsafia

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
7.0-rc1
Development

Successfully merging this pull request may close these issues.

Support running endpoints filters in MVC Expose RoutePatternFactory.Combine
5 participants
@davidfowl @halter73 @captainsafia @Kahbazi @Pilchie