Skip to content

Url generate problem #24288

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
John0King opened this issue Jul 24, 2020 · 7 comments
Open

Url generate problem #24288

John0King opened this issue Jul 24, 2020 · 7 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 investigate severity-minor This label is used by an internal tool
Milestone

Comments

@John0King
Copy link
Contributor

John0King commented Jul 24, 2020

this is not a bug, but it should be, and if you guys still think it should be the designed behaviors please make a vote here

Describe the issue

this is my second time to report this aspnet/Mvc#7046, previously I think it may be fixed by the Endpoint-Routing, but it doesn't.

we create the MVC which we try to separate the View and Controller , and they both follow the contract which is the Model,
but this issue cause the View must know about the controller/action and it's routing metadata.

think follow controller:

// [Area(null)]   //no area
[HttpGet("/[controller]/{a:int}-{b:int}")] // controller "blog" with attribute routing
[HttpGet("/[controller]/{a:int}-0")]
public IActionResult Index(int a, int b, int c)
{
    return view();
}

[Area("foo")] // controller "blog" with `.MapControllerRoute(template)`
public IActionResult Index(int a, int b, int c, int d)
{
    return view();
}

now when you routing in the view, you must know who is the controller and where is it, if you don't, then it's wrong

<a asp-action="Index" asp-controller="Blog" asp-route-a="1" >this link is wrong </a>

because if you in area "foo" , it not point to foo area, instead it point to area null so it will be /blog/1-0 ,
if you in area "null" , then it also point to /blog/1-0 , but if you in /blog/5-3 it will be /blog/1-3 so how can your view know where it is ? so, to correct the link you need to write

<a asp-action="Index" asp-controller="Blog" asp-route-a="1" asp-route-b="null" > 
    the view must know your controller add a `b` route variable 
</a>

To Reproduce

https://github.com/John0King/UrlGenTest it's the old repo , but it explain everything

what cause it

  1. Attribute routing has higher priority, it's now all about endpoint so why it still has difference (eg. with tempalte {controller=Home}/{action=Index}/{id?} map to "HomeController.Index" with [Route("/")][Route("[controller]/[action]/["id"]")]) others with [Route("[controller]/[action]/{id?}")] (the endpoint route-data) on the action , but this one is not the main issue, it the attribute routing with higher priority is OK for this issue, it's jut will some api override the MVC 's path (because api normally we hard code it's api path like /api/foo/getlist instead of generate it )
  2. main issue route data not specific what data will be keep and what data will be drop, current it depend on current route and current endpoint , it cause our view must know about the action's routing data

how to fix

the Attribute-Routing already give us answer to know what kind of routing data need to keep, for example [controller]/[action]/{id} the controller and action will be keep and id will be drop for [area]/[controller]/[action]/{a}/{b} the a and b will be drop, the route value with [ ] is called framework route variables, when you write the view, the view must know those variables but it do not know the route variables with { }, here is the framework route variables https://github.com/John0King/UrlGenTest/blob/8f4ad7aa6ae9c5552845a0fd27100149c63e00b1/Views/Shared/NewsTest.cshtml#L91-L96

we need to let the IUrlHelper to know the ActionContext.ActionDescriptor.RouteValues and keep all the route value if we do not override it we can use string.Empty to override the empty "area" route data

is there a break change after fix

theoretically yes, but actually no. because if your app has correct link , then the link must be correct : for example
case 1:
Non-area has attribute route, and area has conventional route , you must add area so the link in area is correct, and if someone want to goto non-area, then the empty area must be set so it can make conventional route to conventional route to work.

case 2:
from area to non-area with null , it won't work even now, because null is the default value of RouteContext, so it must use string.Empty , and it will work after the change too.

case 3:
same page route like {a}-{b}, to make link link correct , you must specific both a and b to make the link corrent, otherwise even developer can not know what link it will be eg for route {a}-0 / 0-{b} / {a}-{b} , if you ignore a then click a link with b=1 then link maybe /0-1 or 22-1 or {any}-1 so no one will ignore any of them

Further technical details

  • ASP.NET Core version: 3.1.4
@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 24, 2020
@mkArtakMSFT mkArtakMSFT added this to the 5.0.0-rc1 milestone Jul 27, 2020
@John0King
Copy link
Contributor Author

John0King commented Aug 2, 2020

a note for IUrlHelper.Action() the no parameters method must not to drop any route variables.
for example :

// we are in url    /blog/post/33?cate=dog&water=milk

@Url.Action()

output :  /blog/post/33?cate=dot&water=milk

but

@Url.Action(new { id = 32 }) 
output:   /blog/post/32  ,  this is actually be a   "wrong" link, 
because it drop "cate" and "milk" ,  write this meaning 
we want to drop the  "cate" and "water"  
previously we need to write @Url.Action(new {  id= 32, cate="", water = "" })

this will effect

  • IUrlHelper
  • TagHelper for a, form

@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.

@SteveSandersonMS SteveSandersonMS added affected-few This issue impacts only small number of customers bug This issue describes a behavior which is not expected - a bug. severity-minor This label is used by an internal tool labels Oct 7, 2020 — with ASP.NET Core Issue Ranking
@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.

@John0King
Copy link
Contributor Author

@SteveSandersonMS

a simple fix can be done here that add area to normalize path.

internal static void NormalizeRouteValuesForAction(
string? action,
string? controller,
RouteValueDictionary values,
RouteValueDictionary? ambientValues)
{
object? obj = null;
if (action == null)
{
if (!values.ContainsKey("action") &&
(ambientValues?.TryGetValue("action", out obj) ?? false))
{
values["action"] = obj;
}
}
else
{
values["action"] = action;
}
if (controller == null)
{
if (!values.ContainsKey("controller") &&
(ambientValues?.TryGetValue("controller", out obj) ?? false))
{
values["controller"] = obj;
}
}
else
{
values["controller"] = controller;
}
}


and razor page take page in the route value , that cause

  1. razor page can not use page as querystring
  2. mvc using page may cause error
  3. page as querystring is very very very common, even github use it,

and almost all current route value name is very common in querystring

  • action : it may be used as a QueryString in one mvc action , and controll the different api action
  • controller : it may be used as a QueryString
  • area : well , "region" and "area"
  • page : describe above, this will be using as pagination

we really need to find a way to change all the good name to some uncommon name .
an idea is to use an uncommon prefix such like __$action , __$controller , __$area
and the Url.Action(new ActionRouteContext{ Action="index", Controller = "Home" }) will eventually map to __$action=Index ,__$controller=Home

@John0King
Copy link
Contributor Author

John0King commented Jun 14, 2021

@mkArtakMSFT this issue exists so long , Will it be fix in Asp.NetCore 6 ?

//frontend   --- NewController

[HttpGet("/News")]
[HttpGet("/News/c-{cId:int}")]
public IActionResult Index(int cId){}


// manage end

[Area("Admin")]
public class NewsController:Controller
{

//  the url is  "/Admin/News/Index"   by conventional routing
   public IActionResult  Index()
}

and we you in "Admin" Area, and use <a asp-action="Index" asp-controller="News">Manage News</a>
then you will get /News instead of /Admin/News.

@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. 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.

@Gregory-Lange
Copy link

I am having an issue with Url.Action creating incorrect links.

@Url.action("Index", "UserAdmin", new { Area="Admin" });

Creates /Admin?controller=UserAdmin this is wrong with how MVC routes this should be /Admin/UserAdmin as it has done in all other version of .net. Also the system acts like that route doesn't exist when i put it in by hand to the correct url even though i have done a MapControllerRoute()

app.MapControllerRoute(
name: "areas",
pattern: "{area:exists}/{controller=Home}/{action=Index}/{id?}"
);

@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 investigate severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

7 participants