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.49k stars 10.03k forks source link

Url generate problem #24288

Open John0King opened 4 years ago

John0King commented 4 years ago

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 https://github.com/aspnet/Mvc/issues/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

John0King commented 4 years ago

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

ghost commented 4 years ago

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.

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.

John0King commented 3 years ago

@SteveSandersonMS

a simple fix can be done here that add area to normalize path. https://github.com/dotnet/aspnetcore/blob/7e5588b1bc7c66f67f4ad540c7fbef50a596e057/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs#L307-L339


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

John0King commented 3 years ago

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

Gregory-Lange commented 9 months ago

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?}"
);