Bill Schneider added a comment - 13/Sep/06 02:26 PM

Bill Schneider added a comment - 13/Sep/06 02:26 PM I've seen this before and the issues/changelog made it sound like this had been fixed in 2.4.

Bill Schneider added a comment - 13/Sep/06 02:29 PM

Bill Schneider added a comment - 13/Sep/06 02:29 PM This is a duplicate of SM-29

Bill Schneider added a comment - 13/Sep/06 02:41 PM

Bill Schneider added a comment - 13/Sep/06 02:41 PM 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(); ) {               c.menuComponents.add(((Cloneable) it.next()).clone());             }             menucomponent = c;

Matt Raible added a comment - 20/Sep/06 06:16 PM

Matt Raible added a comment - 20/Sep/06 06:16 PM 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(); ) {                     MenuComponent subMenu = (MenuComponent) it.next();                     c.addMenuComponent((MenuComponent) subMenu.clone());                 }             }             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();

roc.bhakta added a comment - 26/Sep/06 05:20 PM

roc.bhakta added a comment - 26/Sep/06 05:20 PM 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++)                  {                     MenuComponent subMenu = (MenuComponent) listArray[i];                     c.addMenuComponent((MenuComponent) subMenu.clone());                 }             }

Robert Lang added a comment - 28/Sep/06 11:54 AM

Robert Lang added a comment - 28/Sep/06 11:54 AM 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(""))) {             menuComponent.setName(this.name + menuComponents.size());         } if (!menuComponents.contains(menuComponent)) { menuComponents.add(menuComponent); menuComponent.setParent(this); } if (!protoMenuComponents.contains(menuComponent)) { protoMenuComponents.add(menuComponent); menuComponent.setParent(this); }     } public MenuComponent[] getMenuComponents() { return (MenuComponent[]) menuComponents.toArray(_menuComponent); } public MenuComponent[] getProtoMenuComponents() { return (MenuComponent[]) protoMenuComponents.toArray(_menuComponent); }     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)) {                 parentMenu.addMenuComponent(this);             }         }         this.parentMenu = parentMenu;     }     public MenuComponent getParent() {         return parentMenu;     } /** * 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])) {                     componentsAllowed.add(components[i]);                     updateAllowedComponents(components[i]);                 }             }             menu.getComponents().clear();             menu.getComponents().addAll(componentsAllowed);         }     } That's for the great menu. Robert

Robert Lang added a comment - 28/Sep/06 12:19 PM

Robert Lang added a comment - 28/Sep/06 12:19 PM 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

Matt Raible added a comment - 03/Oct/06 03:41 PM

Matt Raible added a comment - 03/Oct/06 03:41 PM roc.bhakta - your fix worked, thanks.

Matt Raible added a comment - 17/Oct/06 05:56 AM

Matt Raible added a comment - 17/Oct/06 05:56 AM 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.

Matt Raible added a comment - 17/Oct/06 06:17 AM

Matt Raible added a comment - 17/Oct/06 06:17 AM 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 )   ## set menu title   #set ($title = $displayer.getMessage($menu.title))   #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end   ## 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))     ## set menu title     #set ($title = $displayer.getMessage($menu.title))     #if (!$menu.url) #set ($url="javascript.void(0)") #else #set ($url=$menu.url) #end     ## 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)