diff --git a/core/audiodb_worker.py b/core/audiodb_worker.py index fcb2bcb9..20bfd16e 100644 --- a/core/audiodb_worker.py +++ b/core/audiodb_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.audiodb_client import AudioDBClient -from core.worker_utils import interruptible_sleep +from core.worker_utils import accept_artist_match, interruptible_sleep logger = get_logger("audiodb_worker") @@ -426,14 +426,18 @@ class AudioDBWorker: result = self.client.search_artist(item_name) if result: result_name = result.get('strArtist', '') - if self._name_matches(item_name, result_name): + ok, reason = accept_artist_match( + self.db, 'audiodb_id', result.get('idArtist'), item_id, + item_name, result_name, + ) + if ok: self._update_artist(item_id, result) self.stats['matched'] += 1 logger.info(f"Matched artist '{item_name}' -> AudioDB ID: {result.get('idArtist')}") else: self._mark_status('artist', item_id, 'not_found') self.stats['not_found'] += 1 - logger.debug(f"Name mismatch for artist '{item_name}' (got '{result_name}')") + logger.debug(f"Artist '{item_name}' not matched: {reason}") else: self._mark_status('artist', item_id, 'not_found') self.stats['not_found'] += 1 diff --git a/core/deezer_worker.py b/core/deezer_worker.py index 63958da5..15441c1e 100644 --- a/core/deezer_worker.py +++ b/core/deezer_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.deezer_client import DeezerClient -from core.worker_utils import interruptible_sleep, set_album_api_track_count +from core.worker_utils import accept_artist_match, interruptible_sleep, set_album_api_track_count from core.enrichment.manual_match_honoring import honor_stored_match logger = get_logger("deezer_worker") @@ -404,14 +404,18 @@ class DeezerWorker: result = self.client.search_artist(artist_name) if result: result_name = result.get('name', '') - if self._name_matches(artist_name, result_name): + ok, reason = accept_artist_match( + self.db, 'deezer_id', result.get('id'), artist_id, + artist_name, result_name, + ) + if ok: self._update_artist(artist_id, result) self.stats['matched'] += 1 logger.info(f"Matched artist '{artist_name}' -> Deezer ID: {result.get('id')}") else: self._mark_status('artist', artist_id, 'not_found') self.stats['not_found'] += 1 - logger.debug(f"Name mismatch for artist '{artist_name}' (got '{result_name}')") + logger.debug(f"Artist '{artist_name}' not matched: {reason}") else: self._mark_status('artist', artist_id, 'not_found') self.stats['not_found'] += 1 diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 429ba320..994ee259 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -18,7 +18,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.discogs_client import DiscogsClient -from core.worker_utils import interruptible_sleep, set_album_api_track_count +from core.worker_utils import accept_artist_match, interruptible_sleep, set_album_api_track_count logger = get_logger("discogs_worker") @@ -332,9 +332,13 @@ class DiscogsWorker: self.stats['not_found'] += 1 return - # Find best match by name similarity + # Find best match by name similarity (skipping ids already claimed by + # a differently-named artist, so we don't create a shared/duplicate id). for result in results: - if self._name_matches(artist_name, result.name): + ok, reason = accept_artist_match( + self.db, 'discogs_id', result.id, artist_id, artist_name, result.name, + ) + if ok: # Fetch full artist detail (uses cache) data = self.client._fetch_and_cache_artist(result.id) if data: diff --git a/core/itunes_worker.py b/core/itunes_worker.py index cca60db0..0d0e10a7 100644 --- a/core/itunes_worker.py +++ b/core/itunes_worker.py @@ -9,7 +9,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.itunes_client import iTunesClient -from core.worker_utils import interruptible_sleep, set_album_api_track_count +from core.worker_utils import accept_artist_match, interruptible_sleep, set_album_api_track_count from core.enrichment.manual_match_honoring import honor_stored_match logger = get_logger("itunes_worker") @@ -393,7 +393,11 @@ class iTunesWorker: return for artist_obj in results: - if self._name_matches(artist_name, artist_obj.name): + ok, reason = accept_artist_match( + self.db, 'itunes_artist_id', artist_obj.id, artist_id, + artist_name, artist_obj.name, + ) + if ok: if not self._is_itunes_id(artist_obj.id): logger.warning(f"Rejecting non-iTunes ID '{artist_obj.id}' for artist '{artist_name}'") self._mark_status('artist', artist_id, 'error') diff --git a/core/musicbrainz_worker.py b/core/musicbrainz_worker.py index 4f3adf55..8f7c1241 100644 --- a/core/musicbrainz_worker.py +++ b/core/musicbrainz_worker.py @@ -5,7 +5,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.musicbrainz_service import MusicBrainzService -from core.worker_utils import interruptible_sleep +from core.worker_utils import interruptible_sleep, source_id_conflict logger = get_logger("musicbrainz_worker") @@ -367,8 +367,17 @@ class MusicBrainzWorker: if item_type == 'artist': 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') + mbid = result.get('mbid') if result else None + # MB's combined score can match a weak name ("Grant" -> "Amy + # Grant") when its own relevance rank is high. Guard against + # assigning an mbid a differently-named artist already holds, so + # one mbid can't be smeared across unrelated artists. + conflict = ( + source_id_conflict(self.db, 'musicbrainz_id', mbid, item_id, item_name) + if mbid else None + ) + if mbid and not conflict: + self.mb_service.update_artist_mbid(item_id, 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 @@ -377,7 +386,7 @@ class MusicBrainzWorker: # empty list) so a transient MB outage never regresses # the enrichment outcome. try: - aliases = self.mb_service.fetch_artist_aliases(result['mbid']) + aliases = self.mb_service.fetch_artist_aliases(mbid) if aliases: self.mb_service.update_artist_aliases(item_id, aliases) logger.debug( @@ -388,11 +397,17 @@ class MusicBrainzWorker: "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']}") + logger.info(f"Matched artist '{item_name}' → MBID: {mbid}") else: self.mb_service.update_artist_mbid(item_id, None, 'not_found') self.stats['not_found'] += 1 - logger.debug(f"No match for artist '{item_name}'") + if conflict: + logger.debug( + f"Artist '{item_name}' → MBID {mbid} skipped: " + f"already claimed by '{conflict}'" + ) + else: + logger.debug(f"No match for artist '{item_name}'") elif item_type == 'album': artist_name = item.get('artist') diff --git a/core/qobuz_worker.py b/core/qobuz_worker.py index 030014eb..8051178c 100644 --- a/core/qobuz_worker.py +++ b/core/qobuz_worker.py @@ -8,7 +8,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.qobuz_client import _qobuz_is_rate_limited -from core.worker_utils import interruptible_sleep +from core.worker_utils import accept_artist_match, interruptible_sleep from core.enrichment.manual_match_honoring import honor_stored_match logger = get_logger("qobuz_worker") @@ -423,14 +423,19 @@ class QobuzWorker: if result: result_name = result.get('name', '') - if self._name_matches(artist_name, result_name): - qobuz_artist_id = result.get('id') - if not qobuz_artist_id: - self._mark_status('artist', artist_id, 'error') - self.stats['errors'] += 1 - logger.warning(f"Qobuz search result for '{artist_name}' has no ID") - return - + qobuz_artist_id = result.get('id') + ok, reason = accept_artist_match( + self.db, 'qobuz_id', qobuz_artist_id, artist_id, artist_name, result_name, + ) + if not ok: + self._mark_status('artist', artist_id, 'not_found') + self.stats['not_found'] += 1 + logger.debug(f"Artist '{artist_name}' not matched: {reason}") + elif not qobuz_artist_id: + self._mark_status('artist', artist_id, 'error') + self.stats['errors'] += 1 + logger.warning(f"Qobuz search result for '{artist_name}' has no ID") + else: # Fetch full artist details full_artist = None try: @@ -441,10 +446,6 @@ class QobuzWorker: self._update_artist(artist_id, result, full_artist) self.stats['matched'] += 1 logger.info(f"Matched artist '{artist_name}' -> Qobuz ID: {qobuz_artist_id}") - else: - self._mark_status('artist', artist_id, 'not_found') - self.stats['not_found'] += 1 - logger.debug(f"Name mismatch for artist '{artist_name}' (got '{result_name}')") else: if _qobuz_is_rate_limited(): logger.warning(f"Rate limited while searching artist '{artist_name}', will retry") diff --git a/core/spotify_worker.py b/core/spotify_worker.py index 4e0b3e66..2b113a8a 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -9,7 +9,12 @@ from datetime import datetime, date, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.spotify_client import SpotifyClient, SpotifyRateLimitError -from core.worker_utils import interruptible_sleep, set_album_api_track_count +from core.worker_utils import ( + ARTIST_NAME_MATCH_THRESHOLD, + interruptible_sleep, + set_album_api_track_count, + source_id_conflict, +) from core.enrichment.manual_match_honoring import honor_stored_match logger = get_logger("spotify_worker") @@ -483,12 +488,14 @@ class SpotifyWorker: logger.debug(f"No Spotify results for artist '{artist_name}'") return - # Find best fuzzy match — score all candidates, pick highest above threshold + # Find best fuzzy match — score all candidates, pick highest above the + # (stricter, artist-specific) threshold so short-name false positives + # like "ODESZA"/"odessa" don't slip through. best_obj = None best_score = 0 for artist_obj in results: score = self._name_similarity(artist_name, artist_obj.name) - if score >= self.name_similarity_threshold and score > best_score: + if score >= ARTIST_NAME_MATCH_THRESHOLD and score > best_score: best_obj = artist_obj best_score = score @@ -498,6 +505,19 @@ class SpotifyWorker: self._mark_status('artist', artist_id, 'error') self.stats['errors'] += 1 return + # Don't assign a Spotify id another (differently-named) artist + # already holds — prevents one id smeared across artists. + conflict = source_id_conflict( + self.db, 'spotify_artist_id', best_obj.id, artist_id, artist_name + ) + if conflict: + self._mark_status('artist', artist_id, 'not_found') + self.stats['not_found'] += 1 + logger.debug( + f"Artist '{artist_name}' -> Spotify {best_obj.id} skipped: " + f"already claimed by '{conflict}'" + ) + return self._update_artist(artist_id, best_obj) self.stats['matched'] += 1 logger.info(f"Matched artist '{artist_name}' -> Spotify ID: {best_obj.id} (score: {best_score:.2f})") diff --git a/core/tidal_worker.py b/core/tidal_worker.py index 9996596d..01ac795f 100644 --- a/core/tidal_worker.py +++ b/core/tidal_worker.py @@ -7,7 +7,7 @@ from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase from core.tidal_client import TidalClient -from core.worker_utils import interruptible_sleep +from core.worker_utils import accept_artist_match, interruptible_sleep from core.enrichment.manual_match_honoring import honor_stored_match logger = get_logger("tidal_worker") @@ -435,14 +435,19 @@ class TidalWorker: result = self.client.search_artist(artist_name) if result: result_name = result.get('name', '') - if self._name_matches(artist_name, result_name): - tidal_artist_id = result.get('id') - if not tidal_artist_id: - self._mark_status('artist', artist_id, 'error') - self.stats['errors'] += 1 - logger.warning(f"Tidal search result for '{artist_name}' has no ID") - return - + tidal_artist_id = result.get('id') + ok, reason = accept_artist_match( + self.db, 'tidal_id', tidal_artist_id, artist_id, artist_name, result_name, + ) + if not ok: + self._mark_status('artist', artist_id, 'not_found') + self.stats['not_found'] += 1 + logger.debug(f"Artist '{artist_name}' not matched: {reason}") + elif not tidal_artist_id: + self._mark_status('artist', artist_id, 'error') + self.stats['errors'] += 1 + logger.warning(f"Tidal search result for '{artist_name}' has no ID") + else: # Fetch full artist details for image full_artist = None try: @@ -453,10 +458,6 @@ class TidalWorker: self._update_artist(artist_id, result, full_artist) self.stats['matched'] += 1 logger.info(f"Matched artist '{artist_name}' -> Tidal ID: {tidal_artist_id}") - else: - self._mark_status('artist', artist_id, 'not_found') - self.stats['not_found'] += 1 - logger.debug(f"Name mismatch for artist '{artist_name}' (got '{result_name}')") else: self._mark_status('artist', artist_id, 'not_found') self.stats['not_found'] += 1 diff --git a/core/worker_utils.py b/core/worker_utils.py index a8d965b9..7a8b57cb 100644 --- a/core/worker_utils.py +++ b/core/worker_utils.py @@ -1,10 +1,105 @@ """Shared helpers for background workers.""" import logging +import re import threading +from difflib import SequenceMatcher +from typing import Optional logger = logging.getLogger(__name__) +# Artist-match acceptance gate. Stricter than the 0.80 each worker uses for +# album/track titles: artist names are short, so 0.80 lets distinct artists +# slip through ("ODESZA"/"odessa", "Blance"/"Blanke", "Lady A"/"Lady Gaga" all +# score 0.80-0.83). 0.85 rejects those while still tolerating real variation +# that survives normalization. +ARTIST_NAME_MATCH_THRESHOLD = 0.85 + +# Whitelist of artist source-id columns we'll interpolate into SQL — guards the +# conflict query against any unexpected column name. +_ARTIST_ID_COLUMNS = frozenset({ + 'deezer_id', 'spotify_artist_id', 'itunes_artist_id', 'musicbrainz_id', + 'discogs_id', 'audiodb_id', 'qobuz_id', 'tidal_id', 'amazon_id', 'soul_id', +}) + + +def normalize_artist_name(name: str) -> str: + """Lowercase, drop ' - ...' suffixes / parentheticals / punctuation, and + collapse whitespace — the same normalization the per-worker matchers use.""" + name = (name or '').lower().strip() + name = re.sub(r'\s+[-–—]\s+.*$', '', name) + name = re.sub(r'\s*\(.*?\)\s*', ' ', name) + name = re.sub(r'[^\w\s]', '', name) + name = re.sub(r'\s+', ' ', name).strip() + return name + + +def artist_name_matches(query: str, result: str, + threshold: float = ARTIST_NAME_MATCH_THRESHOLD) -> bool: + """True if two artist names match at/above ``threshold`` after normalization.""" + nq, nr = normalize_artist_name(query), normalize_artist_name(result) + if not nq or not nr: + return False + return SequenceMatcher(None, nq, nr).ratio() >= threshold + + +def _names_equivalent(a: str, b: str) -> bool: + return normalize_artist_name(a) == normalize_artist_name(b) + + +def source_id_conflict(database, id_column: str, source_id, artist_id, + artist_name: str) -> Optional[str]: + """Return the name of a DIFFERENTLY-named library artist that already holds + ``source_id`` in ``id_column``, or None. + + A same-named holder (the same artist indexed on two media servers) is NOT a + conflict — both legitimately share the id. Only a different artist holding + the id signals the kind of corruption where one source id gets smeared + across unrelated artists. + """ + if source_id in (None, ''): + return None + if id_column not in _ARTIST_ID_COLUMNS: + logger.debug(f"source_id_conflict: refusing unknown column {id_column!r}") + return None + try: + with database._get_connection() as conn: + rows = conn.execute( + f"SELECT name FROM artists WHERE {id_column} = ? AND id != ?", + (str(source_id), artist_id), + ).fetchall() + except Exception as e: + logger.debug(f"source_id_conflict check failed for {id_column}={source_id}: {e}") + return None + for (other_name,) in rows: + if not _names_equivalent(artist_name, other_name): + return other_name + return None + + +def accept_artist_match(database, id_column: str, source_id, artist_id, + query_name: str, result_name: str, + threshold: float = ARTIST_NAME_MATCH_THRESHOLD) -> tuple: + """Decide whether to store ``source_id`` on an artist. + + Returns ``(ok: bool, reason: str)``. Accepts only when the result's name + matches the library artist at/above ``threshold`` AND the id isn't already + claimed by a differently-named artist. ``reason`` explains a rejection (for + debug logging). This is the single gate every worker's artist match should + pass through, so the 'one id smeared across many artists' bug can't recur. + """ + if not artist_name_matches(query_name, result_name, threshold): + return False, ( + f"name mismatch '{query_name}' vs '{result_name}' (< {threshold})" + ) + conflict = source_id_conflict(database, id_column, source_id, artist_id, query_name) + if conflict: + return False, ( + f"{id_column}={source_id} already claimed by '{conflict}' — " + f"skipping to avoid a shared/duplicate id" + ) + return True, "" + def interruptible_sleep(stop_event: threading.Event, seconds: float, step: float = 0.5) -> bool: """Sleep in chunks so shutdown can interrupt long waits.""" diff --git a/tests/test_worker_artist_match_gate.py b/tests/test_worker_artist_match_gate.py new file mode 100644 index 00000000..59810852 --- /dev/null +++ b/tests/test_worker_artist_match_gate.py @@ -0,0 +1,114 @@ +"""Tests for the shared artist-match gate in core/worker_utils.py. + +Two jobs: + * artist_name_matches — a stricter (0.85) gate than the 0.80 used for + album/track titles, so short-name false positives ('ODESZA'/'odessa', + 'Blance'/'Blanke', 'Lady A'/'Lady Gaga') are rejected. + * source_id_conflict / accept_artist_match — refuse to store a source id that + a DIFFERENTLY-named artist already holds, while still allowing a same-named + artist (the same act indexed on two media servers) to share it. +""" + +from __future__ import annotations + +import pytest + +from core import worker_utils as wu +from database.music_database import MusicDatabase + + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "music.db")) + + +def _insert(db, *, artist_id, name, **extra): + cols = ["id", "name", "server_source"] + list(extra.keys()) + vals = [artist_id, name, "plex"] + list(extra.values()) + ph = ",".join("?" for _ in cols) + with db._get_connection() as conn: + conn.execute(f"INSERT INTO artists ({','.join(cols)}) VALUES ({ph})", vals) + conn.commit() + + +# --------------------------------------------------------------------------- +# artist_name_matches — the 0.85 gate +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("a,b", [ + ("ODESZA", "odessa"), + ("Blance", "Blanke"), + ("COLLEGE", "Colle"), + ("Lady A", "Lady Gaga"), + ("M&O", "M.O.P."), +]) +def test_near_name_pairs_rejected_at_085(a, b): + assert wu.artist_name_matches(a, b) is False + + +@pytest.mark.parametrize("a,b", [ + ("Saib", "saib."), # punctuation only → identical + ("-Us.", "Us"), + ("Kendrick Lamar", "KENDRICK LAMAR"), + ("Beyoncé", "Beyonce"), +]) +def test_true_variants_still_match(a, b): + assert wu.artist_name_matches(a, b) is True + + +# --------------------------------------------------------------------------- +# source_id_conflict — different name blocks, same name allowed +# --------------------------------------------------------------------------- + +def test_conflict_when_different_named_artist_holds_id(db): + _insert(db, artist_id="1", name="Kendrick Lamar", deezer_id="525046") + # Trying to give Jorja the same id → conflict reports the holder. + assert wu.source_id_conflict(db, "deezer_id", "525046", "2", "Jorja Smith") == "Kendrick Lamar" + + +def test_no_conflict_for_same_named_artist_two_servers(db): + # Radiohead already on plex with this id; the jellyfin Radiohead (id 2) may + # legitimately share it. + _insert(db, artist_id="1", name="Radiohead", deezer_id="999") + assert wu.source_id_conflict(db, "deezer_id", "999", "2", "Radiohead") is None + + +def test_no_conflict_when_same_row_holds_it(db): + _insert(db, artist_id="1", name="Radiohead", deezer_id="999") + # Re-matching the same artist to its own id is fine. + assert wu.source_id_conflict(db, "deezer_id", "999", "1", "Radiohead") is None + + +def test_no_conflict_when_id_unused(db): + assert wu.source_id_conflict(db, "deezer_id", "12345", "1", "Anyone") is None + + +def test_unknown_column_is_refused(db): + assert wu.source_id_conflict(db, "evil_id", "x", "1", "Anyone") is None + + +# --------------------------------------------------------------------------- +# accept_artist_match — combined gate +# --------------------------------------------------------------------------- + +def test_accept_rejects_name_mismatch(db): + ok, reason = wu.accept_artist_match(db, "deezer_id", "1", "1", "ODESZA", "odessa") + assert ok is False + assert "name mismatch" in reason + + +def test_accept_rejects_id_already_claimed_by_other(db): + _insert(db, artist_id="1", name="Kendrick Lamar", deezer_id="525046") + ok, reason = wu.accept_artist_match( + db, "deezer_id", "525046", "2", "Jorja Smith", "Jorja Smith" + ) + assert ok is False + assert "already claimed" in reason + + +def test_accept_passes_clean_match(db): + ok, reason = wu.accept_artist_match( + db, "deezer_id", "111", "1", "Kendrick Lamar", "Kendrick Lamar" + ) + assert ok is True + assert reason == ""