mirror of
https://github.com/rommapp/romm.git
synced 2026-06-28 14:56:01 +00:00
fix(roms): repair multi-file ROM downloads broken by deferred file stats
PR #3425 dropped `lazy="joined"` from `RomFile.rom` and removed the `joinedload(RomFile.rom)` from the ROM loaders to speed up the gallery query. That left the `RomFile.rom` backref unpopulated. Single-file downloads only read `RomFile.full_path` (built from `file_path`/ `file_name`), so they kept working, but multi-file (game folder) downloads call `file_name_for_download()` / `is_top_level`, which read `self.rom.full_path`. With no eager-loaded backref, that triggered a lazy load on a detached instance once the handler session closed, raising `DetachedInstanceError` and returning a 500. Rather than reverting the loader changes (and the gallery gains), wire the `RomFile.rom` backref up in Python from the parent ROM we already hold in memory, via `set_committed_value`. This is zero extra DB cost and only runs on the detail/download paths (`with_details` and `get_roms_by_fs_name`); the optimized `filter_roms` gallery query is untouched. https://claude.ai/code/session_01PSXKmejPRzdxLFMN6P2QQ4
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user