Elements: Delete relations when deleting an element folder (closes #23387)

[AndyButland]

Description

Deleting an element folder through the Management API (DELETE /umbraco/management/api/v1/element/folder/{id}) returned a 500 when the folder had previously contained an element that was moved to the recycle bin.

Root cause: when an element is trashed, RelateOnTrashNotificationHandler creates a relateParentElementContainerOnElementDelete relation whose parent is the folder node (so the element can be restored to its original location). The plain folder-delete endpoint resolves to the base EntityTypeContainerService.DeleteAsync, which removed the folder node without cleaning up umbracoRelation, so the lingering relation violated the FK_umbracoRelation_umbracoNode constraint. The folder passes the existing "not empty" guard because the trashed element lives under the recycle bin, not under the folder. (The recycle-bin delete path already cleaned relations via ElementContainerService.DeleteContainerRelations — added in #22154 — but the direct-delete path never did.)

Fix: a protected virtual OnDeletingContainer(EntityContainer) hook is added to EntityTypeContainerService.DeleteAsync, invoked inside the delete scope immediately before the container node is removed. ElementContainerService overrides it to reuse the existing DeleteContainerRelations, so all element-folder deletions clean up their relations consistently. The hook is a no-op for content-type/media-type/data-type folders, which never appear in umbracoRelation.

After the fix the folder deletes successfully and the trashed element is left intact; having lost its original-parent relation it is restorable to the root or another selected location — mirroring existing document/media behaviour when an original parent no longer exists.

Fixes #23387

Testing

Automated

Added a new integration test ElementContainerServiceTests.Can_Delete_Container_After_Child_Element_Trashed:

1. Create an element type, an element folder, and an element inside the folder.
2. Move the element to the recycle bin (creating the original-parent relation).
3. Delete the folder and assert success.
4. Assert the folder is gone, no umbracoRelation rows reference the folder, the element still exists in the recycle bin, and its original-parent lookup now reports NoParentRecycleRelation (i.e. it will restore to root).

Verified red before the fix (FK violation) and green after; the existing ElementContainerServiceTests suite continues to pass.

Manual

1. Create an element type with library support enabled.
2. Create an element folder, and create an element inside it.
3. Move the element to the recycle bin.
4. Call DELETE /umbraco/management/api/v1/element/folder/{id} for the folder (or delete it from the backoffice).
5. The folder deletes successfully (previously a 500).
6. The trashed element remains in the recycle bin and can be restored to the root or another location, even though it's original parent folder is now gone.

[claude]

Claude finished @AndyButland's task in 2m 46s —— View job

---

PR Review

Target: origin/main · Based on commit: 9c225f7c

Adds a protected virtual OnDeletingContainer hook to EntityTypeContainerService.DeleteAsync and overrides it in ElementContainerService to clean up umbracoRelation rows before deleting the folder node — fixing a FK violation (→ 500) that occurred when a folder's child element had been moved to the recycle bin.

• Modified public API: None — all changed types are internal
• Other changes: Element folders that have a relation record (e.g. a previously-trashed child created a relateParentElementContainerOnElementDelete relation) can now be deleted without a 500. Content-type/media-type/data-type folder deletes are unaffected (hook is a no-op for them).

---

No critical or important issues found.

Suggestions

• src/Umbraco.Core/Services/ElementContainerService.cs:382: DeleteLockedAsync calls DeleteContainerRelations(container) directly, while the base's DeleteAsync now calls it via OnDeletingContainer. The two paths are consistent, but you could replace the direct call in DeleteLockedAsync with OnDeletingContainer(container) for symmetry. Minor — the current code is correct.

---

Approved

Clean, minimal, well-targeted fix. The integration test is thorough — it wires RelateOnTrashNotificationHandler (the real relation creator), asserts the relation exists before and is gone after deletion, and verifies the trashed element is still present and will restore to root. No breaking changes; the hook is additive on internal classes only.

[Copilot]

Pull request overview

Fixes element folder deletion failing with FK violations when a previously-contained element was moved to the recycle bin, by ensuring relations referencing the container node are cleaned up as part of the standard container delete flow.

Changes:

• Add a protected virtual OnDeletingContainer(EntityContainer) hook to EntityTypeContainerService.DeleteAsync and invoke it inside the delete scope immediately before deleting the container node.
• Override the hook in ElementContainerService to reuse existing relation-cleanup logic (DeleteContainerRelations) for all element-folder deletions (not only recycle-bin deletions).
• Add an integration test covering “trash element → delete original folder” and wire up trash-related notification handlers needed to reproduce the relation creation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ElementContainerServiceTests.Delete.cs	Adds an integration test verifying element-folder deletion cleans up relations created when child elements are trashed.
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ElementContainerServiceTests.cs	Registers RelateOnTrashNotificationHandler in the fixture and exposes IElementRecycleBinQueryService for assertions.
src/Umbraco.Core/Services/EntityTypeContainerService.cs	Introduces and invokes an overridable pre-delete hook (OnDeletingContainer) within the delete scope.
src/Umbraco.Core/Services/ElementContainerService.cs	Overrides the pre-delete hook to delete all relations referencing the container node before removal.

[lauraneto]

This works well, but when looking further into it, it feels like an oversight that DeleteAsync is not using ElementContainerService.DeleteLockedAsync as well. This could cause more discrepancies between the different flows in the future.
I've created a branch on top of this one adjusting the code so that DeleteLockedAsync is used, instead of adding more logic to EntityTypeContainerService: v18/bugfix/23387-clear-relations-on-entity-container-delete...v18/bugfix/23387-clear-relations-on-entity-container-delete-v2
Let me know what you think!

[AndyButland]

Thanks @lauraneto - yes, on balance I like your approach better. I'll merge it in and see if there are any other improvements to make (the two primitive booleans of mustBeTrashed and requireEmpty for example seem related and maybe could be combined into an enum).

There is one downside which is similar to your concern about divergence, which is that we now won't be calling the base DeleteAsync for containers for element containers. So if something was added there (e.g. logging, auditing) we wouldn't automatically get it. But I think that's justified. Entity containers are already more complex than the existing ones for data types, document types etc., as they have recycle bin support, so it makes sense we diverge from delete and align more with what we do for content and media.

[AndyButland]

I've applied your update and a couple of follow-up amends @lauraneto, and retested using the steps in the PR description.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud