From 7066233c37ab54f1c0d46765f885cca08486152d Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sun, 10 May 2026 16:33:54 -0700 Subject: [PATCH] =?UTF-8?q?Wire=20alias-aware=20artist=20match=20into=20Ac?= =?UTF-8?q?oustID=20verifier=20=E2=80=94=20fixes=20#442?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the user-visible commit. The reporter's exact two cases (Japanese kanji, Russian Cyrillic) now pass verification instead of being quarantined. # What changed Verifier's three artist-similarity sites now route through the shared `core.matching.artist_aliases.artist_names_match` helper instead of raw `_similarity`: - `_find_best_title_artist_match` (per-recording scoring at the best-match stage) - Secondary scan when title matches but best-match's artist doesn't (line ~355 pre-fix) - Final fallback scan over all recordings (line ~403 pre-fix) Aliases for the expected artist are resolved ONCE at the top of `verify_audio_file` via `_resolve_expected_artist_aliases`, which calls the new `MusicBrainzService.lookup_artist_aliases` chain (library DB → cache → live MB). Single resolution per verification regardless of how many AcoustID recordings come back — pinned by test. New helper `_alias_aware_artist_sim(expected, actual, aliases)` wraps the pure helper with the verifier's normaliser (`_similarity`) and threshold (`ARTIST_MATCH_THRESHOLD`). Returns a single float so existing threshold-comparison code paths keep their shape — minimal diff. # Reporter's cases — verified Case 1 (issue #442 verbatim): File: YAMANAIAME by 澤野弘之 Expected: YAMANAIAME by Hiroyuki Sawano Pre-fix: Quarantined (artist=0%) Post-fix: PASS (alias '澤野弘之' resolved from MB) Case 2 (issue #442 verbatim): File: On the Other Side by Sergey Lazarev Expected: On the other side by Сергей Лазарев Pre-fix: Quarantined (artist=7%) Post-fix: PASS (alias 'Sergey Lazarev' resolved from MB) Both reproduced as regression tests with stubbed MB service. # Backward compat Three test cases pin that no-aliases / failure paths preserve pre-fix behaviour exactly: - Clear artist mismatch (different artist, same script) still FAILs — aliases bridge synonyms, not unrelated artists. - Exact title + artist match still PASSes regardless of aliases. - MB service raise → verifier completes with direct similarity (treats failure as "no aliases available" — same as pre-fix). Also covers manual import: the import-modal "Search for Match" flow goes through the same verifier, so the reporter's complaint that "manual import simply throws them back in quarantine again" is fixed by the same change. # Tests added (11) `tests/matching/test_acoustid_verification_aliases.py`: - `_alias_aware_artist_sim`: alias bridges score ↑, no-aliases falls back, aliases don't mask genuine mismatches - `_find_best_title_artist_match` accepts + uses aliases - Reporter's case 1 (Japanese) end-to-end - Reporter's case 2 (Russian) end-to-end - Backward compat: no-aliases mismatch still fails, exact match still passes, MB-service-raise doesn't break verification - Performance: alias lookup fires ONCE per verification regardless of recording count # Verification - 11 new verifier tests pass - 31 prior service tests pass - 28 prior helper tests pass - 294 matching + imports tests pass total (no regression) - Ruff clean --- core/acoustid_verification.py | 108 ++++++- .../test_acoustid_verification_aliases.py | 276 ++++++++++++++++++ 2 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 tests/matching/test_acoustid_verification_aliases.py diff --git a/core/acoustid_verification.py b/core/acoustid_verification.py index c00008ec..9e4d2d7a 100644 --- a/core/acoustid_verification.py +++ b/core/acoustid_verification.py @@ -87,14 +87,51 @@ def _similarity(a: str, b: str) -> float: return SequenceMatcher(None, na, nb).ratio() +def _alias_aware_artist_sim( + expected_artist: str, + actual_artist: str, + aliases: Optional[List[str]] = None, +) -> float: + """Best artist-similarity across (expected, *aliases) vs actual. + + Issue #442 — when expected and actual are in different scripts + (e.g. `Hiroyuki Sawano` vs `澤野弘之`), raw `_similarity` scores + near 0% even though MusicBrainz aliases bridge them. Routes + through the pure helper so the verifier inherits one shared + contract. + + Returns the highest score across all candidates so existing + threshold checks (>= ARTIST_MATCH_THRESHOLD) keep their + semantics. When `aliases` is None or empty, behaves identically + to the prior raw `_similarity(expected, actual)` call. + """ + from core.matching.artist_aliases import artist_names_match + _matched, score = artist_names_match( + expected_artist, + actual_artist, + aliases=aliases, + threshold=ARTIST_MATCH_THRESHOLD, + similarity=_similarity, + ) + return score + + def _find_best_title_artist_match( recordings: List[Dict[str, Any]], expected_title: str, expected_artist: str, + expected_artist_aliases: Optional[List[str]] = None, ) -> Tuple[Optional[Dict], float, float]: """ Find the AcoustID recording that best matches expected title/artist. + Issue #442 — `expected_artist_aliases` (when supplied) is the + list of alternate spellings for `expected_artist` (Japanese + kanji, Cyrillic, etc.). Each recording's artist is scored + against (expected, *aliases) and the best score wins. When the + list is empty or omitted, behavior is identical to the prior + raw similarity comparison. + Returns: (best_recording, title_similarity, artist_similarity) """ @@ -108,7 +145,9 @@ def _find_best_title_artist_match( artist = rec.get('artist') or '' title_sim = _similarity(expected_title, title) - artist_sim = _similarity(expected_artist, artist) + artist_sim = _alias_aware_artist_sim( + expected_artist, artist, expected_artist_aliases, + ) # Weight title higher since that's the primary identifier combined = (title_sim * 0.6) + (artist_sim * 0.4) @@ -125,6 +164,12 @@ def _find_best_title_artist_match( _mb_client = None _mb_client_lock = threading.Lock() +# Shared MusicBrainzService for alias lookups (issue #442). Service +# layer wraps the raw client + adds caching + DB access — all of which +# the alias resolution chain (library DB → cache → live MB) needs. +_mb_service = None +_mb_service_lock = threading.Lock() + MAX_MB_ENRICHMENT_LOOKUPS = 3 @@ -138,6 +183,42 @@ def _get_mb_client() -> MusicBrainzClient: return _mb_client +def _get_mb_service(): + """Get or create a shared MusicBrainzService instance. + + Used by the alias-resolution chain in `verify_audio_file`. Lazy + init so importing this module doesn't trigger a DB connection on + paths that never run AcoustID verification (test runs, dry runs). + """ + global _mb_service + if _mb_service is None: + with _mb_service_lock: + if _mb_service is None: + from core.musicbrainz_service import MusicBrainzService + from database.music_database import get_database + _mb_service = MusicBrainzService(get_database()) + return _mb_service + + +def _resolve_expected_artist_aliases(expected_artist_name: str) -> List[str]: + """Look up alternate-spelling aliases for the expected artist. + + Issue #442 — bridges cross-script artist comparisons (Japanese + kanji ↔ romanized, Cyrillic ↔ Latin, etc.) without forcing the + verifier to know about the resolution chain. Best-effort: any + failure (no MB service, network down, no library DB) returns + empty list so verification falls back to the prior direct + similarity check. + """ + if not expected_artist_name: + return [] + try: + return _get_mb_service().lookup_artist_aliases(expected_artist_name) + except Exception as e: + logger.debug("alias lookup failed for %r: %s", expected_artist_name, e) + return [] + + def _enrich_recordings_from_musicbrainz( recordings: List[Dict[str, Any]], ) -> List[Dict[str, Any]]: @@ -290,9 +371,23 @@ class AcoustIDVerification: # Enrich recordings that are missing title/artist via MusicBrainz lookup recordings = _enrich_recordings_from_musicbrainz(recordings) + # Issue #442 — resolve alternate-spelling aliases for the + # expected artist ONCE. Multi-tier resolution (library DB + # → cache → live MB), cached per artist name so 100 + # quarantine candidates with the same artist don't trigger + # 100 MB API calls. Empty list on any failure → verifier + # falls back to prior direct-similarity behaviour. + expected_artist_aliases = _resolve_expected_artist_aliases(expected_artist_name) + if expected_artist_aliases: + logger.debug( + "Resolved %d aliases for expected artist '%s'", + len(expected_artist_aliases), expected_artist_name, + ) + # Step 4: Find best title/artist match among AcoustID results best_rec, title_sim, artist_sim = _find_best_title_artist_match( - recordings, expected_track_name, expected_artist_name + recordings, expected_track_name, expected_artist_name, + expected_artist_aliases=expected_artist_aliases, ) if not best_rec: @@ -352,7 +447,10 @@ class AcoustIDVerification: # metadata for this fingerprint, it's likely the right track # (AcoustID's "best" match just picked the wrong variant). for rec in recordings: - if _similarity(expected_artist_name, rec.get('artist', '')) >= ARTIST_MATCH_THRESHOLD: + rec_artist = rec.get('artist', '') + if _alias_aware_artist_sim( + expected_artist_name, rec_artist, expected_artist_aliases, + ) >= ARTIST_MATCH_THRESHOLD: msg = ( f"Audio verified: found '{expected_track_name}' by '{expected_artist_name}' " f"in AcoustID results" @@ -400,7 +498,9 @@ class AcoustIDVerification: if _detect_title_version(t) != expected_version: continue if (_similarity(expected_track_name, t) >= TITLE_MATCH_THRESHOLD and - _similarity(expected_artist_name, a) >= ARTIST_MATCH_THRESHOLD): + _alias_aware_artist_sim( + expected_artist_name, a, expected_artist_aliases, + ) >= ARTIST_MATCH_THRESHOLD): msg = ( f"Audio verified: found '{t}' by '{a}' in AcoustID results " f"matching expected '{expected_track_name}' by '{expected_artist_name}'" diff --git a/tests/matching/test_acoustid_verification_aliases.py b/tests/matching/test_acoustid_verification_aliases.py new file mode 100644 index 00000000..c14ef6fc --- /dev/null +++ b/tests/matching/test_acoustid_verification_aliases.py @@ -0,0 +1,276 @@ +"""Regression tests for issue #442 — AcoustID verifier alias awareness. + +The reporter posted two exact cases: + +Case 1 (Japanese kanji ↔ romanized): + File: YAMANAIAME by 澤野弘之 + Expected: YAMANAIAME by Hiroyuki Sawano + Pre-fix: quarantined (artist_sim=0%) + Post-fix: passes verification because MB aliases bridge the + two spellings. + +Case 2 (Cyrillic ↔ Latin): + File: On the Other Side by Sergey Lazarev + Expected: On the other side by Сергей Лазарев + Pre-fix: quarantined (artist_sim=7%) + Post-fix: passes via aliases. + +These tests pin the verifier through the helper. AcoustID's +fingerprint call is stubbed (no network), MB service's +`lookup_artist_aliases` is stubbed to return the relevant aliases. +The verifier's pass/fail decision is the assertion. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.acoustid_verification import ( + AcoustIDVerification, + VerificationResult, + _alias_aware_artist_sim, + _find_best_title_artist_match, +) + + +# --------------------------------------------------------------------------- +# Pure helper — _alias_aware_artist_sim +# --------------------------------------------------------------------------- + + +class TestAliasAwareArtistSim: + def test_returns_higher_score_when_alias_matches(self): + score = _alias_aware_artist_sim( + 'Hiroyuki Sawano', '澤野弘之', + aliases=['澤野弘之', 'SawanoHiroyuki'], + ) + assert score == 1.0 + + def test_no_aliases_falls_back_to_direct_similarity(self): + """Cross-script with NO aliases → score ~0, pre-fix behaviour.""" + score = _alias_aware_artist_sim( + 'Hiroyuki Sawano', '澤野弘之', aliases=None, + ) + assert score < 0.1 + + def test_aliases_dont_mask_genuine_mismatch(self): + """Different artist entirely → still scores low even when + aliases are provided. Aliases bridge synonyms, not unrelated + artists.""" + score = _alias_aware_artist_sim( + 'Hiroyuki Sawano', 'Khalil Turk & Friends', + aliases=['澤野弘之', 'SawanoHiroyuki'], + ) + assert score < 0.5 + + +# --------------------------------------------------------------------------- +# _find_best_title_artist_match — accepts aliases now +# --------------------------------------------------------------------------- + + +class TestFindBestMatchWithAliases: + def test_japanese_alias_picks_correct_recording(self): + """Reporter's case 1: AcoustID returned recording with kanji + artist. Without aliases the scorer ranks it low and the + verifier later quarantines. With aliases it scores high.""" + recordings = [ + {'title': 'YAMANAIAME', 'artist': '澤野弘之'}, + {'title': 'Different Song', 'artist': 'Hiroyuki Sawano'}, + ] + # Aliases provided — bridge to recording 0 + best, title_sim, artist_sim = _find_best_title_artist_match( + recordings, 'YAMANAIAME', 'Hiroyuki Sawano', + expected_artist_aliases=['澤野弘之', 'SawanoHiroyuki'], + ) + assert best is recordings[0] + assert artist_sim == 1.0 + + def test_no_aliases_legacy_behaviour_preserved(self): + """Default arg / empty aliases → identical to pre-fix + behaviour. Critical for paths not yet wired up to alias + lookup.""" + recordings = [ + {'title': 'Track', 'artist': 'Artist'}, + ] + best, title_sim, artist_sim = _find_best_title_artist_match( + recordings, 'Track', 'Artist', + ) + assert title_sim == 1.0 + assert artist_sim == 1.0 + + +# --------------------------------------------------------------------------- +# End-to-end — reporter's cases through the full verifier +# --------------------------------------------------------------------------- + + +@pytest.fixture +def stubbed_verifier(monkeypatch): + """AcoustIDVerification with the acoustid client + MB service + layer stubbed. Lets us drive the verifier's full decision path + without network or DB. Returns the verifier + mutable handles + to the stubs so each test can shape the AcoustID response + + aliases.""" + verifier = AcoustIDVerification() + verifier.acoustid_client = MagicMock() + verifier.acoustid_client.is_available.return_value = (True, '') + + # Stub the MB service so verifier alias lookup doesn't touch DB + # or network. Each test sets fake_service.lookup_artist_aliases. + fake_service = MagicMock() + fake_service.lookup_artist_aliases.return_value = [] + monkeypatch.setattr( + 'core.acoustid_verification._get_mb_service', lambda: fake_service, + ) + + return verifier, fake_service + + +class TestIssue442Regression: + def test_japanese_kanji_artist_passes_verification(self, stubbed_verifier): + """Reporter's case 1 — verbatim from the issue: + + File: YAMANAIAME by 澤野弘之 + Expected: YAMANAIAME by Hiroyuki Sawano + Pre-fix: Quarantined (artist=0%) + """ + verifier, fake_service = stubbed_verifier + + # AcoustID returns the recording with kanji artist + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'YAMANAIAME', 'artist': '澤野弘之', 'mbid': 'rec-x'}, + ], + } + # MB knows Hiroyuki Sawano's aliases + fake_service.lookup_artist_aliases.return_value = [ + '澤野弘之', 'SawanoHiroyuki', 'Sawano Hiroyuki', + ] + + result, msg = verifier.verify_audio_file( + '/fake/path.mp3', 'YAMANAIAME', 'Hiroyuki Sawano', + ) + + assert result == VerificationResult.PASS, ( + f"Reporter's exact case must pass verification post-fix; " + f"got result={result.value!r} msg={msg!r}" + ) + fake_service.lookup_artist_aliases.assert_called_once_with('Hiroyuki Sawano') + + def test_cyrillic_artist_passes_verification(self, stubbed_verifier): + """Reporter's case 2 — Sergey Lazarev / Сергей Лазарев.""" + verifier, fake_service = stubbed_verifier + + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'On the Other Side', 'artist': 'Sergey Lazarev', 'mbid': 'rec-y'}, + ], + } + fake_service.lookup_artist_aliases.return_value = [ + 'Sergey Lazarev', 'Sergei Lazarev', + ] + + result, msg = verifier.verify_audio_file( + '/fake/path.flac', 'On the other side', 'Сергей Лазарев', + ) + + assert result == VerificationResult.PASS + + +# --------------------------------------------------------------------------- +# Backward compat — no aliases available → behavior identical to pre-fix +# --------------------------------------------------------------------------- + + +class TestBackwardCompat: + def test_no_aliases_clear_artist_mismatch_still_fails(self, stubbed_verifier): + """Pre-fix: clear mismatches (artist sim near 0, NOT a script + difference) should FAIL. Post-fix with empty aliases must + preserve this — aliases bridge synonyms, not unrelated + artists.""" + verifier, fake_service = stubbed_verifier + + # Wrong artist entirely — Latin script both sides, sim ~0 + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'Some Track', 'artist': 'Khalil Turk & Friends'}, + ], + } + fake_service.lookup_artist_aliases.return_value = [] # No aliases + + result, msg = verifier.verify_audio_file( + '/fake/path.mp3', 'Some Track', 'Foreigner', + ) + + assert result == VerificationResult.FAIL + + def test_no_aliases_exact_match_still_passes(self, stubbed_verifier): + """Exact title + artist match → PASS regardless of aliases.""" + verifier, fake_service = stubbed_verifier + + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'Dirty White Boy', 'artist': 'Foreigner'}, + ], + } + fake_service.lookup_artist_aliases.return_value = [] + + result, _ = verifier.verify_audio_file( + '/fake/path.mp3', 'Dirty White Boy', 'Foreigner', + ) + assert result == VerificationResult.PASS + + def test_alias_lookup_failure_does_not_break_verification(self, stubbed_verifier): + """MB service raises → verifier still completes with direct + similarity (pre-fix behaviour preserved).""" + verifier, fake_service = stubbed_verifier + fake_service.lookup_artist_aliases.side_effect = Exception("MB down") + + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'Dirty White Boy', 'artist': 'Foreigner'}, + ], + } + + result, _ = verifier.verify_audio_file( + '/fake/path.mp3', 'Dirty White Boy', 'Foreigner', + ) + # Should still pass — direct similarity works + assert result == VerificationResult.PASS + + +# --------------------------------------------------------------------------- +# Performance contract — alias lookup fires ONCE per verification +# --------------------------------------------------------------------------- + + +class TestAliasLookupCalledOncePerVerify: + def test_single_lookup_call_regardless_of_recordings_count(self, stubbed_verifier): + """The verifier processes multiple recordings + scans through + them at up to 3 sites — but should only call + `lookup_artist_aliases` ONCE per verify_audio_file invocation. + Otherwise verifying a track with 20 AcoustID recordings could + fire 60+ MB lookups (cached or not, that's wasteful).""" + verifier, fake_service = stubbed_verifier + + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'X', 'artist': '澤野弘之'}, + {'title': 'X', 'artist': 'SawanoHiroyuki'}, + {'title': 'X', 'artist': 'Different Artist'}, + ], + } + fake_service.lookup_artist_aliases.return_value = ['澤野弘之', 'SawanoHiroyuki'] + + verifier.verify_audio_file('/fake/path.mp3', 'X', 'Hiroyuki Sawano') + + assert fake_service.lookup_artist_aliases.call_count == 1