Skip to content

Improvements to DynamicRouteValueTransformer #21471

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

Closed
rynowak opened this issue May 4, 2020 · 3 comments
Closed

Improvements to DynamicRouteValueTransformer #21471

rynowak opened this issue May 4, 2020 · 3 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing

Comments

@rynowak
Copy link
Member

rynowak commented May 4, 2020

Summary

3.1 Introduced DynamicRouteValueTransformer - a feature that allows you to use a custom endpoint to dynamically select a controller-action or a razor page.

This is useful when you want to create a slug route that accesses the database to choose where to go, or when you're layering another framework on top of MVC (OData).

Now that we've had a chance to get some feedback on it's clear that we're missing a few features that ought to be there.

Context

The way that DynamicRouteValueTransformer is hooked up, there's no way to pass additional context to it. DynamicRouteValueTransformer comes from DI.

endpoints.MapDynamicControllerRoute<MyTransformer>("...");

If you have multiple endpoints that use the same transformer type, you can't pass any data to them, because they come from DI. You effectively need a distinct type for each instance, which isn't a solution. Both OData and Orchard hit this issue.

Filtering

You might also want to do some filtering based on the results of your transformer. MVC resolves the matching endpoints based on the set of route values returned by a transformer, but the transformer can't do any filtering, or see the endpoints that resulted.

See: https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Core/src/Routing/DynamicControllerEndpointMatcherPolicy.cs#L121

Writing a separate policy to handle this doesn't work well. The transformer has context that is lost when you get past this phase:

  • A transformer could filter just the endpoints it produced.
  • A transformer could store context across method calls if it is transient

A policy that runs later can't see which endpoints came from the transformer, and can't store context because policies are singletons.

Ordering

The methods we provide for registering transformers don't support providing an Order. That's a fairly important thing when considering that transformers will be interleaved with other MVC endpoints.

However MapDynamicControllerRoute doesn't follow the same ordering rules as MapControllerRoute - it always uses order 0.

We should consider a breaking change to make MapDynamicControllerRoute follow the ordering rules of MapControllerRoute and also allow fine-grained control over the order value.

Fixes

We should add some functionality like the following:

public abstract class DynamicRouteValueTransformer
{
        // Will be set by the framework when initialized. This needs to come with the guidance
        // to use Transient DI lifetime.
        public object State { get; set; }

        public abstract ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpContext, RouteValueDictionary values);

        // Will be called when a non-zero amount of endpoints is found by MVC after calling
        // TransformAsync. They must return a new list of endpoints to perform filtering, or can
        // return the original list to skip.
        // The route values will be the ones returned from TransformAsync
        public virtual ValueTask<IReadOnlyList<Endpoint>> FilterEndpointsAsync(HttpContext httpContext, IReadOnlyList<Endpoint> endpoints, RouteValueDictionary values);
}

And the ability to pass in state:

endpoints.MapDynamicControllerRoute<MyTransformer>("...", state: new MyObject());
@rynowak rynowak added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 4, 2020
rynowak added a commit that referenced this issue May 4, 2020
See: #21471

This change just include the product-side changes and just for
controllers. This addresses most the major gaps our partners have found
with dynamic route value transformers. Doesn't include anything
order-related.
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone May 5, 2020
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 5, 2020
@FrankMuehlschlegel
Copy link

Oh i would really need this feature too. But for pages
endpoints.MapDynamicPageRoute<MyTransformer>(...);

@kccsf
Copy link

kccsf commented Jun 4, 2020

@rynowak - will the following scenario be covered with these enhancements?

Localization using a PageRouteModelConvention ie:
Template = AttributeRouteModel.CombineTemplates("{culture}", selector.AttributeRouteModel.Template)

Using a DynamicRouteValueTransformer as pages defined in headless CMS:
endpoints.MapDynamicPageRoute<CustomRouteValueTransformer>("{**url}");

Gives:
InvalidOperationException: Using ExpandEndpoint requires that the replaced endpoint have a unique priority. The following endpoints were found with the same priority

Not sure if relevant but I notice there are routes set up other than the DynamicPageRoute even though I have commented out:
endpoints.MapRazorPages();

Minimal repro:
https://github.com/kccsf/DynamicRouteIssue

Can log new issue if outside the scope of the above enhancements.

Is there a workaround?

TIA
Steve

@mkArtakMSFT
Copy link
Member

Linking this here in case this can be resoled as part of these improvements: #16965

javiercn pushed a commit that referenced this issue Jul 21, 2020
See: #21471

This change just include the product-side changes and just for
controllers. This addresses most the major gaps our partners have found
with dynamic route value transformers. Doesn't include anything
order-related.
@javiercn javiercn added Working Done This issue has been fixed and removed Working labels Jul 21, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2020
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 Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing
Projects
None yet
Development

No branches or pull requests

5 participants