MvcSiteMapProvider renders wrong level if requested level is “empty”

[NightOwl888]

Issue originally reported on StackOverflow.

I currently render two levels of menu using the method

@Html.MvcSiteMap().Menu(2,1,true)

for the 2nd level.

What I THINK I'm saying in this case is "show me all level 2 nodes that are related to the current path". The last param is "allowForwardSearch" and I don't know what that means, I just know that if I don't set it to true, I get nothing rendered.

As it is, this renders the 2nd level menu just fine UNLESS all of the 2nd level nodes are filtered out via security/visibility filters which would leave an empty list of nodes for the 2nd level. If the expected result is NO 2nd level nodes, then it renders 1st level nodes instead. Which winds up duplicating items from the 1st level menu. I would expect that it would just render nothing, but that's not the case.

Is this a bug, or do I need to use a specific override (there's 80!) to get it to NOT revert to showing me 1st level nodes when I ask for 2nd level nodes.

Thanks.

[NightOwl888]

@maartenba

I took a look at this and it is due to the GetNodeAtLevel() method.

public static ISiteMapNode GetNodeAtLevel(ISiteMapNode startingNode, int level, bool allowForwardSearch)
{
    if (startingNode == null)
    {
        return null;
    }
    var startingNodeLevel = startingNode.GetNodeLevel();
    if (startingNodeLevel == level)
    {
        return startingNode;
    }
    else if (startingNodeLevel > level)
    {
        var node = startingNode;
        while (node != null)
        {
            if (node.GetNodeLevel() == level)
            {
                return node;
            }
            node = node.ParentNode;
        }
    }
    else if (startingNodeLevel < level && allowForwardSearch)
    {
        var node = startingNode;
        while (node != null)
        {
            if (node.GetNodeLevel() == level)
            {
                return node;
            }
            if (node.HasChildNodes)
            {
                node = node.ChildNodes[0];
            }
            else
            {
                break;
            }
        }
    }

    if (startingNode != null && startingNode.SiteMap.RootNode != null && allowForwardSearch)
    {
        return startingNode.SiteMap.RootNode;
    }

    return null;
}

The problem is that when allowForwardSearch (which I don't fully understand why you wouldn't want to allow) is true, the root node is being returned in the case where there are no accessible nodes at the specified level.

If I comment out these lines

    if (startingNode != null && startingNode.SiteMap.RootNode != null && allowForwardSearch)
    {
        return startingNode.SiteMap.RootNode;
    }

the problem is fixed.

However, I am wondering if there is just a logic bug - that is, should it be

    if (startingNode != null && startingNode.SiteMap.RootNode != null && !allowForwardSearch)
    {
        return startingNode.SiteMap.RootNode;
    }

that is, !allowForwardSearch instead of allowForwardSearch?

It would really help to understand why you would ever not want to search forward when dealing with levels. Apparently, searching backward means that the current node is deeper than the actual level you want shown, and searching forward means that the current node is not as deep as the level you want shown.

If you have no answer for that, I would appreciate your opinion on whether to remove those 3 lines (never return a "default" node, which will cause the menu to be not rendered), or to reverse the logic (meaning always return null when searching forward, but return the root node if searching backward). I guess another option is to leave the lines alone and explain another method for configuring the menu the way the OP wants. Removing those lines will have a smaller impact on behavior than reversing the logic. I am not sure it ever makes sense to show the menu when there is no node in the level range that is specified, but I have not used the menu enough to know what its quirks are.

One more thing - here is the comment for allowForwardSearch:

if set to true allow forward search. Forward search will search all parent nodes and child nodes, where in other circumstances only parent nodes are searched.

This doesn't agree with the code. If forward search is enabled, the code does not currently search parent nodes. Should the code be changed to agree with the comment or the comment be changed to agree with the code?

[maartenba]

This does look like a logic bug. The description should be the one to follow.

[NightOwl888]

Wait a minute, the logic looks fine. It checks the current node's level and decides which way to go, which is much more efficient than searching both directions. I think the comment should be the thing to change.

And what about returning the root node in the case where the node found (startingNode variable) is null? This logic might be right, but the assumption was made that this will never happen in a correctly configured SiteMap (it can when the current user doesn't have permission to access any of the nodes at the starting level that is passed).

It might be nice to make this an option, but we really don't need any more overloads on the Menu. IMHO it makes sense to hide the menu when it is configured this way in this case, but I don't know what other use cases we might be stepping on by doing so.

Thoughts?