Re: Refactoring required to remove Roller's dependency on Hibernate feature cascade="all-delete-orphan"

[Craig L Russell]

Hi Mitesh,




On Dec 18, 2006, at 11:03 AM, Mitesh Meswani wrote:

This is a good analysis of a problem that I've seen as well. Comments follow...

Apache mail host seemed to remove the text of the mail. Resending.........

Hi,

I am working to port Roller's backend code to use JPA/JDO (see trunk/sandbox/jdobackend). I ran across a dependency on Hibernate specific feature that can be easily removed with some refactoring.

This Hibernate feature is not portable to either JDO or JPA, and I think it's a bit confusing when reading the code, since you have to either be a Hibernate expert or look up the behavior in the Hibernate documentation to figure out what is going on. So I support the notion of making relationship handling explicit and thereby removing the confusion. For Hibernate's "orphan" relationships, both sides of the relationship are affected by the operation, which I'd characterize as a "delete" operation that has a side effect of removing the deleted instance from the collection on the "owner" side. This is in contrast to Hibernate's definition of removing an instance which has a side effect of deleting the instance from the database. If the deleted instance had its own XXXManager, I'd favor putting the behavior into the XXXManager interface, since there is supposed to be no knowledge of the persistence layer inside the pojo layer. And in fact, it's unlikely that such a pojo would have such a behavior since the nature of the relationship is a composition relationship, and it makes sense that the behavior be in the aggregate instance manager. So I agree with your proposal to remove the method from the pojo and instead add it to the XXXManager interface that can both remove the owned instance from the collection and remove the owned instance from the database. As a performance optimization, we should also consider changing the relationship from a Set to a Map. This would avoid the issue of iterating the entire Set just to find the right entry. This is a portable JPA/JDO/Hibernate construct (to be further looked at later). In the code that follows, sometimes you remove the entity from the collection and then remove it from the database. In other cases, you remove it from the database and then remove it from the collection. We should probably be consistent in the order (although it should not matter; I don't believe that either JDO or JPA requires an order).

...snipped

Index: src/org/apache/roller/pojos/UserData.java
===================================================================
--- src/org/apache/roller/pojos/UserData.java    (revision 488141)
+++ src/org/apache/roller/pojos/UserData.java    (working copy)
@@ -434,38 +434,14 @@
        return false;
    }
//Moved the method revokeRole to HibernateUserManagerImpl
-       /**
-     * Revokes specified role from user.
-     */
-    public void revokeRole(String roleName) throws RollerException {
-        RoleData removeme = null;
-        Iterator iter = roles.iterator();
-        while (iter.hasNext()) {
-            RoleData role = (RoleData) iter.next();
-            if (role.getRole().equals(roleName)) {
-                removeme = role;
-            }
-        }
-       -        /*

-         * doing so causes a ConcurrentModificationException

- * NOTE: we do this outside the loop above because we are not allowed - * to modify the contents of the Set while we are iterating over it.

This looks like a bug to me. The algorithm iterates the collection and actually ignores all but the last role found. Since the semantic is that there should be only one such instance, it should stop at the first match of role name and therefore there is no ConcurrentModificationException.

    public void savePage(WeblogTemplate data) throws RollerException;

===================================================================

@@ -26,6 +26,8 @@
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
+import java.util.Collection;
+
import org.apache.roller.pojos.StatCount;
import org.hibernate.Criteria;
import org.hibernate.HibernateException;
@@ -58,6 +60,7 @@
import org.apache.roller.pojos.WeblogCategoryData;
import org.apache.roller.pojos.WeblogEntryData;
import org.apache.roller.pojos.WebsiteData;
+import org.apache.roller.pojos.RoleData;
import org.hibernate.Query;
@@ -406,6 +409,29 @@
        website.removePermission(target);
        this.strategy.remove(target);
    }
// Method moved from UserData to here
+

+        RoleData removeme = null;
+        Collection roles = user.getRoles();
+        Iterator iter = roles.iterator();
+        while (iter.hasNext()) {
+            RoleData role = (RoleData) iter.next();
+            if (role.getRole().equals(roleName)) {
+                removeme = role;

Index: src/org/apache/roller/business/hibernate/ HibernateUserManagerImpl.java --- src/org/apache/roller/business/hibernate/ HibernateUserManagerImpl.java (revision 488141) +++ src/org/apache/roller/business/hibernate/ HibernateUserManagerImpl.java (working copy) + public void revokeRole(String roleName, UserData user) throws RollerException {

I'd think that the remove operations should be here, followed by break;

The rest of the code looks good.

Craig

Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:[EMAIL PROTECTED]
P.S. A good JDO? O, Gasp!

smime.p7s
Description: S/MIME cryptographic signature