search

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.44k stars 10.01k forks source link

Razor Pages: Urls generated by built-in taghelpers do not honor PageRouteModelConvention #16960

Open oliverw opened 4 years ago

oliverw commented 4 years ago

Describe the bug

In a project that utilizes Razor Pages and request localization in conjunction with an IPageRouteModelConvention, Urls generated through standard TagHelpers for anchors or forms do not honor routing conventions and route values.

To Reproduce

Current route values (page url is /de)

Markup: <a asp-page="Search">Search</a>

Expected Result: <a href="/de/search">Search</a>

Actual Result: <a href="/search">Search</a>

There's a workaround but it is quite ugly:

<a asp-page="Search" asp-route-culture="@Request.RouteValues["culture"]">Search</a>

Startup.cs:

public class Startup
{
    public Startup(IConfiguration configuration, IWebHostEnvironment env)
    {
        Configuration = configuration;
        Env = env;

        var builder = new ConfigurationBuilder()
            .SetBasePath(Environment.CurrentDirectory)
            .AddJsonFile("appsettings.json", optional: true)
            .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true);

        if (env.IsDevelopment())
            builder.AddUserSecrets<Startup>();

        Configuration = builder.Build();
    }

    public IConfiguration Configuration { get; }
    public IWebHostEnvironment Env { get; }
    public static RequestCulture DefaultRequestCulture = new RequestCulture("en-US", "en-US");

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddOptions();
        services.AddSingleton(Configuration);

        services.AddRazorPages(options =>
        {
            options.Conventions.Add(new CultureTemplateRouteModelConvention());
        }).SetCompatibilityVersion(CompatibilityVersion.Version_3_0);

        services.AddLocalization(options => options.ResourcesPath = "Resources");

        services.Configure<RequestLocalizationOptions>(options =>
        {
            options.DefaultRequestCulture = DefaultRequestCulture;
            options.SupportedCultures = AppConstants.SupportedCultures;
            options.SupportedUICultures = AppConstants.SupportedCultures;

            options.RequestCultureProviders.Insert(0, new RouteDataRequestCultureProvider { Options = options });
        });

        services.AddRouting(options =>
        {
            options.LowercaseUrls = true;
        });

        services.AddOptions();
        services.AddLogging();
        services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();
        services.AddSingleton(Configuration);
    }

    public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
    {
        Container = app.ApplicationServices.GetAutofacRoot();

        app.UseStaticFiles();
        app.UseRouting();
        app.UseRequestLocalization();
        app.UseAuthorization();

        app.UseEndpoints(endpoints =>
        {
            endpoints.MapRazorPages();
    }
}

CultureTemplateRouteModelConvention.cs:

public class CultureTemplateRouteModelConvention : IPageRouteModelConvention
{
    public void Apply(PageRouteModel model)
    {
        var selectorCount = model.Selectors.Count;

        for (var i = 0; i < selectorCount; i++)
        {
            var selector = model.Selectors[i];

            model.Selectors.Add(new SelectorModel
            {
                AttributeRouteModel = new AttributeRouteModel
                {
                    Order = -1,
                    Template = AttributeRouteModel.CombineTemplates(
                          "{culture?}", selector.AttributeRouteModel.Template),
                }
            });
        }
    }
}

Further technical details

javiercn commented 4 years ago

@oliverw Thanks for contacting us.

@rynowak any idea of what's going on here?

oliverw commented 4 years ago

Further investigation revealed that the problem also applies to link to controller actions, not just pages. The reason is that AnchorTagHelper (as well as others) only pass Route Values explicitly set using the asp-route-x attribute to its HtmlGenerator instance which in turn invokes urlHelper.Page or urlHelper.Action etc. I wouldn't be surprised if this is by design to avoid unwanted side effects. But as a matter of fact, manually forwarding route values using asp-route-culture on each and every anchor or form was not necessary in earlier versions of Net Core.

I've come up with the following workaround for the time being. One that I'm not entirely happy with:

_ViewImports.cshtml:

@*Remove built-in TagHelpers that interfere with our overrides*@
@removeTagHelper Microsoft.AspNetCore.Mvc.TagHelpers.AnchorTagHelper, Microsoft.AspNetCore.Mvc.TagHelpers
@removeTagHelper Microsoft.AspNetCore.Mvc.TagHelpers.FormActionTagHelper, Microsoft.AspNetCore.Mvc.TagHelpers

@addTagHelper *, WebApp

CultureAwareAnchorTagHelper:

[HtmlTargetElement("a", Attributes = ActionAttributeName)]
[HtmlTargetElement("a", Attributes = ControllerAttributeName)]
[HtmlTargetElement("a", Attributes = AreaAttributeName)]
[HtmlTargetElement("a", Attributes = PageAttributeName)]
[HtmlTargetElement("a", Attributes = PageHandlerAttributeName)]
[HtmlTargetElement("a", Attributes = FragmentAttributeName)]
[HtmlTargetElement("a", Attributes = HostAttributeName)]
[HtmlTargetElement("a", Attributes = ProtocolAttributeName)]
[HtmlTargetElement("a", Attributes = RouteAttributeName)]
[HtmlTargetElement("a", Attributes = RouteValuesDictionaryName)]
[HtmlTargetElement("a", Attributes = RouteValuesPrefix + "*")]
public class CultureAwareAnchorTagHelper : AnchorTagHelper
{
    public CultureAwareAnchorTagHelper(IHttpContextAccessor contextAccessor, IHtmlGenerator generator) :
        base(generator)
    {
        this.contextAccessor = contextAccessor;
    }

    #region Verbatim Copy: https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.TagHelpers/src/AnchorTagHelper.cs

    private const string ActionAttributeName = "asp-action";
    private const string ControllerAttributeName = "asp-controller";
    private const string AreaAttributeName = "asp-area";
    private const string PageAttributeName = "asp-page";
    private const string PageHandlerAttributeName = "asp-page-handler";
    private const string FragmentAttributeName = "asp-fragment";
    private const string HostAttributeName = "asp-host";
    private const string ProtocolAttributeName = "asp-protocol";
    private const string RouteAttributeName = "asp-route";
    private const string RouteValuesDictionaryName = "asp-all-route-data";
    private const string RouteValuesPrefix = "asp-route-";
    private const string Href = "href";

    #endregion

    private readonly IHttpContextAccessor contextAccessor;
    private readonly string defaultRequestCulture = Startup.DefaultRequestCulture.Culture.TwoLetterISOLanguageName;

    public override void Process(TagHelperContext context, TagHelperOutput output)
    {
        var culture = (string) contextAccessor.HttpContext.Request.RouteValues["culture"];

        if(culture != null && culture != defaultRequestCulture)
            RouteValues["culture"] = culture;

        base.Process(context, output);
    }
}

Remarks:

The above solution kind of sucks because ...

Perhaps I've missed an extension point that executes before tag helpers that would allow for a cleaner implementing of this functionality?

gerardfoley commented 4 years ago

I'm running into the same problem myself.

It looks like this behavior is by design as oliverw suspected. From the page https://docs.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-3.0 Section: Endpoint routing differences from earlier versions of routing

In endpoint routing with ASP.NET Core 2.2 or later, the result is /Login. Ambient values aren't reused when the linked destination is a different action or page.

This might also explain why this was not necessary in earlier versions of Net Core.

I really hope for a work around for this. Having to manually pass the culture in every link and redirect throughout the entire site is a real pain.

natelaff commented 4 years ago

Following

jeffreyrivor commented 4 years ago

@gerardfoley: Yes it is definitely the ambient values behavior change after the switch to endpoint routing.

@oliverw: I've worked around needing to inject the culture in each caller that is associated with a HttpContext by adding a decorator class around the default LinkGenerator that's added to the container.

public class LinkGeneratorDecorator : LinkGenerator
{
  private LinkGenerator _innerLinkGenerator;
  public LinkGeneratorDecorator(LinkGenerator inner) =>
    _innerLinkGenerator = inner ?? throw new ArgumentNullException(nameof(inner));

  public override string GetPathByAddress<TAddress>(
    HttpContext httpContext,
    TAddress address,
    RouteValueDictionary values,
    RouteValueDictionary ambientValues = null,
    PathString? pathBase = null,
    FragmentString fragment = null,
    LinkOptions options = null)
  {
    // clone the explicit route values
    var newValues = new RouteValueDictionary(values);

    // get culture from ambient route values
    if (ambientValues?.TryGetValue("culture", out var value) == true)
    {
      // add to the cloned route values to make it explicit
      // respects existing explicit value if specified
      newValues.TryAdd("culture", value);
    }

    return _innerLinkGenerator.GetPathByAddress(httpContext, address, newValues,
      ambientValues, pathBase, fragment, options);
  }

  // do the same with GetUriByAddress<TAddress> overload with ambient values,
  // straight pass-through for overloads without ambient values to use
}

If you're using UseRequestLocalization and detecting locales using other methods besides route values, there will be instances where the ambient route values don't have the locale. You could instead switch this code to use the IRequestCultureFeature of the HttpContext to get the resolved locale.

var requestCulture = httpContext.Features.Get<IRequestCultureFeature>().RequestCulture;
newValues.TryAdd("culture", requestCulture.Culture.Name); // or UiCulture.Name if preferred

If you're using a IPageRouteModelConvention to take every page route and adding a new route with a culture prefix, you could make the culture required on that route and disallow the original non-prefix route from being used in any link generation to be defensive. This is how I originally stumbled onto the problem; after I disabled non-prefix routes for link generation, I was getting blank results for any link where the ambient values were not used (e.g. going from Page = "/Index" to Page = "/Search").

public class CultureTemplateRouteModelConvention : IPageRouteModelConvention
{
    public void Apply(PageRouteModel model)
    {
        var selectorCount = model.Selectors.Count;

        for (var i = 0; i < selectorCount; i++)
        {
            var selector = model.Selectors[i];

            model.Selectors.Add(new SelectorModel
            {
                AttributeRouteModel = new AttributeRouteModel
                {
                    Order = -1,
                    Template = AttributeRouteModel.CombineTemplates(
                          "{culture}", selector.AttributeRouteModel.Template), // changed to required
                }
            });

            // suppress original route so URLs can't be made using its non-prefixed template
            selector.AttributeRouteModel.SuppressLinkGenerator = true;
        }
    }
}

@rynowak and @javiercn: It seems like the bigger theme here is that we need a seam to be able to describe/override policy behavior on the treatment of specific ambient values to allow them to continue to be route value candidates in link generation.

The DefaultLinkGenerator is too complex to re-implement, so a custom LinkGenerator implementation registered in DI doesn't seem like the right approach. The decorator I added to wrap the initially registered DefaultLinkGenerator with my own seems hacky.

Custom IRouteConstraint or IOutboundRouteValuesTransformer is not a candidate because constraints are only checked after filtering out candidate routes that have all the required route values set (the ambient value for culture is dropped so the routes requiring that route value are not candidates, and routes with the value set as optional don't run the constraint check because the value was not specified at all).

In conclusion, it seems that a new extensibility point is needed. Here's some options I've thought about.

Option: New Constraint

Constraint to mark a route value in a template that is always allowed to be taken from ambient values: "{culture:alwaysallowambient}/Search".

Pros:

Cons:

Option: Better LinkGenerator abstraction

A simpler LinkGenerator interface to be able to implement from scratch.

Pros:

Cons:

Alternative: DynamicRouteValueTransformer?

There are no docs about DymamicRouteValueTransformer. I've tried setting up a generic route for Razor Pages like endpoints.MapRazorPages(); endpoints.MapDynamicPageRoute("{page}"); with a transformer that would add the detected locale as a route value back into the RouteValuesDictionary, but I kept getting an AmbiguousMatchException.

fuzl-llc commented 4 years ago

I've just run into the same issue. Thanks for all the work on this. The workaround mostly works for me, but one thing I've noticed is that for routes with the culture present, it generates links that include "Index" in them when it shouldn't. For example, I have a site with default culture "en" and alternate supported culture "ja." Links for "en" all work fine, for example I have a link generated with asp-page="/Puzzles/Index" and that generates a link like this: "/puzzles/" which is what I want (no "index" part as expected). However, when I am on Japanese pages, all the links look good, except those that are default "Index" pages... and the puzzle link above ends up as "/ja/puzzles/index" when what I want and expect is "/ja/puzzles/." Can you confirm this is not as expected? Any workaround for this issue?

kccsf commented 4 years ago

@fuzl-llc - does this work for removing your index suffix?

services.AddRazorPages()
    .AddRazorPagesOptions(options =>
    {
        // Remove routes ending with /Index
        options.Conventions.AddFolderRouteModelConvention("/", model =>
        {
            var selectorCount = model.Selectors.Count;
            for (var i = selectorCount - 1; i >= 0; i--)
            {
                var selectorTemplate = model.Selectors[i].AttributeRouteModel.Template;
                if (selectorTemplate.EndsWith("Index"))
                {
                    model.Selectors.RemoveAt(i);
                }
            }
        });
    });
nfplee commented 4 years ago

@jeffreyrivor I have the same issue where if you suppress link generation for the non-locale route/selector then it will generate a blank string for non localized urls. There doesn't seem to be a great way to solve this other than overriding the anchor tag helper and the url generator or by passing in the culture every time, both are extremely ugly.

ghost commented 4 years 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.

MichalSznajder commented 4 years ago

I am also hit by this. I wanted to add multi tenant support to an existing app by modifying all links from /{area}/{page} to /{tenant}/{area}/{page}/. I thought it is going to be trivial as adding custom IPageRouteModelConvention but unfortunately all links are broken between pages.

enricoe73 commented 3 years ago

Hello, anyone check for this issue ? DynamicRouteValueTransformer it's useless without that.

ghost commented 2 years ago

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 1 year ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

nfplee commented 1 year ago

Please could you make sure this is addressed in the next version. We basically need some way of adding a route value (which can be overridden) to all URLs generated from tag helpers, Url.Action, LinkGenerator etc.

I built my own Symfony framework in PHP and this was as simple as adding a default to the UrlGenerator (which is used to generate all URLs).

rwasef1830 commented 1 year ago

@nfplee it is currently possible by replacing the LinkGenerator service in the dependency injection registration with your decorator that adds ambient values to the route values being used to generate the link.

nfplee commented 1 year ago

@rwasef1830 thanks but the only way to override the tag helpers I believe is to create your own custom tag helper, which all gets a bit messy.

rwasef1830 commented 1 year ago

The tag helpers call into UrlHelper which ultimately goes into LinkGenerator which is the actual core mechanism for generating urls from route values used by the whole ASP.net Core infrastructure. Overriding this service will change the behavior throughout the entire framework including tag helpers, url helper, html helper, everything.

nfplee commented 1 year ago

@rwasef1830 thanks much appreciated. What part of the LinkGenerator would I need to override to add my own custom defaults?

rwasef1830 commented 1 year ago

All abstract methods, you need to modify the contents of the values parameter.

nfplee commented 1 year ago

Thanks @rwasef1830, I hadn't really looked at how the LinkGenerator was implemented much before. However I have noticed that it is marked as internal. I'm not sure how comfortable I am implementing this knowing it will likely change in the future. I guess it'll work for now, but hopefully in the future they create an easier way of doing this.

rwasef1830 commented 1 year ago

Hello, The LinkGenerator abstract class is public. Use a library like scrutor to decorate the existing internal class without needing to know its type. Modify the parameters, then pass it back to the original.

[image: image.png]

On Thu, Mar 2, 2023 at 5:53 PM Lee Timmins @.***> wrote:

Thanks @rwasef1830 https://github.com/rwasef1830, I hadn't really looked at how the LinkGenerator was implemented much before. However I have noticed that it is marked as internal. I'm not sure how comfortable I am implementing this knowing it will likely change in the future. I guess it'll work for now, but hopefully in the future they create an easier way of doing this.

— Reply to this email directly, view it on GitHub https://github.com/dotnet/aspnetcore/issues/16960#issuecomment-1452095613, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDK5PIMZWLW6ITDDKGDRLDW2C66LANCNFSM4JLN45CA . You are receiving this because you were mentioned.Message ID: @.***>

MichalSznajder commented 1 year ago

Have a look at https://gitlab.com/NonFactors/AspNetCore.Template/-/blob/master/src/MvcTemplate.Web/Startup.cs#L152 and https://gitlab.com/NonFactors/AspNetCore.Template/-/blob/master/src/MvcTemplate.Components/Mvc/Routing/DefaultLinkGenerator.cs to get an idea on overriding LinkGneerator.

I am using this approach for links like {tenant}/SomeRazorPage with success since a long time.

jeffreyrivor commented 10 months ago

I've just run into the same issue. Thanks for all the work on this. The workaround mostly works for me, but one thing I've noticed is that for routes with the culture present, it generates links that include "Index" in them when it shouldn't. For example, I have a site with default culture "en" and alternate supported culture "ja." Links for "en" all work fine, for example I have a link generated with asp-page="/Puzzles/Index" and that generates a link like this: "/puzzles/" which is what I want (no "index" part as expected). However, when I am on Japanese pages, all the links look good, except those that are default "Index" pages... and the puzzle link above ends up as "/ja/puzzles/index" when what I want and expect is "/ja/puzzles/." Can you confirm this is not as expected? Any workaround for this issue?

This is a bit late of a reply (over 3 years!), but the reason this is happening is because "/Index.cshtml" Razor pages have a special case that generates additional SelectorModel objects than other pages (this behavior has existed prior to v8, probably as far back as v2.1 when attribute routing was introduced).

https://github.com/dotnet/aspnetcore/blob/3f1acb59718cadf111a0a796681e3d3509bb3381/src/Mvc/Mvc.RazorPages/src/ApplicationModels/PageRouteModelFactory.cs#L56-L77

If using the code samples in this thread as-is to add prefixed attribute route templates, you'll end up making copies of both.

@fuzl-llc - does this work for removing your index suffix?

services.AddRazorPages()
    .AddRazorPagesOptions(options =>
    {
        // Remove routes ending with /Index
        options.Conventions.AddFolderRouteModelConvention("/", model =>
        {
            var selectorCount = model.Selectors.Count;
            for (var i = selectorCount - 1; i >= 0; i--)
            {
                var selectorTemplate = model.Selectors[i].AttributeRouteModel.Template;
                if (selectorTemplate.EndsWith("Index"))
                {
                    model.Selectors.RemoveAt(i);
                }
            }
        });
    });

This workaround is to remove all the "/Index" suffixed route templates, including the one added by PageRouteModelFactory, which is likely to cause other unintended side effects, namely that incoming requests with the "/Index" suffix will not work, with or without the "{culture}/" prefix.

Instead, more robust route template cloning logic should be used to preserve properties like AttributeRouteModel.SuppressLinkGeneration and AttributeRouteModel.SuppressPathMatching appropriately.

public class CultureTemplateRouteModelConvention : IPageRouteModelConvention
{
    public void Apply(PageRouteModel model)
    {
        var selectorCount = model.Selectors.Count;

        for (var i = 0; i < selectorCount; i++)
        {
            var selector = model.Selectors[i];

            if (selector.AttributeRouteModel?.Template is null)
            {
                return; // no template to prefix, bail out
            }

            // use copy constructor to clone SelectorModel to preserve existing properties
            var prefixedSelector = new SelectorModel(selector);

            // (use null-forgiving operator since static analysis doesn't realize that the copy constructor preserves non-null property)
            // decrement order to give prefixed selector higher priority relative to original selector
            prefixedSelector.AttributeRouteModel!.Order--;

            // update the template string with prefix
            prefixedSelector.AttributeRouteModel.Template = AttributeRouteModel.CombineTemplates(
                "{culture}", prefixedSelector.AttributeRouteModel.Template);

            // append prefixed selector to end of selector list
            model.Selectors.Add(prefixedSelector);

            // optional: suppress original selector for link generation to ensure prefixed routes are used,
            // has side effect of empty LinkGenerator return values when making links if culture route value is not explicitly set
            // (including UrlHelper and AnchorTagHelper, which depend on LinkGenerator) requiring either explicit route value
            // (like `asp-route-culture` parameter or RouteValueDictionary.TryAdd("culture", currentValue)) or decorated
            // LinkGenerator that adds the route value explicitly to every RouteValueDictionary
            selector.AttributeRouteModel.SuppressLinkGenerator = true;
        }
    }
}
d2soft commented 1 month ago

@jeffreyrivor Your route template worked! It allows to remove the "/Index" from URL while keeping them still accessible (solution from @kccsf would not allow access to the Index). Thanks for the help.