Thanks for the review Craig. Please find attached a cumulative patch
that addresses your review comments.
Please note that this patch contains some additional changes that I had
missed last time. Following is a summary of additional changes.
HibernateBookMarkManagerImpl:
removeBookmark(BookmarkData bookmark) now removes bookmark from the
parent folder's collection of bookmarks also
FolderData, UserData,WeblogEntryData:
Replaced the cascade="all-delete-orphan" with cascade="all"
All the business unit tests passes with the attached patch. ( I am
getting a failure while running org.apache.roller.ui.UIPluginManagerTest
with or without these changes on a freshly checked out workspace)
Thanks,
Mitesh
Craig L Russell wrote:
Hi Mitesh,
This is a good analysis of a problem that I've seen as well. Comments
follow...
On Dec 18, 2006, at 11:03 AM, Mitesh Meswani wrote:
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;
- }
- }
- - /*
- * 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.
- * doing so causes a ConcurrentModificationException
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;
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)
@@ -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
+
+ public void revokeRole(String roleName, UserData user) throws
RollerException {
+ 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;
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!
Index: src/org/apache/roller/pojos/FolderData.java
===================================================================
--- src/org/apache/roller/pojos/FolderData.java (revision 488141)
+++ src/org/apache/roller/pojos/FolderData.java (working copy)
@@ -274,7 +274,7 @@
*
* @roller.wrapPojoMethod type="pojo-collection"
class="org.apache.roller.pojos.BookmarkData"
*
- * @hibernate.set lazy="true" order-by="name" inverse="true"
cascade="all-delete-orphan"
+ * @hibernate.set lazy="true" order-by="name" inverse="true" cascade="all"
* @hibernate.collection-key column="folderid"
* @hibernate.collection-one-to-many
class="org.apache.roller.pojos.BookmarkData"
*/
@@ -296,15 +296,6 @@
getBookmarks().add(bookmark);
}
-
- /**
- * Remove a boomkark from folder.
- */
- public void removeBookmark(BookmarkData bookmark) {
- getBookmarks().remove(bookmark);
- }
-
-
/**
* @roller.wrapPojoMethod type="pojo-collection"
class="org.apache.roller.pojos.BookmarkData"
*
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)
@@ -404,7 +404,7 @@
/**
- * @hibernate.set lazy="true" inverse="true" cascade="all-delete-orphan"
+ * @hibernate.set lazy="true" inverse="true" cascade="all"
* @hibernate.collection-key column="userid"
* @hibernate.collection-one-to-many
class="org.apache.roller.pojos.RoleData"
*/
@@ -434,38 +434,14 @@
return false;
}
-
/**
- * 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;
- }
- }
-
- /*
- * 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.
- * doing so causes a ConcurrentModificationException
- */
- if(removeme != null) {
- roles.remove(removeme);
- }
- }
-
-
- /**
* Grant to user role specified by role name.
*/
public void grantRole(String roleName) throws RollerException {
if (!hasRole(roleName)) {
RoleData role = new RoleData(null, this, roleName);
roles.add(role);
+ role.setUser(this);
}
}
Index: src/org/apache/roller/pojos/WeblogEntryData.java
===================================================================
--- src/org/apache/roller/pojos/WeblogEntryData.java (revision 488141)
+++ src/org/apache/roller/pojos/WeblogEntryData.java (working copy)
@@ -353,7 +353,7 @@
*
* @roller.wrapPojoMethod type="pojo-collection"
class="org.apache.roller.pojos.EntryAttributeData"
* @ejb:persistent-field
- * @hibernate.set lazy="true" order-by="name" inverse="true"
cascade="all-delete-orphan"
+ * @hibernate.set lazy="true" order-by="name" inverse="true" cascade="all"
* @hibernate.collection-key column="entryid" type="String"
* @hibernate.collection-one-to-many
class="org.apache.roller.pojos.EntryAttributeData"
*/
@@ -404,12 +404,8 @@
att.setValue(value);
}
}
- public void removeEntryAttribute(String name) throws RollerException {
- EntryAttributeData att = (EntryAttributeData)attMap.get(name);
- if (att != null) {
- attMap.remove(att);
- attSet.remove(att);
- }
+ public void onRemoveEntryAttribute(EntryAttributeData att) throws
RollerException {
+ attMap.remove(att);
}
//-------------------------------------------------------------------------
@@ -609,7 +605,7 @@
*
* @ejb:persistent-field
*
- * @hibernate.set lazy="true" order-by="name" inverse="true"
cascade="all-delete-orphan"
+ * @hibernate.set lazy="true" order-by="name" inverse="true" cascade="all"
* @hibernate.collection-key column="entryid"
* @hibernate.collection-one-to-many
class="org.apache.roller.pojos.WeblogEntryTagData"
*/
@@ -654,16 +650,10 @@
addedTags.add(name);
}
- public void removeTag(String name) throws RollerException {
- for (Iterator it = tagSet.iterator(); it.hasNext();) {
- WeblogEntryTagData tag = (WeblogEntryTagData) it.next();
- if (tag.getName().equals(name)) {
- removedTags.add(name);
- it.remove();
- }
- }
+ public void onRemoveTag(String name) throws RollerException {
+ removedTags.add(name);
}
-
+
public Set getAddedTags() {
return addedTags;
}
@@ -696,9 +686,10 @@
newTags.remove(tag.getName());
}
}
-
+
+ WeblogManager weblogManager =
RollerFactory.getRoller().getWeblogManager();
for (Iterator it = removeTags.iterator(); it.hasNext();) {
- removeTag((String) it.next());
+ weblogManager.removeWeblogEntryTag((String) it.next(), this);
}
for (Iterator it = newTags.iterator(); it.hasNext();) {
Index: src/org/apache/roller/business/UserManager.java
===================================================================
--- src/org/apache/roller/business/UserManager.java (revision 488141)
+++ src/org/apache/roller/business/UserManager.java (working copy)
@@ -27,7 +27,6 @@
import org.apache.roller.pojos.UserData;
import org.apache.roller.pojos.WebsiteData;
-
/**
* Manages users, weblogs, permissions, and weblog pages.
*/
@@ -325,8 +324,15 @@
public void retireUser(WebsiteData website, UserData user)
throws RollerException;
-
/**
+ * Retire user from a website
+ * @param roleName Name of the role to be revoked
+ * @param user User for whom the role is to be revoked
+ */
+ public void revokeRole(String roleName, UserData user)
+ throws RollerException;
+
+ /**
* Store page.
*/
public void savePage(WeblogTemplate data) throws RollerException;
Index: src/org/apache/roller/business/WeblogManager.java
===================================================================
--- src/org/apache/roller/business/WeblogManager.java (revision 488141)
+++ src/org/apache/roller/business/WeblogManager.java (working copy)
@@ -194,9 +194,22 @@
* @return Collection of WeblogEntryData objects.
*/
public List getWeblogEntriesPinnedToMain(Integer max) throws
RollerException;
-
-
+
/**
+ * Remove attribute with given name from given WeblogEntryData
+ * @param name Name of attribute to be removed
+ */
+ public void removeWeblogEntryAttribute(String name, WeblogEntryData entry)
+ throws RollerException;
+
+ /**
+ * Remove tag with given name from given WeblogEntryData
+ * @param name Name of tag to be removed
+ */
+ public void removeWeblogEntryTag(String name, WeblogEntryData entry)
+ throws RollerException;
+
+ /**
* Save weblog category.
*/
public void saveWeblogCategory(WeblogCategoryData cat) throws
RollerException;
Index:
src/org/apache/roller/business/hibernate/HibernateBookmarkManagerImpl.java
===================================================================
--- src/org/apache/roller/business/hibernate/HibernateBookmarkManagerImpl.java
(revision 488141)
+++ src/org/apache/roller/business/hibernate/HibernateBookmarkManagerImpl.java
(working copy)
@@ -81,10 +81,13 @@
public void removeBookmark(BookmarkData bookmark) throws RollerException {
+ //Remove the bookmark from its parent folder
+ bookmark.getFolder().getBookmarks().remove(bookmark);
+ //Now remove it from database
this.strategy.remove(bookmark);
-
// update weblog last modified date. date updated by saveWebsite()
-
RollerFactory.getRoller().getUserManager().saveWebsite(bookmark.getWebsite());
+ RollerFactory.getRoller().getUserManager()
+ .saveWebsite(bookmark.getWebsite());
}
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)
@@ -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,20 @@
website.removePermission(target);
this.strategy.remove(target);
}
+
+ public void revokeRole(String roleName, UserData user) throws
RollerException {
+ RoleData removeme = null;
+ Collection roles = user.getRoles();
+ Iterator iter = roles.iterator();
+ while (iter.hasNext()) {
+ RoleData role = (RoleData) iter.next();
+ if (role.getRole().equals(roleName)) {
+ iter.remove();
+ this.strategy.remove(role);
+ }
+ }
+ }
+
public WebsiteData getWebsite(String id) throws RollerException {
return (WebsiteData) this.strategy.load(id,WebsiteData.class);
Index: src/org/apache/roller/business/hibernate/HibernateWeblogManagerImpl.java
===================================================================
--- src/org/apache/roller/business/hibernate/HibernateWeblogManagerImpl.java
(revision 488141)
+++ src/org/apache/roller/business/hibernate/HibernateWeblogManagerImpl.java
(working copy)
@@ -50,6 +50,7 @@
import org.apache.roller.pojos.WeblogEntryTagData;
import org.apache.roller.pojos.WeblogEntryTagAggregateData;
import org.apache.roller.pojos.WebsiteData;
+import org.apache.roller.pojos.EntryAttributeData;
import org.apache.roller.util.DateUtil;
import org.hibernate.Criteria;
import org.hibernate.HibernateException;
@@ -60,13 +61,7 @@
import org.hibernate.criterion.MatchMode;
import org.hibernate.criterion.Order;
import org.hibernate.criterion.Restrictions;
-import org.hibernate.dialect.DerbyDialect;
-import org.hibernate.dialect.Dialect;
-import org.hibernate.dialect.OracleDialect;
-import org.hibernate.dialect.SQLServerDialect;
-import org.hibernate.engine.SessionFactoryImplementor;
-
/**
* Hibernate implementation of the WeblogManager.
*/
@@ -547,10 +542,39 @@
throw new RollerException(e);
}
}
-
-
- public WeblogEntryData getWeblogEntryByAnchor(WebsiteData website, String
anchor)
+
+ public void removeWeblogEntryAttribute(String name, WeblogEntryData entry)
throws RollerException {
+ for (Iterator it = entry.getEntryAttributes().iterator();
it.hasNext();) {
+ EntryAttributeData entryAttribute = (EntryAttributeData) it.next();
+ if (entryAttribute.getName().equals(name)) {
+ //Call back the entity to adjust its internal state
+ entry.onRemoveEntryAttribute(entryAttribute);
+ //Remove it from the collection
+ it.remove();
+ //Remove it from database
+ this.strategy.remove(entryAttribute);
+ }
+ }
+ }
+
+ public void removeWeblogEntryTag(String name, WeblogEntryData entry)
+ throws RollerException {
+ for (Iterator it = entry.getTags().iterator(); it.hasNext();) {
+ WeblogEntryTagData tag = (WeblogEntryTagData) it.next();
+ if (tag.getName().equals(name)) {
+ //Call back the entity to adjust its internal state
+ entry.onRemoveTag(name);
+ //Remove it from the collection
+ it.remove();
+ //Remove it from database
+ this.strategy.remove(tag);
+ }
+ }
+ }
+
+ public WeblogEntryData getWeblogEntryByAnchor(WebsiteData website, String
anchor)
+ throws RollerException {
if (website == null)
throw new RollerException("Website is null");
Index:
src/org/apache/roller/ui/authoring/struts/actions/WeblogEntryFormAction.java
===================================================================
---
src/org/apache/roller/ui/authoring/struts/actions/WeblogEntryFormAction.java
(revision 488141)
+++
src/org/apache/roller/ui/authoring/struts/actions/WeblogEntryFormAction.java
(working copy)
@@ -513,10 +513,11 @@
}
if (!valid || empty) {
mLogger.debug("Removing MediaCast attributes");
+ WeblogManager weblogManager =
RollerFactory.getRoller().getWeblogManager();
try {
- entry.removeEntryAttribute("att_mediacast_url");
- entry.removeEntryAttribute("att_mediacast_type");
- entry.removeEntryAttribute("att_mediacast_length");
+ weblogManager.removeWeblogEntryAttribute("att_mediacast_url",
entry);
+ weblogManager.removeWeblogEntryAttribute("att_mediacast_type",
entry);
+
weblogManager.removeWeblogEntryAttribute("att_mediacast_length", entry);
} catch (RollerException e) {
mLogger.error("ERROR removing invalid MediaCast attributes");
}
Index: src/org/apache/roller/ui/admin/struts/formbeans/UserAdminForm.java
===================================================================
--- src/org/apache/roller/ui/admin/struts/formbeans/UserAdminForm.java
(revision 488141)
+++ src/org/apache/roller/ui/admin/struts/formbeans/UserAdminForm.java
(working copy)
@@ -23,6 +23,9 @@
import org.apache.struts.action.ActionMapping;
import org.apache.roller.RollerException;
+import org.apache.roller.business.Roller;
+import org.apache.roller.business.RollerImpl;
+import org.apache.roller.business.RollerFactory;
import org.apache.roller.pojos.UserData;
import org.apache.roller.ui.authoring.struts.forms.UserForm;
@@ -118,7 +121,7 @@
}
else
{
- user.revokeRole("admin");
+ RollerFactory.getRoller().getUserManager().revokeRole("admin",
user);
}
}
Index: tests/org/apache/roller/business/WeblogEntryTest.java
===================================================================
--- tests/org/apache/roller/business/WeblogEntryTest.java (revision
488141)
+++ tests/org/apache/roller/business/WeblogEntryTest.java (working copy)
@@ -415,8 +415,8 @@
TestUtils.endSession(true);
entry = mgr.getWeblogEntry(id);
- entry.removeTag("testtag");
- entry.removeTag("testtag2");
+ mgr.removeWeblogEntryTag("testtag", entry);
+ mgr.removeWeblogEntryTag("testtag2", entry);
mgr.saveWeblogEntry(entry);
TestUtils.endSession(true);
Index: tests/org/apache/roller/business/BookmarkTest.java
===================================================================
--- tests/org/apache/roller/business/BookmarkTest.java (revision 488141)
+++ tests/org/apache/roller/business/BookmarkTest.java (working copy)
@@ -328,7 +328,7 @@
bookmarkb = (BookmarkData)testFolder.getBookmarks().iterator().next();
// Remove one bookmark
- testFolder.removeBookmark(bookmarka);
+ bmgr.removeBookmark(bookmarka);
bmgr.saveFolder(testFolder);
TestUtils.endSession(true);
Index: tests/org/apache/roller/business/UserTest.java
===================================================================
--- tests/org/apache/roller/business/UserTest.java (revision 488141)
+++ tests/org/apache/roller/business/UserTest.java (working copy)
@@ -186,7 +186,7 @@
assertTrue(user.hasRole("admin"));
// remove role
- user.revokeRole("admin");
+ mgr.revokeRole("admin",user);
mgr.saveUser(user);
TestUtils.endSession(true);
