[Port to dtq-dev] Issue dspace-customers#903: fix SWORDv2 item double-delete with WorkflowManagerDefault - #1407
Merged
Merged
Conversation
…emoveItem 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 f79e704 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 66af883). 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR restores a previously reverted SWORDv2 delete-path fix in ContainerManagerDSpace, preventing an item from being deleted twice in a single request/transaction (notably when the workflow manager is org.dspace.sword2.WorkflowManagerDefault).
Changes:
- Switch workspace deletion from
workspaceItemService.deleteWrapper()toworkspaceItemService.deleteAll()(which also deletes the underlyingItem). - Add an
isItemAlreadyDeleted(Context, UUID)helper that scansContextevents for anEvent.DELETEon theItem, and guards the subsequentitemService.delete()call accordingly. - Add required imports (
UUID,Event) to support the new guard logic.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 <noreply@anthropic.com>
Author
|
Follow-up
Validation: Re: Copilot's automated review — it posted a descriptive overview only, with no inline comments or actionable findings, so there is nothing to resolve from it. |
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 <noreply@anthropic.com>
5 tasks
milanmajchrak
added a commit
that referenced
this pull request
Sep 9, 2026
…v2 double-delete guard, LDAP hardening) to the v9 base (#1430) * Port #1409 to dtq-dev-9-base: fix(mediafilter): log unparsable-PDF filter-media errors as WARN, not ERROR (#1409) Source: b0c4850 (dtq-dev PR #1409) A corrupt or malformed PDF makes PDFBoxThumbnail and TikaTextExtractionFilter throw a parse IOException that MediaFilterServiceImpl re-logs at ERROR, flooding the nightly filter-media job and tripping log-based alerting even though the job already skips the file and continues. Both filters now catch the IOException, log at WARN and return null, so the bitstream is skipped cleanly. The encrypted -PDF branch (InvalidPasswordException) stays at ERROR. Applied verbatim -- both files are byte-identical with vanilla 9.3 on this branch, and the v9 `Loader.loadPDF(new RandomAccessReadBuffer(source))` rewrite did not disturb the catch chain the new block attaches to. No test on either branch covers this path (the source PR has none either). Co-authored-by: MatusBeke <matus.beke7@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Port #1407 to dtq-dev-9-base: [Port to dtq-dev] fix: SWORDv2 item double-delete with WorkflowManagerDefault (#1407) Source: f241c47 (dtq-dev PR #1407) ContainerManagerDSpace.doContainerDelete() now checks, before the final itemService.delete(), whether a DELETE event for the same item UUID is already queued on the context -- i.e. the item was deleted earlier in this transaction -- and skips the second delete. Historically that second delete threw when the item came in through WorkflowManagerDefault. Both delete paths keep deleteWrapper(); the guard is the explicit safety net the source PR settled on, in its NPE-hardened form (itemUUID.equals(event.getSubjectID()), constrained to Event.DELETE on Constants.ITEM subjects). As the source PR states, the current base does not double-delete on any path, so this is behaviour-neutral today. Applied verbatim: the file is byte-identical with vanilla 9.3 on this branch and Context.getEvents() / Event.getEventType / getSubjectType / getSubjectID are unchanged in v9. The new java.util.UUID import lands after java.util.TreeMap and org.dspace.event.Event after org.dspace.core.LogHelper, so checkstyle import order (com < jakarta < org) is preserved. Known gap carried over from the source PR: the negative branch of the guard (delete skipped because a DELETE event is already queued) has no test on any branch. The two existing Swordv2IT delete tests only cover the positive path. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Port #1408 to dtq-dev-9-base: [Port to dtq-dev] fix: harden LDAP auth (groupmap guard, DN logging) (#1408) Source: 6615b01 (dtq-dev PR #1408) Two of the source commit's three hunks are ported; the third is already here. Ported (both in assignGroups()): - A groupmap entry without a ':' separator, or with a blank left or right part, no longer reaches `t[1]` (ArrayIndexOutOfBoundsException) and no longer leaves an empty ldapSearchString -- which made containsIgnoreCase(dn, "" + ",") match essentially every DN and assign the mapped group to all LDAP users. The entry is now logged at ERROR with its index and skipped, and scanning continues at the next index. Parsing uses split(":", 2) so a colon inside a DSpace group name is preserved instead of truncated. - System.out.println("dn:" + dn) becomes log.debug(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)) -- a semi-sensitive DN off the default log level and out of stdout. NOT ported -- VANILLA-COVERED: the e-mail fallback hunk (the 5-arg setEpersonAttributes overload plus its two call sites). That fix went upstream as 23b999e (PRs DSpace#11293/DSpace#11331, authored by DataQuest) and is in dspace-9.3: git merge-base --is-ancestor 23b999e dspace-9.3 -> 0 git diff dspace-9.3 origin/dtq-dev-9-base -- .../LDAPAuthentication.java -> empty On this branch setEpersonAttributes already has both overloads, both call sites already pass `email`, and `StringUtils.isNotEmpty(email)` occurs exactly twice -- the same count as on dtq-dev. Re-applying the hunk would have duplicated it. Added beyond the source commit: LDAPAuthenticationTest, 5 unit tests over the groupmap parsing (no LDAP server; ConfigurationService and GroupService mocked, assignGroups reached by reflection because it is private). The source PR lists the missing test under its own "Open points"; card BE-06's AC-3 needs exactly this behaviour proven, and with no LDAP server anywhere in the estate a unit test is the only way to prove it. Worth backporting to dtq-dev. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: jurinecko <juraj.roka@dataquest.sk> Co-authored-by: MatusBeke <matus.beke7@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a defensive guard to the SWORDv2 edit-media DELETE path (
ContainerManagerDSpace.doContainerDelete) so the underlyingItemis deleted at most once per transaction, even if an upstream method deletes it first. The workspace and workflow branches keep base'sdeleteWrapper()shape; only the finalitemService.delete()is now guarded.Relates to dataquest-dev/dspace-customers#903 (item 3).
Background
Issue #903 (item 3) tracked a SWORDv2 item double-delete originally fixed on
dtq-devbyf79e7043fc("UFAL/Copy SWORDv2 fixes #957"). That historical bug came fromworkspaceItemService.deleteAll()(which deletes the item itself) followed by an unguardeditemService.delete(). The #1031 7.6.5 upgrade merge reverted the workspace path todeleteWrapper()+ a singleitemService.delete(), which is already double-delete-safe by construction — so the currentdtq-devbase does not double-delete on any path (verified by trace and confirmed independently in review).Change set
ContainerManagerDSpace.javaonly:isItemAlreadyDeleted(context, uuid)— scanscontext.getEvents()for a pendingEvent.DELETEonConstants.ITEM, and guards the finalitemService.delete()so it runs at most once.java.util.UUID,org.dspace.event.Event(and useorg.dspace.core.Constants).deleteWrapper()— unchanged from base.Effect
Behaviour-neutral vs base by design. Nothing on the SWORDv2 delete path currently deletes the item before the final
itemService.delete(), so the guard acts purely as a safety net — it changes behaviour only if a future upstream change deletes the item within the same transaction. The two pre-existing integration tests (Swordv2IT.testDeleteWorkspaceManagerDefault,testDeleteWorkflowManagerDefault) exercise the delete path and should remain green on both base and this branch.Validation
Runtime integration tests are delegated to CI (docker unavailable locally).
Risk & rollback
Isolated to the SWORDv2 delete path; the guard is inert in normal operation. Revert = single commit.