Skip to content

library-manager: delete_folder corrupts on subfolder name collision when reparenting under a non-root parent #447

Description

@NoveliaYuki

Problem

delete_folder (library-manager/backend/services/folder_service.py:274-278) re-homes a deleted folder's subfolders to the deleted folder's parent, deduping names on collision. But it assigns sub.parent_folder_id = new_parent_id before calling _next_available_name(...):

for sub in subfolders:
    sub.parent_folder_id = new_parent_id            # reparent first…
    sub.name = _next_available_name(                # …then dedupe the name
        db, folder.library_id, new_parent_id, sub.name, exclude_id=sub.id
    )

_next_available_name issues a query, which triggers a SQLAlchemy autoflush. The flush writes the reparented row with its still-colliding name and violates the uq_folder_sibling_name UNIQUE constraint → sqlalchemy.exc.IntegrityError, aborting the whole delete.

This only manifests when the deleted folder's parent is non-NULL (a nested folder). The root case (parent_folder_id IS NULL) is masked because SQLite treats NULL as distinct in UNIQUE constraints, so the premature flush doesn't collide — which is why the existing root-only test never caught it.

Reproduction: a folder Parent contains both Drafts and Mid; Mid also contains Drafts. Deleting Mid should reparent its Drafts under Parent as Drafts (2), but instead raises IntegrityError.

Expected

Deleting a folder reparents its subfolders to the parent, renaming any colliding subfolder with a numeric suffix (Drafts (2), Drafts (3), …), regardless of whether the parent is root or nested. No IntegrityError.

Fix

Assign the deduped sub.name before sub.parent_folder_id, or wrap the re-home loop in with db.no_autoflush:. Keep the existing root-case behaviour intact.

Tests

The bug is pinned by a strict xfail that already encodes the expected behaviour:

  • tests/unit/test_service_folder.py::test_delete_folder_collision_under_nonroot_parent

The fix MUST remove the @pytest.mark.xfail decorator so the test becomes a normal passing assertion (a strict xfail that starts passing is reported as a failure). After the fix, the full suite (./scripts/run_tests.sh) MUST be green with the ≥95% coverage gate satisfied, and tests/unit/test_service_folder.py must pass with no xfails.

Scope

library-manager/backend/services/folder_service.py only (plus removing the xfail marker in the pinned test).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions