mirror of
https://github.com/rommapp/romm.git
synced 2026-10-12 02:21:58 +00:00
perf(scan): stop eager-loading files in get_roms_by_fs_name
Restore the "platform only" contract of `get_roms_by_fs_name` (per its docstring) by dropping the `selectinload(Rom.files)` + `joinedload`. That load only existed for `scan_rom`'s rare `fs_rom["files"] or rom.files` fallback, but it forced files (and a per-file join back to roms) for every ROM in a scan batch — expensive on large platforms, and only used when the filesystem scan yielded no files. Instead, fetch the persisted files on demand: `scan_rom` now resolves match files via a small helper that returns the filesystem-scanned files, falling back to `db_rom_handler.get_rom_files_by_rom_id(rom.id)` only when there are none. The new getter eager-loads the `RomFile.rom` backref so `is_top_level` keeps working on the detached results (the rare path was already latently broken on master, which loaded files without the backref). https://claude.ai/code/session_01PSXKmejPRzdxLFMN6P2QQ4
This commit is contained in:
@@ -986,9 +986,6 @@ class DBRomsHandler(DBBaseHandler):
|
||||
select(Rom)
|
||||
.options(
|
||||
selectinload(Rom.platform),
|
||||
selectinload(Rom.files).options(
|
||||
joinedload(RomFile.rom).load_only(Rom.fs_path, Rom.fs_name)
|
||||
),
|
||||
)
|
||||
.where(
|
||||
and_(
|
||||
@@ -1189,6 +1186,29 @@ class DBRomsHandler(DBBaseHandler):
|
||||
) -> RomFile | None:
|
||||
return session.scalar(select(RomFile).filter_by(id=id).limit(1))
|
||||
|
||||
@begin_session
|
||||
def get_rom_files_by_rom_id(
|
||||
self,
|
||||
rom_id: int,
|
||||
session: Session = None, # type: ignore
|
||||
) -> list[RomFile]:
|
||||
"""Return a ROM's files with the parent `rom` backref eager-loaded.
|
||||
|
||||
Used by the scan fallback when the filesystem yielded no files: the
|
||||
backref keeps `is_top_level` / `file_name_for_download` usable on the
|
||||
detached results without `get_roms_by_fs_name` eager-loading files for
|
||||
every ROM in a scan batch.
|
||||
"""
|
||||
return list(
|
||||
session.scalars(
|
||||
select(RomFile)
|
||||
.options(joinedload(RomFile.rom).load_only(Rom.fs_path, Rom.fs_name))
|
||||
.filter_by(rom_id=rom_id)
|
||||
)
|
||||
.unique()
|
||||
.all()
|
||||
)
|
||||
|
||||
@begin_session
|
||||
def update_rom_file(
|
||||
self,
|
||||
|
||||
@@ -47,7 +47,7 @@ from logger.logger import log
|
||||
from models.assets import Save, Screenshot, State
|
||||
from models.firmware import Firmware
|
||||
from models.platform import Platform
|
||||
from models.rom import Rom
|
||||
from models.rom import Rom, RomFile
|
||||
from models.user import User
|
||||
from utils import emoji
|
||||
|
||||
@@ -373,6 +373,20 @@ async def scan_rom(
|
||||
}
|
||||
)
|
||||
|
||||
# Files fed to metadata matchers: the freshly-scanned filesystem files, or
|
||||
# the persisted files as a fallback. `rom` is detached and no longer carries
|
||||
# eager-loaded files (get_roms_by_fs_name only loads `platform`), so fetch
|
||||
# persisted files on demand — and only when the filesystem yielded none.
|
||||
_persisted_files: list[RomFile] | None = None
|
||||
|
||||
def rom_files_for_matching() -> list[RomFile]:
|
||||
nonlocal _persisted_files
|
||||
if fs_rom["files"]:
|
||||
return fs_rom["files"]
|
||||
if _persisted_files is None:
|
||||
_persisted_files = db_rom_handler.get_rom_files_by_rom_id(rom.id)
|
||||
return _persisted_files
|
||||
|
||||
async def fetch_playmatch_hash_match() -> PlaymatchRomMatch:
|
||||
if (
|
||||
meta_playmatch_handler.is_enabled()
|
||||
@@ -385,7 +399,7 @@ async def scan_rom(
|
||||
or scan_type == ScanType.UNMATCHED
|
||||
)
|
||||
):
|
||||
return await meta_playmatch_handler.lookup_rom(fs_rom["files"] or rom.files)
|
||||
return await meta_playmatch_handler.lookup_rom(rom_files_for_matching())
|
||||
|
||||
return PlaymatchRomMatch(
|
||||
igdb_id=None,
|
||||
@@ -418,7 +432,7 @@ async def scan_rom(
|
||||
)
|
||||
):
|
||||
return await meta_hasheous_handler.lookup_rom(
|
||||
platform.slug, fs_rom["files"] or rom.files
|
||||
platform.slug, rom_files_for_matching()
|
||||
)
|
||||
|
||||
return HasheousRom(hasheous_id=None, igdb_id=None, tgdb_id=None, ra_id=None)
|
||||
@@ -645,7 +659,7 @@ async def scan_rom(
|
||||
|
||||
# Use the file hashes for lookup
|
||||
game_by_hash, is_not_game = await meta_ss_handler.lookup_rom(
|
||||
rom, platform.ss_id, fs_rom["files"] or rom.files
|
||||
rom, platform.ss_id, rom_files_for_matching()
|
||||
)
|
||||
if game_by_hash.get("ss_id") or is_not_game:
|
||||
return game_by_hash
|
||||
|
||||
@@ -96,6 +96,22 @@ def test_multi_file_rom_backref_survives_session_close(multi_file_rom: Rom):
|
||||
assert file.rom.full_path == folder_path
|
||||
|
||||
|
||||
def test_get_rom_files_by_rom_id_eager_loads_parent(multi_file_rom: Rom):
|
||||
"""The scan fallback fetches a ROM's files on demand (since
|
||||
`get_roms_by_fs_name` no longer eager-loads them). The returned files must
|
||||
carry the `rom` backref so `is_top_level` works on the detached results.
|
||||
"""
|
||||
folder_path = f"{multi_file_rom.fs_path}/{multi_file_rom.fs_name}"
|
||||
|
||||
files = db_rom_handler.get_rom_files_by_rom_id(multi_file_rom.id)
|
||||
|
||||
assert {f.file_name for f in files} == {"disc1.bin", "disc2.bin"}
|
||||
for file in files:
|
||||
# Detached access — would raise DetachedInstanceError without the backref.
|
||||
assert file.rom.full_path == folder_path
|
||||
assert file.is_top_level
|
||||
|
||||
|
||||
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