aspnet / Routing

[Archived] Middleware for routing requests to application logic. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
272 stars 122 forks source link

Attribute routing: GetVirtualPath() (e.g. called by Url.Action) doesn't respect constraints from IApplicationModelConvention #896

Closed cypressious closed 6 years ago

cypressious commented 6 years ago

Moving from https://github.com/aspnet/AspNetCore/issues/3745

Short version

When adding selectors with constraints using an IApplicationModelConvention like so

action.Selectors.Add(new SelectorModel(defaultSelector)
{
    AttributeRouteModel = localizedVersion,
    ActionConstraints =
    {
        new CultureActionConstraint { Culture = ... }
    }
});

public class CultureActionConstraint : IActionConstraint
{
    public string Culture { get; set; }

    public int Order => 0;

    public bool Accept(ActionConstraintContext context)
    {
        return CultureInfo.CurrentCulture.TwoLetterISOLanguageName == Culture;
    }
}

anything that calls GetVirtualPath() (e.g. Url.Action()) doesn't respect the constraints.

To reproduce, checkout and run https://github.com/cypressious/asp-net-core-localized-routes-issue, then open http://localhost:5000/about_ru?culture=ru. The generated link should be to http://localhost:5000/privacy_ru but it's actually http://localhost:5000/privacy_bg

cypressious commented 6 years ago

@hishamco anybody specific I should ping?

hishamco commented 6 years ago

/cc @rynowak @JamesNK

rynowak commented 6 years ago

Action constraints only run during action selection - they have no role during URL generation.

rynowak commented 6 years ago

What are you trying to accomplish?

cypressious commented 6 years ago

My application should run on different domains, each corresponding to a culture. Every culture has its localized routes, but I don't have the culture as part of the url. The generated links must be correct for every domain/culture, e.g. on the frontpage I have a link to the user page which is

http://your-domain.com/user for en culture http://your-domain.de/benutzer for de culture

BoasE commented 6 years ago

Any news on this ? @mkArtakMSFT

hishamco commented 6 years ago

@BoasE Could you use my workaround that I mentioned, while the team got a time to investigate on this, because if there's a bug, will not show up until the next release, or you can using a nightly builds

Hope @mkArtakMSFT will reply to use, because he knows better than me about the team 😄

BoasE commented 6 years ago

The most important thing is to know wether it is a bug :)

I still see a small chance that we are just missing something on how to implement it in the supported way.

So its mainly important to get a information wether it is a bug or if we should use the asp in an other way for this

mkArtakMSFT commented 6 years ago

@JamesNK, can you please look into this one as @rynowak is out.

JamesNK commented 6 years ago

The most important thing is to know wether it is a bug :)

It is not a bug. Like Ryan said, action constraints only run when matching incoming requests to actions. They don't run when generating a route.

hishamco commented 6 years ago

@JamesNK even though if it's not a bug I'm not sure what's the secret behind GetVirtualPath() because it has a strange behavior. Hope you have a look to the original issue, to know what i'm talking about

JamesNK commented 6 years ago

You have multiple attribute routes setup against an action. When that action is matched then the link generate is choosing one, I think the last registered one based on the order of your languages. This isn't code I'm familiar with but if you're really curious then I can spend time investigating why further.

hishamco commented 6 years ago

What made me crazy is when I did a workaround for the issue, the link with TagHelper <a asp-action="Privacy" asp-controller="Home">Privacy</a> point to the correct wrote, while the HtmlHelper one doesn't @Html.ActionLink("Privacy", "Privacy", "Home")?!!

BoasE commented 6 years ago

@hishamco the point i don't get is that if this is a expected behavior then this means you were not allowed to use Url.Action as soon as you have constrains because Url.Action may create Links that are not valid

@JamesNK do you agree ?

JamesNK commented 6 years ago

IActionConstraint is not supported in URL generation. IRouteConstraint is supported.

Instead of a route per language against an action, have you considered having a single route with the language as a route parameter? This post on StackOverflow goes into setting this up in detail.

BoasE commented 6 years ago

@JamesNK the topic is that the page we are migrating already has a url pattern without the culture in the parameter. changing this would break existing very well google ranked routes. So this is not an option.

The culture is only defined by the domain.

We found a solution by manually creating routes which seems to work , but it feels a little like reeinventing the wheel an d my expectation was that asp core has a easy way to manage this. Especially my assumption was that Url.Action only generates valid urls which is not given in this scenario

JamesNK commented 6 years ago

Url.Action is returning a valid URL for one of its registered routes, and the additional route data is appended to the query string.

Do you have anymore questions or could this issue be closed?

BoasE commented 6 years ago

I think you can close it. but i want to point out that it isn't returning a valid url. as the url that it creates can't be called du to the constraints and therefore results in a errorcode.

But I understand its workin the way it is designed for.

JamesNK commented 6 years ago

You are mixing together route matching with MVC resolving the route to an action. IActionConstraint is used by MVC to resolve a matched route to an action. This step doesn't apply when generating a link so IActionConstraint is not called.

What you want to use is IRouteConstraint.

cypressious commented 6 years ago

@JamesNK but that's exactly the point, there's no way to add an IRouteConstraint using IApplicationModelConvention

BoasE commented 5 years ago

@JamesNK do you have an example on how to add RouteConstraints instead of action constraints?

BoasE commented 5 years ago

Any news? @JamesNK

JamesNK commented 5 years ago

I linked one here - https://github.com/aspnet/Routing/issues/896#issuecomment-436156772