Skip to content

MapDynamicControllerRoute doesn't work with DefaultLinkGenerator #16965

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
ollieferns opened this issue Nov 11, 2019 · 17 comments
Open

MapDynamicControllerRoute doesn't work with DefaultLinkGenerator #16965

ollieferns opened this issue Nov 11, 2019 · 17 comments
Labels
affected-medium This issue impacts approximately half of our customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing needs-prototype Priority:2 Work that is important, but not critical for the release severity-blocking This label is used by an internal tool
Milestone

Comments

@ollieferns
Copy link

ollieferns commented Nov 11, 2019

Describe the bug

The DefaultLink Generator (and the AnchorTagHelper) do not work when mapping dynamic routes. They do not find any routes and produce a null href.

To Reproduce

  1. create a vanilla mvc site
  2. add one dynamic route
  3. use the anchor tag helper or the LinkGen class
  4. Result is href is null.

see v simple example here.

https://github.com/ollieferns/dynamiclinkgenissue

Further technical details

  • ASP.NET Core version 3.1 (preview)
    .NET Core SDK (reflecting any global.json):
    Version: 3.1.100-preview2-014569
    Commit: 4bd5d24d87

Runtime Environment:
OS Name: Mac OS X
OS Version: 10.15
OS Platform: Darwin
RID: osx.10.15-x64
Base Path: /usr/local/share/dotnet/sdk/3.1.100-preview2-014569/

Host (useful for support):
Version: 3.1.0-preview2.19525.6
Commit: 5672978d91

.NET Core SDKs installed:
3.0.100 [/usr/local/share/dotnet/sdk]
3.1.100-preview1-014459 [/usr/local/share/dotnet/sdk]
3.1.100-preview2-014569 [/usr/local/share/dotnet/sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.App 3.0.0 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.0-preview1.19508.20 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.0-preview2.19528.8 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.13 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.0 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.0-preview1.19506.1 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.0-preview2.19525.6 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

@javiercn javiercn added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates investigate labels Nov 11, 2019
@javiercn
Copy link
Member

@ollieferns thanks for contacting us.

@rynowak can you look into this?

@oferns
Copy link

oferns commented Nov 13, 2019

@javiercn @rynowak I cloned the source and had a look. The issue appears to me to be in RouteValuesAddressScheme around line 104

var metadata = endpoint.Metadata.GetMetadata<IRouteNameMetadata>();

Because the endpoint is a IDynamicEndpointMetadata not a IRouteNameMetadata it doesnt consider the route. I made IDynamicEndpointMetadata inherit from IRouteNameMetadata as a fix. It fixed my issue but made 3 tests fail. If I get to the bottom of the issue, how would I go about submitting a PR? I would rather like this fixed. In the meantime, what is the best way of referencing my altered MVC assemblies instead of the default ones for my own project?

@oferns
Copy link

oferns commented Nov 15, 2019

Hi, any chance of a quick answer to this while the issue is being looked at?
" In the meantime, what is the best way of referencing my altered MVC assemblies instead of the default ones for my own project?"

Is there an easy way to do this? Or do I need to add the MVC projects to my solution?

Thanks,
O

@ollieferns
Copy link
Author

@rynowak @javiercn could I get a response to my questions at least if there is no progress? thanks

@ollieferns
Copy link
Author

@rynowak @javiercn could somebody reach out to me please regarding this issue. It seems to me to be a fairly serious issue with endpoint routing. Happy to discuss privately if required but the total radio silence is a bit unnerving when trying to convince my powers-that-be that .net core and aspnet is a viable technology choice for what we are trying to do here.

@oferns
Copy link

oferns commented Mar 17, 2020

So as an update this still does not work in .NET 5.01 Preview. How depressing. No answers to my questions for 4+months and no fix.

@MaxHorstmann
Copy link
Contributor

Same issue here, see Stack Overflow. Is there an update? Would a PR for this be considered?

@mkArtakMSFT
Copy link
Member

Thanks for suggesting help, @MaxHorstmann.
It's not clear at the moment what the fix would look like here as we haven't yet investigated this.
@javiercn do you have something on mind about this for @MaxHorstmann to consider?

@JeremyCaney
Copy link

I’m really glad to hear that this is being evaluated for .NET 5.0.0. In the meanwhile, at minimum, it’d be useful to acknowledge this limitation in the documentation. It took me a bit to isolate this behavior and discover this issue.

(If I have a chance, I can look into submitting a PR for the documentation—though I’ll need to investigate the conventions for documenting feature limitations, as I’m not clear how that’s generally handled.)

JeremyCaney added a commit to OnTopicCMS/OnTopic-Library that referenced this issue Aug 17, 2020
The existing `MapTopicAreaRoute()` extension method recently introduced (6c6ad74) requires, at minimum, an `areaName` parameter, and thus is limited to setting up routes for one individual area at a time.

This extension doesn't (currently) accept any parameters, and will instead map _all_ areas to support the `{**path}` catch-all expected by the OnTopic CMS. It does this by implementing the new `TopicRouteValueTransformer` (865f26a) to automatically set the `controller` and the `rootTopic` routing variables to the `area` name, which is the convention we most frequently follow.

As part of this, I also registered `TopicRouteValueTransformer` with ASP.NET Core dependent injection, as part of the `AddTopicSupport()` extension.

Be aware that this implementation trips of a feature limitation of ASP.NET Core 3.0 which prevents e.g. `@Url.Action()` references from correctly returning values (see dotnet/aspnetcore#16965). Hopefully that will be resolved in ASP.NET 4.0.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Library that referenced this issue Aug 17, 2020
…l areas

The existing `MapImplictAreaControllerRoute()` extension method recently introduced (8a65c95) requires, at minimum, an `areaName` parameter, and thus is limited to setting up routes for one individual area at a time.

This extension doesn't (currently) accept any parameters, and will instead map _all_ areas. It does this by implementing the new `TopicRouteValueTransformer` (865f26a) to automatically set the `controller` routing variable to the `area` name, which is the convention we most frequently follow.

Be aware that, as with the recently introduced `MapTopicAreaRoute()` (762d229), this implementation trips of a feature limitation of ASP.NET Core 3.0 which prevents e.g. `@Url.Action()` references from correctly returning values (see dotnet/aspnetcore#16965). Hopefully that will be resolved in ASP.NET 4.0.
@javiercn javiercn added affected-medium This issue impacts approximately half of our customers severity-blocking This label is used by an internal tool enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Oct 9, 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.

@enricoe73
Copy link

enricoe73 commented Jul 5, 2021

Hello, any news on this ? It's not woking also for razor pages with
endpoints.MapDynamicPageRoute and the tag helper ( asp-page-handler attribute)
@mkArtakMSFT

@ghost
Copy link

ghost commented Aug 10, 2021

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.

@oferns
Copy link

oferns commented Aug 10, 2021

(facepalm)

@thilap-web
Copy link

any workarounds on this ?

@rjgotten
Copy link

rjgotten commented Nov 8, 2021

any workarounds on this ?

You have to glue your own link generation into things. To make it work you have to overwrite the default registered address schemes used by the link generator with versions that can handle whatever kind of link generation you need. GetPathByAction is implemented as an extension method on top of GetPathByAddress so you can't override it with a custom implementation directly, you have to go through the whole address scheme pipeline instead.

(I've written it myself before, but it's closed source company code. Having to write it that way blows, to say the least.)

Note also that the whole URL generation pipeline running through LinkGenerator and the address schemes is synchronous through-and-through. So if you need to pull data from a database at any one point -- which is not entirely unthinkable for data-driven localized URL generation -- you have to settle for blocking operations.

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

@mkArtakMSFT mkArtakMSFT added needs-prototype Priority:2 Work that is important, but not critical for the release and removed investigate labels Nov 15, 2022
@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-medium This issue impacts approximately half of our customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing needs-prototype Priority:2 Work that is important, but not critical for the release severity-blocking This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests