VelocityDisplayer: Removing menu items for a particular role effects all users

This is a duplicate of SM-29

Apparently, an attempt was made to fix this in 2.4 (SM-29) but it was not adequate.
The fix is to clone the MenuComponent before filtering out non-displayed items.
But it doesn't work because MenuComponent.clone() only does a shallow copy with BeanUtils.copyProperties; the new MenuComponent instance still references the same collections as the original MenuComponent.

You need to clone the collection and the constituent MenuComponents as well... so clone could look something like this:

MenuComponent c = new MenuComponent();
BeanUtils.copyProperties(c, this);
c.menuComponents = Collections.synchronizedList(new ArrayList());
for (Iterator it = this.menuComponents.iterator(); it.hasNext(); )

menucomponent = c;

Bill - the code you posted doesn't compile. I tried the following:

MenuComponent c = new MenuComponent();
BeanUtils.copyProperties(c, this);

// BeanUtils doesn't do a deep copy, so copy children to new object
List subMenus = Collections.synchronizedList(this.menuComponents);
synchronized (subMenus) {
for (Iterator it = subMenus.iterator(); it.hasNext(); )

}

return c;

But it results in:

java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:449)
at java.util.AbstractList$Itr.next(AbstractList.java:420)
at net.sf.navigator.menu.MenuComponent.clone(MenuComponent.java:205)
at net.sf.navigator.displayer.VelocityMenuDisplayer.displayComponents(VelocityMenuDisplayer.java:156)

Line 205 is:

MenuComponent subMenu = (MenuComponent) it.next();

I was able to do the following with some success, it may or may not lead the the actual fix.
Note the: 1)use of array
and 2) clearing of the clone's menuComponents (i was getting dups.)

MenuComponent c = new MenuComponent();
BeanUtils.copyProperties(c, this);

c.menuComponents = Collections.synchronizedList(new ArrayList());

// BeanUtils doesn't do a deep copy, so copy children to new object
Object[] listArray = this.menuComponents.toArray();
synchronized ( listArray )
{
for (int i=0; i < listArray.length; i++)

}

Hello. I too am having this issue.

I believe that the problem lies in the fact that MenuComponent's original list is being modified. When the list is retrieved again, the list no longer contains all of the items.

Here's the offending code from VelocityMenuDisplayer:

menu.getComponents().clear();
menu.getComponents().addAll(componentsAllowed);

So, if the original list had 5 items and one person only has access to one, the original list now only has one item. Cloning will not help.

I repaired this by keeping a "proto" list separate from the component list. Instead of asking for menuComponents() in the Displayer, it asks for the proto list, then sets the menuComponents list as usual.

It's a bit of a hack, but it allows me to move on. Here's the revelant source:

Component.java

protected List menuComponents = Collections.synchronizedList(new ArrayList());
protected List protoMenuComponents = Collections.synchronizedList(new ArrayList());
protected MenuComponent parentMenu;
private boolean last;
private String breadCrumb;

//~ Methods ================================================================

public void addMenuComponent(MenuComponent menuComponent) {
if ((menuComponent.getName() == null) || (menuComponent.getName().equals("")))

if (!menuComponents.contains(menuComponent))

if (!protoMenuComponents.contains(menuComponent))

}

public MenuComponent[] getMenuComponents()

public MenuComponent[] getProtoMenuComponents()

public void setParent(MenuComponent parentMenu) {
if (parentMenu != null) {
// look up the parent and make sure that it has this menu as a child
if (!parentMenu.getComponents().contains(this))

}
this.parentMenu = parentMenu;
}

public MenuComponent getParent()

/**

• Convenience method for Velocity templates
  */
  public List getComponents() { return menuComponents; }
  
  /**
  * Convenience method for Velocity templates
  */
  public List getProtoComponents() { return menuComponents; }

From VelocityMenuDislayer.java:

private void updateAllowedComponents(MenuComponent menu) {
MenuComponent[] components = menu.getProtoMenuComponents();

if (components.length > 0) {
List componentsAllowed = new ArrayList();

for (int i = 0; i < components.length; i++) {
if (isAllowed(components[i]))

}

menu.getComponents().clear();
menu.getComponents().addAll(componentsAllowed);
}
}

That's for the great menu.

Robert

ooops. I didn't think this through. You fellas are correct, the MenuComponent object needs to be cloned. All that I've managed to do is introduce a race condition. Sorry. I'll implement the clone and give that a go.

Robert

roc.bhakta - your fix worked, thanks.

Latest fix in CVS works for Velocity-based menus, but ends up modifying menus in repository as well - resulting in duplicates if you're using anything other than Velocity-based menus. I'm going to try putting the permission check into globalMacros.vm and the templates themselves for optimization.

After struggling with cloning for several hours, and even trying to get deep-copy cloning (http://javatechniques.com/public/java/docs/basics/faster-deep-copy.html) to work - I decided it's easier to call $displayer.isAllowed($menu)) in the Velocity macros themselves. I added this by default to globalMacros.vm and moved the file to be in net.sf.navigator.displayer. You can override the displayMenu macro in this file by putting a menuMacros.vm file at the root of your classpath.

For templates (in the sample app) that don't use this macro - i.e. cssMenu.html - it's important to put the permission check w/in the macro. If you put the permission check surrounding the macro call, it doesn't work. For example, the following doesn't work:

#macro( displayCssMenu $menu )

1. 1. set menu title
      #set ($title = $displayer.getMessage($menu.title))

#if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end

1. 1. create a single menu item
      #if ($menu.components.size() == 0)
      #if ($velocityCount == $menu.parent.components.size())
      <li class="last">
      #else
      <li>
      #end
      #if ($menu.name == $currentMenu)
      <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a>
      #else
      <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}

      px"#end>$

      {title}</a>
      #end
      #else ## create multiple menu items in a menu
      <li class="menubar">
      <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title}

      </a>
      #end

#if ($menu.components.size() > 0)
<ul>
#foreach ($menuIt in $menu.components)
#displayCssMenu($menuIt)
#end
</li>
#else
</li>
#if ($velocityCount == $menu.parent.components.size())
</ul>
#end
#end
#end

#if ($displayer.isAllowed($menu))
#displayCssMenu($menu)
#end

But this one does:

#macro( displayCssMenu $menu )
#if ($displayer.isAllowed($menu))

1. 1. set menu title
      #set ($title = $displayer.getMessage($menu.title))

#if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end

1. 1. create a single menu item
      #if ($menu.components.size() == 0)
      #if ($velocityCount == $menu.parent.components.size())
      <li class="last">
      #else
      <li>
      #end
      #if ($menu.name == $currentMenu)
      <a href="$url" title="$title" class="current" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: $ {menu.width}px"#end>${title}</a>
      #else
      <a href="$url" title="$title" #if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}

      px"#end>$

      {title}</a>
      #end
      #else ## create multiple menu items in a menu
      <li class="menubar">
      <a href="$url" title="$title" #if ($menu.name == $currentMenu) class="current" #end#if($menu.target)target="$menu.target"#end#if($menu.width)style="width: ${menu.width}px"#end>${title}

      </a>
      #end

#if ($menu.components.size() > 0)
<ul>
#foreach ($menuIt in $menu.components)
#displayCssMenu($menuIt)
#end
</li>
#else
</li>
#if ($velocityCount == $menu.parent.components.size())
</ul>
#end
#end
#end
#end

#displayCssMenu($menu)