diff --git a/backend/endpoints/roms/__init__.py b/backend/endpoints/roms/__init__.py index 8178eb920..622091cdf 100644 --- 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 diff --git a/backend/tests/endpoints/roms/test_rom.py b/backend/tests/endpoints/roms/test_rom.py index 181ffb445..fc45acba4 100644 --- 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 ):