IsInCurrentPath & IsCurrentNode are always false with caching turned off

[mathiasvanbeeck]

Okay, title may be a bit cryptic, so I'll try my best to explain what is going wrong.

I have created an MVC5 project which uses StructureMap as its DI container. Furthermore, because of very volatile data for our dynamic menu, I have turned off caching completely (absoluteCacheExpiration = "0").

Because of the layout of our page (horizontal menu), we are rendering the SiteMapPath before the menu. This is important, because when switching the order of rendering the bug does not occur.

When we render the SiteMapPath, it works as intended. However, when rendering the menu, I believe the Equals method on the nodes fails, because IsInCurrentPath and IsCurrentNode both return false when they shouldn't.

I have not tested this with other forms of DI.

You can find a sample project here (this uses NuGet packages. In order to debug you'll probably have to switch some stuff around)

[NightOwl888]

I have confirmed your findings and agree that this is a bug that should be fixed. The Equals and GetHashCode methods need to be overridden in the SiteMapNode to prevent the Equals method from returning incorrect results between cache cycles.

That said, "very volitile data" is not really a good reason to turn off caching. IMHO the only good reason to turn off caching is if you are pulling your node data from a source that is already cached. The main problem with turning off caching stems from the fact that every HTML helper you add to the page will cause the SiteMap to be reloaded again, which can cause a huge performance drain if you have a lot of nodes.

If your data is always updated through your MVC application, you can use the SiteMapCacheReleaseAttribute on each action method that updates data to flush your cache when any data is changed. There is no upper limit to the number of times it can be called between requests - calling it more than once is equivalent to calling it one time.

If the data is being updated by means other than MVC, I suggest you implement ICacheDependency and use it to wrap the SqlChangeMonitor to make a RuntimeSqlCacheDependency class similar to the RuntimeFileCacheDependency class. Just be sure to provide any configuration data (such as connection string, table name, etc.) through the constructor so it is DI friendly. In fact, creating a RuntimeSqlCacheDependency class would be a good candidate for a contribution.

The only thing unusual about it is that you need to return an IList<ChangeMonitor> containing the instance of SqlChangeMonitor. That is primarily because Microsoft didn't provide much common ground between configuring a SqlChangeMonitor (System.Runtime.Caching) and a SqlCacheDependency (System.Web.Caching) class, especially when it comes to multiplicity.

[mathiasvanbeeck]

@NightOwl888 Thanks for your reply.

About your concerns for performance: I agree completely.

Currently the sitemap is user-dependent, so I would probably have to implement a cache based on session state, rather than application cache. (I believe you've made a SO post about it somewhere)

That being said, the information can change outside of our application, and seeing as the only way for us to retrieve the data is through WCF, a change monitor would be hard to implement. We have, however, implemented a local caching mechanism which caches the WCF results during each request, so the amount of sitemaps or html helpers wouldn't really have any impact.

[NightOwl888]

We have, however, implemented a local caching mechanism which caches the WCF results during each request, so the amount of sitemaps or html helpers wouldn't really have any impact.

Actually, that is not true. The reason why the == comparison fails in this case is because the SiteMap is being rebuilt between the SiteMapPath and Menu calls. The CurrentNode property is request cached, so it maintains the reference to the ISiteMapNode instance from the original SiteMap until the end of the request. Therefore, the ReferenceEquals comparison that is done by the System.Object type fails because we are no longer comparing the same memory location in this case.

At the very least, the lifetime of the SiteMap cache should equal the lifetime of the request. I managed to do that using a SiteMapSpooler that was basically a request-cached wrapper around the shared cache. Its purpose is to ensure the SiteMap doesn't go out of scope until all of the requests that are using it have completed. I abandoned it before because it was primarily intended as a way to clean up the circular object references (SiteMap > SiteMapNode > SiteMap), but my assumption that the .NET garbage collector couldn't deal with circular references proved to be false.

It seems that the SiteMapSpooler still needs to be in place in case someone (like you) sets the cache timeout to 0, but it does mean that there will be overlaps between the time a new SiteMap is created and an old one is destroyed, so there could actually be 2 identical SiteMaps in memory at the same time whenever the cache expires. That said, using double the memory for a couple of seconds is likely much better than loading the SiteMap up to several times per request when the cache timeout is 0, depending on how many HTML helpers are on a given page. It occurred to me that is probably what is happening now anyway, since the RootNode and CurrentNode properties (that are request cached) are keeping the original SiteMap object from being garbage collected until all of the request that are using it have finished, even though a new SiteMap has already been loaded.

Anyway, I have discovered that there is no way to override the == operator for an interface, so it seems that the only realistic way to fix the Equals problem is to use inherit IEquatable<T> from ISiteMapNode and change all of the references within MvcSiteMapProvider to use ISiteMapNode.Equals instead of ==. While the SiteMapSpooler would effectively fix this problem, it still makes sense to fall back on a check to ensure the ISiteMap.CacheKey and ISiteMapNode.Key properties for the current node match if the memory reference check fails.

[NightOwl888]

FYI - The issue you were experiencing with inequality has been fixed in v4.6.5, but this is still open because there is also a fix coming to prevent the sitemap from going out of scope within the context of a request.

[hidegh]

Actually this fnc. works only if the given controller/action is defined only once is the sitemap.

So my solution is: a) custom node template b) checking sitemap controller/action/area against route data c) setting selectedNode class when sitemap data and route data is the same

The helper I use:

public static class HtmlExtensions
{
    public static bool IsRoute<T>(this HtmlHelper<T> html, string controller, string action, string area = "")
    {
        var routeController = html.ViewContext.RouteData.Values["controller"];
        var routeAction = html.ViewContext.RouteData.Values["action"];
        var routeArea = html.ViewContext.RouteData.Values["area"];

        return
            routeController.EmptyIfNull().ToLower() == controller.ToLower()
            && routeAction.EmptyIfNull().ToLower() == action.ToLower()
            && (routeArea != null ? routeArea.ToString().ToLower() : "") == area.ToLower();
    }
}

And the node cshtml:

@model MvcSiteMapProvider.Web.Html.Models.SiteMapNodeModel

@if (Model.IsCurrentNode && Model.SourceMetadata["HtmlHelper"].ToString() != "MvcSiteMapProvider.Web.Html.MenuHelper")
{
    <text>@Model.Title</text>
}
else
{
    if (Model.IsClickable)
    {
        // NOTE: this does not work's if the same Controller/Action is mapped more than once...
        // var isRoute = Model.IsInCurrentPath;

        var isRoute = Html.IsRoute(Model.Controller, Model.Action, Model.Area);

        if (isRoute || Model.IsInCurrentPath)
        {
            <a class="selectedNode" href="@Model.Url">@Model.Title</a>
        }
        else
        {
            <a href="@Model.Url">@Model.Title</a>
        }
    }
    else
    {
        <a>@Model.Title</a>
    }
}

[NightOwl888]

Using the exact same route values on two different nodes is not supported. This creates ambiguity on which node should selected as the current node.

Ideally, MvcSiteMapProvider should throw an exception in this case, but there are some edge cases to work around (route constraints and HttpMethod from what I recall) so this has been put off. However, it is inevitable that it will happen, since there is no reasonable way to make the node matching tell the nodes apart without providing extra route or querystring parameters (or perhaps cookies, but that is currently not in the box).

For that reason, I strongly recommend against this approach because your configuration is guaranteed to break in a future version. If you are having problems getting a matching node, read How to Make MvcSiteMapProvider Remember a User's Position and inspect the downloadable demos there that show the correct way to configure MvcSiteMapProvider.

Do note that the original issue reported here has been fixed in v4.6.4, but this issue remains open because there is another fix that requires changing the DI configuration that is waiting for the next major version release. See #352 and #360. So, I am not sure what you are trying to solve here since this issue has already been patched.

[hidegh]

Actually this hack mine was not needed, until multiple sitemaps were supported.

Now i have to manage main nodes (which are nodes for a sub menu) and rendering the concrete menu like:

var menuParentNode = HtmlHelperExtensions.MvcSiteMap(html).SiteMap.RootNode.ChildNodes.First(i => i.Title == "Menu");
            return HtmlHelperExtensions.MvcSiteMap(html).Menu("CustomMenuL12", menuParentNode, true, false);

I currently manage: top/bottom menu links (quick links) and a main menu, which has 3 levels and level 1-2 is displayed on the left, the level 3 on the top of the main content...

[hidegh]

The behavior as you described in your link: remembering last position (that is: the first node is matched) is acceptable. Hope that will not be changed and exception will be never raised on such a sitemap...

Having multiple times the same controller/action inside a sitemap is not prohibited, so throwing exception in that cases could not be considered to be a good practice. Not allowing us creating such sitemaps in the future is almost like if you would support us with a fixed sitemap we would have to use with your component :smile

[NightOwl888]

Sorry, I didn't mean to sound so harsh. But I put that example in the article to demonstrate why that it is not a good idea to do add multiple nodes with the exact same signature.

Microsoft's original design throws an exception every time the exact same URL is specified and our goal is to achieve the same thing (without limiting any of the features of MVC, that is). Not allowing multiple nodes with the same URL is definitely a best practice for this design (at the very least it leads to confusion). So in that regard MvcSiteMapProvider is currently broken in that it doesn't throw an exception in every one of these cases.

But there are some issues to overcome (and some that were problematic, but have been overcome) to fix this correctly without limiting features of MVC. For example, URLs that are specified using the Url property need to be checked against those that are specified using routes, but using route constraints prohibits the check from being done at application startup because the route constrained URL may require additional context to be resolved. And so this isn't such an easy task to do.

While I have added many exceptions to throw for a lot of misconfiguration cases, the best I can do now for this is discourage people from misconfiguring the SiteMap like this, because there is no reason that it should be done. The one possible exception is if you need a separate node for GET and POST, but I haven't analyzed that use case in detail.

As for your menu, there are other ways you could achieve the same thing without such measures. You could use multiple SiteMaps, for example. I am not sure what you meant by your comment, but multiple sitemaps were supported

The SiteMap is a data store similar to a database. It should have constraints to keep the data internally consistent. Adding multiple nodes the SiteMap so the menu displays multiple times on the page is akin to putting the same data into 2 different database records because you want it to appear 2 times on the UI.

The menu is based on a view-model. If you need to duplicate links for different menus that are arranged differently, you might consider building a view or view-model with custom business logic rather than trying to change the datasource to suit your whims (and putting your business rules in the datastore, which is never good).

[hidegh]

The multiple sitemap config in a previous version (mvc3/4 a year ago) was easier to apply. Anyhow, my solution works, needs little extra code.

But actually I write to explain why multiple controller/action may be needed inside a sitemap. I realize that this may result in a not working IsInCurrentPath... So why i need it:

1. Currently I'm building a web-site with fluent design, that should work for mobile and for desktop also. On the mobile (< 768px) it has a slide menu to access business related pages/functions, on desktop it behaves like a regular page. Due the CSS and html, the only way to make a menu for PC and mobile was to render a menu in 2 div(s) and controlling visibility via CSS and media queries. Off course for the mobile menu I put only the most necessary stuff, not necessarily in the order as on the PC.
2. My site also has menu for static pages (home | about us | contacts) - these I display on the top and also on the footer. On the footer the menu is different. Again I like to choose what to put on the top menu and what to put to the bottom. F.e. on the bottom I do not have link to Contacts inside the menu, because there's a separate div for contacts/www/and to social channels...
3. Quick links - both PC and mobile version has large buttons to access the most important function, that should make $... This is business related link, so it's also in the main menu...