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

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions