dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.21k stars 9.95k forks source link

Routing - cannot change precedence of DynamicRouteValueTransformer #45175

Closed razzemans closed 1 year ago

razzemans commented 1 year ago

Is there an existing issue for this?

Describe the bug

This is related to https://github.com/dotnet/aspnetcore/issues/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 https://github.com/dotnet/aspnetcore/issues/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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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.

ghost commented 1 year ago

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 commented 1 year ago

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

javiercn commented 1 year ago

@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

razzemans commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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