Cover Art Filler: validate search results to stop wrong cover art
The title/artist fallback search took results[0]'s artwork unconditionally, so a loose full-text match returned the wrong album's cover (the 'new sources give incorrect art' reports). Now it pulls a few results and only accepts one whose title matches (subset, to allow Deluxe/Remaster) AND whose artist matches exactly — the artist being the strong guard against wrong covers. Falls back to an exact title match when a result carries no artist. The album's own stored source-id path is unchanged (that id is authoritative). Tests: wrong-artist rejected, skips wrong result for a matching one, + unit coverage of the matcher (deluxe/feat/stopwords accepted, wrong artist/title rejected).
This commit is contained in:
BoulderBadgeDad
parent
45f91fd318
commit
80828b86cf
2 changed files with 151 additions and 7 deletions
|
|
@ -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':
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
Loading…
Reference in a new issue