DS-2086 fix. Corrects the logic to deleting Communities & Collections such that hierarchical deletion test now succeeds. Also corrects minor permissions error in a different unit test.

dspace-api/src/main/java/org/dspace/content/Community.java

@@ -994,10 +994,15 @@ public class Community extends DSpaceObject
     }
 
     /**
-     * Remove a collection. Any items then orphaned are deleted.
+     * Remove a collection. If it only belongs to one parent community,
+     * then it is permanently deleted. If it has more than one parent community,
+     * it is simply unmapped from the current community.
      * 
      * @param c
      *            collection to remove
+     * @throws SQLException
+     * @throws AuthorizeException
+     * @throws IOException
      */
     public void removeCollection(Collection c) throws SQLException,
             AuthorizeException, IOException
@@ -1005,149 +1010,185 @@ public class Community extends DSpaceObject
         // Check authorisation
         AuthorizeManager.authorizeAction(ourContext, this, Constants.REMOVE);
 
-        // will be the collection an orphan?
+        // Do the removal in a try/catch, so that we can rollback on error
-        TableRow trow = DatabaseManager.querySingle(ourContext,
+        try
-                "SELECT COUNT(DISTINCT community_id) AS num FROM community2collection WHERE collection_id= ? ",
-                c.getID());
-        DatabaseManager.setConstraintDeferred(ourContext, "comm2coll_collection_fk");
-        
-        if (trow.getLongColumn("num") == 1)
         {
-            // Orphan; delete it            
+            // Capture ID & Handle of Collection we are removing, so we can trigger events later
-            c.delete();
+            int removedId = c.getID();
-        }
+            String removedHandle = c.getHandle();
-        
-        log.info(LogManager.getHeader(ourContext, "remove_collection",
-                "community_id=" + getID() + ",collection_id=" + c.getID()));
-        
-        // Remove any mappings
-        DatabaseManager.updateQuery(ourContext,
-                "DELETE FROM community2collection WHERE community_id= ? "+
-                "AND collection_id= ? ", getID(), c.getID());
 
-        DatabaseManager.setConstraintImmediate(ourContext, "comm2coll_collection_fk");
+            // How many parent(s) does this collection have?
+            TableRow trow = DatabaseManager.querySingle(ourContext,
+                    "SELECT COUNT(DISTINCT community_id) AS num FROM community2collection WHERE collection_id= ? ",
+                    c.getID());
+            long numParents = trow.getLongColumn("num");
+
+            // Remove the parent/child mapping with this collection
+            // We do this before deletion, so that the deletion doesn't throw database integrity violations
+            DatabaseManager.updateQuery(ourContext,
+                    "DELETE FROM community2collection WHERE community_id= ? "+
+                    "AND collection_id= ? ", getID(), c.getID());
+
+            // As long as this Collection only had one parent, delete it
+            // NOTE: if it had multiple parents, we will keep it around,
+            // and just remove that single parent/child mapping
+            if (numParents == 1)
+            {
+                c.delete();
+            }
         
-        ourContext.addEvent(new Event(Event.REMOVE, Constants.COMMUNITY, getID(), Constants.COLLECTION, c.getID(), c.getHandle()));
+            // log the removal & trigger any associated event(s)
+            log.info(LogManager.getHeader(ourContext, "remove_collection",
+                    "community_id=" + getID() + ",collection_id=" + removedId));
+        
+            ourContext.addEvent(new Event(Event.REMOVE, Constants.COMMUNITY, getID(), Constants.COLLECTION, removedId, removedHandle));
+        }
+        catch(SQLException|IOException e)
+        {
+            // Immediately abort the deletion, rolling back the transaction
+            ourContext.abort();
+            // Pass exception upwards for additional reporting/handling as needed
+            throw e;
+        }
     }
 
     /**
-     * Remove a subcommunity. Any substructure then orphaned is deleted.
+     * Remove a subcommunity. If it only belongs to one parent community,
+     * then it is permanently deleted. If it has more than one parent community,
+     * it is simply unmapped from the current community.
      * 
      * @param c
      *            subcommunity to remove
+     * @throws SQLException
+     * @throws AuthorizeException
+     * @throws IOException
      */
     public void removeSubcommunity(Community c) throws SQLException,
             AuthorizeException, IOException
     {
-        // Check authorisation
+        // Check authorisation.
         AuthorizeManager.authorizeAction(ourContext, this, Constants.REMOVE);
 
-        // will be the subcommunity an orphan?
+        // Do the removal in a try/catch, so that we can rollback on error
-        TableRow trow = DatabaseManager.querySingle(ourContext,
+        try
-                "SELECT COUNT(DISTINCT parent_comm_id) AS num FROM community2community WHERE child_comm_id= ? ",
-                c.getID());
-
-        DatabaseManager.setConstraintDeferred(ourContext, "com2com_child_fk");
-        if (trow.getLongColumn("num") == 1)
         {
-            // Orphan; delete it
+            // Capture ID & Handle of Community we are removing, so we can trigger events later
-            c.rawDelete();
+            int removedId = c.getID();
+            String removedHandle = c.getHandle();
+        
+            // How many parent(s) does this subcommunity have?
+            TableRow trow = DatabaseManager.querySingle(ourContext,
+                    "SELECT COUNT(DISTINCT parent_comm_id) AS num FROM community2community WHERE child_comm_id= ? ",
+                    c.getID());
+            long numParents = trow.getLongColumn("num");
+
+            // Remove the parent/child mapping with this subcommunity
+            // We do this before deletion, so that the deletion doesn't throw database integrity violations
+            DatabaseManager.updateQuery(ourContext,
+                    "DELETE FROM community2community WHERE parent_comm_id= ? " +
+                    " AND child_comm_id= ? ", getID(),c.getID());
+
+            // As long as this Community only had one parent, delete it
+            // NOTE: if it had multiple parents, we will keep it around,
+            // and just remove that single parent/child mapping
+            if (numParents == 1)
+            {
+                c.rawDelete();
+            }
+
+            // log the removal & trigger any related event(s)
+            log.info(LogManager.getHeader(ourContext, "remove_subcommunity",
+                    "parent_comm_id=" + getID() + ",child_comm_id=" + removedId));
+
+            ourContext.addEvent(new Event(Event.REMOVE, Constants.COMMUNITY, getID(), Constants.COMMUNITY, removedId, removedHandle));
+        }
+        catch(SQLException|IOException e)
+        {
+            // Immediately abort the deletion, rolling back the transaction
+            ourContext.abort();
+            // Pass exception upwards for additional reporting/handling as needed
+            throw e;
         }
-
-        log.info(LogManager.getHeader(ourContext, "remove_subcommunity",
-                "parent_comm_id=" + getID() + ",child_comm_id=" + c.getID()));
-        
-        // Remove any mappings
-        DatabaseManager.updateQuery(ourContext,
-                "DELETE FROM community2community WHERE parent_comm_id= ? " +
-                " AND child_comm_id= ? ", getID(),c.getID());
-
-        ourContext.addEvent(new Event(Event.REMOVE, Constants.COMMUNITY, getID(), Constants.COMMUNITY, c.getID(), c.getHandle()));
-        
-        DatabaseManager.setConstraintImmediate(ourContext, "com2com_child_fk");
     }
 
     /**
-     * Delete the community, including the metadata and logo. Collections and
+     * Delete the community, including the metadata and logo. Any children
-     * subcommunities that are then orphans are deleted.
+     * (subcommunities or collections) are also deleted.
+     * 
+     * @throws SQLException
+     * @throws AuthorizeException
+     * @throws IOException
      */
     public void delete() throws SQLException, AuthorizeException, IOException
     {
-        // Check authorisation
+        // Check for a parent Community
-        // FIXME: If this was a subcommunity, it is first removed from it's
+        Community parent = getParentCommunity();
-        // parent.
+
-        // This means the parentCommunity == null
+        // Check authorisation.
-        // But since this is also the case for top-level communities, we would
+        // MUST have either REMOVE permissions on parent community (if exists)
-        // give everyone rights to remove the top-level communities.
+        // OR have DELETE permissions on current community
-        // The same problem occurs in removing the logo
+        if (parent!= null && !AuthorizeManager.authorizeActionBoolean(ourContext,
-        if (!AuthorizeManager.authorizeActionBoolean(ourContext,
                 getParentCommunity(), Constants.REMOVE))
         {
+            // If we don't have Parent Community REMOVE permissions, then
+            // we MUST at least have current Community DELETE permissions
             AuthorizeManager
                     .authorizeAction(ourContext, this, Constants.DELETE);
         }
 
-        // If not a top-level community, have parent remove me; this
+        // Check if this is a top-level Community or not
-        // will call rawDelete() before removing the linkage
-        Community parent = getParentCommunity();
-
         if (parent == null)
         {
-            // if removing a top level Community, simulate a REMOVE event at the Site.
+            // Call rawDelete to clean up all sub-communities & collections
-            if (getParentCommunity() == null)
+            // under this Community, then delete the Community itself
-            {
+            rawDelete();
-                ourContext.addEvent(new Event(Event.REMOVE, Constants.SITE, Site.SITE_ID, 
+
-                        Constants.COMMUNITY, getID(), getHandle()));
+            // Since this is a top level Community, simulate a REMOVE event at the Site.
-            }
+            ourContext.addEvent(new Event(Event.REMOVE, Constants.SITE, Site.SITE_ID,
+                    Constants.COMMUNITY, getID(), getHandle()));
         } else {
-            // remove the subcommunities first
+            // This is a subcommunity, so let the parent remove it
-            Community[] subcommunities = getSubcommunities();
+            // NOTE: this essentially just logs event and calls "rawDelete()"
-            for (int i = 0; i < subcommunities.length; i++)
-            {
-                subcommunities[i].delete();
-            }
-            // now let the parent remove the community
             parent.removeSubcommunity(this);
-
-            return;
         }
-
-        rawDelete();        
     }
     
     /**
-     * Internal method to remove the community and all its childs from the database without aware of eventually parent  
+     * Internal method to remove the community and all its children from the
+     * database, and perform any pre/post-cleanup
+     *
+     * @throws SQLException
+     * @throws AuthorizeException
+     * @throws IOException
      */
     private void rawDelete() throws SQLException, AuthorizeException, IOException
     {
         log.info(LogManager.getHeader(ourContext, "delete_community",
                 "community_id=" + getID()));
 
-        ourContext.addEvent(new Event(Event.DELETE, Constants.COMMUNITY, getID(), getHandle()));
+        // Capture ID & Handle of object we are removing, so we can trigger events later
+        int deletedId = getID();
+        String deletedHandle = getHandle();
 
-        // Remove from cache
+        // Remove Community object from cache
         ourContext.removeCached(this, getID());
 
-        // Remove collections
+        // Remove any collections directly under this Community
-        Collection[] cols = getCollections();
+        for (Collection collection : getCollections())
-
-        for (int i = 0; i < cols.length; i++)
         {
-            removeCollection(cols[i]);
+            removeCollection(collection);
         }
 
-        // delete subcommunities
+        // Remove any SubCommunities under this Community
-        Community[] comms = getSubcommunities();
+        for (Community community : getSubcommunities())
-
-        for (int j = 0; j < comms.length; j++)
         {
-            comms[j].delete();
+            removeSubcommunity(community);
         }
 
-        // Remove the logo
+        // Remove the Community logo
         setLogo(null);
 
-        // Remove all authorization policies
+        // Remove all associated authorization policies
         AuthorizeManager.removeAllPolicies(ourContext, this);
 
         // get rid of the content count cache if it exists
@@ -1163,19 +1204,22 @@ public class Community extends DSpaceObject
             throw new IllegalStateException(e.getMessage(),e);
         }
 
-        // Remove any Handle
+        // Unbind any handle associated with this Community
         HandleManager.unbindHandle(ourContext, this);
 
-        // Delete community row
+        // Delete community row (which actually removes the Community)
         DatabaseManager.delete(ourContext, communityRow);
 
-        // Remove administrators group - must happen after deleting community
+        // Remove Community administrators group (if exists)
+        // NOTE: this must happen AFTER deleting community
         Group g = getAdministrators();
-
         if (g != null)
         {
             g.delete();
         }
+
+        // If everything above worked, then trigger any associated events
+        ourContext.addEvent(new Event(Event.DELETE, Constants.COMMUNITY, deletedId, deletedHandle));
     }
 
     /**

dspace-api/src/test/java/org/dspace/content/CommunityTest.java

@@ -1027,15 +1027,15 @@ public class CommunityTest extends AbstractDSpaceObjectTest
     {
         new NonStrictExpectations(AuthorizeManager.class)
         {{
-            // Allow current Community ADD perms
+            // Allow Community ADD perms (in order to add a new subcommunity to parent)
             AuthorizeManager.authorizeAction((Context) any, (Community) any,
-                    Constants.ADD); result = null;
+                    Constants.ADD, true); result = null;
-            // Allow *parent* Community ADD perms
+            // Allow Community REMOVE perms (needed to unmap/remove subcommunity)
-            AuthorizeManager.authorizeActionBoolean((Context) any, (Community) any,
-                    Constants.ADD); result = true;
-            // Allow current Community REMOVE perms
             AuthorizeManager.authorizeAction((Context) any, (Community) any,
-                    Constants.REMOVE); result = null;
+                    Constants.REMOVE, true); result = null;
+            // Allow Community DELETE perms (needed to actually delete subcommunity)
+            AuthorizeManager.authorizeAction((Context) any, (Community) any,
+                    Constants.DELETE, true); result = null;
         }};
 
         // Turn off authorization temporarily to create a new top-level community