Merge pull request #3216 from rommapp/copilot/fix-delete-roms-in-subdirectories

fix: correctly delete nested (subdirectory) ROMs from filesystem
2026-06-29 07:16:28 +00:00 · 2026-04-03 14:37:07 -04:00
parent f90d5d5e58 ce42957162
commit 25362cf80b
2 changed files with 168 additions and 12 deletions
--- a/backend/endpoints/roms/init.py
+++ b/backend/endpoints/roms/init.py
@@ -1472,6 +1472,37 @@ async def delete_roms(
            continue

        try:
+            if id in delete_from_fs:
+                log.info(f"Deleting {hl(rom.fs_name)} from filesystem")
+                try:
+                    rom_path = f"{rom.fs_path}/{rom.fs_name}"
+                    full_path = fs_rom_handler.validate_path(rom_path)
+                    if full_path.is_dir():
+                        await fs_rom_handler.remove_directory(rom_path)
+                    else:
+                        await fs_rom_handler.remove_file(rom_path)
+                        # Clean up empty parent directory if it becomes empty
+                        parent = full_path.parent
+                        if (
+                            parent != fs_rom_handler.base_path
+                            and parent.is_dir()
+                            and not any(parent.iterdir())
+                        ):
+                            try:
+                                await fs_rom_handler.remove_directory(
+                                    str(parent.relative_to(fs_rom_handler.base_path))
+                                )
+                            except OSError as dir_err:
+                                log.warning(
+                                    f"Couldn't clean up empty parent directory for {hl(rom.fs_name)}: {dir_err}"
+                                )
+                except FileNotFoundError:
+                    error = f"Rom file {hl(rom.fs_name)} not found for platform {hl(rom.platform_display_name, color=BLUE)}[{hl(rom.platform_slug)}]"
+                    log.error(error)
+                    errors.append(error)
+                    failed_items += 1
+                    continue
+
            log.info(
                f"Deleting {hl(str(rom.name or 'ROM'), color=BLUE)} [{hl(rom.fs_name)}] from database"
            )
@@ -1484,18 +1515,6 @@ async def delete_roms(
                    f"Couldn't find resources to delete for {hl(str(rom.name or 'ROM'), color=BLUE)}"
                )

-            if id in delete_from_fs:
-                log.info(f"Deleting {hl(rom.fs_name)} from filesystem")
-                try:
-                    file_path = f"{rom.fs_path}/{rom.fs_name}"
-                    await fs_rom_handler.remove_file(file_path=file_path)
-                except FileNotFoundError:
-                    error = f"Rom file {hl(rom.fs_name)} not found for platform {hl(rom.platform_display_name, color=BLUE)}[{hl(rom.platform_slug)}]"
-                    log.error(error)
-                    errors.append(error)
-                    failed_items += 1
-                    continue
-
            successful_items += 1
        except Exception as e:
            failed_items += 1
--- a/backend/tests/endpoints/roms/test_rom.py
+++ b/backend/tests/endpoints/roms/test_rom.py
@@ -124,6 +124,143 @@ def test_delete_roms(client: TestClient, access_token: str, rom: Rom):
    assert body["successful_items"] == 1


+@patch(
+    "endpoints.roms.fs_rom_handler.remove_directory",
+    new_callable=AsyncMock,
+)
+@patch(
+    "endpoints.roms.fs_rom_handler.remove_file",
+    new_callable=AsyncMock,
+)
+@patch(
+    "endpoints.roms.fs_rom_handler.validate_path",
+)
+def test_delete_roms_from_fs_flat(
+    mock_validate_path,
+    mock_remove_file,
+    mock_remove_directory,
+    client: TestClient,
+    access_token: str,
+    rom: Rom,
+):
+    """Test that flat (non-directory) ROM files are deleted from filesystem."""
+    from pathlib import Path
+    from unittest.mock import MagicMock
+
+    mock_path = MagicMock(spec=Path)
+    mock_path.is_dir.return_value = False
+    mock_path.parent.is_dir.return_value = False
+    mock_validate_path.return_value = mock_path
+
+    response = client.post(
+        "/api/roms/delete",
+        headers={"Authorization": f"Bearer {access_token}"},
+        json={"roms": [rom.id], "delete_from_fs": [rom.id]},
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+    body = response.json()
+    assert body["successful_items"] == 1
+    assert body["failed_items"] == 0
+    mock_remove_file.assert_called_once()
+    mock_remove_directory.assert_not_called()
+
+
+@patch(
+    "endpoints.roms.fs_rom_handler.remove_directory",
+    new_callable=AsyncMock,
+)
+@patch(
+    "endpoints.roms.fs_rom_handler.remove_file",
+    new_callable=AsyncMock,
+)
+@patch(
+    "endpoints.roms.fs_rom_handler.validate_path",
+)
+def test_delete_roms_from_fs_flat_cleans_empty_parent(
+    mock_validate_path,
+    mock_remove_file,
+    mock_remove_directory,
+    client: TestClient,
+    access_token: str,
+    rom: Rom,
+):
+    """Test that empty parent directories are cleaned up after flat ROM file deletion."""
+    from pathlib import Path
+    from unittest.mock import MagicMock
+
+    mock_path = MagicMock(spec=Path)
+    mock_path.is_dir.return_value = False
+    # Parent is not the base_path (a MagicMock will not equal a real Path), is a dir, and is empty
+    mock_path.parent.is_dir.return_value = True
+    mock_path.parent.__iter__ = lambda self: iter([])  # empty directory
+    mock_validate_path.return_value = mock_path
+
+    response = client.post(
+        "/api/roms/delete",
+        headers={"Authorization": f"Bearer {access_token}"},
+        json={"roms": [rom.id], "delete_from_fs": [rom.id]},
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+    body = response.json()
+    assert body["successful_items"] == 1
+    assert body["failed_items"] == 0
+    mock_remove_file.assert_called_once()
+    # remove_directory should be called to clean up the empty parent dir
+    mock_remove_directory.assert_called_once()
+
+
+@patch(
+    "endpoints.roms.fs_rom_handler.remove_directory",
+    new_callable=AsyncMock,
+)
+@patch(
+    "endpoints.roms.fs_rom_handler.validate_path",
+)
+def test_delete_roms_from_fs_nested(
+    mock_validate_path,
+    mock_remove_directory,
+    client: TestClient,
+    access_token: str,
+    platform: Platform,
+):
+    """Test that nested (directory) ROMs are deleted using remove_directory."""
+    from pathlib import Path
+    from unittest.mock import MagicMock
+
+    from handler.database import db_rom_handler
+    from models.rom import Rom
+
+    nested_rom = Rom(
+        platform_id=platform.id,
+        name="Nested Game",
+        slug="nested-game",
+        fs_name="Nested Game",
+        fs_name_no_tags="Nested Game",
+        fs_name_no_ext="Nested Game",
+        fs_extension="",
+        fs_path=f"{platform.slug}/roms",
+    )
+    nested_rom = db_rom_handler.add_rom(nested_rom)
+
+    mock_path = MagicMock(spec=Path)
+    mock_path.is_dir.return_value = True
+    mock_validate_path.return_value = mock_path
+
+    response = client.post(
+        "/api/roms/delete",
+        headers={"Authorization": f"Bearer {access_token}"},
+        json={"roms": [nested_rom.id], "delete_from_fs": [nested_rom.id]},
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+    body = response.json()
+    assert body["successful_items"] == 1
+    assert body["failed_items"] == 0
+    mock_remove_directory.assert_called_once()
+
+
 def test_update_rom_user_props_with_data_envelope(
    client: TestClient, access_token: str, rom: Rom
 ):