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);
