Skip to content
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.

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

Routing - cannot change precedence of DynamicRouteValueTransformer #45175

Closed
1 task done
razzemans opened this issue Nov 18, 2022 · 10 comments
Closed
1 task done

Routing - cannot change precedence of DynamicRouteValueTransformer #45175

razzemans opened this issue Nov 18, 2022 · 10 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved

Comments

@razzemans
Copy link

razzemans commented Nov 18, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

This is related to #29594

For our applications, we developed a CMS which implies that most of our routes are dynamic and determined during runtime - users can create and publish pages. For this I created a custom DynamicRouteValueTransformer which would check if a page for a given url exists in the database. Typically we would configure the routing in our applications like this:

app.UseEndpoints(endpoints =>
{
	endpoints.MapDynamicControllerRoute<CmsRoute>("{**slug}");
	endpoints.MapDefaultControllerRoute();
});

This worked like a charm, in .NET 5.0. If we had a route configured using attribute routing, that route would match and the CmsRoute would not be invoked. I believe since updating to .NET 6.0, the DynamicRouteValueTransformer is always invoked, just as described in the #29594. I am not sure about the solution though.

Mapping to a fallback controller doesn't work, since we have defined custom Controllers for certain page types (a page of type Foobar in the Cms is eventually mapped, in the DynamicRouteValueTransformer, to the FoobarController by setting the correct values in the RouteValueDictionary). I would somehow need to invoke a FoobarController from the FallbackController, which I am not even sure is possible (a redirect obviously is not desirable).

I came across #34316 which mentions using the ApplicationModelProvider. While I haven't researched it yet, it sounds like quite some refactoring to make it working again. Moreover, I am concerned about performance issues, since pages may be created at runtime, can be scheduled to go live at certain points in time, so it sounds like I would have to invoke the IChangeToken quite often (e.g. every 10 seconds, which I am not sure is desirable).

I was actually hoping I am missing a small change I can make to make the DynamicRouteValueTransformer working again so it is not invoking for endpoints configured using conventional routes. Also, after reading a lot about the routing I still don't understand why a catch-all route like {**slug} is considered to have more precedence than a controller endpoint with a [Route("/xyz")] defined as endpoint.

.NET Version

6.0+

@javiercn javiercn added feature-routing area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 18, 2022
@javiercn
Copy link
Member

@razzemans thanks for contacting us.

lso, after reading a lot about the routing I still don't understand why a catch-all route like {**slug} is considered to have more precedence than a controller endpoint with a [Route("/xyz")] defined as endpoint.

It's the order. Map*(Controller|Page)Route define an incremental sequential order based on the order in which they were originally called.

endpoints.MapDynamicControllerRoute<CmsRoute>("{**slug}"); // Order 0
endpoints.MapDefaultControllerRoute(); // Order 1

Would reordering the routes achieve what you are trying to accomplish?

endpoints.MapDefaultControllerRoute(); // Order 0
endpoints.MapDynamicControllerRoute<CmsRoute>("{**slug}"); // Order 1

I do not think the mentioned bug has anything to do, since it refers to an existing bug, we had in our implementation in 3.1 that we fixed shortly after.

@razzemans
Copy link
Author

@javiercn Thank you for your reply. Unfortunately the reordering does not fix it. All routes are still handled by CmsRoute. That's why I stated that I don't get why it always has precedence over the MapDefaultControllerRoute.

On a related note, we were investigating if switching to using an EndpointDataSource is an option but in #34316 you state that "you would not do that though an endpoint data source.". That makes me wonder if that would be a valid approach. Any clarification on that is also appreciated. The official documentation says it is something you can "consider" doing.

@javiercn
Copy link
Member

@razzemans all routes get evaluated, they get to produce candidates and then the best candidate is selected. Since you have a dynamic route, it needs to run to produce the final endpoints and then those are ranked in along with the rest of the candidates to choose the final endpoint.

So it does not matter that "/xyz" is more specific than "{**slug}" they both need to be computed because "/xyz" might be discarded by something else later on (like for example an HTTP constraint).

@javiercn javiercn added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Nov 23, 2022
@javiercn
Copy link
Member

For triage, there seems to be a limitation to explicitly override the order defined for dynamic endpoints. We should consider adding an overload to specify the order explicitly.

We do not want to expose the endpointconventionbuilder as it will result in unexpected behaviors, since the endpoint gets replaced at runtime, metadata applied to the conventionbuilder will not be actually used, which can be a source of confusion.

@javiercn javiercn added this to the Backlog milestone Nov 23, 2022
@ghost
Copy link

ghost commented Nov 23, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@razzemans
Copy link
Author

razzemans commented Nov 23, 2022

I thought that could be done using endpoints.MapDynamicControllerRoute<CmsRoute>("{**slug}", new object(), 1000); but alas that did not help.

@javiercn
Copy link
Member

@razzemans thanks for the additional details.

I was actually wrong. We already have a method that lets you control the order of the route; however, it won't do what you expect because routes need to always be evaluated before we choose the final target endpoint.

That said, you can actually achieve this in a different way with a matcher policy. Create one like the one below and register it in DI

internal sealed class EagerOrderMatcherPolicy : MatcherPolicy, IEndpointSelectorPolicy

public async Task ApplyAsync(HttpContext httpContext, CandidateSet candidates)
{
  // Iterate over candidates, detect a candidate with `DynamicControllerMetadata` or the transformer type metadata (can't remember the exact details).
 // Check other candidates to see if there is one with lower order
 // Discard the endpoint you captured if that is the case.
}

https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Builder/ControllerEndpointRouteBuilderExtensions.cs#L547

@javiercn javiercn removed this from the Backlog milestone Nov 23, 2022
@javiercn javiercn added question ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. and removed enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Nov 23, 2022
@ghost ghost added the Status: Resolved label Nov 23, 2022
@razzemans
Copy link
Author

@javiercn Interesting approach and I really appreciate your effort. Unfortunately the DynamicRouteValueTransformer is always invoked before my custom MatcherPolicy and I tried playing around with it but can't seem to change that. That means for conventional routes the DynamicRouteValueTransformer is still invoked which is precisely what I am trying to prevent.

In the DynamicRouteValueTransformer I am returning a RouteValueDictionary with values that results in some controller action being invoked (the correct controller for some CMS page). If I could do something similar in the MatcherPolicy or something related then that would work for sure, but I couldn't find something useful.

@javiercn
Copy link
Member

@razzemans did you set the order for your matcher policy? It needs to be very low, like int.MinValue + 10 so that it runs before the one for dynamic controllers.
https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs#L35

@razzemans
Copy link
Author

@javiercn Big thank you! Setting it really low works. This is a much nicer solution, so thanks again for your time!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved
Projects
None yet
Development

No branches or pull requests

2 participants