diff --git a/core/acoustid_verification.py b/core/acoustid_verification.py index c00008ec..bd2e917c 100644 --- a/core/acoustid_verification.py +++ b/core/acoustid_verification.py @@ -87,14 +87,105 @@ 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[Any] = 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. + + `aliases` accepts two shapes: + + - **Iterable** (list/tuple/set of strings): used directly. Used + by tests that already know the aliases. + - **Callable**: invoked LAZILY only when direct similarity + falls below the threshold. Lets the verifier pass a memoizing + thunk that resolves aliases (DB / cache / live MB) only when + needed. Verifications where the direct match already passes + never trigger the lookup chain — no wasted DB query for the + happy path. + + Diagnostic logging: emits an INFO line whenever an alias rescues + a comparison that direct similarity would have failed. Lets + future bug reports trace which alias triggered which PASS + decision (e.g. "this file passed because alias `澤野弘之` matched + the file's artist tag"). + """ + from core.matching.artist_aliases import artist_names_match + + direct = _similarity(expected_artist, actual_artist) + # Fast path — direct match already passes the threshold OR caller + # supplied no aliases handle. Avoids any lookup work. + if aliases is None: + return direct + if direct >= ARTIST_MATCH_THRESHOLD: + return direct + + # Resolve the iterable. Callable provider invoked NOW (lazily — + # the caller can memoize the result across multiple invocations + # within one verify_audio_file call). + resolved = aliases() if callable(aliases) else aliases + if not resolved: + return direct + + _matched, score = artist_names_match( + expected_artist, + actual_artist, + aliases=resolved, + threshold=ARTIST_MATCH_THRESHOLD, + similarity=_similarity, + ) + + # Diagnostic — alias rescued a comparison that direct would + # have failed. Worth logging at INFO since it's a user-visible + # decision (file PASS instead of FAIL). One line per rescue + # within a single verify call. + if score >= ARTIST_MATCH_THRESHOLD and direct < ARTIST_MATCH_THRESHOLD: + from core.matching.artist_aliases import best_alias_match + winner, _ = best_alias_match( + expected_artist, actual_artist, resolved, similarity=_similarity, + ) + logger.info( + "Artist alias rescued comparison: expected=%r vs actual=%r " + "(direct sim=%.2f, alias %r → score=%.2f)", + expected_artist, actual_artist, direct, winner, score, + ) + + return score + + def _find_best_title_artist_match( recordings: List[Dict[str, Any]], expected_title: str, expected_artist: str, + expected_artist_aliases: Optional[Any] = 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.). Accepts either: + + - An iterable of alias strings (used eagerly), or + - A callable returning the list (resolved lazily — only fires + when at least one recording fails direct artist similarity). + + Each recording's artist is scored against (expected, *aliases) + and the best score wins. When the list is empty/omitted/None, + behavior is identical to the prior raw similarity comparison. + Returns: (best_recording, title_similarity, artist_similarity) """ @@ -108,7 +199,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 +218,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 +237,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 +425,33 @@ class AcoustIDVerification: # Enrich recordings that are missing title/artist via MusicBrainz lookup recordings = _enrich_recordings_from_musicbrainz(recordings) + # Issue #442 — alias resolution is LAZY. We pass a memoising + # thunk to the artist-comparison sites; it only fires the + # multi-tier lookup (library DB → cache → live MB) when + # direct artist similarity falls below threshold. Verifications + # where the direct match already passes (the common case for + # same-script artist names) never trigger any lookup work, + # so the fix doesn't add a per-verification DB query for the + # happy path. When the thunk DOES fire, the result is cached + # in the closure so the 3 comparison sites within one + # verification share a single resolution pass. + _alias_cache: Dict[str, Any] = {} + + def _aliases_provider() -> List[str]: + if 'value' not in _alias_cache: + resolved = _resolve_expected_artist_aliases(expected_artist_name) + _alias_cache['value'] = resolved + if resolved: + logger.debug( + "Resolved %d aliases for expected artist '%s'", + len(resolved), expected_artist_name, + ) + return _alias_cache['value'] + # 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=_aliases_provider, ) if not best_rec: @@ -352,7 +511,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, _aliases_provider, + ) >= ARTIST_MATCH_THRESHOLD: msg = ( f"Audio verified: found '{expected_track_name}' by '{expected_artist_name}' " f"in AcoustID results" @@ -400,7 +562,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, _aliases_provider, + ) >= 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/core/matching/__init__.py b/core/matching/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/core/matching/artist_aliases.py b/core/matching/artist_aliases.py new file mode 100644 index 00000000..1b1e5565 --- /dev/null +++ b/core/matching/artist_aliases.py @@ -0,0 +1,175 @@ +"""Pure-function artist-name comparison with alias awareness. + +Issue #442 — cross-script artist quarantines +----------------------------------------------------- + +A file tagged with one spelling of an artist's name (e.g. the +Japanese kanji `澤野弘之`) was being quarantined when SoulSync's +expected-artist metadata used the romanized spelling +(`Hiroyuki Sawano`). Raw similarity comparison scores 0% across +scripts even though MusicBrainz already knows both names belong to +the same artist (its alias list). + +This module is the shared resolution helper. Given an expected +artist name, an actual artist name, and an iterable of known +aliases, it returns whether they should be treated as the same +artist + the highest similarity score across the candidate set. + +Pure function design: +- No I/O, no DB access, no network +- Caller supplies aliases (looked up from library DB or live MB) +- Caller supplies normalize + similarity functions to keep the + helper provider-neutral (the verifier and the matching engine + use slightly different normalizers — let each pass its own) +- Returns ``(matched: bool, score: float)`` so callers can log + the score they made the decision on + +Backward compat: when ``aliases`` is empty (or the looking-up +caller hasn't been wired yet), the helper degrades to a plain +direct similarity comparison — identical to the pre-fix behaviour. +""" + +from __future__ import annotations + +from difflib import SequenceMatcher +from typing import Callable, Iterable, Optional, Tuple + + +# Default threshold matches the existing ARTIST_MATCH_THRESHOLD in +# core/acoustid_verification.py. Callers can override but the helper +# defaults are tuned to preserve current verifier behaviour. +DEFAULT_ARTIST_MATCH_THRESHOLD = 0.6 + + +def _default_normalize(text: str) -> str: + """Lowercase + strip whitespace. Minimal — caller's normaliser + almost always replaces this with something stricter (parenthetical + stripping, punctuation removal). Used only when the caller + doesn't pass a custom one.""" + if not text: + return '' + return str(text).strip().lower() + + +def _default_similarity(a: str, b: str) -> float: + """SequenceMatcher ratio after the default normaliser. Matches + the verifier's existing ``_similarity`` semantics for the no- + custom-callable path.""" + na = _default_normalize(a) + nb = _default_normalize(b) + if not na or not nb: + return 0.0 + if na == nb: + return 1.0 + return SequenceMatcher(None, na, nb).ratio() + + +def _coerce_aliases(aliases: Optional[Iterable[str]]) -> Tuple[str, ...]: + """Normalise the aliases input to a tuple of clean strings. + + Accepts ``None``, empty iterables, lists, tuples, sets. Drops + None / empty / non-string entries silently — callers feeding us + raw MusicBrainz response dicts shouldn't have to clean first. + """ + if not aliases: + return () + cleaned = [] + for value in aliases: + if value is None: + continue + text = str(value).strip() + if text: + cleaned.append(text) + return tuple(cleaned) + + +def artist_names_match( + expected: str, + actual: str, + *, + aliases: Optional[Iterable[str]] = None, + threshold: float = DEFAULT_ARTIST_MATCH_THRESHOLD, + similarity: Optional[Callable[[str, str], float]] = None, +) -> Tuple[bool, float]: + """Compare ``expected`` and ``actual`` artist names with alias + awareness. + + Args: + expected: The artist name the caller expected (typically from + metadata-source data — Spotify / iTunes / Deezer track + payload). + actual: The artist name the caller observed (typically from + an AcoustID recording or a downloaded file's tag). + aliases: Iterable of known alternate spellings for ``expected``. + Each one gets compared against ``actual``; the best score + wins. Empty or omitted → plain direct comparison + (backward-compat with pre-fix behaviour). + threshold: Score at or above which we consider the names a + match. Defaults to 0.6 to match the verifier's existing + ``ARTIST_MATCH_THRESHOLD``. + similarity: Optional caller-supplied similarity function + ``(a, b) -> float in [0, 1]``. Lets the verifier pass its + stricter normaliser (parenthetical stripping etc.) without + this module having to know about it. Defaults to a + lowercase + SequenceMatcher comparison. + + Returns: + ``(matched, best_score)`` where ``matched`` is True iff the + best score across (actual, *aliases) ≥ threshold and + ``best_score`` is that maximum. ``best_score`` is informative + for callers that want to log "matched at 0.83" or similar. + """ + sim = similarity or _default_similarity + + # Direct compare first — both for the fast path and so the + # returned score reflects the actual-vs-expected baseline (callers + # may want it for logging even when an alias is the actual winner). + direct_score = sim(expected, actual) + best_score = direct_score + if direct_score >= threshold: + return True, direct_score + + # Alias compare: each alias is a known alternate spelling of the + # EXPECTED artist; match it against the ACTUAL name we observed. + # Highest score wins. + for alias in _coerce_aliases(aliases): + score = sim(alias, actual) + if score > best_score: + best_score = score + if score >= threshold: + return True, score + + return False, best_score + + +def best_alias_match( + expected: str, + actual: str, + aliases: Optional[Iterable[str]] = None, + *, + similarity: Optional[Callable[[str, str], float]] = None, +) -> Tuple[Optional[str], float]: + """Return the alias that best matched ``actual`` (or None for the + direct expected-vs-actual comparison) and its score. + + Companion to ``artist_names_match`` for callers that want to + surface which alias triggered the match (debug logging, UI + explanations). Doesn't apply a threshold — purely informative. + + Returns: + ``(winner, score)`` where ``winner`` is the alias string when + an alias outscored the direct comparison, ``None`` when the + direct comparison won (or both tied at zero). + """ + sim = similarity or _default_similarity + direct_score = sim(expected, actual) + winner: Optional[str] = None + best = direct_score + + for alias in _coerce_aliases(aliases): + score = sim(alias, actual) + if score > best: + best = score + winner = alias + + return winner, best diff --git a/core/musicbrainz_service.py b/core/musicbrainz_service.py index fced6dcb..20f24023 100644 --- a/core/musicbrainz_service.py +++ b/core/musicbrainz_service.py @@ -388,6 +388,216 @@ class MusicBrainzService: logger.error(f"Error matching recording '{track_name}': {e}") return None + def lookup_artist_aliases(self, artist_name: str) -> list: + """Find alternate-spelling aliases for an artist by NAME. + + Multi-tier resolution: + 1. Library DB row (`artists.aliases` populated by the MB + worker when the artist was enriched). Fast path — no + network. + 2. Existing musicbrainz_cache entry (entity_type='artist_aliases') + — caches a prior live MB lookup for this name. + 3. Live MB lookup: search artist → fetch aliases for the best + MBID → cache the result. + + Always returns a list (possibly empty) — never raises. Empty + result on any tier means "no alternate spellings found, fall + back to direct match" which is identical to pre-fix behaviour. + + Used by the AcoustID verifier when an artist comparison fails + the direct similarity check. Caching means each unique artist + name only hits MB once per cache TTL even if 100 download + candidates fail verification with that artist. + """ + if not artist_name: + return [] + + # Tier 1: library DB + library = self.get_artist_aliases(artist_name) + if library: + return library + + # Tier 2: cached live lookup (re-uses musicbrainz_cache table) + cached = self._check_cache('artist_aliases', artist_name) + if cached: + metadata = cached.get('metadata') or {} + aliases = metadata.get('aliases') if isinstance(metadata, dict) else None + if isinstance(aliases, list): + return [str(x).strip() for x in aliases if x] + # Cache hit with empty result — respect it (don't re-query) + return [] + + # Tier 3: live MB lookup. Search → fetch by MBID → cache. + try: + results = self.mb_client.search_artist(artist_name, limit=3) + except Exception as e: + logger.debug("lookup_artist_aliases: search_artist(%r) raised: %s", artist_name, e) + self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) + return [] + + if not results: + self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) + return [] + + # Score each result: combined of name-similarity + MB's own + # relevance. Score range 0.0-1.0. + scored = [] + for result in results: + mb_name = result.get('name', '') + mb_score = result.get('score', 0) + sim = self._calculate_similarity(artist_name, mb_name) + combined = (sim * 0.7) + (mb_score / 100 * 0.3) + mbid = result.get('id') + if mbid: + scored.append((combined, mbid)) + + if not scored: + self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) + return [] + + scored.sort(key=lambda x: -x[0]) + best_score, best_mbid = scored[0] + + # Strict trust threshold: real matches for distinctive cross- + # script artists (the user-reported case) score >= 0.95. + # Anything below 0.85 is ambiguous and not worth the false- + # positive risk of pulling in aliases for the wrong artist. + if best_score < 0.85: + logger.debug( + "lookup_artist_aliases: best match for %r below trust " + "threshold (score=%.2f)", artist_name, best_score, + ) + self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) + return [] + + # Ambiguity detection: when 2+ results both score high (within + # 0.1 of the best), the search hit multiple distinct artists + # with similar names ("John Smith" returning 10 different + # John Smiths all at score 100). Pulling aliases for one of + # them could produce wrong matches. Skip + cache empty. + if len(scored) >= 2 and (scored[0][0] - scored[1][0]) < 0.1: + logger.debug( + "lookup_artist_aliases: ambiguous match for %r — top " + "two results within 0.1 (%.2f / %.2f). Skipping alias lookup.", + artist_name, scored[0][0], scored[1][0], + ) + self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) + return [] + + aliases = self.fetch_artist_aliases(best_mbid) + self._save_to_cache( + 'artist_aliases', artist_name, None, best_mbid, + {'aliases': aliases}, int(best_score * 100), + ) + return aliases + + def fetch_artist_aliases(self, mbid: str) -> list: + """Fetch the alias list for an artist from MusicBrainz. + + Issue #442 — Japanese kanji / Cyrillic / etc. spellings of an + artist's name are stored as `aliases` on the MusicBrainz + artist record. Pull them so SoulSync can recognise that + `澤野弘之` and `Hiroyuki Sawano` refer to the same artist. + + Returns the deduplicated list of alias `name` strings. Returns + empty list (NOT None) on any failure — caller should treat + empty as "no aliases available, fall back to direct match" so + a transient MB outage never causes a stricter verification + decision than today. + """ + if not mbid: + return [] + try: + data = self.mb_client.get_artist(mbid, includes=['aliases']) + except Exception as e: + logger.debug("fetch_artist_aliases: get_artist(%s) raised: %s", mbid, e) + return [] + if not data: + return [] + raw_aliases = data.get('aliases') or [] + # MB returns each alias as a dict with `name`, `sort-name`, + # `locale`, `primary`, `type`, etc. We only care about the + # display name — that's what `actual` artist strings will + # match against. + seen = set() + cleaned = [] + for entry in raw_aliases: + if not isinstance(entry, dict): + continue + name = (entry.get('name') or '').strip() + if not name: + continue + key = name.lower() + if key in seen: + continue + seen.add(key) + cleaned.append(name) + return cleaned + + def update_artist_aliases(self, artist_id: int, aliases: list) -> None: + """Persist the alias list to `artists.aliases` as a JSON array. + + Idempotent — overwrites any existing value. Empty list + clears the column (caller may want this if MB has no aliases + for the artist anymore). + """ + if artist_id is None: + return + conn = None + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute( + "UPDATE artists SET aliases = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?", + (json.dumps(aliases) if aliases else None, artist_id), + ) + conn.commit() + logger.debug("Updated artist %s aliases (%d entries)", artist_id, len(aliases or [])) + except Exception as e: + logger.error(f"Error updating artist aliases for {artist_id}: {e}") + if conn: + conn.rollback() + finally: + if conn: + conn.close() + + def get_artist_aliases(self, artist_name: str) -> list: + """Look up cached aliases for an artist by NAME (not id). + + Used by the verifier where the expected artist comes from a + download's metadata-source data — we don't have a library + row's `id` to query, just the display name. Returns empty + list when the artist isn't in the library or has no aliases + recorded. The verifier falls back to live MB lookup in that + case. + """ + if not artist_name: + return [] + conn = None + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute( + "SELECT aliases FROM artists WHERE name = ? COLLATE NOCASE LIMIT 1", + (artist_name,), + ) + row = cursor.fetchone() + if not row or not row[0]: + return [] + try: + parsed = json.loads(row[0]) + except (TypeError, json.JSONDecodeError): + return [] + if not isinstance(parsed, list): + return [] + return [str(x).strip() for x in parsed if x] + except Exception as e: + logger.debug("get_artist_aliases lookup failed for %r: %s", artist_name, e) + return [] + finally: + if conn: + conn.close() + def update_artist_mbid(self, artist_id: int, mbid: Optional[str], status: str): """Update artist with MusicBrainz ID""" conn = None diff --git a/core/musicbrainz_worker.py b/core/musicbrainz_worker.py index fe6461fc..bc494433 100644 --- a/core/musicbrainz_worker.py +++ b/core/musicbrainz_worker.py @@ -283,6 +283,31 @@ class MusicBrainzWorker: if conn: conn.close() + def _artist_aliases_empty(self, artist_id: Any) -> bool: + """Check if `artists.aliases` for this row is NULL or empty. + + Used by the existing-MBID backfill path to skip the MB call + when aliases are already populated (re-scan cycles after + backfill complete should be no-ops). Defensive: returns True + on any error so the backfill attempt happens — a redundant MB + call is cheaper than missing the backfill entirely. + """ + conn = None + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute("SELECT aliases FROM artists WHERE id = ? LIMIT 1", (artist_id,)) + row = cursor.fetchone() + if not row: + return False # Row doesn't exist — nothing to backfill + value = row[0] + return value is None or value == '' or value == '[]' + except Exception: + return True + finally: + if conn: + conn.close() + def _process_item(self, item: Dict[str, Any]): """Process a single item (artist, album, or track)""" try: @@ -301,6 +326,27 @@ class MusicBrainzWorker: try: if item_type == 'artist': self.mb_service.update_artist_mbid(item_id, existing_id, 'matched') + # Issue #442 — one-time backfill for artists + # enriched before alias support landed. Users with + # pre-existing libraries on day-one of this PR have + # MBIDs but NULL aliases. Fetch ONLY when the + # column is empty so re-scan cycles after backfill + # don't re-query MB. Best-effort: failures are + # logged at debug, don't regress the match outcome. + try: + if self._artist_aliases_empty(item_id): + aliases = self.mb_service.fetch_artist_aliases(existing_id) + if aliases: + self.mb_service.update_artist_aliases(item_id, aliases) + logger.debug( + "Backfilled %d aliases for artist '%s'", + len(aliases), item_name, + ) + except Exception as backfill_err: + logger.debug( + "Alias backfill failed for artist '%s': %s", + item_name, backfill_err, + ) elif item_type == 'album': self.mb_service.update_album_mbid(item_id, existing_id, 'matched') elif item_type == 'track': @@ -313,6 +359,24 @@ class MusicBrainzWorker: result = self.mb_service.match_artist(item_name) if result and result.get('mbid'): self.mb_service.update_artist_mbid(item_id, result['mbid'], 'matched') + # Issue #442 — pull alternate-spelling aliases (Japanese + # kanji, Cyrillic, etc.) so the verifier can recognise + # cross-script artist names without re-querying MB on + # every quarantine candidate. Best-effort: failures are + # swallowed inside `fetch_artist_aliases` (returns + # empty list) so a transient MB outage never regresses + # the enrichment outcome. + try: + aliases = self.mb_service.fetch_artist_aliases(result['mbid']) + if aliases: + self.mb_service.update_artist_aliases(item_id, aliases) + logger.debug( + "Stored %d aliases for artist '%s'", len(aliases), item_name, + ) + except Exception as alias_err: + logger.debug( + "Alias enrichment failed for artist '%s': %s", item_name, alias_err, + ) self.stats['matched'] += 1 logger.info(f"Matched artist '{item_name}' → MBID: {result['mbid']}") else: diff --git a/database/music_database.py b/database/music_database.py index 857d16b0..e6a1c35a 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -1792,6 +1792,17 @@ class MusicDatabase: if 'musicbrainz_match_status' not in artists_columns: cursor.execute("ALTER TABLE artists ADD COLUMN musicbrainz_match_status TEXT") columns_added = True + # MusicBrainz exposes alternate-spelling aliases on every artist + # record (Japanese kanji ↔ romanized, Cyrillic ↔ Latin, etc.). + # SoulSync's artist matching used to compare expected vs actual + # name with raw similarity — cross-script comparison scored 0% + # and the file got quarantined even when MusicBrainz knew both + # names belonged to the same artist (issue #442). Persist the + # alias list as JSON so the verifier + matcher can consult it + # without re-querying MB on every comparison. + if 'aliases' not in artists_columns: + cursor.execute("ALTER TABLE artists ADD COLUMN aliases TEXT") + columns_added = True if columns_added: logger.info("Added MusicBrainz columns to artists table") diff --git a/tests/matching/__init__.py b/tests/matching/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/matching/test_acoustid_verification_aliases.py b/tests/matching/test_acoustid_verification_aliases.py new file mode 100644 index 00000000..2f2748b7 --- /dev/null +++ b/tests/matching/test_acoustid_verification_aliases.py @@ -0,0 +1,498 @@ +"""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 + + +# --------------------------------------------------------------------------- +# Lazy alias resolution — happy path skips MB lookup entirely +# --------------------------------------------------------------------------- + + +class TestLazyAliasResolution: + """Issue #442 perf followup: alias lookup should ONLY fire when + the direct artist comparison fails. Verifications where artist + names already match (the 95% common case for same-script + libraries) must NOT trigger the lookup chain — no wasted DB + query, no wasted MB call.""" + + def test_no_lookup_when_direct_artist_match_passes(self, stubbed_verifier): + """Exact-match Latin-script artist passes verification with + zero alias lookups — no DB query, no MB call. Same-script + libraries (the 95% common case) inherit zero perf cost from + this PR.""" + verifier, fake_service = stubbed_verifier + + 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', + ) + + assert result == VerificationResult.PASS + # Critical — alias lookup must NOT have been called for the + # happy path. Otherwise every successful verification adds a + # DB query for nothing. + fake_service.lookup_artist_aliases.assert_not_called() + + def test_lookup_fires_only_when_direct_artist_match_fails(self, stubbed_verifier): + """Cross-script case where direct sim is 0% → lookup fires + as expected.""" + verifier, fake_service = stubbed_verifier + + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'YAMANAIAME', 'artist': '澤野弘之'}, + ], + } + fake_service.lookup_artist_aliases.return_value = ['澤野弘之'] + + result, _ = verifier.verify_audio_file( + '/fake/path.mp3', 'YAMANAIAME', 'Hiroyuki Sawano', + ) + + assert result == VerificationResult.PASS + # Lookup fired BECAUSE direct match would have failed + fake_service.lookup_artist_aliases.assert_called_once() + + def test_lookup_memoised_across_three_comparison_sites(self, stubbed_verifier): + """When lookup DOES fire, the result must be reused across + the three artist-comparison sites in the verifier (best-match + scoring, secondary scan, fallback scan). One resolution per + verification — not three.""" + verifier, fake_service = stubbed_verifier + + # Force a code path that hits multiple sites: title matches + # several recordings but the best-match's artist sim is below + # threshold (forces secondary scan path). + verifier.acoustid_client.fingerprint_and_lookup.return_value = { + 'best_score': 0.95, + 'recordings': [ + {'title': 'X', 'artist': 'Different Latin Artist'}, # 0 alias hit + {'title': 'X', 'artist': '澤野弘之'}, # alias hit + ], + } + fake_service.lookup_artist_aliases.return_value = ['澤野弘之'] + + verifier.verify_audio_file('/fake/path.mp3', 'X', 'Hiroyuki Sawano') + + # Memoised — one resolution shared across all sites + assert fake_service.lookup_artist_aliases.call_count == 1 + + +# --------------------------------------------------------------------------- +# Provider-callable contract on the helper +# --------------------------------------------------------------------------- + + +class TestAliasProviderCallable: + """Pin the dual-shape contract on `_alias_aware_artist_sim`: + accepts an iterable OR a callable. Callable resolves lazily.""" + + def test_iterable_passed_directly(self): + """Plain list — used as-is, no lazy semantics.""" + score = _alias_aware_artist_sim( + 'Hiroyuki Sawano', '澤野弘之', aliases=['澤野弘之'], + ) + assert score == 1.0 + + def test_callable_resolves_lazily_only_when_direct_fails(self): + """Callable provider — invoked ONLY when direct sim falls + below threshold.""" + call_count = [0] + + def provider(): + call_count[0] += 1 + return ['澤野弘之'] + + # Direct match passes → provider NOT called + _alias_aware_artist_sim('Foreigner', 'Foreigner', aliases=provider) + assert call_count[0] == 0 + + # Direct match fails → provider IS called + _alias_aware_artist_sim('Hiroyuki Sawano', '澤野弘之', aliases=provider) + assert call_count[0] == 1 + + def test_callable_returning_empty_list_falls_back_to_direct(self): + """Provider returns empty (e.g. MB had no aliases) → + score = direct sim, no error.""" + score = _alias_aware_artist_sim( + 'Hiroyuki Sawano', '澤野弘之', aliases=lambda: [], + ) + # ~0 because direct cross-script comparison fails + assert score < 0.1 + + +# --------------------------------------------------------------------------- +# Diagnostic logging — alias rescues are visible in logs +# --------------------------------------------------------------------------- + + +class TestAliasRescueLogging: + """When an alias bridges a comparison that direct similarity + would have failed, log it at INFO level. Future bug reports + where a file passed verification incorrectly can be traced back + to which alias triggered which decision. + + Uses a directly-attached handler instead of pytest's caplog — + full-suite caplog is intermittently flaky for soulsync namespace + loggers (handler ordering, parallel test state). An owned + handler on the specific logger sidesteps both issues, same + pattern as the prior watchdog-test fix. + """ + + @staticmethod + def _capture_records(): + """Attach an owned ListHandler to the verifier's logger. + Returns (records list, teardown callable).""" + import logging as _logging + records: list = [] + + class _ListHandler(_logging.Handler): + def emit(self, record): + records.append(record) + + handler = _ListHandler(level=_logging.INFO) + # Logger name is `soulsync.acoustid.verification` per + # `core.acoustid_verification`'s `get_logger("acoustid_verification")` + # — dot-separated, NOT underscored. + verifier_logger = _logging.getLogger('soulsync.acoustid.verification') + verifier_logger.addHandler(handler) + prior_level = verifier_logger.level + verifier_logger.setLevel(_logging.INFO) + + def teardown(): + verifier_logger.removeHandler(handler) + verifier_logger.setLevel(prior_level) + + return records, teardown + + def test_alias_rescue_emits_info_log(self): + records, teardown = self._capture_records() + try: + _alias_aware_artist_sim( + 'Hiroyuki Sawano', '澤野弘之', aliases=['澤野弘之'], + ) + finally: + teardown() + + rescue_logs = [ + r.getMessage() for r in records + if 'alias rescued' in r.getMessage().lower() + ] + assert len(rescue_logs) >= 1, ( + f"Expected an INFO log line about alias rescue; got " + f"{[r.getMessage() for r in records]}" + ) + + def test_no_log_when_direct_match_succeeds(self): + """Happy path doesn't spam logs — only rescue cases log.""" + records, teardown = self._capture_records() + try: + _alias_aware_artist_sim( + 'Foreigner', 'Foreigner', aliases=['ignored-alias'], + ) + finally: + teardown() + + rescue_logs = [ + r.getMessage() for r in records + if 'alias rescued' in r.getMessage().lower() + ] + assert rescue_logs == [] + + def test_no_log_when_alias_doesnt_help(self): + """If aliases were available but didn't bridge the gap (still + below threshold), no rescue log — there was no rescue.""" + records, teardown = self._capture_records() + try: + _alias_aware_artist_sim( + 'Hiroyuki Sawano', 'Khalil Turk', + aliases=['Sergey Lazarev'], # unrelated alias + ) + finally: + teardown() + + rescue_logs = [ + r.getMessage() for r in records + if 'alias rescued' in r.getMessage().lower() + ] + assert rescue_logs == [] diff --git a/tests/matching/test_artist_alias_service.py b/tests/matching/test_artist_alias_service.py new file mode 100644 index 00000000..2e2f1a6a --- /dev/null +++ b/tests/matching/test_artist_alias_service.py @@ -0,0 +1,560 @@ +"""Pin the MusicBrainz service alias methods + worker enrichment. + +Issue #442 — these methods feed the alias data the helper compares +against. Three layers covered: + +1. ``fetch_artist_aliases`` — pulls aliases off the MB get-artist + response, defensive against missing fields, broken JSON, network + errors. +2. ``update_artist_aliases`` — persists to ``artists.aliases`` as a + JSON array. Empty/None → column cleared. +3. ``get_artist_aliases`` — reads back by artist NAME (not id) since + that's what the verifier has at quarantine time. + +Worker enrichment integration covered separately: when MB worker +matches an artist, it calls fetch + update so subsequent verifier +calls find aliases in the library DB without firing live MB. +""" + +from __future__ import annotations + +import json +import sqlite3 +from unittest.mock import MagicMock + +import pytest + + +@pytest.fixture +def temp_db(tmp_path, monkeypatch): + """Real MusicDatabase against a tmp file. Uses the production + schema (so the `aliases` column from commit 1's migration is + present) and the production update/get methods we're pinning.""" + monkeypatch.setenv('DATABASE_PATH', str(tmp_path / 'test.db')) + from database.music_database import MusicDatabase + return MusicDatabase() + + +@pytest.fixture +def service(temp_db): + """MusicBrainzService with stubbed mb_client. The DB is real; + the network is not.""" + from core.musicbrainz_service import MusicBrainzService + svc = MusicBrainzService(temp_db) + svc.mb_client = MagicMock() + return svc + + +_seed_counter = 0 + + +def _seed_artist(db, name: str, **fields) -> str: + """Insert a row into the artists table. + + `artists.id` is TEXT (NOT INTEGER auto-increment), so we mint a + deterministic test id rather than relying on rowid magic. + Returns the id as str — that's what the production code paths + use too (read methods, joins, etc.). + """ + global _seed_counter + _seed_counter += 1 + artist_id = f"test-artist-{_seed_counter}" + conn = db._get_connection() + cursor = conn.cursor() + cols = ['id', 'name'] + list(fields.keys()) + placeholders = ','.join('?' * len(cols)) + cursor.execute( + f"INSERT INTO artists ({','.join(cols)}) VALUES ({placeholders})", + [artist_id, name] + list(fields.values()), + ) + conn.commit() + conn.close() + return artist_id + + +# --------------------------------------------------------------------------- +# fetch_artist_aliases — MB get-artist response parser +# --------------------------------------------------------------------------- + + +class TestFetchArtistAliases: + def test_extracts_alias_names_from_mb_response(self, service): + """Reporter's case 1 shape: MB returns aliases for Hiroyuki + Sawano including the Japanese kanji form. Extract the `name` + from each alias entry.""" + service.mb_client.get_artist.return_value = { + 'id': '60d2ea34-1912-425f-bf9c-fc544e4448cd', + 'name': 'Hiroyuki Sawano', + 'aliases': [ + {'name': '澤野弘之', 'sort-name': '澤野弘之', 'locale': 'ja', 'primary': True}, + {'name': 'SawanoHiroyuki', 'sort-name': 'SawanoHiroyuki', 'locale': None}, + {'name': 'Sawano Hiroyuki', 'sort-name': 'Sawano, Hiroyuki', 'locale': 'en'}, + ], + } + + aliases = service.fetch_artist_aliases('60d2ea34-1912-425f-bf9c-fc544e4448cd') + + assert '澤野弘之' in aliases + assert 'SawanoHiroyuki' in aliases + assert 'Sawano Hiroyuki' in aliases + assert len(aliases) == 3 + + def test_dedup_case_insensitive(self, service): + """Same name with different casing should collapse — MB + sometimes returns duplicate-looking entries with locale + variations.""" + service.mb_client.get_artist.return_value = { + 'aliases': [ + {'name': 'Hiroyuki Sawano'}, + {'name': 'hiroyuki sawano'}, + {'name': 'HIROYUKI SAWANO'}, + ], + } + aliases = service.fetch_artist_aliases('mbid-x') + assert len(aliases) == 1 + + def test_empty_alias_entries_skipped(self, service): + service.mb_client.get_artist.return_value = { + 'aliases': [ + {'name': ''}, + {'name': ' '}, + {'name': None}, + {'name': 'Real Name'}, + ], + } + aliases = service.fetch_artist_aliases('mbid-x') + assert aliases == ['Real Name'] + + def test_missing_aliases_key_returns_empty(self, service): + """MB artist record might not have any aliases. Returns [] + not raises.""" + service.mb_client.get_artist.return_value = { + 'id': 'mbid-x', + 'name': 'Some Artist', + } + assert service.fetch_artist_aliases('mbid-x') == [] + + def test_aliases_null_returns_empty(self, service): + """MB sometimes returns `aliases: null` instead of empty array.""" + service.mb_client.get_artist.return_value = {'aliases': None} + assert service.fetch_artist_aliases('mbid-x') == [] + + def test_get_artist_failure_returns_empty(self, service): + """Network / API failure → empty list, NOT raise. Caller + must treat empty as 'no aliases available, fall back to + direct match' so transient MB outages never trigger + stricter quarantine decisions than today.""" + service.mb_client.get_artist.side_effect = Exception("network error") + assert service.fetch_artist_aliases('mbid-x') == [] + + def test_get_artist_returns_none_returns_empty(self, service): + service.mb_client.get_artist.return_value = None + assert service.fetch_artist_aliases('mbid-x') == [] + + def test_empty_mbid_returns_empty_without_api_call(self, service): + assert service.fetch_artist_aliases('') == [] + assert service.fetch_artist_aliases(None) == [] + service.mb_client.get_artist.assert_not_called() + + def test_includes_aliases_in_request(self, service): + """Verify the MB API call requests the aliases include + explicitly — without `inc=aliases` the response wouldn't + carry them.""" + service.mb_client.get_artist.return_value = {'aliases': []} + service.fetch_artist_aliases('mbid-x') + service.mb_client.get_artist.assert_called_once_with( + 'mbid-x', includes=['aliases'], + ) + + +# --------------------------------------------------------------------------- +# update_artist_aliases — persistence +# --------------------------------------------------------------------------- + + +class TestUpdateArtistAliases: + def test_persists_as_json_array(self, service, temp_db): + artist_id = _seed_artist(temp_db, 'Hiroyuki Sawano') + service.update_artist_aliases(artist_id, ['澤野弘之', 'SawanoHiroyuki']) + + conn = temp_db._get_connection() + row = conn.execute("SELECT aliases FROM artists WHERE id = ?", (artist_id,)).fetchone() + conn.close() + parsed = json.loads(row[0]) + assert parsed == ['澤野弘之', 'SawanoHiroyuki'] + + def test_idempotent_overwrite(self, service, temp_db): + artist_id = _seed_artist(temp_db, 'X') + service.update_artist_aliases(artist_id, ['a']) + service.update_artist_aliases(artist_id, ['b', 'c']) + conn = temp_db._get_connection() + row = conn.execute("SELECT aliases FROM artists WHERE id = ?", (artist_id,)).fetchone() + conn.close() + assert json.loads(row[0]) == ['b', 'c'] + + def test_empty_list_clears_column(self, service, temp_db): + artist_id = _seed_artist(temp_db, 'X', aliases=json.dumps(['old'])) + service.update_artist_aliases(artist_id, []) + conn = temp_db._get_connection() + row = conn.execute("SELECT aliases FROM artists WHERE id = ?", (artist_id,)).fetchone() + conn.close() + assert row[0] is None + + def test_none_artist_id_is_noop(self, service, temp_db): + """Defensive: caller might pass None on edge cases. Don't crash.""" + service.update_artist_aliases(None, ['x']) # no exception + + +# --------------------------------------------------------------------------- +# get_artist_aliases — read back by artist NAME (verifier path) +# --------------------------------------------------------------------------- + + +class TestGetArtistAliases: + def test_returns_aliases_for_known_artist(self, service, temp_db): + artist_id = _seed_artist( + temp_db, 'Hiroyuki Sawano', + aliases=json.dumps(['澤野弘之', 'SawanoHiroyuki']), + ) + aliases = service.get_artist_aliases('Hiroyuki Sawano') + assert '澤野弘之' in aliases + assert 'SawanoHiroyuki' in aliases + + def test_case_insensitive_lookup(self, service, temp_db): + """Verifier passes the artist name from track metadata — + casing might differ from how the library stored it.""" + _seed_artist(temp_db, 'Hiroyuki Sawano', aliases=json.dumps(['澤野弘之'])) + assert service.get_artist_aliases('hiroyuki sawano') == ['澤野弘之'] + assert service.get_artist_aliases('HIROYUKI SAWANO') == ['澤野弘之'] + + def test_returns_empty_for_unknown_artist(self, service): + assert service.get_artist_aliases('NeverHeardOf') == [] + + def test_returns_empty_for_artist_without_aliases(self, service, temp_db): + _seed_artist(temp_db, 'X') # no aliases column set + assert service.get_artist_aliases('X') == [] + + def test_handles_corrupt_json_gracefully(self, service, temp_db): + _seed_artist(temp_db, 'X', aliases='not-valid-json') + # Returns [] instead of raising — defensive against legacy + # rows that might have been written by an older format + assert service.get_artist_aliases('X') == [] + + def test_handles_non_list_json_gracefully(self, service, temp_db): + _seed_artist(temp_db, 'X', aliases=json.dumps({'wrong': 'shape'})) + assert service.get_artist_aliases('X') == [] + + def test_empty_name_returns_empty_without_query(self, service): + assert service.get_artist_aliases('') == [] + assert service.get_artist_aliases(None) == [] + + +# --------------------------------------------------------------------------- +# Worker integration — alias enrichment fires on successful match +# --------------------------------------------------------------------------- + + +class TestWorkerAliasEnrichment: + def test_matched_artist_triggers_alias_fetch_and_persist(self, temp_db, monkeypatch): + """End-to-end: worker matches an artist, immediately fetches + + persists aliases. Subsequent verifier calls find them in + the library DB without firing live MB.""" + from core.musicbrainz_worker import MusicBrainzWorker + + artist_id = _seed_artist(temp_db, 'Hiroyuki Sawano') + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.mb_service = MagicMock() + worker.mb_service.match_artist.return_value = { + 'mbid': '60d2ea34-1912-425f-bf9c-fc544e4448cd', 'name': 'Hiroyuki Sawano', + } + worker.mb_service.fetch_artist_aliases.return_value = ['澤野弘之', 'SawanoHiroyuki'] + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + + # Bypass _get_existing_id (would query DB for prior MBID) + worker._get_existing_id = MagicMock(return_value=None) + + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'Hiroyuki Sawano'}) + + worker.mb_service.update_artist_mbid.assert_called_once_with( + artist_id, '60d2ea34-1912-425f-bf9c-fc544e4448cd', 'matched', + ) + worker.mb_service.fetch_artist_aliases.assert_called_once_with( + '60d2ea34-1912-425f-bf9c-fc544e4448cd', + ) + worker.mb_service.update_artist_aliases.assert_called_once_with( + artist_id, ['澤野弘之', 'SawanoHiroyuki'], + ) + + def test_no_alias_call_when_artist_not_matched(self, temp_db): + """If MB returned no MBID match, don't fetch aliases — + nothing to enrich.""" + from core.musicbrainz_worker import MusicBrainzWorker + artist_id = _seed_artist(temp_db, 'Unknown') + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.mb_service = MagicMock() + worker.mb_service.match_artist.return_value = None + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + worker._get_existing_id = MagicMock(return_value=None) + + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'Unknown'}) + + worker.mb_service.fetch_artist_aliases.assert_not_called() + worker.mb_service.update_artist_aliases.assert_not_called() + + def test_existing_mbid_path_backfills_aliases_when_column_empty(self, temp_db): + """Issue #442 perf followup: existing-MBID short-circuit path + was skipping alias enrichment entirely. Users with libraries + enriched BEFORE this PR shipped have MBIDs but NULL aliases. + Worker should fetch aliases on the existing-id path too — + one-time backfill on first re-scan post-deploy.""" + from core.musicbrainz_worker import MusicBrainzWorker + artist_id = _seed_artist(temp_db, 'Hiroyuki Sawano') + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.db = temp_db # _artist_aliases_empty uses self.db + worker.mb_service = MagicMock() + worker.mb_service.fetch_artist_aliases.return_value = ['澤野弘之', 'SawanoHiroyuki'] + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + # Existing MBID path + worker._get_existing_id = MagicMock(return_value='mb-existing-id') + + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'Hiroyuki Sawano'}) + + # MBID was preserved + worker.mb_service.update_artist_mbid.assert_called_once_with( + artist_id, 'mb-existing-id', 'matched', + ) + # Aliases backfilled + worker.mb_service.fetch_artist_aliases.assert_called_once_with('mb-existing-id') + worker.mb_service.update_artist_aliases.assert_called_once_with( + artist_id, ['澤野弘之', 'SawanoHiroyuki'], + ) + + def test_existing_mbid_path_skips_backfill_when_aliases_already_set(self, temp_db): + """If aliases are already populated, don't re-fetch — re-scan + cycles after backfill complete should be no-ops.""" + from core.musicbrainz_worker import MusicBrainzWorker + artist_id = _seed_artist( + temp_db, 'X', aliases=json.dumps(['existing-alias']), + ) + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.db = temp_db + worker.mb_service = MagicMock() + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + worker._get_existing_id = MagicMock(return_value='mb-x') + + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'X'}) + + # No alias work — column already populated + worker.mb_service.fetch_artist_aliases.assert_not_called() + worker.mb_service.update_artist_aliases.assert_not_called() + + def test_existing_mbid_backfill_failure_does_not_break_match(self, temp_db): + """Backfill is best-effort — failure to fetch aliases must + NOT prevent the MBID-preservation update from happening.""" + from core.musicbrainz_worker import MusicBrainzWorker + artist_id = _seed_artist(temp_db, 'X') + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.db = temp_db + worker.mb_service = MagicMock() + worker.mb_service.fetch_artist_aliases.side_effect = Exception("MB down") + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + worker._get_existing_id = MagicMock(return_value='mb-x') + + # Should NOT raise + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'X'}) + + # MBID still preserved despite alias backfill failure + worker.mb_service.update_artist_mbid.assert_called_once_with( + artist_id, 'mb-x', 'matched', + ) + + def test_alias_fetch_failure_does_not_break_match(self, temp_db): + """If alias fetch raises (network error, malformed response, + whatever), the artist match still gets recorded — alias + enrichment is best-effort, not a gate.""" + from core.musicbrainz_worker import MusicBrainzWorker + artist_id = _seed_artist(temp_db, 'X') + + worker = MusicBrainzWorker.__new__(MusicBrainzWorker) + worker.database = temp_db + worker.mb_service = MagicMock() + worker.mb_service.match_artist.return_value = {'mbid': 'mb-x', 'name': 'X'} + worker.mb_service.fetch_artist_aliases.side_effect = Exception("boom") + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0} + worker._get_existing_id = MagicMock(return_value=None) + + worker._process_item({'type': 'artist', 'id': artist_id, 'name': 'X'}) + + # MBID still got updated despite alias failure + worker.mb_service.update_artist_mbid.assert_called_once_with( + artist_id, 'mb-x', 'matched', + ) + # No alias write attempted (fetch raised before update) + worker.mb_service.update_artist_aliases.assert_not_called() + # And the match was still counted + assert worker.stats['matched'] == 1 + + +# --------------------------------------------------------------------------- +# lookup_artist_aliases — multi-tier resolution (library → cache → live) +# --------------------------------------------------------------------------- + + +class TestLookupArtistAliasesMultiTier: + def test_tier1_library_db_hit(self, service, temp_db): + """Fast path: artist already enriched in library DB. + No MB API call fired.""" + _seed_artist(temp_db, 'Hiroyuki Sawano', + aliases=json.dumps(['澤野弘之', 'SawanoHiroyuki'])) + + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + + assert '澤野弘之' in aliases + service.mb_client.search_artist.assert_not_called() + service.mb_client.get_artist.assert_not_called() + + def test_tier3_live_mb_lookup_when_not_in_library(self, service, temp_db): + """Cache miss + library miss → MB search → fetch by MBID → + cache the result.""" + service.mb_client.search_artist.return_value = [ + {'id': 'mb-sawano', 'name': 'Hiroyuki Sawano', 'score': 100}, + ] + service.mb_client.get_artist.return_value = { + 'aliases': [{'name': '澤野弘之'}, {'name': 'SawanoHiroyuki'}], + } + + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + + assert '澤野弘之' in aliases + service.mb_client.search_artist.assert_called_once() + service.mb_client.get_artist.assert_called_once_with( + 'mb-sawano', includes=['aliases'], + ) + + def test_tier2_cache_hit_skips_live_lookup(self, service, temp_db): + """Second call for same artist hits the cache, doesn't + re-query MB. Critical for the verifier path — 100 quarantine + candidates with the same artist must NOT trigger 100 MB + calls.""" + service.mb_client.search_artist.return_value = [ + {'id': 'mb-x', 'name': 'X', 'score': 100}, + ] + service.mb_client.get_artist.return_value = { + 'aliases': [{'name': 'X-alias'}], + } + + # First call — populates cache + first = service.lookup_artist_aliases('X') + # Second call — should be cached + second = service.lookup_artist_aliases('X') + + assert first == second == ['X-alias'] + # Only ONE round-trip to MB despite two calls + assert service.mb_client.search_artist.call_count == 1 + assert service.mb_client.get_artist.call_count == 1 + + def test_empty_name_returns_empty_no_api_call(self, service): + assert service.lookup_artist_aliases('') == [] + assert service.lookup_artist_aliases(None) == [] + service.mb_client.search_artist.assert_not_called() + + def test_search_failure_returns_empty(self, service): + """Network outage on search — return empty, cache the empty + result so we don't keep retrying.""" + service.mb_client.search_artist.side_effect = Exception("network down") + aliases = service.lookup_artist_aliases('Anyone') + assert aliases == [] + + def test_no_search_results_returns_empty(self, service): + """Artist not found on MB — empty return, cached so we + don't re-search the same name forever.""" + service.mb_client.search_artist.return_value = [] + aliases = service.lookup_artist_aliases('NeverHeardOf') + assert aliases == [] + # Second call should hit cache, not re-search + service.lookup_artist_aliases('NeverHeardOf') + assert service.mb_client.search_artist.call_count == 1 + + def test_low_confidence_match_skipped(self, service): + """Search returned something but the name similarity is too + low — don't trust it. Could pull in aliases for the wrong + artist (e.g. searching 'John Smith' returns a different + John Smith). Empty return + cached.""" + service.mb_client.search_artist.return_value = [ + {'id': 'mb-different', 'name': 'Completely Different Artist', 'score': 30}, + ] + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + assert aliases == [] + # Didn't even try fetching aliases for the bad match + service.mb_client.get_artist.assert_not_called() + + def test_moderate_confidence_match_now_skipped_strict_threshold(self, service): + """Threshold tightened to 0.85 (was 0.6) — moderate matches + (sim ~0.7) are no longer trusted. Reduces false-positive + risk on ambiguous artist names.""" + service.mb_client.search_artist.return_value = [ + # Different name, MB matched on weak signal — combined + # score lands around 0.6-0.7, below the new 0.85 floor. + {'id': 'mb-x', 'name': 'John Williams', 'score': 50}, + ] + aliases = service.lookup_artist_aliases('John Smith') + assert aliases == [] + service.mb_client.get_artist.assert_not_called() + + def test_ambiguous_results_skipped(self, service): + """When MB search returns multiple results with similar high + scores (within 0.1 of each other), the artist name is + ambiguous — common name with multiple distinct artists + ('John Smith' returning 10 different John Smiths). Pulling + aliases for one could mismatch the wrong artist's data + against our file. Skip + cache empty.""" + service.mb_client.search_artist.return_value = [ + {'id': 'mb-smith-1', 'name': 'John Smith', 'score': 100}, + {'id': 'mb-smith-2', 'name': 'John Smith', 'score': 100}, + {'id': 'mb-smith-3', 'name': 'John Smith', 'score': 100}, + ] + aliases = service.lookup_artist_aliases('John Smith') + assert aliases == [] + # Didn't fetch aliases for either ambiguous candidate + service.mb_client.get_artist.assert_not_called() + + def test_unambiguous_high_confidence_match_succeeds(self, service): + """Sanity: a clear winner (top result high, no near-tie with + runner-up) still triggers alias fetch — the ambiguity gate + doesn't break the legit case.""" + service.mb_client.search_artist.return_value = [ + {'id': 'mb-sawano', 'name': 'Hiroyuki Sawano', 'score': 100}, + {'id': 'mb-other', 'name': 'Unrelated Artist', 'score': 30}, + ] + service.mb_client.get_artist.return_value = { + 'aliases': [{'name': '澤野弘之'}], + } + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + assert '澤野弘之' in aliases + + def test_library_with_empty_aliases_falls_through_to_live(self, service, temp_db): + """Edge case: library has the artist but `aliases` column is + NULL (worker hasn't enriched yet). Don't get stuck — fall + through to live MB lookup.""" + _seed_artist(temp_db, 'Hiroyuki Sawano') # no aliases + service.mb_client.search_artist.return_value = [ + {'id': 'mb-sawano', 'name': 'Hiroyuki Sawano', 'score': 100}, + ] + service.mb_client.get_artist.return_value = { + 'aliases': [{'name': '澤野弘之'}], + } + + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + + assert '澤野弘之' in aliases + service.mb_client.search_artist.assert_called_once() diff --git a/tests/matching/test_artist_aliases.py b/tests/matching/test_artist_aliases.py new file mode 100644 index 00000000..382b5f81 --- /dev/null +++ b/tests/matching/test_artist_aliases.py @@ -0,0 +1,264 @@ +"""Pin the alias-aware artist comparison helper. + +Issue #442 — files tagged with one spelling of an artist's name +(Japanese kanji `澤野弘之`) were quarantined when SoulSync expected +the romanized spelling (`Hiroyuki Sawano`). MusicBrainz aliases +should bridge the two — this helper does the bridging. + +These tests cover the helper in total isolation: no DB, no network, +no MusicBrainz client. Pure-function contract pinned at the right +boundary so every consumer (verifier, matching engine, future +callers) inherits the same correctness guarantees. +""" + +from __future__ import annotations + +import pytest + +from core.matching.artist_aliases import ( + DEFAULT_ARTIST_MATCH_THRESHOLD, + artist_names_match, + best_alias_match, +) + + +# --------------------------------------------------------------------------- +# Direct compare path — no aliases +# --------------------------------------------------------------------------- + + +class TestDirectCompareNoAliases: + def test_exact_match(self): + matched, score = artist_names_match('Foreigner', 'Foreigner') + assert matched is True + assert score == 1.0 + + def test_case_insensitive(self): + matched, score = artist_names_match('foreigner', 'FOREIGNER') + assert matched is True + assert score == 1.0 + + def test_whitespace_tolerant(self): + matched, score = artist_names_match(' Foreigner ', 'Foreigner') + assert matched is True + + def test_completely_different_artists(self): + matched, score = artist_names_match('Foreigner', 'Khalil Turk') + assert matched is False + assert score < DEFAULT_ARTIST_MATCH_THRESHOLD + + def test_fuzzy_match_above_threshold(self): + # 'Beatles' vs 'The Beatles' — sim ~0.78 + matched, score = artist_names_match('The Beatles', 'Beatles') + assert matched is True + assert score >= DEFAULT_ARTIST_MATCH_THRESHOLD + + +# --------------------------------------------------------------------------- +# Cross-script — the headline of issue #442 +# --------------------------------------------------------------------------- + + +class TestCrossScriptWithAliases: + def test_japanese_kanji_to_romanized(self): + """Reporter's case 1: file tagged 澤野弘之, expected + Hiroyuki Sawano. MusicBrainz alias `澤野弘之` on the artist + record bridges the two.""" + matched, score = artist_names_match( + 'Hiroyuki Sawano', + '澤野弘之', + aliases=['澤野弘之', 'SawanoHiroyuki', 'Sawano Hiroyuki'], + ) + assert matched is True, ( + f"Expected alias match for Japanese spelling; got matched=False score={score}" + ) + + def test_romanized_to_japanese_kanji(self): + """Symmetric direction — file tagged Hiroyuki Sawano, expected + 澤野弘之. Aliases should resolve either way.""" + matched, score = artist_names_match( + '澤野弘之', + 'Hiroyuki Sawano', + aliases=['Hiroyuki Sawano', 'SawanoHiroyuki'], + ) + assert matched is True + + def test_cyrillic_to_latin(self): + """Reporter's case 2: file tagged Sergey Lazarev, expected + Сергей Лазарев.""" + matched, score = artist_names_match( + 'Сергей Лазарев', + 'Sergey Lazarev', + aliases=['Sergey Lazarev', 'Sergei Lazarev'], + ) + assert matched is True + + def test_no_alias_match_falls_through_to_fail(self): + """Aliases provided but none match the actual artist — + should still fail. Aliases bridge synonyms, they don't mask + genuine mismatches.""" + matched, score = artist_names_match( + 'Hiroyuki Sawano', + 'Khalil Turk', + aliases=['澤野弘之', 'SawanoHiroyuki'], + ) + assert matched is False + + +# --------------------------------------------------------------------------- +# Aliases input handling — defensive coercion +# --------------------------------------------------------------------------- + + +class TestAliasesInputCoercion: + def test_none_aliases_treated_as_empty(self): + matched, _ = artist_names_match('A', 'A', aliases=None) + assert matched is True + + def test_empty_list_aliases(self): + matched, _ = artist_names_match('A', 'A', aliases=[]) + assert matched is True + + def test_aliases_can_be_set(self): + matched, _ = artist_names_match( + 'Hiroyuki Sawano', '澤野弘之', aliases={'澤野弘之', 'SawanoHiroyuki'}, + ) + assert matched is True + + def test_aliases_can_be_tuple(self): + matched, _ = artist_names_match( + 'Hiroyuki Sawano', '澤野弘之', aliases=('澤野弘之',), + ) + assert matched is True + + def test_none_entries_in_aliases_skipped(self): + """Defensive: caller might pass aliases pulled directly from + a partial MB response. None / empty entries shouldn't crash.""" + matched, _ = artist_names_match( + 'Hiroyuki Sawano', '澤野弘之', + aliases=[None, '', '澤野弘之', None], + ) + assert matched is True + + def test_non_string_entries_coerced(self): + """Defensive: aliases parsed from JSON might surface as ints + or other non-string types. str() coercion in helper handles it.""" + matched, _ = artist_names_match( + 'A', '123', aliases=[123], + ) + assert matched is True + + +# --------------------------------------------------------------------------- +# Threshold behaviour +# --------------------------------------------------------------------------- + + +class TestThreshold: + def test_default_threshold_matches_verifier(self): + """Default threshold must equal the verifier's existing + ARTIST_MATCH_THRESHOLD so wiring the helper into the + verifier preserves current pass/fail semantics on the + no-alias path.""" + assert DEFAULT_ARTIST_MATCH_THRESHOLD == 0.6 + + def test_custom_threshold_stricter(self): + # Direct comparison would normally pass at 0.6 default, + # but a stricter threshold should reject it. + matched, score = artist_names_match( + 'The Beatles', 'Beatles', threshold=0.95, + ) + assert matched is False + + def test_custom_threshold_looser(self): + matched, score = artist_names_match( + 'AAAAA', 'AAABB', threshold=0.4, + ) + # ~0.6 sim, passes loose threshold + assert matched is True + + +# --------------------------------------------------------------------------- +# Custom similarity callable +# --------------------------------------------------------------------------- + + +class TestCustomSimilarity: + def test_custom_sim_used_for_direct_compare(self): + """Caller (verifier) passes its own normaliser-aware + similarity. Helper must route through it instead of using + the default.""" + def stricter(a, b): + # Always returns 0 — proves we're using the custom callable + return 0.0 + + matched, score = artist_names_match( + 'Foreigner', 'Foreigner', similarity=stricter, + ) + assert matched is False + assert score == 0.0 + + def test_custom_sim_used_for_alias_compare(self): + """Custom similarity also applies to alias scoring — not just + the direct comparison.""" + def alias_only_perfect(a, b): + # Returns 1.0 only when comparing the alias 'aliasX' + return 1.0 if 'aliasX' in (a, b) else 0.0 + + matched, score = artist_names_match( + 'Foreigner', 'observed', + aliases=['aliasX'], + similarity=alias_only_perfect, + ) + assert matched is True + assert score == 1.0 + + +# --------------------------------------------------------------------------- +# Best-alias-match introspection helper +# --------------------------------------------------------------------------- + + +class TestBestAliasMatch: + def test_direct_wins_no_alias_winner(self): + winner, score = best_alias_match( + 'Foreigner', 'Foreigner', aliases=['otherthing'], + ) + assert winner is None + assert score == 1.0 + + def test_alias_wins_returns_alias(self): + winner, score = best_alias_match( + 'Hiroyuki Sawano', '澤野弘之', + aliases=['澤野弘之', 'SawanoHiroyuki'], + ) + assert winner == '澤野弘之' + assert score == 1.0 + + def test_no_aliases_just_direct_score(self): + winner, score = best_alias_match('A', 'B', aliases=None) + assert winner is None + assert isinstance(score, float) + + +# --------------------------------------------------------------------------- +# Backward compat — pre-fix behaviour preserved when no aliases +# --------------------------------------------------------------------------- + + +class TestBackwardCompatNoAliases: + """When callers don't supply aliases (initial wiring, or live MB + unreachable), the helper must behave EXACTLY like a direct + similarity check — no surprises for paths that haven't been + wired up to alias lookup yet.""" + + @pytest.mark.parametrize('expected,actual,should_match', [ + ('Foreigner', 'Foreigner', True), # exact + ('foreigner', 'FOREIGNER', True), # case + ('The Beatles', 'Beatles', True), # fuzzy passes + ('Foreigner', 'Khalil Turk', False), # different + ('Hiroyuki Sawano', '澤野弘之', False), # cross-script no aliases → fail (pre-fix behaviour) + ]) + def test_no_alias_path_matches_direct_similarity(self, expected, actual, should_match): + matched, _ = artist_names_match(expected, actual) + assert matched is should_match diff --git a/webui/static/helper.js b/webui/static/helper.js index abaef5bc..bf418715 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.4.3': [ // --- post-release patch work on the 2.4.3 line — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.3 patch work' }, + { title: 'Cross-Script Artist Names No Longer Quarantine Files (Hiroyuki Sawano / 澤野弘之, Сергей Лазарев / Sergey Lazarev)', desc: 'github issue #442 (afonsog6): files where the artist tag was in one script and the expected metadata was in another — japanese kanji `澤野弘之` for `hiroyuki sawano`, cyrillic `сергей лазарев` for `sergey lazarev`, etc. — got quarantined post-download because acoustid verification scored the artist similarity at 0% (the two scripts share no characters). reporter could not even rescue the file via manual import — the import-modal goes through the same verifier and re-quarantined the same file. cause: verifier compared expected vs actual artist with raw `_similarity` and never consulted musicbrainz aliases, even though MB exposes them on every artist record. fix: new `core/matching/artist_aliases.py` pure helper with alias-aware comparison + new `artists.aliases` JSON column populated by the existing MB enrichment worker on every artist match (one extra `inc=aliases` request per artist) + new multi-tier resolver `MusicBrainzService.lookup_artist_aliases` (library DB → cache → live MB) so the verifier finds aliases even for un-enriched artists without thrashing the MB API. verifier resolves aliases ONCE per `verify_audio_file` call and feeds them through three artist comparison sites (best-match scoring, secondary scan when title matches but artist doesn\'t, final fallback scan). reporter\'s exact two cases reproduced as regression tests with stubbed MB service. backward compat: aliases unavailable / MB unreachable → verifier falls back to direct similarity (identical to pre-fix behaviour — never quarantines stricter than today). 70 new tests pin every layer: pure helper (28), service methods (31), verifier integration (11). audited adjacent artist-comparison sites (auto-import single-track id, discovery scoring, matching engine) — left untouched per scope discipline since they aren\'t the user-reported pain.', page: 'downloads' }, { title: 'Plex: Library Scan Trigger No Longer Fails On Non-English Section Names', desc: 'github issue #535 (adrigzr): plex servers with the music library named anything other than "music" — Música, Musique, Musik, Musica, etc. — got a `Failed to trigger library scan for "Music": Invalid library section: Music` error after every import cycle, and `wishlist.processing` kept reporting "missing from media server after sync" for tracks that DID import correctly because the post-import scan never fired. cause: `trigger_library_scan` and `is_library_scanning` ignored the auto-detected `self.music_library` (correctly populated by `_find_music_library` filtering by `section.type == "artist"`) and called `self.server.library.section(library_name)` with a hardcoded "music" default — raised NotFound on any non-english server. read methods like `get_artists` already routed through `_get_music_sections` so they didn\'t have the bug; this aligns the scan-trigger path with the same resolution. fix: both single-library branches prefer `self.music_library` first, fall back to literal section lookup only when auto-detection hasn\'t run. activity-feed match in `is_library_scanning` also corrected to use the resolved section\'s actual title instead of the unused `library_name` arg — the prior log line read "triggered scan for music" even on Spanish servers. 13 new tests pin: trigger uses auto-detected section across 6 locale variants (Música / Musique / Musik / Musica / 音乐 / موسيقى), backward-compat fallback when music_library is None, explicit library_name kwarg ignored when auto-detected section exists, log line surfaces correct section title, scan-status check uses auto-detected section\'s `refreshing` attr, activity-feed match filters by resolved title (not library_name).', page: 'settings' }, { title: 'Search For Match: No More Karaoke / Cover / "Originally Performed By" Junk At The Top', desc: 'github issue #534 (radoslav-orlov): typing "dirty white boy" + "foreigner" into the import-modal "search for match" dialog returned karaoke versions, "originally performed by" compilations, and tribute-band cuts ranked above the actual foreigner studio recording in some regions. user had to scroll past 5+ junk results before finding the canonical track. fix: new `core/metadata/relevance.py` helper reranks results locally with cover/karaoke/tribute/re-recorded penalties (multiplier 0.05× — effectively buries) + exact-artist-match boost (1.5×) + variant-tag (live/acoustic/remix/remaster) penalty (0.4×, skipped when user explicitly typed the variant — searching "track (live)" still ranks live versions correctly). applied at the deezer + itunes + spotify search-tracks endpoints so all three sources behave consistently. validated against live deezer api with the actual #534 query: real foreigner head games cut now lands at #1, live versions follow, karaoke / cover / tribute variants drop to positions 11-15. deezer client also gained optional field-scoped query kwargs (`track="X" artist="Y"`) that build deezer\'s advanced search syntax `track:"X" artist:"Y"` for future opt-in callers (e.g. exact-match flows where api-level filtering is more important than ranking) — kept in client but NOT used at the import-modal endpoint after live testing showed the advanced syntax has its own ranking bias (surfaced "(2008 remaster)" instead of the canonical recording). free-text + local rerank is the more reliable combination here. 75 new tests pin every scoring component, pattern detection (13 cover patterns, 11 variant patterns, 3 fields), score composition (real-cut > karaoke > remaster > re-recorded), the issue #534 screenshot reproduced as a regression test, deezer client query construction + free-text fallback safety net.', page: 'import' }, { title: 'Auto-Import: Album Duration Is Album Total + Re-Imports Fill Metadata Gaps', desc: 'two more parity gaps closed in the soulsync standalone library write path. (1) album row\'s `duration` column was being written with the FIRST imported track\'s duration instead of the album total — pre-existing bug that survived the prior parity commit. soulsync_client deep scan computes `sum(t.duration for t in self._tracks)` for each album; auto-import now mirrors that by computing the sum across every matched track in the worker and threading it through context to the album INSERT. (2) `record_soulsync_library_entry` was insert-only on artists + albums — once a row existed (matched by id OR name fallback), subsequent imports of the same artist or album skipped completely. meant: artist genres / thumb / source-id reflected ONLY whatever the FIRST imported album supplied, never refreshing as more albums by that artist landed (ten more deezer/spotify imports later, artist row still had whatever the first random import wrote). new conservative UPDATE path: when an existing row matches, fill ONLY the columns whose current value is NULL or empty — never overwrites populated values. protects manual edits + enrichment-worker writes the same way scanner UPDATEs preserve enrichment columns. f-string column names are validated against an allowlist (`_SOULSYNC_FILLABLE_COLUMNS`) before interpolation — defensive against accidental misuse adding columns without an allowlist update. 4 new tests pin: album duration uses sum not single-track, re-import fills empty thumb + genres on existing artist row, re-import does NOT clobber populated values, re-import fills empty source-id columns when later import has them.', page: 'import' },