Bug 103037 - ContentItem.removePendingVersion() does not delete pending versions
Summary: ContentItem.removePendingVersion() does not delete pending versions
Alias: None
Product: Red Hat Enterprise CMS
Classification: Retired
Component: other
Version: nightly
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Archit Shah
QA Contact: Jon Orris
Depends On:
TreeView+ depends on / blocked
Reported: 2003-08-25 19:09 UTC by Mike Bonnet
Modified: 2007-04-18 16:57 UTC (History)
3 users (show)

Clone Of:
Last Closed: 2006-09-05 17:45:35 UTC

Attachments (Terms of Use)
Proposed patch for deleting the pending/live version after removal. (2.52 KB, patch)
2003-09-03 19:49 UTC, Mike Bonnet
no flags Details | Diff
New patch to delete pending/live versions when they are removed from the draft version. (3.72 KB, patch)
2003-09-04 13:06 UTC, Mike Bonnet
no flags Details | Diff

Description Mike Bonnet 2003-08-25 19:09:03 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 Galeon/1.2.7 (X11; Linux i686; U;) Gecko/20030131

Description of problem:
The unpublish() method loops through all pending versions and removes them from
the draft version.  However, it does not delete them.  A pending version with no
draft version has no use, and just takes up space in the DB.  It also violates
the "com.arsdigita.cms.dbinvariants.pendingLiveMarkedMap" invariant.  The
removePendingVersion() method should be deleting the pending versions after
removing them from the draft version.

Version-Release number of selected component (if applicable):
Troika 6.0.0

How reproducible:

Steps to Reproduce:
1. Create a new ContentItem.
2. Apply a lifecycle with a start date of tomorrow.
3. Call unpublish() on it.

Actual Results:  The pending version created in step 2 remains in the database.

Expected Results:  The pending version should have been deleted.

Additional info:

Comment 1 Mike Bonnet 2003-09-03 18:44:49 UTC
Changed this to a high priority bug because it's impacting a client right now. 
During their import process they end up publishing an item multiple times.  This
currently results in multiple pending versions of items getting orphaned in the
DB, which is problematic.  I'm going to have to patch this for them very soon,
and I'd like to know what the correct solution is.  I feel like the right thing
is for removeVersion() to delete the removed version, but I'd like the
engineering perspective on it.  Please look into this ASAP.

Comment 2 Vadim Nasardinov 2003-09-03 19:35:43 UTC
It seems like deleting the pending version after it has been removed  
from the draft is the right thing to do. (Provided we don't run into  
DB constraint violations.  It wouldn't be surprising, if we did.)  
While we are on this subject, I'd like to ask for your opinion on  
making the removePendingVersion method protected, instead of public.  
ContentItem only uses it in the unpublish() method.  
    (Self-use of public methods is generally a bad idea, 'cause you  
    don't really know what you are calling.  You may *think* you are  
    calling the removePendingVersion method of the ContentItem class,  
    while in reality, due to polymorphic method dispatch, you are  
    actually calling the removePendingVersion method of the most  
    specific subclass, which may have been overridden in a dumb way  
    that violates your assumptions.)  
Folder and ContentBundle basically override this method to do nothing.  
    public void removePendingVersion(final ContentItem version) {  
        Assert.unequal(PENDING, version.getVersion());  
ContentItem only uses it in unpublish()  
ArticleImageChooser.java uses it in the deleteImage method. It removes  
pending versions of the asset, before deleting the asset.  This logic  
can be moved into the Asset class, which would have access to the  
protected removePendingVersion in the superclass.  
So, based on current usage of this method, I see no reason for this  
method to be public.  
I will get to this by the end of tomorrow, at the latest. 

Comment 3 Mike Bonnet 2003-09-03 19:44:37 UTC
Yes, think making removePendingVersion() protected is a very good idea.  I don't
know why it was public to begin with.

Comment 4 Mike Bonnet 2003-09-03 19:49:16 UTC
Created attachment 94178 [details]
Proposed patch for deleting the pending/live version after removal.

I think this is the correct solution, but it is totally untested at this point.
 The client will be testing it rigorously later today, at which point I may
modify the patch.

Comment 5 Vadim Nasardinov 2003-09-03 19:52:41 UTC
In that case, I am going to wait until tomorrow morning before taking the patch.

Comment 6 Mike Bonnet 2003-09-04 13:06:08 UTC
Created attachment 94196 [details]
New patch to delete pending/live versions when they are removed from the draft version.

This patch tries to address deadlock conditions that were occurring with the
previous patch.  It removes a deadlock that was occurring when
ContentBundle.beforeSave() was being called during the delete (apparently
related to bz 103022).	Deadlocks are still occurring between the deleting
thread and the Lifecycle thread.  Always removing the lifecycle before deleting
the pending version may resolve this, but I haven't had time to implement/test
it yet.

Comment 7 Vadim Nasardinov 2003-09-05 16:19:29 UTC

Can you explain the deadlock issue in more detail?

At this point, the wisest course of action would seem to
be to let you guys test your patch out a little more,
before I merge it onto the trunk.

Seeing the words "patch" and "deadlock" in the same sentence
makes me nervous.

Comment 8 Vadim Nasardinov 2003-09-08 14:50:07 UTC
I am handing this off back to Archit, because

 a) he understands delete()-related issue better than I;
 b) I am going back to working on categorization.


Comment 9 Scott Seago 2003-09-25 21:09:56 UTC
When I integrated the publishing changes at 36166, I added a call to delete() in
removePendingVersion(). My code doesn't deal with the deadlock issues (which may
or may not still be an issue with more recent CMS changes), so Mike's patch or
some variant may still be needed. The relevant portion of the diff for 36166 is:

>       version.delete();

In addition, we need some code to clean up the current orphaned pending
versions. Nuno ran into a support issue where removed (but not deleted) pending
versions no longer show up in the UI (as the connection to the draft item was
severed), but they are still in the live folder, preventing the folder from
being deleted or moved. We need something along the lines of:
1) retrieve all on ContentItem.
2) Filter on "version='pending'" and "masterVersion is null"
3) delete these items, unpublishing the folder/bundle/etc. if appropriate

Comment 10 Archit Shah 2003-12-02 20:46:46 UTC
fixed on 6.0.x (38304)

Comment 11 Archit Shah 2003-12-02 21:01:54 UTC
still needs upgrade script/code

Comment 12 Jon Orris 2003-12-15 17:31:47 UTC
Verified fix, awaiting upgrade code.

Comment 13 Archit Shah 2003-12-18 07:35:32 UTC
upgrade added at 38928

Comment 14 David Lawrence 2006-07-18 03:43:10 UTC
QA_READY has been deprecated in favor of ON_QA. Please use ON_QA in the future.
Moving to ON_QA.

Comment 15 Jon Orris 2006-09-05 17:45:35 UTC
Closing old tickets

Note You need to log in before you can comment on or make changes to this bug.