Bug 103037
Summary: | ContentItem.removePendingVersion() does not delete pending versions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Retired] Red Hat Enterprise CMS | Reporter: | Mike Bonnet <mikeb> | ||||||
Component: | other | Assignee: | Archit Shah <archit.shah> | ||||||
Status: | CLOSED WONTFIX | QA Contact: | Jon Orris <jorris> | ||||||
Severity: | high | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | nightly | CC: | jross, sseago, vnasardinov | ||||||
Target Milestone: | --- | ||||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2006-09-05 17:45:35 UTC | Type: | --- | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Attachments: |
|
Description
Mike Bonnet
2003-08-25 19:09:03 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. Mike, 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. Folder public void removePendingVersion(final ContentItem version) { Assert.unequal(PENDING, version.getVersion()); return; } 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. Thoughts? I will get to this by the end of tomorrow, at the latest. Yes, think making removePendingVersion() protected is a very good idea. I don't know why it was public to begin with. 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.
In that case, I am going to wait until tomorrow morning before taking the patch. 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. Mike, 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. 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. 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:
930a934
> 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
fixed on 6.0.x (38304) still needs upgrade script/code Verified fix, awaiting upgrade code. upgrade added at 38928 QA_READY has been deprecated in favor of ON_QA. Please use ON_QA in the future. Moving to ON_QA. Closing old tickets |