Skip to content
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

Inconsistent UrlHelper behavior when two different endpoints have the same action name and use [Admin("url template")] #16213

Closed
sarahelsaig opened this issue Jun 2, 2024 · 4 comments · Fixed by #16217
Labels
Milestone

Comments

@sarahelsaig
Copy link
Contributor

Describe the bug

I found inconsistent behavior when your module has two different controllers with matching action names (one with default routing and one with custom routing). Manually typing in the expected route works, but if you use IUrlHelper.Action() or LinkGenerator then it generates the path using the /{area}/{controller}/{action}/{id?} route instead of the expected custom one.

We found this in a Lombiq.OSOCE UI test with the following failure report:

❌ RecipeDataShouldBeDisplayedCorrectly
	Shouldly.ShouldAssertException : context
	            .GetAll(bySubMenu)
	            .Select(element => new Uri(element.GetAttribute("href")).PathAndQuery)
	            .ToArray()
	    should be
	["/Lombiq.DataTables.Samples/Sample/DataTableTagHelper", "/Lombiq.DataTables.Samples/Sample/ProviderWithShape", "/Admin/DataTable/SampleJsonResultDataTableDataProvider?paging=true&viewAction=false", "/Admin/DataTable/SampleIndexBasedDataTableDataProvider?paging=true&viewAction=false"]
	    but was (case sensitive comparison)
	["/Lombiq.DataTables.Samples/Sample/DataTableTagHelper", "/Lombiq.DataTables.Samples/Sample/ProviderWithShape", "/Lombiq.DataTables/Table/Get?providerName=SampleJsonResultDataTableDataProvider&paging=true&viewAction=false", "/Lombiq.DataTables/Table/Get?providerName=SampleIndexBasedDataTableDataProvider&paging=true&viewAction=false"]
	    difference
	["/Lombiq.DataTables.Samples/Sample/DataTableTagHelper", "/Lombiq.DataTables.Samples/Sample/ProviderWithShape", *"/Lombiq.DataTables/Table/Get?providerName=SampleJsonResultDataTableDataProvider&paging=true&viewAction=false"*, *"/Lombiq.DataTables/Table/Get?providerName=SampleIndexBasedDataTableDataProvider&paging=true&viewAction=false"*]

To Reproduce

Steps to reproduce the behavior:

  1. Create a module, e.g. DataTable.csproj
  2. Create a controller with an action, e.g. ChildRowsController.Get()
  3. Create another controller with the same action name, e.g.TableController.Get()
  4. Add a custom routing attribute to the second one, e.g.
    [Admin("DataTable/{providerName}/{queryId?}")]
    public async Task<IActionResult> Get(string providerName, string queryId) { ... }
  5. Create a cshtml with @Url.Action("Get", "Table", { area = "DataTable", providerName = "foo", queryId = "bar" })
  6. Observe that it displays /DataTable/Table/Get?providerName=foo&queryId=bar instead of /DataTable/foo/bar.
    Note that this outcome only happens if the controller with the default path config is resolved first. I think something has changed this resolution order, because I didn't encounter this problem in 1.8.x and it manifested when I upgraded from 2.0.0-preview-18200 to 2.0.0-preview-18226.

Expected behavior

Automatic IAreaControllerRouteMapper route maps should only affect their own controller.

Suggestion

DefaultAreaControllerRouteMapper and AdminAreaControllerRouteMapper should substitute {controller} too, not just {action}. See this example.

Screenshots

This shows the routes.DataSources.First()._routes private field, which contains the routes registered when using the routes.MapAreaControllerRoute() extension method. As you can see these overly generic default routes are taking up priority.
image

@Piedone Piedone changed the title Inconsistent UrlHelper behavior when two different endpoints have the same action name. Inconsistent UrlHelper behavior when two different endpoints have the same action name and use [Admin("url template")] Jun 2, 2024
@Piedone Piedone added this to the 2.0 milestone Jun 2, 2024
Copy link

github-actions bot commented Jun 2, 2024

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@Piedone
Copy link
Member

Piedone commented Jun 2, 2024

This is a regression. To confirm: does this only happen with [Admin] or any other custom routing ([Route] or routes declared in the Startup) too?

This has a chance to be related to #15793. @gvkries do you perhaps have an idea if this is plausible?

In any case, the suggested fixes look good, though I'd like to understand why this got broken (or rather, how did it work before).

@sarahelsaig
Copy link
Contributor Author

To confirm: does this only happen with [Admin] or any other custom routing ([Route] or routes declared in the Startup) too?

This applies to any Startup route too. I haven't tested [Route] yet.

I'd like to understand why this got broken

The problem is that the route definition in DefaultAreaControllerRouteMapper ("/{area}/{controller}/{action}/{id?}".Replace("{action}", descriptor.ActionName)) is overly broad. The routes.MapAreaControllerRoute() extension method automatically constrains the accepted values of area and the action is substituted in the pattern, but the controller is left as a wildcard for no reason. Effectively it creates the route /MyArea/*/MyAction. Route definitions are prioritized by order of registration, so any subsequent routes are superseded by this accidental wildcard.

or rather, how did it work before

By accident. I think a recent change (likely the one you linked) revealed this already existing problem. Note that this is an extremely rare edge case. It only happens if you have two controllers in the same module with overlapping action names. Even then only if some have custom routing instead of none or all.

@gvkries
Copy link
Contributor

gvkries commented Jun 3, 2024

I can't confirm or disagree that it's caused by #15793. I'm seeing the same behavior before and after this change, but I don't see any other PR that could have caused this behavior change in the specified time period. Maybe I did something wrong when reproducing the issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants