Skip to content

Generating URLs for a different action on the same controller reuses ambient values #18400

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
danports opened this issue Jan 17, 2020 · 8 comments
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-routing severity-minor This label is used by an internal tool
Milestone

Comments

@danports
Copy link

The documentation states that "In ASP.NET Core 2.2 or later, both [conventional and attribute] routing invalidate [ambient] values when linking to another action." However, this only seems to be the case for actions on another controller. I am unsure whether this is a bug or the expected behavior with unclear or incorrect documentation.

Repro: Run this sample project, click the Orders tab in the navigation bar, select an order from the list, and then observe that the URL for the Orders tab in the navigation bar as generated on the order details page is /Orders/5/Index rather than the expected /Orders (since the documentation says the ambient value for id shouldn't be reused since this is a different action). Note also that the Home and Privacy navigation links on the order details page do not reuse ambient values and do not include id in their URLs.

@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 17, 2020
@javiercn
Copy link
Member

@danports thanks for contacting us.

endpoints.MapControllerRoute("itemRoute", "{controller}/{id:int}/{action=Details}");
endpoints.MapControllerRoute("default", "{controller=Home}/{action=Index}/{id?}");
<li><a asp-action="Details" asp-route-id="@i">@i</a></li>

I think this has to do with the fact that you are specifying the details. If you remove the asp-action="Details" it should give you the right outcome. Is that not the case?

@danports
Copy link
Author

Removing asp-action="Details" breaks the links from the order list page to the details page - instead of generating /Orders/4, it generates /Orders/4/Index. And if you do navigate directly to the details page, the link back to the orders list in the navigation bar is still wrong.

@mkArtakMSFT
Copy link
Contributor

@rynowak can you please look into this? Thanks!

@danports
Copy link
Author

danports commented Mar 5, 2020

Any updates on this? Would be helpful to know if what I'm seeing is the expected/desired behavior or a bug.

@danports
Copy link
Author

Alright, I dug through the code until I found the bits that do ambient value invalidation and I think the code is behaving as designed, though the documentation for ambient value invalidation is a bit confusing. I think the intent of the algorithm is to keep things DRY by avoiding the need to specify every route value for every link, which means that cases like this where you don't want ambient values to be inherited require a bit more work. Including asp-route-id="@null" in the Orders link in the layout page in the sample I created earlier does cause the correct URL to be generated on the order details page. So I can deal with this by adding that attribute to any link where the target action does not expect an id parameter.

To sum up then, a couple of suggestions:

  1. Improve the documentation for ambient value invalidation, e.g. by explaining how the left to right matching works and showing a few more nontrivial examples, including some with non-default route patterns and one where an ambient value is overridden with null, which was the workaround I needed but didn't see until I read the framework code.
  2. Make endpoint routing smarter: Not sure how feasible this is, but it would be nice if the framework knew that the Index action didn't take an id parameter and preferred to generate links for that action using routes that didn't have an id parameter.

@mkArtakMSFT mkArtakMSFT modified the milestones: 5.0.0-preview8, 5.0.0-rc1 Jul 22, 2020
@pranavkm pranavkm added bug This issue describes a behavior which is not expected - a bug. and removed investigate labels Aug 19, 2020
@ghost
Copy link

ghost commented Aug 28, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@javiercn javiercn added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Oct 9, 2020
@ghost
Copy link

ghost commented Oct 9, 2020

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.

@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, .NET 8 Planning Oct 12, 2022
@ghost
Copy link

ghost commented Oct 12, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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.

@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
@javiercn javiercn removed their assignment Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates bug This issue describes a behavior which is not expected - a bug. feature-routing severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

6 participants