[Mvc] Add support for order in dynamic controller routes

[javiercn]

Summary

We want to support order with dynamic endpoints in the same way it is supported for conventionally routed actions.

Motivation

Conventional routes have an order concept that is tied up with the order they are defined in code. That means that a route that is defined earlier during the execution will have
precendence over a route that was defined later. Dynamic routes don't support this functionality and that discrepance is the source of to big problems:

• It is confusing and surprising for customers since they expect these routes to have a similar behavior as conventional routes.
• It makes it hard to use more than one dynamic route in the same application, since these tend to be very generic routes, its easy for them to catch many incoming requests.

Goals

• Make using multiple dynamic routes in the same app easier.
• Make using dynamic routes in conjunction with conventional and attribute routes easier.

Non-goals

• Link generation
• Order is global now for all registered MVC/Pages, we don't plan to change that as part of this PR.

Scenarios

• Mapping dynamic routes and attribute routes.
• Mapping multiple dynamic routes.
• Mapping multiple dynamic routes and conventional routes.
• Mapping dynamic controller routes and dynamic page routes.

Risks

• Existing applications might experience changes in the matched actions for a given pattern:

  • An attribute route with a lower routing precedence might match a request that was previously matched by a dynamic action.
  • Conflicting routing configurations can be resolved in favor of attribute routed actions by default.

• We can make this opt-in and enabled in the template by default, with the behavior becomming the default in 6.0.

Interaction with other parts of the framework

• These routes will now have a lower order than endpoints registered by other frameworks like SignalR.

Detailed design

We want dynamic routes to behave more like conventional routes and participate in the ordering these routes have:

• Dynamic controller routes have the same order as conventional routes by default.
  • All attribute routed controller routes have lower order than dynamic controller routes, which means if they match, they win.

builder.UseEndpoints(endpoints => {
  endpoints.MapControllers();
  endpoints.MapDynamicController<DbTransformer>(...);
});

• Dynamic controller routes order is based on the definion order
  • Routes that get defined first have a lower order, which means they win even if they are less specific than routes after them/
  • This behavior is inline with conventional mvc routing behavior.

builder.UseEndpoints(endpoints => {
  endpoints.MapDynamicController<DbTransformer>(...);
  endpoints.MapDynamicController<CoolBeansTransformer>(...);
});

• Dynamic controllers routes order and conventional controller routes share order
  • Routes that get defined first have a lower order, which means that a dynamic route that is less specific than a conventional route wins when both match.

builder.UseEndpoints(endpoints => {
  endpoints.MapDynamicController<DbTransformer>(...);
  endpoints.MapControllerRoute(...);
});

• Dynamic page routes order and dynamic controller routes share order
  • Routes that get defined first have a lower order, which means that a dynamic route that is less specific than a dynamic page route wins when both match.

builder.UseEndpoints(endpoints => {
  endpoints.MapDynamicController<DbTransformer>(...);
  endpoints.MapDynamicPage<DocumentTransformer>(...);
});

Drawbacks

• You need to order your routes from most specific to less specific since they now behave in the same way as conventional routes.

Considered alternatives

The alternative is to keep the current behavior, which has the problems we indicated above.

[JamesNK]

Could you link to issues this would solve? How many times has this been reported?

Overall this sounds like a good change. I'm a little worried about breaking people. Routing changes are hard to debug/communicate in a nice way. I agree that we need a flag to control the new behavior.

Ryan added dynamic routing after I worked on routing so I'm not super knowledgeably about it. I'd like to get @rynowak's input on why dynamic routing don't have an order at the moment, and whether there are things we haven't considered.

[JamesNK]

readonly

[javiercn]

Could you link to issues this would solve? How many times has this been reported?
#21471

See the section about ordering.

It also makes it easier to do/implement things like #22601

#21083

Ultimately this brings it closer to the old IRouter functionality, which is what we want to replace.

[rynowak]

Could you link to issues this would solve? How many times has this been reported?

Overall this sounds like a good change. I'm a little worried about breaking people. Routing changes are hard to debug/communicate in a nice way. I agree that we need a flag to control the new behavior.

Ryan added dynamic routing after I worked on routing so I'm not super knowledgeably about it. I'd like to get @rynowak's input on why dynamic routing don't have an order at the moment, and whether there are things we haven't considered.

This was an oversight, and we had low usage of the dynamic routing feature during the few previews it had. We got a trickle of feedback once it went RTM and people started using it. Users in general were confused why it didn't behave like other MVC conventional routing features.

[javiercn]

Hey folks, is there anything else you don't feel is addressed? (- this comment that I don't plan to trigger another build for, I'll fix change it in the future as part of a separate RC2 change)

I would like to get this merged today.

[JamesNK]

Is there going to be an option or switch to continue using the old behavior or do you feel it is not needed?

[javiercn]

I don’t think we need it, we can add an overload to explicitly set the order if we receive feedback about it.

[JamesNK]

Will this be called out as a breaking change and/or mentioned in the 3 -> 5 migration guide?

[javiercn]

Will this be called out as a breaking change and/or mentioned in the 3 -> 5 migration guide?

Yes, and on the RC1 Blog post

[alienwareone]

As a workaround right now I do this:

    public class CatchAllRouteValueTransformerDispatcher : DynamicRouteValueTransformer
    {
        public CatchAllRouteValueTransformerDispatcher(params DynamicRouteValueTransformer[] dynamicRouteValueTransformers)
        {
            DynamicRouteValueTransformers = dynamicRouteValueTransformers;
        }

        public DynamicRouteValueTransformer[] DynamicRouteValueTransformers { get; }

        public async override ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpContext, RouteValueDictionary values)
        {
            foreach (var dynamicRouteValueTransformer in DynamicRouteValueTransformers)
            {
                var result = await dynamicRouteValueTransformer.TransformAsync(httpContext, values);
                if (result != null)
                {
                    return result;
                }
            }
            return null;
        }
    }

    // Startup:
    services.AddScoped(x =>
        new CatchAllRouteValueTransformerDispatcher(new ARouteValueTransformer(), new BRouteValueTransformer()));

    // Endpoint:
    routeBuilder.MapDynamicControllerRoute<CatchAllRouteValueTransformerDispatcher>("{**path}");