From e2702ac67364533d9e17f7ea00cd57bb04373984 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 16 Jun 2026 23:35:23 +0200 Subject: [PATCH] fix(trash): cascade soft-delete and restore across folder subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes G9. DELETE on a folder now flips is_trashed on every descendant folder and file under it in a single CTE pipeline (lpath <@ root.lpath covers the whole subtree via the GiST index). Previously only the root row was flipped — descendants stayed live, directly addressable via their full path, and confused desktop-sync tree walks that expected the parent-collection 404 to imply the children were gone too. restore_from_trash mirrors the cascade: descendants where the original_*_parent_id column is NULL are the ones we cascade-trashed, so they get cascade-restored too. Descendants that were independently trashed before the parent went to trash have original_*_parent_id set, so they stay in trash and remain visible as top-level entries in storage.trash_items. No schema migration needed — both original_parent_id (folders) and original_folder_id (files) were already nullable and already encoded 'where this came from when it was independently trashed'; using NULL as the cascade-marker reuses that existing distinction cleanly. Test G9 flipped from KNOWN BUG to assert every descendant 404s after the parent DELETE; new G9b proves the inverse cascade by restoring the trashed root and verifying every descendant comes back at its original path. --- .../repositories/pg/folder_db_repository.rs | 86 ++++++++++++++++--- .../webdav/test_nc_move_copy_delete_trash.sh | 57 ++++++++---- 2 files changed, 114 insertions(+), 29 deletions(-) diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 521388d9..97852fae 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -581,23 +581,55 @@ impl FolderRepository for FolderDbRepository { // ── Trash operations ── async fn move_to_trash(&self, folder_id: &str) -> Result<(), DomainError> { - // Only mark the folder itself as trashed. - // Child files and sub-folders are implicitly hidden because their - // ancestor is trashed — list queries already filter NOT is_trashed, - // and folder navigation won't reach a trashed folder's children. + // Soft-delete the whole subtree in one statement: the root flips + // `is_trashed` and records `original_parent_id` so restore knows + // where to put it back; every descendant (folder or file) that + // wasn't already in trash flips `is_trashed` too but leaves the + // `original_*` column NULL. That NULL is the marker the restore + // path uses to tell "cascade-trashed with the root" from + // "independently trashed earlier" — the latter must stay in + // trash even when the root is restored. + // + // Without this cascade, descendants used to remain `is_trashed = false` + // and stay directly addressable by their full path (PROPFIND on + // `/g9-tree/file.txt` still resolved 207 even though the parent + // collection was gone) — a class of data-integrity drift that + // confused desktop-sync tree walks. let result = retry_on_deadlock("folders.trash", || { sqlx::query_scalar::<_, i64>( r#" - WITH trash_folder AS ( + WITH trash_root AS ( UPDATE storage.folders SET is_trashed = TRUE, trashed_at = NOW(), original_parent_id = parent_id, updated_at = NOW() WHERE id = $1::uuid AND NOT is_trashed + RETURNING id, lpath + ), + trash_descendant_folders AS ( + UPDATE storage.folders f + SET is_trashed = TRUE, + trashed_at = NOW(), + updated_at = NOW() + FROM trash_root tr + WHERE f.lpath <@ tr.lpath + AND f.id != tr.id + AND NOT f.is_trashed + RETURNING 1 + ), + trash_descendant_files AS ( + UPDATE storage.files fi + SET is_trashed = TRUE, + trashed_at = NOW(), + updated_at = NOW() + FROM trash_root tr + JOIN storage.folders f ON f.lpath <@ tr.lpath + WHERE fi.folder_id = f.id + AND NOT fi.is_trashed RETURNING 1 ) - SELECT COUNT(*) FROM trash_folder + SELECT COUNT(*) FROM trash_root "#, ) .bind(folder_id) @@ -618,16 +650,18 @@ impl FolderRepository for FolderDbRepository { folder_id: &str, _original_path: &str, ) -> Result<(), DomainError> { - // Only restore the folder itself. - // Child files were never marked as trashed — they become visible - // again automatically once their parent folder is un-trashed. - // The BEFORE UPDATE trigger recomputes path/lpath when - // original_parent_id is restored; the cascade trigger - // batch-updates all descendants via the GiST lpath index. + // Inverse of the cascade in `move_to_trash`: restore the root + // (BEFORE UPDATE trigger recomputes path/lpath via the parent_id + // change), then un-trash every descendant whose `original_*` + // column is NULL — those are the rows we cascade-trashed + // ourselves. Descendants that were independently trashed + // *before* this folder went to trash have `original_*` set, so + // they correctly stay in trash and continue to show up as + // top-level trash entries via `storage.trash_items`. let result = retry_on_deadlock("folders.restore", || { sqlx::query_scalar::<_, i64>( r#" - WITH restore_folder AS ( + WITH restore_root AS ( UPDATE storage.folders SET is_trashed = FALSE, trashed_at = NULL, @@ -635,9 +669,33 @@ impl FolderRepository for FolderDbRepository { original_parent_id = NULL, updated_at = NOW() WHERE id = $1::uuid AND is_trashed + RETURNING id, lpath + ), + restore_descendant_folders AS ( + UPDATE storage.folders f + SET is_trashed = FALSE, + trashed_at = NULL, + updated_at = NOW() + FROM restore_root rr + WHERE f.lpath <@ rr.lpath + AND f.id != rr.id + AND f.is_trashed + AND f.original_parent_id IS NULL + RETURNING 1 + ), + restore_descendant_files AS ( + UPDATE storage.files fi + SET is_trashed = FALSE, + trashed_at = NULL, + updated_at = NOW() + FROM restore_root rr + JOIN storage.folders f ON f.lpath <@ rr.lpath + WHERE fi.folder_id = f.id + AND fi.is_trashed + AND fi.original_folder_id IS NULL RETURNING 1 ) - SELECT COUNT(*) FROM restore_folder + SELECT COUNT(*) FROM restore_root "#, ) .bind(folder_id) diff --git a/tests/webdav/test_nc_move_copy_delete_trash.sh b/tests/webdav/test_nc_move_copy_delete_trash.sh index 93235d9a..4a26930c 100755 --- a/tests/webdav/test_nc_move_copy_delete_trash.sh +++ b/tests/webdav/test_nc_move_copy_delete_trash.sh @@ -256,7 +256,7 @@ pass "G8: DELETE → 204 + GET 404" # descendant assertions below will trip and you can flip them # to strict 404. # ───────────────────────────────────────────────────────────── -echo " G9: DELETE folder (pinned: descendants currently orphan — KNOWN BUG)" +echo " G9: DELETE folder cascades soft-delete to descendants" nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/g9-tree/" > /dev/null nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/g9-tree/inner/" > /dev/null put_nc_file "g9-tree/file.txt" "G9 file" @@ -265,22 +265,49 @@ STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X DELETE "$NC_FILES_BASE/g9-tre [[ "$STATUS" == "204" ]] \ || fail "G9: folder DELETE expected 204, got $STATUS" -# Folder itself: correctly 404. +# Folder itself: 404. [[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/")" == "404" ]] \ - || fail "G9: folder still present after DELETE — that part should always be 404" + || fail "G9: folder still resolvable after DELETE" -# Descendants: pin the current (buggy) "still alive" status. -# Either current 207 (bug) or future 404 (fix) is acceptable; -# anything else means something has drifted unexpectedly. -CHILD_STATUS=$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt") -DEEP_STATUS=$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt") -if [[ "$CHILD_STATUS" == "207" && "$DEEP_STATUS" == "207" ]]; then - pass "G9: descendants still reachable (file=207, deep=207) — KNOWN BUG pinned: move_to_trash isn't recursive at the row level" -elif [[ "$CHILD_STATUS" == "404" && "$DEEP_STATUS" == "404" ]]; then - fail "G9: descendants now correctly 404 (file=$CHILD_STATUS, deep=$DEEP_STATUS) — bug is fixed, flip this case to strict 404 assertions." -else - fail "G9: mixed/unexpected descendant statuses (file=$CHILD_STATUS, deep=$DEEP_STATUS) — pin needs review" -fi +# Descendants must now also be 404 (cascade soft-delete reaches the +# whole subtree). Previous behaviour left them reachable at their +# full path while the parent was gone — a data-integrity drift that +# confused desktop-sync tree walks. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt")" == "404" ]] \ + || fail "G9: direct-child file still resolvable after parent DELETE — cascade not working" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/")" == "404" ]] \ + || fail "G9: descendant folder still resolvable after parent DELETE — cascade not working" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt")" == "404" ]] \ + || fail "G9: descendant file still resolvable after parent DELETE — cascade not working" +pass "G9: DELETE folder → 204, descendants all 404 (cascade reaches the whole subtree)" + +# G9b — restore the trashed root and verify cascade-restore brings +# every descendant back with the same paths. Cascade-trashed +# descendants (original_parent_id IS NULL) get un-trashed; rows that +# were independently trashed before the folder went to trash stay +# trashed. +echo " G9b: restore the trashed g9-tree → cascade-restore reaches descendants" +BODY=$(nc_curl -X PROPFIND -H "Depth: 1" "$NC_TRASH_BASE/") +G9_TRASHED_HREF=$(extract_response_href_containing "$BODY" "g9-tree") +G9_TRASHED_ID=$(basename "$G9_TRASHED_HREF") +[[ -n "$G9_TRASHED_ID" ]] || fail "G9b: trashed g9-tree not found via PROPFIND" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ + -H "Destination: $NC_FILES_BASE/g9-tree-restored/" \ + "$NC_TRASH_BASE/$G9_TRASHED_ID") +[[ "$STATUS" == "201" || "$STATUS" == "204" ]] \ + || fail "G9b: restore expected 201/204, got $STATUS" +# The folder and ALL its descendants are reachable again at their +# original paths (restore goes to original location, not the +# Destination header). +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/")" == "207" ]] \ + || fail "G9b: root folder not back after restore" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt")" == "207" ]] \ + || fail "G9b: direct-child file not restored alongside parent" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/")" == "207" ]] \ + || fail "G9b: descendant folder not restored alongside parent" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt")" == "207" ]] \ + || fail "G9b: descendant file not restored alongside parent" +pass "G9b: restored g9-tree carries the whole subtree back" # ═════════════════════════════════════════════════════════════ # Group K — Trashbin DAV (depends on G8's deletion above)