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

parameters for current request injected into nodes where the action has the same parameter name #213

Closed
awrigley opened this issue Aug 22, 2013 · 15 comments

Comments

@awrigley
Copy link

I have found that if an action has one or more parameters with the same name(s) as one or more of the parameters of the current request, then the value of the current request parameters are used in the method, unless the configuration of the node EXPLICITLY uses a different value.

This is an anomalous and unexpected behaviour. There is no reason for the values of the current request parameters to have any relevance to any node other than the current node.

For example, if the current request calls an action with an order and a size parameter, with respective values of 7 and 204, then the menu item for another node where the action ALSO has parameters order and size AND the configuration of the node DOES NOT specifiy values for these parameters, then the rendered menu item will use the values of order and size of 7 and 204 respectively.

Ie:

Current node url:

mysite.com/SolarSystem/Mercury/7/204

Other node for an action with order and size parameters (that are unspecified in the configuration of the node) will render as:

mysite.com/LocalCluster/MilkyWay/7/204

This seems to me WRONG and totally anomalous.

The correct url for the "other node" would be:

mysite.com/LocalCluster/MilkyWay

There is no reason for the other node to render the values for the current request.

I believe that the correct way would be to ignore the parameters and ONLY use the values EXPLICITLY stated in the configuration of the node. The current situation is that, UNLESS the values are explicitly stated, then the values of the current request are used.

Has to be wrong.

Seems to me, anyway.

@NightOwl888
Copy link
Collaborator

Hmm, when you posted your original question on StackOverflow, I assumed you were talking about the node matching behavior. Now it sounds more like you are talking about the URL resolver. The URL resolver gets the route values from the current node, not the current request. And by default the values are not copied from the current request to the node (not even when using action method parameters).

There are 3 ways to configure MvcSiteMapProvider to copy values from the current request to the current node:

  1. Use the preservedRouteParameters attribute/property of the node
  2. Use the SiteMapPreserveRouteDataAttribute on the action method
  3. Set the route values in code using something like MvcSiteMapProvider.SiteMaps.Current.CurrentNode.RouteValues.Add("id", value)

I would suggest to check to ensure you don't have any SiteMapPreserveRouteData attributes declared on your action methods, as that is a brute force way to copy the values from the current request to all of the nodes. Also, double check whether you are doing one of the other 2 methods for preserving route parameters from the current request.

Honestly, I am still learning how the node matching features work and so far I haven't thought of a single use case where SiteMapPreserveRouteData would actually be useful. Totally anomalous is a good description of the way it behaves. In fact, when I was refactoring I suggested leaving it out of the new version.

The best way I have found to configure MvcSiteMapProvider is to ensure that there is a unique node for every one of the possible action method parameter values (or combinations of values). This not only makes the matching work right, but it ensures you don't have to worry about the action parameters being "forgotten" between requests, and also ensures that all of your pages will be included in the /sitemap.xml endpoint for search engines. Furthermore, it is how the Microsoft implementation that this is still heavily based on is meant to work.

All nodes can be added from an external data source through the use of Dynamic Node Providers, and if the number of nodes gets out of hand, you can even extend the cache to write to the file system. There are few reasons not to take this road.

Areas

I also didn't notice the first time around that you are using areas. The documentation hasn't been updated yet to reflect this, but there was only one configuration I could find where areas would work right. Have a look at this answer to ensure your areas (and routes for areas) are defined correctly.

Your Case

I am not ruling out that there is a bug just yet, but let's make sure your configuration is set up right before going down that road.

If you check the above and are still having issues, please build a small demo project that exhibits the anomalous behavior and either post it on GitHub or zip it and make it available for download. Discovering where the problem lies is extremely difficult without working with the exact same configuration. Not to mention, sometimes the act of building a demo can reveal problems with the configuration.

@NightOwl888
Copy link
Collaborator

Update

In v4.3.0, SiteMapPreserveRouteDataAttribute has been deprecated. I have also added a post to my blog (with working demos) that goes into detail about the node matching process and how and when to use preservedRouteParameters.

http://www.shiningtreasures.com/post/2013/09/02/how-to-make-mvcsitemapprovider-remember-a-user-position

If you get a chance, I would appreciate you having another look at your configuration. What you describe is happening doesn't sound like the default behavior. But if you can reproduce it and build a demo project, I would be happy to take a look at it to try to determine if there is a bug or it is misconfigured in some way (and add an exception so the misconfiguration is no longer allowed).

@awrigley
Copy link
Author

I will do that, however, I am travelling at the moment, for the next month. 
 
When I get back to the UK, I will look at it all again.
 
Many thanks for your concern.
 
Regards
 
Andrew

Andrew Wrigley
[email protected]


From: NightOwl888 [email protected]
To: maartenba/MvcSiteMapProvider [email protected]
Cc: awrigley [email protected]
Sent: Friday, September 13, 2013 10:11 AM
Subject: Re: [MvcSiteMapProvider] parameters for current request injected into nodes where the action has the same parameter name (#213)

Update
In v4.3.0, SiteMapPreserveRouteDataAttribute has been deprecated. I have also added a post to my blog (with working demos) that goes into detail about the node matching process and how and when to use preservedRouteParameters.
http://www.shiningtreasures.com/post/2013/09/02/how-to-make-mvcsitemapprovider-remember-a-user-position
If you get a chance, I would appreciate you having another look at your configuration. What you describe is happening doesn't sound like the default behavior. But if you can reproduce it and build a demo project, I would be happy to take a look at it to try to determine if there is a bug or it is misconfigured in some way (and add an exception so the misconfiguration is no longer allowed).

Reply to this email directly or view it on GitHub.

@wwuck
Copy link

wwuck commented Oct 16, 2013

I just upgraded from v3 to v4 and I think I have the same problem. I put together a small test site at https://github.com/tomascassidy/MvcSiteMapProviderTest that demonstrates this.

E.g. if I navigate to http://localhost:49429/Home/A on the test site then the A without ID link will correctly point to http://localhost:49429/Home/A. If I navigate to http://localhost:49429/Home/A/123 then the A without ID link will incorrectly point to http://localhost:49429/Home/A/123, despite the link definition being @Html.ActionLink("A without ID", "A") without any "id" parameter specified.

@NightOwl888
Copy link
Collaborator

I did another experiment here: https://github.com/NightOwl888/MvcSiteMapTest, removing MvcSiteMapProvider from the project altogether. The behavior you mentioned still exists.

If this is a bug (and I am not quite certain it is), it is happening because of some behavior in MVC, not MvcSiteMapProvider. I suggest you report this to the MVC team (and darn quickly, since MVC 5 is just around the corner from being released).

@wwuck
Copy link

wwuck commented Oct 17, 2013

Reported to the MVC developers at https://aspnetwebstack.codeplex.com/workitem/1346

@wwuck
Copy link

wwuck commented Oct 28, 2013

Looks like my bug test case is actually a "feature" in MVC. See the reply on the workitem at https://aspnetwebstack.codeplex.com/workitem/1346 and the SO answer for https://aspnetwebstack.codeplex.com/workitem/1346.

I still think there's something to fix in MvcSiteMapProvider v4 as I see this behaviour happening when I generate a menu using @Html.MvcSiteMap().Menu(startingNodeLevel: 0, maxDepth: 2, showStartingNode: true, startingNodeInChildLevel: true)
Whether you see that as a "feature" or a "problem" for MvcSiteMapProvider is up to you.

I didn't see this happening with MvcSiteMapProvider v3 because I was doing my own DIY menu based off the System.Web.SiteMap class. I would rather use/customise the built-in DisplayTemplates that come with MvcSiteMapProvider than have to create my own menu templates to fix this.

@wwuck
Copy link

wwuck commented Nov 27, 2013

I just tried again to upgrade from MvcSiteMapProvider v3 to v4 without success, as this problem is still occurring. I tried to look at the MvcSiteMapProvider source code to see where it was generating the Urls but just got hopelessly lost 😕

I also tried adding "id" to the "MvcSiteMapProvider_AttributesToIgnore" app setting in web.config without success.

I can't use any of the workarounds specified in the MVC bug report, because I don't see any way to control how MvcSiteMapProvider generates the Urls internally for SiteMapNodeModel or the other template helpers.

@NightOwl888
Copy link
Collaborator

The URLs are generated in the SiteMapNodeUrlResolver class. Just like the rest of MVC, we use the UrlHelper class to generate the URLs.

By default the URLs are generated at the time the SiteMap is constructed and then stored in the shared cache. If you have user session values in your routes, then you can disable this caching by setting "cacheResolvedUrl" to "false" at the node level or by setting "MvcSiteMapProvider_EnableResolvedUrlCaching" to "false" in web.config/appSettings. The net effect of disabling this caching is that the URLs will be resolved on demand (much like the rest of MVC).

I did a lot of experimenting with MvcSiteMapProvider and found 2 ways to configure it that make sense.

  1. Adding every RouteValue combination as a unique node using IDynamicNodeProvider or ISiteMapNodeProvider.
  2. Creating a small set of nodes that match every potential custom RouteValue (such as "id) by setting the preservedRouteParameters value to the names of those parameters (i.e. preservedRouteParameters="id"). This will force it to match every "id".

I went into some depth on my findings at How to Make MvcSiteMapProvider Remember a User's Position. If you are using a configuration that is unusual, you may need to tweak your configuration to get it to work (or it may simply not be supported). It might help if you post your routes, nodes, and appSettings configuration here to see if we can spot the problem.

The "MvcSiteMapProvider_AttributesToIgnore" setting is to specify custom attributes without having them automatically added to the RouteValues dictionary. Typically, you would not want to exclude your "id" parameter from RouteValues.

@wwuck
Copy link

wwuck commented Nov 28, 2013

I read that blog post and now I'm thinking that I am "doing it wrong", but I'm not quite sure what is the correct way to achieve this. I've updated the test project at https://github.com/tomascassidy/MvcSiteMapProviderTest to show how my project is structured.

There are a few Actions/Views that are accessed with an optional Id parameter when clicking an in-page link from another view, but are accessed without the Id parameter from the main menu (the one generated by the HTML Helpers/display templates).

Eg.
The menu has links for /Contact/Contact, /Contact/Contact2, /Contact/Contact3. One of the other pages (/About/About2) has a link to /Contact/Contact/4.

If I access /Contact/Contact via the auto-generated menu, then all is good and the menu links point to the correct action methods. But if I access /Contact/Contact/4 via the link on /About/About2 then all the menu links pointing to action methods with optional Id parameter will also contain the value 4 in their URLs.

My question is: how do I tell MvcSiteMapProvider's @Html.MvcSiteMap() helper to ignore the route values when generating the URLs? Does my problem exist because I'm using nullable int (optional) parameters for some of the action methods?

@NightOwl888
Copy link
Collaborator

I was able to achieve what (I think) you are aiming for by explicitly setting the id attribute to empty string on the node without any other changes to your demo project.

<mvcSiteMapNode title="About2"
                      controller="About"
                      action="About2"
                      id=""/>

This will make the menu ignore any "id" that is in the current request. So when I am on the /Contact/Contact2/2 page, the About2 menu URL is /About/About2.

Using preservedRouteParameters="id" has the opposite effect - it always copies the "id" from the current request to the node.

I imagine if you want to, you can also set the id explicitly to empty string (or perhaps null?) on the @Html.ActionLink() helper as well.

@wwuck
Copy link

wwuck commented Nov 29, 2013

Thanks for those tips. I always thought that using preservedRouteParameters="id" was the wrong thing, but when I take it off the relevant nodes in Mvc.sitemap I get a NullReferenceException for MvcSiteMapProvider.SiteMaps.Current.CurrentNode.

I got it working by keeping the preservedRouteParameters="id" in Mvc.sitemap and also changing the SiteMapNodeModel.cshtml template to use @Html.ActionLink(Model.Title, Model.Action, Model.Controller, new { id = "" }, new { }) instead of <a href="@Model.Url">@Model.Title</a>.

@NightOwl888
Copy link
Collaborator

That will work for this case, but keep in mind that if you do that you won't be able to use any custom parameters in any node. Seems quite limited if you ask me.

The reason why you are getting a null reference exception is because your node doesn't match your request route.

Node Route Request Route
Name Value Name Value
controller About controller About
action About2 action About2
id 2

This is not a match.

Adding preservedRouteParameters="id" copies the value from the current request so it will always match.

Node Route Request Route
Name Value Name Value
controller About controller About
action About2 action About2
id 2 id 2

But doing that gives it a side effect that if the id changes to something you don't want (for example the id of a different action method), it will copy the wrong value.

So the solution is to force a non-match in every case except the default one by specifying id="" explicitly.

Node Route Request Route
Name Value Name Value
controller About controller About
action About2 action About2
id "" id 2

Not a match.

Node Route Request Route
Name Value Name Value
controller About controller About
action About2 action About2
id "" id UrlParameter.Optional

Is a match.

This will satisfy the optional id parameter in every case and also only match the default route. Keep in mind the matching infrastructure of MvcSiteMapProvider is completely separate from how MVC matches a controller action.

@NightOwl888
Copy link
Collaborator

@awrigley

I reread your original post and original question on SO, and if I understand correctly the behavior you are describing is the same as what @tomascassidy has described.

If so, this is a "feature" of MVC which (if you are following along) is something that they don't intend to fix. It is by design that the URL generation will pick up ambient values to attempt to match them when generating the URLs.

Their advice about generating URLs appropriately is the following:

  1. Use named routes to ensure that only the route you want will get used to generate the URL (this is often a good practice, though it won't help in this particular scenario)
  2. Specify all route parameters explicitly - even the values that you want to be empty. That is one way to solve this particular problem.
  3. Instead of using Routing to generate the URLs, you can use Razor's / syntax or call Url.Content("/someurl") to ensure that no extra (or unexpected) processing will happen to the URL you're trying to generate.

I have described how to accomplish item 2 above with MvcSiteMapProvider, by specifying the value explicitly.

While I agree that this behavior seems anomalous, I feel we have done our part to successfully replicate what MVC does, which is what we are aiming for. Although we have provided ISiteMapNodeUrlResolver as an extensibility point where you can override this behavior, according to this post it is not possible to override the URL generation (UrlHelper) of MVC.

So without further ado, I am going to mark this issue closed. Feel free to reopen it if I misunderstood your OP and this is a different issue entirely.

@NightOwl888
Copy link
Collaborator

This turns out to be a bigger issue than I first thought because of the resolved URL caching. Because MVC grabs "ambient" values from the user's request, caching these URLs has the side effect of making one user request affect the other users.

To reproduce this problem, run a project that is configured to use MvcSiteMapProvider with the default route and then modify the Mvc.sitemap file to invalidate the cache. Manually type /Home/Index/3 into the browser and then press enter. This will produce an error (as expected), but if you click the back button and then go to a page that is showing the menu, you will notice that the home page URL is now /Home/Index/3. Furthermore, any user that clicks the URL is shown an error.

A solution for this issue is already in the works - to make a fake HTTP context that has no route values or query string and using it with the UrlHelper to change the result.

@NightOwl888 NightOwl888 reopened this Feb 28, 2014
NightOwl888 added a commit to NightOwl888/MvcSiteMapProvider that referenced this issue Mar 1, 2014
NightOwl888 added a commit to NightOwl888/MvcSiteMapProvider that referenced this issue Mar 1, 2014
…f ISiteMapNode and provided support for populating this property through XML, Dynamic Nodes, and MvcSiteMapNodeAttribute.
NightOwl888 added a commit to NightOwl888/MvcSiteMapProvider that referenced this issue Mar 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants