Skip to content

Commit 79a8322

Browse files
authored
Merge pull request #4060 from rommapp/fix/hasheous-mameredump-verified
fix(hasheous): read the CHD and DOS signature sources
2 parents 0b816f0 + 509e4d1 commit 79a8322

8 files changed

Lines changed: 226 additions & 5 deletions

File tree

backend/handler/database/roms_handler.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -849,15 +849,22 @@ def _filter_by_verified(self, query: Query, value: bool) -> Query:
849849
"mame_mess_match",
850850
"nointro_match",
851851
"redump_match",
852+
"mame_redump_match",
852853
"whdload_match",
853854
"ra_match",
854855
"fbneo_match",
855856
"puredos_match",
856857
]
857858

859+
# A key absent from `hasheous_metadata` (rows stored before it existed, or
860+
# rows with no Hasheous match at all) extracts as NULL, and NULL poisons
861+
# both the OR and its negation, so the unverified side would drop those
862+
# rows. The JSON path below folds a missing key into false on its own;
863+
# `->>` does not, hence the coalesce.
858864
if ROMM_DB_DRIVER == "postgresql":
859865
conditions = " OR ".join(
860-
f"(hasheous_metadata->>'{key}')::boolean" for key in keys_to_check
866+
f"COALESCE((hasheous_metadata->>'{key}')::boolean, false)"
867+
for key in keys_to_check
861868
)
862869
predicate = text(f"({conditions})")
863870
if not value:

backend/handler/metadata/hasheous_handler.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ class HasheousMetadata(TypedDict):
2828
mame_mess_match: bool
2929
nointro_match: bool
3030
redump_match: bool
31+
mame_redump_match: bool
3132
whdload_match: bool
3233
ra_match: bool
3334
fbneo_match: bool
@@ -346,16 +347,19 @@ async def lookup_rom(
346347
tgdb_id=int(tgdb_id) if tgdb_id else None,
347348
ra_id=int(ra_id) if ra_id else None,
348349
url_cover=url_cover,
350+
# Keys are Hasheous' SignatureSourceType names, spelled exactly
351+
# as its API returns them.
349352
hasheous_metadata=HasheousMetadata(
350353
tosec_match="TOSEC" in signatures,
351354
mame_arcade_match="MAMEArcade" in signatures,
352355
mame_mess_match="MAMEMess" in signatures,
353356
nointro_match="NoIntros" in signatures,
354357
redump_match="Redump" in signatures,
358+
mame_redump_match="MAMERedump" in signatures,
355359
whdload_match="WHDLoad" in signatures,
356360
ra_match="RetroAchievements" in signatures,
357361
fbneo_match="FBNeo" in signatures,
358-
puredos_match="PureDOS" in signatures,
362+
puredos_match="PureDOSDAT" in signatures,
359363
),
360364
),
361365
True,
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
"""The `verified` filter over the Hasheous signature-match flags.
2+
3+
`hasheous_metadata` is a JSON blob whose keys grow as RomM maps more of
4+
Hasheous' signature sources (`mame_redump_match` was the latest addition), so
5+
rows written before a key existed simply don't carry it. Extracting a missing
6+
key yields NULL, and an OR chain containing a NULL is NULL rather than false,
7+
which makes `NOT (...)` NULL too: the unverified side would drop every row it
8+
should have returned.
9+
10+
The JSON path already collapses a missing key into false (SQLAlchemy compiles
11+
`as_boolean()` to a CASE whose ELSE branch catches it), so only the PostgreSQL
12+
`->>` extraction needs the coalesce. The suite runs against one driver at a
13+
time, hence the compiled-SQL check below.
14+
"""
15+
16+
import pytest
17+
18+
from handler.database import db_rom_handler
19+
from handler.database.roms_handler import DBRomsHandler
20+
from models.platform import Platform
21+
from models.rom import Rom
22+
from models.user import User
23+
24+
# The keys as they were written before `mame_redump_match` joined them.
25+
LEGACY_KEYS = [
26+
"tosec_match",
27+
"mame_arcade_match",
28+
"mame_mess_match",
29+
"nointro_match",
30+
"redump_match",
31+
"whdload_match",
32+
"ra_match",
33+
"fbneo_match",
34+
"puredos_match",
35+
]
36+
37+
38+
def _add_rom(platform: Platform, user: User, name: str, metadata: dict) -> Rom:
39+
rom = db_rom_handler.add_rom(
40+
Rom(
41+
platform_id=platform.id,
42+
name=name,
43+
slug=name,
44+
fs_name=f"{name}.zip",
45+
fs_name_no_tags=name,
46+
fs_name_no_ext=name,
47+
fs_extension="zip",
48+
fs_path=f"{platform.slug}/roms",
49+
hasheous_metadata=metadata,
50+
)
51+
)
52+
db_rom_handler.add_rom_user(rom_id=rom.id, user_id=user.id)
53+
return rom
54+
55+
56+
@pytest.fixture
57+
def legacy_unverified_rom(platform: Platform, admin_user: User) -> Rom:
58+
"""Scanned before `mame_redump_match` existed, and matched nothing."""
59+
return _add_rom(
60+
platform,
61+
admin_user,
62+
"legacy_unverified",
63+
{key: False for key in LEGACY_KEYS},
64+
)
65+
66+
67+
@pytest.fixture
68+
def legacy_verified_rom(platform: Platform, admin_user: User) -> Rom:
69+
return _add_rom(
70+
platform,
71+
admin_user,
72+
"legacy_verified",
73+
{key: key == "nointro_match" for key in LEGACY_KEYS},
74+
)
75+
76+
77+
@pytest.fixture
78+
def chd_verified_rom(platform: Platform, admin_user: User) -> Rom:
79+
"""Only the newest key is set, as a CHD rescan writes it."""
80+
return _add_rom(
81+
platform,
82+
admin_user,
83+
"chd_verified",
84+
{key: False for key in LEGACY_KEYS} | {"mame_redump_match": True},
85+
)
86+
87+
88+
class TestVerifiedFilter:
89+
def test_unverified_keeps_roms_missing_the_newest_key(
90+
self,
91+
admin_user: User,
92+
legacy_unverified_rom: Rom,
93+
legacy_verified_rom: Rom,
94+
chd_verified_rom: Rom,
95+
):
96+
roms = db_rom_handler.get_roms_scalar(user_id=admin_user.id, verified=False)
97+
98+
assert [r.id for r in roms] == [legacy_unverified_rom.id]
99+
100+
def test_verified_matches_both_legacy_and_newest_keys(
101+
self,
102+
admin_user: User,
103+
legacy_unverified_rom: Rom,
104+
legacy_verified_rom: Rom,
105+
chd_verified_rom: Rom,
106+
):
107+
roms = db_rom_handler.get_roms_scalar(user_id=admin_user.id, verified=True)
108+
109+
assert sorted(r.id for r in roms) == sorted(
110+
[legacy_verified_rom.id, chd_verified_rom.id]
111+
)
112+
113+
def test_unverified_keeps_roms_without_any_hasheous_metadata(
114+
self, admin_user: User, rom: Rom, legacy_verified_rom: Rom
115+
):
116+
roms = db_rom_handler.get_roms_scalar(user_id=admin_user.id, verified=False)
117+
118+
assert [r.id for r in roms] == [rom.id]
119+
120+
121+
class TestVerifiedPostgresPredicate:
122+
"""The PostgreSQL branch builds raw SQL, so it can only be checked by
123+
compiling it (the suite runs on a single driver at a time)."""
124+
125+
@pytest.fixture
126+
def postgres_handler(self, monkeypatch: pytest.MonkeyPatch) -> DBRomsHandler:
127+
monkeypatch.setattr(
128+
"handler.database.roms_handler.ROMM_DB_DRIVER", "postgresql"
129+
)
130+
return db_rom_handler
131+
132+
@pytest.mark.parametrize("verified", [True, False])
133+
def test_every_key_is_coalesced_to_false(
134+
self, postgres_handler: DBRomsHandler, verified: bool
135+
):
136+
query, _ = postgres_handler.get_roms_query()
137+
filtered = postgres_handler.filter_roms(query=query, verified=verified)
138+
139+
sql = str(filtered.compile(compile_kwargs={"literal_binds": True}))
140+
141+
for key in [*LEGACY_KEYS, "mame_redump_match"]:
142+
assert f"COALESCE((hasheous_metadata->>'{key}')::boolean, false)" in sql

backend/tests/handler/test_fastapi.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,7 @@ async def test_scan_rom_hashes_rematches_hasheous(
534534
mame_mess_match=False,
535535
nointro_match=True,
536536
redump_match=False,
537+
mame_redump_match=False,
537538
whdload_match=False,
538539
ra_match=True,
539540
fbneo_match=False,
@@ -770,6 +771,59 @@ async def test_lookup_rom_sends_all_top_level_file_hashes(
770771
]
771772

772773

774+
@patch.object(meta_hasheous_handler, "_request", new_callable=AsyncMock)
775+
@patch.object(meta_hasheous_handler, "is_enabled", return_value=True)
776+
async def test_lookup_rom_maps_every_hasheous_signature_source(
777+
mock_is_enabled, mock_request
778+
):
779+
"""Each match flag reads a Hasheous SignatureSourceType name verbatim, so a
780+
typo silently pins that flag to False."""
781+
mock_request.return_value = {
782+
"id": 1,
783+
"signatures": {
784+
"TOSEC": {},
785+
"MAMEArcade": {},
786+
"MAMEMess": {},
787+
"NoIntros": {},
788+
"Redump": {},
789+
"MAMERedump": {},
790+
"WHDLoad": {},
791+
"RetroAchievements": {},
792+
"FBNeo": {},
793+
"PureDOSDAT": {},
794+
},
795+
}
796+
797+
files = [
798+
_top_level_rom_file(file_name="game.n64", file_size_bytes=100, md5_hash="md5")
799+
]
800+
801+
result, _ = await meta_hasheous_handler.lookup_rom("n64", files)
802+
803+
assert all(result["hasheous_metadata"].values())
804+
805+
806+
@patch.object(meta_hasheous_handler, "_request", new_callable=AsyncMock)
807+
@patch.object(meta_hasheous_handler, "is_enabled", return_value=True)
808+
async def test_lookup_rom_marks_a_chd_matched_by_mameredump_as_verified(
809+
mock_is_enabled, mock_request
810+
):
811+
"""Hasheous indexes CHD conversions under MAMERedump, not Redump, so a CHD
812+
match sets no other flag and the ROM would otherwise never read as
813+
verified."""
814+
mock_request.return_value = {"id": 1, "signatures": {"MAMERedump": {}}}
815+
816+
files = [
817+
_top_level_rom_file(
818+
file_name="game.chd", file_size_bytes=100, chd_sha1_hash="discsha1"
819+
)
820+
]
821+
822+
result, _ = await meta_hasheous_handler.lookup_rom("dc", files)
823+
824+
assert result["hasheous_metadata"]["mame_redump_match"] is True
825+
826+
773827
@patch.object(meta_hasheous_handler, "_request", new_callable=AsyncMock)
774828
@patch.object(meta_hasheous_handler, "is_enabled", return_value=True)
775829
async def test_lookup_rom_skips_request_when_no_hashes(mock_is_enabled, mock_request):

backend/tools/generate_test_data.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,7 @@ def build_hasheous_metadata(rng: random.Random) -> dict[str, Any]:
616616
"mame_mess_match": rng.random() < 0.1,
617617
"nointro_match": rng.random() < 0.6,
618618
"redump_match": rng.random() < 0.4,
619+
"mame_redump_match": rng.random() < 0.1,
619620
"whdload_match": False,
620621
"ra_match": rng.random() < 0.3,
621622
"fbneo_match": rng.random() < 0.1,

frontend/src/__generated__/models/RomHasheousMetadata.ts

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

frontend/src/v2/utils/romVerification.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,17 @@ describe("matchesDatabase", () => {
4545
);
4646
});
4747

48+
it("matches Redump on either the disc-image or the CHD flag", () => {
49+
const redump = VERIFICATION_DATABASES.find((db) => db.label === "Redump")!;
50+
expect(matchesDatabase(rom({ redump_match: true }), redump.keys)).toBe(
51+
true,
52+
);
53+
// Hasheous indexes CHD conversions under its own MAMERedump source.
54+
expect(matchesDatabase(rom({ mame_redump_match: true }), redump.keys)).toBe(
55+
true,
56+
);
57+
});
58+
4859
it("treats RetroAchievements as a database match (ra_match, not ra_id)", () => {
4960
const ra = VERIFICATION_DATABASES.find(
5061
(db) => db.label === "RetroAchievements",

frontend/src/v2/utils/romVerification.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,16 @@ import type { SimpleRom } from "@/stores/roms";
99

1010
// Each database this ROM's hash can be checked against, with the Hasheous
1111
// match flag(s) that count as a hit. MAME reports Arcade and MESS
12-
// separately; either one means the ROM matched MAME. Order is the display
13-
// order for the Metadata tab chips.
12+
// separately; either one means the ROM matched MAME. Redump likewise
13+
// reports disc images and their CHD conversions separately. Order is the
14+
// display order for the Metadata tab chips.
1415
export const VERIFICATION_DATABASES: {
1516
label: string;
1617
keys: (keyof RomHasheousMetadata)[];
1718
}[] = [
1819
{ label: "TOSEC", keys: ["tosec_match"] },
1920
{ label: "No-Intro", keys: ["nointro_match"] },
20-
{ label: "Redump", keys: ["redump_match"] },
21+
{ label: "Redump", keys: ["redump_match", "mame_redump_match"] },
2122
{ label: "MAME", keys: ["mame_arcade_match", "mame_mess_match"] },
2223
{ label: "FBNeo", keys: ["fbneo_match"] },
2324
{ label: "WHDLoad", keys: ["whdload_match"] },

0 commit comments

Comments
 (0)