Skip to content

IUrlHelper.Action() inexplicably takes callsite action context into account for determining target action. #52665

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

Open
1 task done
StringEpsilon opened this issue Dec 8, 2023 · 3 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Milestone

Comments

@StringEpsilon
Copy link

StringEpsilon commented Dec 8, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

In a controller action that has an overload with an additional parameter, redirecting from that overloaded implementation to the default action is impossible using Controller.Url.Action().

A small reproduction can be seen in this repository: https://github.com/StringEpsilon/route_bug

Within the Do(string, Mode) method, any attempt to create an URL to Do(string) will result instead in a link to Do(string, Mode). In this particular scenario that results in an infinite loop of redirects.

Expected Behavior

The default IUrlHelper implementation should not pull route parameter information from the current ActionContext to distinguish between method overloads on the same controller. In other words the following call should always produce the same URL, regardless of which method makes that call:

this.Url.Action(nameof(Do), new {templateId = "SomeTemplate"});
// should always equal: /do/SomeTemplate
// but will return: /do/SomeTemplate/<mode> when called from Do(string, Mode)

Steps To Reproduce

See this small reproduction repository: https://github.com/StringEpsilon/route_bug

Build & run the project, then click on the link that is rendered by the "Do" view that says "Do with mode". That will trigger the redirect loop described above.

Exceptions (if any)

No response

.NET Version

8.0.100

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Dec 8, 2023
@StringEpsilon StringEpsilon changed the title IUrlHelper.Action() inexplicablytakes callsite action context into account for determining target action. IUrlHelper.Action() inexplicably takes callsite action context into account for determining target action. Dec 8, 2023
@StringEpsilon
Copy link
Author

StringEpsilon commented Dec 15, 2023

One workaround for this issue is to manually remove offending values from the route context:

[Route("do/{templateId}/{mode}")]
public IActionResult Do(string templateId, Mode mode)
{
    var wrongUrl  = this.CreateUrl(templateId); // creates the wrong URL, see reproduction repo.
    this.ControllerContext.RouteData.Values.Remove("mode");
    var redirectUrl = this.CreateUrl(templateId); // now creates the correct url.
    return Redirect(redirectUrl);
}

But this may have side effects I am not aware of.

Edit: One can also re-add the route value back after making the offending IUrlHelper.Action() call.

@captainsafia
Copy link
Member

@StringEpsilon Thanks for reporting this issue! It looks like this is related to the meta-issue that we have opened on link generation over at #38121. Admittedly, there hasn't been much work in this space due to prioritization but a PR resolving this issue would be welcome.

In this particular case, it looks like we always construct a URL helper given the current controller context (ref). I suspect the issue might be in how RouteValues are normalized here.

@captainsafia captainsafia added this to the Backlog milestone Dec 19, 2023
@ghost
Copy link

ghost commented Dec 19, 2023

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.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

No branches or pull requests

3 participants