Skip to content

Commit 4d54673

Browse files
gantoinegantoine
authored
Merge pull request #3091 from rommapp/copilot/fix-saturn-platform-error
Fix 500 error when loading platforms with ROMs whose filenames start with region tags
2 parents 38b311d + e3d9bfe commit 4d54673

File tree

3 files changed

+244
-3
lines changed

3 files changed

+244
-3
lines changed

‎backend/alembic/versions/0071_sibling_roms_fs_name.py‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
"""Fix sibling_roms view to exclude empty fs_name_no_tags from matching
2+
3+
Revision ID: 0071_sibling_roms_fs_name
4+
Revises: 0070_ss_age_ratings
5+
Create Date: 2026-03-08 22:45:44.767000
6+
7+
"""
8+
9+
import sqlalchemy as sa
10+
from alembic import op
11+
12+
from utils.database import is_postgresql
13+
14+
# revision identifiers, used by Alembic.
15+
revision = "0071_sibling_roms_fs_name"
16+
down_revision = "0070_ss_age_ratings"
17+
branch_labels = None
18+
depends_on = None
19+
20+
21+
def upgrade() -> None:
22+
connection = op.get_bind()
23+
null_safe_equal_operator = (
24+
"IS NOT DISTINCT FROM" if is_postgresql(connection) else "<=>"
25+
)
26+
27+
connection.execute(
28+
sa.text(f"""
29+
CREATE OR REPLACE VIEW sibling_roms AS
30+
SELECT
31+
r1.id AS rom_id,
32+
r2.id AS sibling_rom_id,
33+
r1.platform_id AS platform_id,
34+
NOW() AS created_at,
35+
NOW() AS updated_at,
36+
CASE WHEN r1.igdb_id {null_safe_equal_operator} r2.igdb_id THEN r1.igdb_id END AS igdb_id,
37+
CASE WHEN r1.moby_id {null_safe_equal_operator} r2.moby_id THEN r1.moby_id END AS moby_id,
38+
CASE WHEN r1.ss_id {null_safe_equal_operator} r2.ss_id THEN r1.ss_id END AS ss_id,
39+
CASE WHEN r1.launchbox_id {null_safe_equal_operator} r2.launchbox_id THEN r1.launchbox_id END AS launchbox_id,
40+
CASE WHEN r1.ra_id {null_safe_equal_operator} r2.ra_id THEN r1.ra_id END AS ra_id,
41+
CASE WHEN r1.hasheous_id {null_safe_equal_operator} r2.hasheous_id THEN r1.hasheous_id END AS hasheous_id,
42+
CASE WHEN r1.tgdb_id {null_safe_equal_operator} r2.tgdb_id THEN r1.tgdb_id END AS tgdb_id
43+
FROM
44+
roms r1
45+
JOIN
46+
roms r2
47+
ON
48+
r1.platform_id = r2.platform_id
49+
AND r1.id != r2.id
50+
AND (
51+
(r1.igdb_id = r2.igdb_id AND r1.igdb_id IS NOT NULL)
52+
OR
53+
(r1.moby_id = r2.moby_id AND r1.moby_id IS NOT NULL)
54+
OR
55+
(r1.ss_id = r2.ss_id AND r1.ss_id IS NOT NULL)
56+
OR
57+
(r1.launchbox_id = r2.launchbox_id AND r1.launchbox_id IS NOT NULL)
58+
OR
59+
(r1.ra_id = r2.ra_id AND r1.ra_id IS NOT NULL)
60+
OR
61+
(r1.hasheous_id = r2.hasheous_id AND r1.hasheous_id IS NOT NULL)
62+
OR
63+
(r1.tgdb_id = r2.tgdb_id AND r1.tgdb_id IS NOT NULL)
64+
OR
65+
(r1.fs_name_no_tags = r2.fs_name_no_tags AND r1.fs_name_no_tags != '')
66+
);
67+
"""), # nosec B608
68+
)
69+
70+
71+
def downgrade() -> None:
72+
connection = op.get_bind()
73+
null_safe_equal_operator = (
74+
"IS NOT DISTINCT FROM" if is_postgresql(connection) else "<=>"
75+
)
76+
77+
# Restore previous view without the non-empty guard on fs_name_no_tags
78+
connection.execute(
79+
sa.text(f"""
80+
CREATE OR REPLACE VIEW sibling_roms AS
81+
SELECT
82+
r1.id AS rom_id,
83+
r2.id AS sibling_rom_id,
84+
r1.platform_id AS platform_id,
85+
NOW() AS created_at,
86+
NOW() AS updated_at,
87+
CASE WHEN r1.igdb_id {null_safe_equal_operator} r2.igdb_id THEN r1.igdb_id END AS igdb_id,
88+
CASE WHEN r1.moby_id {null_safe_equal_operator} r2.moby_id THEN r1.moby_id END AS moby_id,
89+
CASE WHEN r1.ss_id {null_safe_equal_operator} r2.ss_id THEN r1.ss_id END AS ss_id,
90+
CASE WHEN r1.launchbox_id {null_safe_equal_operator} r2.launchbox_id THEN r1.launchbox_id END AS launchbox_id,
91+
CASE WHEN r1.ra_id {null_safe_equal_operator} r2.ra_id THEN r1.ra_id END AS ra_id,
92+
CASE WHEN r1.hasheous_id {null_safe_equal_operator} r2.hasheous_id THEN r1.hasheous_id END AS hasheous_id,
93+
CASE WHEN r1.tgdb_id {null_safe_equal_operator} r2.tgdb_id THEN r1.tgdb_id END AS tgdb_id
94+
FROM
95+
roms r1
96+
JOIN
97+
roms r2
98+
ON
99+
r1.platform_id = r2.platform_id
100+
AND r1.id != r2.id
101+
AND (
102+
(r1.igdb_id = r2.igdb_id AND r1.igdb_id IS NOT NULL)
103+
OR
104+
(r1.moby_id = r2.moby_id AND r1.moby_id IS NOT NULL)
105+
OR
106+
(r1.ss_id = r2.ss_id AND r1.ss_id IS NOT NULL)
107+
OR
108+
(r1.launchbox_id = r2.launchbox_id AND r1.launchbox_id IS NOT NULL)
109+
OR
110+
(r1.ra_id = r2.ra_id AND r1.ra_id IS NOT NULL)
111+
OR
112+
(r1.hasheous_id = r2.hasheous_id AND r1.hasheous_id IS NOT NULL)
113+
OR
114+
(r1.tgdb_id = r2.tgdb_id AND r1.tgdb_id IS NOT NULL)
115+
OR
116+
(r1.fs_name_no_tags = r2.fs_name_no_tags)
117+
);
118+
"""), # nosec B608
119+
)

‎backend/handler/database/roms_handler.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
noload,
3131
selectinload,
3232
)
33-
from sqlalchemy.sql.elements import KeyedColumnElement
33+
from sqlalchemy.sql.elements import ColumnElement
3434
from sqlalchemy.sql.selectable import Select
3535

3636
from config import ROMM_DB_DRIVER
@@ -99,7 +99,9 @@
9999

100100

101101
def _create_metadata_id_case(
102-
prefix: str, id_column: KeyedColumnElement, platform_id_column: KeyedColumnElement
102+
prefix: str,
103+
id_column: ColumnElement,
104+
platform_id_column: ColumnElement,
103105
):
104106
return case(
105107
(
@@ -693,7 +695,7 @@ def filter_roms(
693695
),
694696
_create_metadata_id_case(
695697
"fs",
696-
base_subquery.c.fs_name_no_tags,
698+
func.nullif(base_subquery.c.fs_name_no_tags, ""),
697699
base_subquery.c.platform_id,
698700
),
699701
_create_metadata_id_case(
@@ -866,6 +868,7 @@ def get_roms_scalar(
866868
statuses_logic=kwargs.get("statuses_logic", "any"),
867869
player_counts_logic=kwargs.get("player_counts_logic", "any"),
868870
user_id=kwargs.get("user_id", None),
871+
group_by_meta_id=kwargs.get("group_by_meta_id", False),
869872
)
870873
return session.scalars(roms).all()
871874

‎backend/tests/handler/test_db_handler.py‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,125 @@ def test_filter_by_search_term_with_multiple_terms(platform: Platform):
181181
assert actual_rom_ids_single == expected_rom_ids_single
182182

183183

184+
def test_sibling_roms_empty_fs_name_no_tags_not_matched(platform: Platform):
185+
"""ROMs with empty fs_name_no_tags should NOT be matched as siblings.
186+
187+
Japanese ROMs often have names starting with region tags (e.g., "(Japan) Sonic Jam.iso"),
188+
which results in an empty fs_name_no_tags. Without a guard, all such ROMs on the same
189+
platform would incorrectly be matched as siblings of each other.
190+
"""
191+
rom1 = db_rom_handler.add_rom(
192+
Rom(
193+
platform_id=platform.id,
194+
name="(Japan) Game A",
195+
slug="japan-game-a",
196+
fs_name="(Japan) Game A.iso",
197+
fs_name_no_tags="", # Empty due to leading region tag
198+
fs_name_no_ext="(Japan) Game A",
199+
fs_extension="iso",
200+
fs_path=f"{platform.slug}/roms",
201+
)
202+
)
203+
rom2 = db_rom_handler.add_rom(
204+
Rom(
205+
platform_id=platform.id,
206+
name="(Japan) Game B",
207+
slug="japan-game-b",
208+
fs_name="(Japan) Game B.iso",
209+
fs_name_no_tags="", # Empty due to leading region tag
210+
fs_name_no_ext="(Japan) Game B",
211+
fs_extension="iso",
212+
fs_path=f"{platform.slug}/roms",
213+
)
214+
)
215+
216+
loaded_rom1 = db_rom_handler.get_rom(rom1.id)
217+
loaded_rom2 = db_rom_handler.get_rom(rom2.id)
218+
assert loaded_rom1 is not None
219+
assert loaded_rom2 is not None
220+
221+
# ROMs with empty fs_name_no_tags should NOT be siblings of each other
222+
sibling_ids1 = {s.id for s in loaded_rom1.sibling_roms}
223+
sibling_ids2 = {s.id for s in loaded_rom2.sibling_roms}
224+
assert rom2.id not in sibling_ids1
225+
assert rom1.id not in sibling_ids2
226+
227+
228+
def test_sibling_roms_nonempty_fs_name_no_tags_matched(platform: Platform):
229+
"""ROMs with matching non-empty fs_name_no_tags SHOULD be matched as siblings.
230+
231+
For example, "Sonic Jam (USA).iso" and "Sonic Jam (Japan).iso" both have
232+
fs_name_no_tags = "Sonic Jam" and should be considered siblings.
233+
"""
234+
rom1 = db_rom_handler.add_rom(
235+
Rom(
236+
platform_id=platform.id,
237+
name="Sonic Jam (USA)",
238+
slug="sonic-jam-usa",
239+
fs_name="Sonic Jam (USA).iso",
240+
fs_name_no_tags="Sonic Jam",
241+
fs_name_no_ext="Sonic Jam (USA)",
242+
fs_extension="iso",
243+
fs_path=f"{platform.slug}/roms",
244+
)
245+
)
246+
rom2 = db_rom_handler.add_rom(
247+
Rom(
248+
platform_id=platform.id,
249+
name="Sonic Jam (Japan)",
250+
slug="sonic-jam-japan",
251+
fs_name="Sonic Jam (Japan).iso",
252+
fs_name_no_tags="Sonic Jam",
253+
fs_name_no_ext="Sonic Jam (Japan)",
254+
fs_extension="iso",
255+
fs_path=f"{platform.slug}/roms",
256+
)
257+
)
258+
259+
loaded_rom1 = db_rom_handler.get_rom(rom1.id)
260+
loaded_rom2 = db_rom_handler.get_rom(rom2.id)
261+
assert loaded_rom1 is not None
262+
assert loaded_rom2 is not None
263+
264+
# ROMs with same non-empty fs_name_no_tags should be siblings
265+
sibling_ids1 = {s.id for s in loaded_rom1.sibling_roms}
266+
sibling_ids2 = {s.id for s in loaded_rom2.sibling_roms}
267+
assert rom2.id in sibling_ids1
268+
assert rom1.id in sibling_ids2
269+
270+
271+
def test_group_by_meta_id_with_empty_fs_name_no_tags(platform: Platform):
272+
"""ROMs with empty fs_name_no_tags should each get their own group when using
273+
group_by_meta_id, not be grouped into a single catch-all group.
274+
275+
Without the fix, all unmatched ROMs with empty fs_name_no_tags would be
276+
grouped under "fs-<platform_id>-" and only 1 would be shown.
277+
"""
278+
rom_names = ["(Japan) Game A", "(Japan) Game B", "(Japan) Game C"]
279+
for name in rom_names:
280+
db_rom_handler.add_rom(
281+
Rom(
282+
platform_id=platform.id,
283+
name=name,
284+
slug=name.lower().replace(" ", "-").replace("(", "").replace(")", ""),
285+
fs_name=f"{name}.iso",
286+
fs_name_no_tags="", # Empty due to leading region tag
287+
fs_name_no_ext=name,
288+
fs_extension="iso",
289+
fs_path=f"{platform.slug}/roms",
290+
)
291+
)
292+
293+
roms = db_rom_handler.get_roms_scalar(
294+
platform_ids=[platform.id],
295+
order_by="name",
296+
order_dir="asc",
297+
group_by_meta_id=True,
298+
)
299+
# All 3 ROMs should be shown, not collapsed into 1
300+
assert len(roms) == len(rom_names)
301+
302+
184303
def test_natural_sort_order(platform: Platform):
185304
"""Numbers in names should sort numerically, not lexicographically."""
186305
for name in ["Game 10", "Game 2", "Game 1"]:

0 commit comments

Comments
 (0)