* Fix several bugs in grouped collections:

<http://helma.org/bugs/show_bug.cgi?id=597>
  <http://helma.org/bugs/show_bug.cgi?id=614>
  <http://helma.org/bugs/show_bug.cgi?id=615>

src/helma/objectmodel/db/DbMapping.java

@@ -124,7 +124,10 @@ public final class DbMapping {
     HashSet dependentMappings = new HashSet();
 
     // does this DbMapping describe a virtual node (collection, mountpoint, groupnode)?
-    private boolean virtual = false;
+    private boolean isVirtual = false;
+
+    // does this Dbmapping describe a group node?
+    private boolean isGroup = false;
 
     /**
      * Create an internal DbMapping used for "virtual" mappings aka collections, mountpoints etc.
@@ -132,7 +135,7 @@ public final class DbMapping {
     public DbMapping(Application app, String parentTypeName) {
         this(app, parentTypeName, null);
         // DbMappings created with this constructor always define virtual nodes
-        virtual = true;
+        isVirtual = true;
         if (parentTypeName != null) {
             parentMapping = app.getDbMapping(parentTypeName);
             if (parentMapping == null) {
@@ -756,9 +759,9 @@ public final class DbMapping {
      * db-mapping with the right relations to create the group-by nodes
      */
     public synchronized DbMapping getGroupbyMapping() {
-        if ((subRelation == null)  && (parentMapping != null)) {
+        if ((subRelation == null) && (parentMapping != null)) {
             return parentMapping.getGroupbyMapping();
-        } else if (subRelation.groupby == null) {
+        } else if (subRelation == null || subRelation.groupby == null) {
             return null;
         } else if (groupbyMapping == null) {
             initGroupbyMapping();
@@ -774,6 +777,7 @@ public final class DbMapping {
         // if a prototype is defined for groupby nodes, use that
         // if mapping doesn' exist or isn't defined, create a new (anonymous internal) one
         groupbyMapping = new DbMapping(app, subRelation.groupbyPrototype);
+        groupbyMapping.isGroup = true;
 
         // set subnode and property relations
         groupbyMapping.subRelation = subRelation.getGroupbySubnodeRelation();
@@ -1596,6 +1600,14 @@ public final class DbMapping {
      * @return true if this instance describes a virtual node.
      */
     public boolean isVirtual() {
-        return virtual;
+        return isVirtual;
+    }
+
+    /**
+     * Find if this DbMapping describes a group node.
+     * @return true if this instance describes a group node.
+     */
+    public boolean isGroup() {
+        return isGroup;
     }
 }

src/helma/objectmodel/db/Node.java

@@ -869,26 +869,10 @@ public final class Node implements INode, Serializable {
         loadNodes();
 
         // check if this node has a group-by subnode-relation
-        if (dbmap != null) {
+        INode groupbyNode = getGroupbySubnode(node, true);
-            Relation srel = dbmap.getSubnodeRelation();
+        if (groupbyNode != null) {
-
+            groupbyNode.addNode(node);
-            if ((srel != null) && (srel.groupby != null)) {
+            return node;
-                Relation groupbyRel = srel.otherType.columnNameToRelation(srel.groupby);
-                String groupbyProp = (groupbyRel != null) ? groupbyRel.propName
-                                                              : srel.groupby;
-                String groupbyValue = node.getString(groupbyProp);
-                INode groupbyNode = (INode) getChildElement(groupbyValue);
-
-                // if group-by node doesn't exist, we'll create it
-                if (groupbyNode == null) {
-                    groupbyNode = getGroupbySubnode(groupbyValue, true);
-                } else {
-                    groupbyNode.setDbMapping(dbmap.getGroupbyMapping());
-                }
-
-                groupbyNode.addNode(node);
-                return node;
-            }
         }
 
         NodeHandle nhandle = node.getHandle();
@@ -1198,6 +1182,38 @@ public final class Node implements INode, Serializable {
         return retval;
     }
 
+    protected Node getGroupbySubnode(Node node, boolean create) {
+        if (node.dbmap != null && node.dbmap.isGroup()) {
+            return null;
+        }
+        
+        if (dbmap != null) {
+            Relation srel = dbmap.getSubnodeRelation();
+
+            if ((srel != null) && (srel.groupby != null)) {
+                Relation groupbyRel = srel.otherType.columnNameToRelation(srel.groupby);
+                String groupbyProp = (groupbyRel != null) ? groupbyRel.propName
+                                                              : srel.groupby;
+                String groupbyValue = node.getString(groupbyProp);
+                Node groupbyNode = (Node) getChildElement(groupbyValue);
+
+                // if group-by node doesn't exist, we'll create it
+                if (groupbyNode == null) {
+                    groupbyNode = getGroupbySubnode(groupbyValue, create);
+                    // mark subnodes as changed as we have a new group node
+                    if (create && groupbyNode != null) {
+                        Transactor.getInstance().visitParentNode(this);
+                    }
+                } else {
+                    groupbyNode.setDbMapping(dbmap.getGroupbyMapping());
+                }
+
+                return groupbyNode;
+            }
+        }
+        return null;
+    }
+
     /**
      *
      *
@@ -1211,10 +1227,7 @@ public final class Node implements INode, Serializable {
             throw new IllegalArgumentException("Can't create group by null");
         }
 
-        if (state == TRANSIENT) {
+        boolean persistent = state != TRANSIENT;
-            throw new RuntimeException("Can't add grouped child on transient node. "+
-                                       "Make parent persistent before adding grouped nodes.");
-        }
 
         loadNodes();
 
@@ -1228,34 +1241,40 @@ public final class Node implements INode, Serializable {
                 boolean relational = groupbyMapping.getSubnodeMapping().isRelational();
 
                 if (relational || create) {
-                    Node node = relational ? new Node(this, sid, nmgr, null)
+                    Node node = relational && persistent ?
-                                           : new Node(sid, null, nmgr);
+                            new Node(this, sid, nmgr, null) :
+                            new Node(sid, null, nmgr);
 
                     // set "groupname" property to value of groupby field
                     node.setString("groupname", sid);
-
+                    // Set the dbmapping on the group node
                     node.setDbMapping(groupbyMapping);
+                    node.setPrototype(groupbyMapping.getTypeName());
 
-                    if (!relational) {
+                    // if we're relational and persistent, make new node persistable
-                        // if we're not transient, make new node persistable
+                    if (!relational && persistent) {
-                        if (state != TRANSIENT) {
+                        node.makePersistable();
-                            node.makePersistable();
+                        node.checkWriteLock();
-                            node.checkWriteLock();
+                    }
-                        }
+
-                        subnodes.add(node.getHandle());
+                    // if we created a new node, check if we need to add it to subnodes
+                    if (create) {
+                        NodeHandle handle = node.getHandle();
+                        if (!subnodes.contains(handle))
+                            subnodes.add(handle);
                     }
 
-                    // Set the dbmapping on the group node
-                    node.setPrototype(groupbyMapping.getTypeName());
                     // If we created the group node, we register it with the
                     // nodemanager. Otherwise, we just evict whatever was there before
-                    if (create) {
+                    if (persistent) {
-                        // register group node with transactor
+                        if (create) {
-                        Transactor tx = Transactor.getInstanceOrFail();
+                            // register group node with transactor
-                        tx.visitCleanNode(node);
+                            Transactor tx = Transactor.getInstanceOrFail();
-                        nmgr.registerNode(node);
+                            tx.visitCleanNode(node);
-                    } else {
+                            nmgr.registerNode(node);
-                        nmgr.evictKey(node.getKey());
+                        } else {
+                            nmgr.evictKey(node.getKey());
+                        }
                     }
 
                     return node;
@@ -1299,6 +1318,13 @@ public final class Node implements INode, Serializable {
      * {@link #removeNode(INode)}.
      */
     protected void releaseNode(Node node) {
+
+        Node groupNode = getGroupbySubnode(node, false);
+        if (groupNode != null) {
+            groupNode.releaseNode(node);
+            return;
+        }
+
         INode parent = node.getParent();
 
         checkWriteLock();
@@ -1314,7 +1340,9 @@ public final class Node implements INode, Serializable {
             synchronized (subnodes) {
                 removed = subnodes.remove(node.getHandle());
             }
-            if (removed) {
+            if (dbmap != null && dbmap.isGroup() && subnodes.size() == 0) {
+                remove();
+            } else if (removed) {
                 registerSubnodeChange();
             }
         }
@@ -1341,6 +1369,12 @@ public final class Node implements INode, Serializable {
                             nmgr.evictKey(new SyntheticKey(getKey(), prop));
                         }
                     }
+                } else if (prel.groupby != null) {
+                    String prop = node.getString("groupname");
+                    if (prop != null && state != TRANSIENT) {
+                        nmgr.evictKey(new SyntheticKey(getKey(), prop));
+                    }
+
                 }
                 // TODO: We should unset constraints to actually remove subnodes here,
                 // but omit it by convention and to keep backwards compatible.