From 15bfb134d3b457f12dc323b94779d9447f688685 Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:02:07 +0200 Subject: [PATCH 1/3] fix(swordv2): guard against double-delete in ContainerManagerDSpace.removeItem Deleting a SWORDv2 item with WorkflowManagerDefault deleted the item twice and the second itemService.delete() threw. The fix -- deleteAll on the workspace path plus an isItemAlreadyDeleted guard before the final delete -- was present on dtq-dev via f79e7043fc and reverted by the #1031 7.6.5 upgrade merge, while the two Swordv2IT tests that cover it were left behind. This restores the reverted delta (identical to customer/zcu-pub 66af8837c4). Not taken from zcu-pub: its SwordUrlManager changes -- dtq-dev is ahead there. Port of dataquest-dev/dspace-customers#903 (item 3). Co-Authored-By: Claude Fable 5 --- .../dspace/sword2/ContainerManagerDSpace.java | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java index c0e8ef6bdc4..00f4e25f7d4 100644 --- a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java +++ b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java @@ -13,6 +13,7 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; +import java.util.UUID; import org.apache.logging.log4j.Logger; import org.dspace.authorize.AuthorizeException; @@ -25,6 +26,7 @@ import org.dspace.core.Constants; import org.dspace.core.Context; import org.dspace.core.LogHelper; +import org.dspace.event.Event; import org.dspace.workflow.WorkflowItem; import org.dspace.workflow.WorkflowItemService; import org.dspace.workflow.factory.WorkflowServiceFactory; @@ -755,14 +757,19 @@ protected void doContainerDelete(SwordContext swordContext, Item item, WorkflowTools wft = new WorkflowTools(); if (wft.isItemInWorkspace(swordContext.getContext(), item)) { WorkspaceItem wsi = wft.getWorkspaceItem(context, item); - workspaceItemService.deleteWrapper(context, wsi); + workspaceItemService.deleteAll(context, wsi); + // the item is deleted in the above call } else if (wft.isItemInWorkflow(context, item)) { WorkflowItem wfi = wft.getWorkflowItem(context, item); workflowItemService.deleteWrapper(context, wfi); } - // then delete the item - itemService.delete(context, item); + // then delete the item, but only if it hasn't already been deleted by the methods above. + // the delete method is called in `workspaceItemService.deleteAll(context, wsi);`, + // so it should not be called again here, as that would throw an exception. + if (!isItemAlreadyDeleted(context, item.getID())) { + itemService.delete(context, item); + } } catch (SQLException | IOException e) { throw new DSpaceSwordException(e); } catch (AuthorizeException e) { @@ -788,4 +795,20 @@ private Item getDSpaceTarget(Context context, String editUrl, return item; } + + /** + * Check if the item is already deleted in the context. + */ + private boolean isItemAlreadyDeleted(Context context, UUID itemUUID) { + if (context.getEvents() == null) { + return false; + } + + for (Event event : context.getEvents()) { + if (event.getEventType() == Event.DELETE && event.getSubjectID().equals(itemUUID)) { + return true; + } + } + return false; + } } From 4e9e9765c9ee76acbf49fb7701c8bbaf03b3c878 Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:20:31 +0200 Subject: [PATCH 2/3] refactor(swordv2): harden isItemAlreadyDeleted guard Make the double-delete guard NPE-safe and more precise, and clarify its Javadoc. Invert the UUID comparison to dereference the non-null itemUUID instead of the nullable Event.getSubjectID(), and constrain the match to Event.DELETE events on Constants.ITEM subjects. Behaviour is unchanged; this only removes a theoretical NPE and documents that the guard checks a pending (not-yet-dispatched) DELETE event queued on the context. Co-Authored-By: Claude Opus 4.8 --- .../java/org/dspace/sword2/ContainerManagerDSpace.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java index 00f4e25f7d4..d99555391ce 100644 --- a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java +++ b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java @@ -797,7 +797,9 @@ private Item getDSpaceTarget(Context context, String editUrl, } /** - * Check if the item is already deleted in the context. + * Returns true if a DELETE event for this item is already queued on the context + * (i.e. the item was deleted earlier in this transaction), so the caller can skip + * a second {@code itemService.delete()} that would otherwise fail. */ private boolean isItemAlreadyDeleted(Context context, UUID itemUUID) { if (context.getEvents() == null) { @@ -805,7 +807,9 @@ private boolean isItemAlreadyDeleted(Context context, UUID itemUUID) { } for (Event event : context.getEvents()) { - if (event.getEventType() == Event.DELETE && event.getSubjectID().equals(itemUUID)) { + if (event.getEventType() == Event.DELETE + && event.getSubjectType() == Constants.ITEM + && itemUUID.equals(event.getSubjectID())) { return true; } } From 2e8506989634e9bce75ea816d7f2ea701903f450 Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:36:15 +0200 Subject: [PATCH 3/3] refactor(swordv2): keep deleteWrapper on workspace delete path Revert the workspace branch of doContainerDelete from deleteAll() back to deleteWrapper() per PR review. deleteAll() imposed a stricter admin-or-submitter authorization gate and deleted the item itself, which only narrowed authorization (never enabled a delete) and reintroduced the very double-delete the guard then had to cancel. deleteWrapper() removes only the workspace wrapper row and leaves the single itemService.delete() below to remove the item, matching the base branch and the sibling workflow path. The isItemAlreadyDeleted() guard is retained as an explicit safety net against a future upstream method deleting the item within the same transaction. Co-Authored-By: Claude Opus 4.8 --- .../java/org/dspace/sword2/ContainerManagerDSpace.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java index d99555391ce..281f13fc470 100644 --- a/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java +++ b/dspace-swordv2/src/main/java/org/dspace/sword2/ContainerManagerDSpace.java @@ -757,16 +757,15 @@ protected void doContainerDelete(SwordContext swordContext, Item item, WorkflowTools wft = new WorkflowTools(); if (wft.isItemInWorkspace(swordContext.getContext(), item)) { WorkspaceItem wsi = wft.getWorkspaceItem(context, item); - workspaceItemService.deleteAll(context, wsi); - // the item is deleted in the above call + // remove only the workspace wrapper row; the item itself is deleted below. + workspaceItemService.deleteWrapper(context, wsi); } else if (wft.isItemInWorkflow(context, item)) { WorkflowItem wfi = wft.getWorkflowItem(context, item); workflowItemService.deleteWrapper(context, wfi); } - // then delete the item, but only if it hasn't already been deleted by the methods above. - // the delete method is called in `workspaceItemService.deleteAll(context, wsi);`, - // so it should not be called again here, as that would throw an exception. + // then delete the item, unless an upstream method already queued its deletion + // in this transaction (safety net against a double itemService.delete()). if (!isItemAlreadyDeleted(context, item.getID())) { itemService.delete(context, item); }