diff --git a/backend/handler/database/roms_handler.py b/backend/handler/database/roms_handler.py index b91be4e16..4a75bb96a 100644 --- a/backend/handler/database/roms_handler.py +++ b/backend/handler/database/roms_handler.py @@ -30,6 +30,7 @@ from sqlalchemy.orm import ( selectinload, undefer, ) +from sqlalchemy.orm.attributes import set_committed_value from sqlalchemy.sql.elements import ColumnElement from sqlalchemy.sql.selectable import Select @@ -123,6 +124,26 @@ def _create_metadata_id_case( ) +def _link_rom_files_to_parent(result: "Rom | Iterable[Rom] | None") -> None: + """Populate the `RomFile.rom` backref from the already-loaded parent ROM. + + The detail/download loaders eager-load `Rom.files` but not the reverse + `RomFile.rom` relationship. `RomFile.full_path` / `is_top_level` / + `file_name_for_download` read `self.rom`, which would otherwise trigger a + lazy load that fails once the session closes (`DetachedInstanceError`, + surfacing as a 500 on multi-file ROM downloads). We already hold the parent + in memory, so wire the backref up directly instead of re-adding a per-file + `joinedload` (which would undo the gallery query gains from #3425). + """ + if result is None: + return + + roms = [result] if isinstance(result, Rom) else result + for rom in roms: + for file in rom.files: + set_committed_value(file, "rom", rom) + + def with_details(func): @functools.wraps(func) def wrapper(*args, **kwargs): @@ -158,7 +179,9 @@ def with_details(func): undefer(Rom.multi_file), undefer(Rom.top_level_file_count), ) - return func(*args, **kwargs) + result = func(*args, **kwargs) + _link_rom_files_to_parent(result) + return result return wrapper @@ -993,6 +1016,7 @@ class DBRomsHandler(DBBaseHandler): .all() ) + _link_rom_files_to_parent(roms) return {rom.fs_name: rom for rom in roms} @begin_session diff --git a/backend/tests/handler/test_db_handler.py b/backend/tests/handler/test_db_handler.py index df1389a4d..54b7f85f1 100644 --- a/backend/tests/handler/test_db_handler.py +++ b/backend/tests/handler/test_db_handler.py @@ -14,7 +14,7 @@ from handler.database import ( ) from models.assets import Save, Screenshot, State from models.platform import Platform -from models.rom import Rom +from models.rom import Rom, RomFile from models.user import Role, User @@ -73,6 +73,40 @@ def test_roms(rom: Rom, platform: Platform): assert len(roms) == 1 +def test_multi_file_rom_backref_survives_session_close(rom: Rom, platform: Platform): + """Multi-file ROM downloads read `file.rom.full_path` after the handler + session closes. The detail loaders eager-load `Rom.files` but not the + reverse `RomFile.rom` relationship, so without the backref being populated + this raises `DetachedInstanceError` (a 500 on the download endpoint). + """ + folder_path = f"{rom.fs_path}/{rom.fs_name}" + for file_name in ("disc1.bin", "disc2.bin"): + db_rom_handler.add_rom_file( + RomFile( + rom_id=rom.id, + file_name=file_name, + file_path=folder_path, + file_size_bytes=1, + ) + ) + + fetched = db_rom_handler.get_rom(rom.id) + assert fetched is not None + assert len(fetched.files) == 2 + + # These all dereference `file.rom` on a now-detached instance. + for file in fetched.files: + assert file.rom.full_path == folder_path + assert file.is_top_level + assert file.file_name_for_download() == file.file_name + + # `get_roms_by_ids` (used by the bulk zip download) must behave the same. + by_ids = db_rom_handler.get_roms_by_ids([rom.id]) + assert len(by_ids) == 1 + for file in by_ids[0].files: + assert file.rom.full_path == folder_path + + def test_filter_last_played(rom: Rom, platform: Platform, admin_user: User): second_rom = db_rom_handler.add_rom( Rom(