diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index 4c292873..9ebc9d57 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -1,5 +1,7 @@ """Missing Cover Art Filler Job — finds albums without artwork and locates art from APIs.""" +import re + from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob @@ -7,6 +9,33 @@ from utils.logging_config import get_logger logger = get_logger("repair_job.cover_art") +# Stopwords dropped before comparing album/artist names so trivial words +# ("the", "and") don't make two different names look like a match. +_NAME_STOPWORDS = {'the', 'a', 'an', 'and', 'of', 'feat', 'ft', 'featuring'} + + +def _norm_name(value) -> str: + """Lowercase, strip bracketed qualifiers (Deluxe/Remaster/feat.) and + punctuation so names can be compared on their significant words.""" + s = (value or '').lower() + s = re.sub(r'[\(\[\{].*?[\)\]\}]', ' ', s) # drop (...) [...] qualifiers + s = re.sub(r'\b(?:feat|ft|featuring)\b.*', ' ', s) # drop trailing "feat. X" + s = re.sub(r'[^a-z0-9]+', ' ', s) + return ' '.join(s.split()) + + +def _name_tokens(value) -> set: + return set(_norm_name(value).split()) - _NAME_STOPWORDS + + +def _names_match(a, b) -> bool: + """True when two names share all the significant words of the shorter one + (so "Album" matches "Album (Deluxe)", but unrelated titles don't).""" + ta, tb = _name_tokens(a), _name_tokens(b) + if not ta or not tb: + return False + return ta <= tb or tb <= ta + @register_job class MissingCoverArtJob(RepairJob): @@ -192,19 +221,70 @@ class MissingCoverArtJob(RepairJob): return artwork_url if query and hasattr(client, 'search_albums'): - results = client.search_albums(query, limit=1) - if results: - artwork_url = self._extract_artwork_url(results[0]) + # Pull a few results and only accept one whose title AND artist + # actually match this album. The old code grabbed results[0]'s + # artwork unconditionally, so a loose full-text search returning + # the wrong album gave the wrong cover. + results = client.search_albums(query, limit=5) or [] + for res in results: + if not self._result_matches(res, title, artist_name): + continue + artwork_url = self._extract_artwork_url(res) if artwork_url: return artwork_url - candidate_id = self._extract_album_id(results[0]) + candidate_id = self._extract_album_id(res) if candidate_id: album_data = self._get_album_for_source(source, client, candidate_id) - return self._extract_artwork_url(album_data) + artwork_url = self._extract_artwork_url(album_data) + if artwork_url: + return artwork_url except Exception as e: logger.debug("%s art lookup failed for '%s': %s", source.capitalize(), title, e) return None + @staticmethod + def _result_title_artist(item): + """Pull (title, artist) from a search result that may be a dict or an + Album-like object, across the various source clients.""" + if item is None: + return '', '' + if isinstance(item, dict): + title = item.get('title') or item.get('name') or item.get('album') or '' + artist = item.get('artist') or item.get('artist_name') or '' + if not artist: + artists = item.get('artists') or [] + if isinstance(artists, list) and artists: + a0 = artists[0] + artist = a0.get('name', '') if isinstance(a0, dict) else str(a0) + else: + title = getattr(item, 'title', None) or getattr(item, 'name', None) or getattr(item, 'album', None) or '' + artist = getattr(item, 'artist', None) or getattr(item, 'artist_name', None) or '' + if not artist: + arts = getattr(item, 'artists', None) or [] + if isinstance(arts, list) and arts: + a0 = arts[0] + artist = a0.get('name', '') if isinstance(a0, dict) else str(a0) + return str(title or ''), str(artist or '') + + @classmethod + def _result_matches(cls, result, album_title, album_artist) -> bool: + """Reject a search result unless it confidently matches the album. + + Title must match; if both the result and the album carry an artist, the + artist must match too (the strongest guard against wrong covers). When + the result has no artist to compare, require an exact title match. + """ + r_title, r_artist = cls._result_title_artist(result) + # Title may carry extra qualifiers (Deluxe/Remaster) → allow subset. + if not _names_match(r_title, album_title): + return False + # Artist is the strong guard, so require its significant words to match + # EXACTLY (not subset) — "Different Artist" must NOT match "Artist". + if r_artist and album_artist: + return _name_tokens(r_artist) == _name_tokens(album_artist) + # No artist on the result → require an exact title match instead. + return _norm_name(r_title) == _norm_name(album_title) + @staticmethod def _get_album_for_source(source, client, album_id): if source == 'spotify': diff --git a/tests/test_missing_cover_art.py b/tests/test_missing_cover_art.py index 5313633f..6796a6bb 100644 --- a/tests/test_missing_cover_art.py +++ b/tests/test_missing_cover_art.py @@ -59,7 +59,10 @@ class _FakeClient: self.search_calls.append((query, limit)) if self.search_image is None: return [] - return [SimpleNamespace(id='search-album', image_url=self.search_image)] + # Real source clients return album results carrying title + artist; + # the filler now validates those before trusting the artwork. + return [SimpleNamespace(id='search-album', image_url=self.search_image, + title='Album', artist='Artist')] def _make_db(album_row): @@ -165,7 +168,68 @@ def test_missing_cover_art_uses_primary_when_prefer_unset(monkeypatch): result = mca.MissingCoverArtJob().scan(context) assert result.findings_created == 1 - assert discogs_client.search_calls == [('Artist Album', 1)] + assert discogs_client.search_calls == [('Artist Album', 5)] assert spotify_client.search_calls == [] assert itunes_client.search_calls == [] assert context.findings[0]['details']['found_artwork_url'] == 'https://img/discogs-search' + + +# ── Stricter matching (issue: new sources returning WRONG cover art) ── + +class _SearchClient: + """search_albums returns whatever results it's given (title/artist/image).""" + def __init__(self, results): + self._results = results + self.search_calls = [] + + def get_album(self, album_id, include_tracks=False): + return None + + def search_albums(self, query, limit=1): + self.search_calls.append((query, limit)) + return list(self._results) + + +def test_search_rejects_wrong_artist_result(monkeypatch): + """A result with the right-ish title but a DIFFERENT artist must be rejected + (this is what produced wrong covers from the new sources).""" + conn = _make_db((1, 'Album', 1, '', None, None, None, None, None)) + context = _make_context(conn) + client = _SearchClient([SimpleNamespace(id='x', image_url='https://img/wrong', + title='Album', artist='Different Artist')]) + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'discogs') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: client if s == 'discogs' else None) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 0 # wrong-artist art not accepted + assert context.findings == [] + + +def test_search_skips_wrong_result_and_takes_matching_one(monkeypatch): + """Given several results, take the first that actually matches title+artist.""" + conn = _make_db((1, 'Album', 1, '', None, None, None, None, None)) + context = _make_context(conn) + client = _SearchClient([ + SimpleNamespace(id='a', image_url='https://img/wrong', title='Other Record', artist='Someone'), + SimpleNamespace(id='b', image_url='https://img/right', title='Album', artist='Artist'), + ]) + monkeypatch.setattr(mca, 'get_primary_source', lambda: 'discogs') + monkeypatch.setattr(mca, 'get_client_for_source', lambda s: client if s == 'discogs' else None) + + result = mca.MissingCoverArtJob().scan(context) + assert result.findings_created == 1 + assert context.findings[0]['details']['found_artwork_url'] == 'https://img/right' + + +def test_result_matches_unit(): + m = mca.MissingCoverArtJob._result_matches + # exact + deluxe variant + featuring all accepted when artist matches + assert m({'title': 'Album', 'artist': 'Artist'}, 'Album', 'Artist') + assert m({'title': 'Album (Deluxe Edition)', 'artist': 'Artist'}, 'Album', 'Artist') + assert m({'title': 'Album', 'artist': 'The Artist'}, 'Album', 'Artist') # stopword 'the' + # wrong artist / wrong title rejected + assert not m({'title': 'Album', 'artist': 'Nope'}, 'Album', 'Artist') + assert not m({'title': 'Totally Other', 'artist': 'Artist'}, 'Album', 'Artist') + # no artist on result → require exact title + assert m({'title': 'Album'}, 'Album', 'Artist') + assert not m({'title': 'Album Deluxe'}, 'Album', 'Artist')