diff --git a/core/deezer_client.py b/core/deezer_client.py index 59330239..71265a12 100644 --- a/core/deezer_client.py +++ b/core/deezer_client.py @@ -298,10 +298,78 @@ class DeezerClient: # can serve as a drop-in fallback metadata source in SpotifyClient. @rate_limited - def search_tracks(self, query: str, limit: int = 20) -> List[Track]: - """Search for tracks — returns Track dataclass list (metadata source interface)""" + def search_tracks( + self, + query: str = '', + limit: int = 20, + *, + track: Optional[str] = None, + artist: Optional[str] = None, + album: Optional[str] = None, + ) -> List[Track]: + """Search for tracks — returns Track dataclass list (metadata source interface). + + Two call modes: + + 1. **Free-text** (`query='Foreigner Dirty White Boy'`) — legacy + shape, passes the string straight to Deezer's `q` param. + Same behaviour as before, kept for backward compat. + + 2. **Field-scoped** (`track='Dirty White Boy', artist='Foreigner'`) — + builds Deezer's advanced search syntax (`track:"X" artist:"Y"`). + Massively tighter relevance than the free-text path because + the API matches each term in the right field instead of + anywhere across title / lyrics / artist / album / contributors. + Without this, the Deezer ranking buries the canonical track + under karaoke / cover / "originally performed by" variants + — see issue #534. + + Field-scoped form is used whenever ``track`` or ``artist`` is + provided. ``query`` is ignored in that case (the field params + are authoritative). When both are missing, falls through to + ``query``. The cache key is the constructed query string in + either case so the two paths share entries naturally. + """ + # Build the actual API query — advanced syntax when callers pass + # field hints, raw query otherwise. + used_advanced = bool(track or artist or album) + if used_advanced: + api_query = self._build_advanced_query(track=track, artist=artist, album=album) + else: + api_query = query + + if not api_query: + return [] + + tracks = self._search_tracks_with_query(api_query, limit) + + # Safety net: Deezer's advanced syntax is `artist:"X"`-style + # substring match, but in practice it's brittle on artist name + # variants ("Foreigner [US]", "The Foreigner", etc.) and on + # tracks indexed under non-canonical title spellings. When the + # advanced query returns nothing, fall back to a free-text join + # so the user sees the prior (less-relevant but non-empty) result + # set rather than "No matches" — same behaviour as pre-fix for + # this edge case. Caller-side rerank still tightens the result. + if not tracks and used_advanced: + fallback_parts = [p for p in (track, artist, album) if p] + fallback_query = ' '.join(fallback_parts) + if fallback_query and fallback_query != api_query: + logger.debug( + "[Deezer] Advanced query returned 0 results, falling back " + "to free-text: %r → %r", api_query, fallback_query, + ) + tracks = self._search_tracks_with_query(fallback_query, limit) + + return tracks + + def _search_tracks_with_query(self, api_query: str, limit: int) -> List[Track]: + """Cache-aware single API call. Pulled out so the + ``search_tracks`` orchestration can call this twice (advanced + query → free-text fallback) without duplicating the cache + + parse + store dance.""" cache = get_metadata_cache() - cached_results = cache.get_search_results('deezer', 'track', query, limit) + cached_results = cache.get_search_results('deezer', 'track', api_query, limit) if cached_results is not None: tracks = [] for raw in cached_results: @@ -312,25 +380,54 @@ class DeezerClient: if tracks: return tracks - data = self._api_get('search/track', {'q': query, 'limit': min(limit, 100)}) + data = self._api_get('search/track', {'q': api_query, 'limit': min(limit, 100)}) if not data or 'data' not in data: return [] tracks = [] raw_items = [] for track_data in data['data']: - track = Track.from_deezer_track(track_data) - tracks.append(track) + track_obj = Track.from_deezer_track(track_data) + tracks.append(track_obj) raw_items.append(track_data) entries = [(str(td.get('id', '')), td) for td in raw_items if td.get('id')] if entries: cache.store_entities_bulk('deezer', 'track', entries) - cache.store_search_results('deezer', 'track', query, limit, + cache.store_search_results('deezer', 'track', api_query, limit, [str(td.get('id', '')) for td in raw_items if td.get('id')]) return tracks + @staticmethod + def _build_advanced_query( + *, + track: Optional[str] = None, + artist: Optional[str] = None, + album: Optional[str] = None, + ) -> str: + """Compose Deezer's advanced search syntax from field hints. + + Per Deezer's docs: + https://developers.deezer.com/api/search + + q=track:"X" artist:"Y" album:"Z" + + Quotes around each value preserve multi-word phrases. Empty + fields are skipped. Embedded double-quotes get stripped (no + escape mechanism in Deezer's syntax) — rare in practice, but + a search for `O"Hara` would otherwise produce a malformed + query. + """ + parts = [] + if track: + parts.append(f'track:"{track.replace(chr(34), "")}"') + if artist: + parts.append(f'artist:"{artist.replace(chr(34), "")}"') + if album: + parts.append(f'album:"{album.replace(chr(34), "")}"') + return ' '.join(parts) + @rate_limited def search_artists(self, query: str, limit: int = 20) -> List[Artist]: """Search for artists — returns Artist dataclass list (metadata source interface)""" diff --git a/core/metadata/relevance.py b/core/metadata/relevance.py new file mode 100644 index 00000000..37aed5e0 --- /dev/null +++ b/core/metadata/relevance.py @@ -0,0 +1,343 @@ +"""Local relevance re-ranking for metadata-source search results. + +Background +---------- + +Some metadata sources (Deezer notably) return search results in a +relevance order that puts karaoke covers, "originally performed by", +re-recorded versions, tribute compilations, and Vocal/Backing-Track +variants ABOVE the actual studio recording the user is looking for. +Their global popularity ordering means anything that appears across +many compilations outranks the canonical track. Issue #534 is the +canonical example: searching `Dirty White Boy` + `Foreigner` returned +five karaoke / cover variants before the real Foreigner studio cut. + +This module is a provider-neutral helper. Given a list of typed +``Track`` results plus an expected title + artist, it re-ranks by +local heuristics that the source's own ranking ignores: + +- Hard penalty for known cover/karaoke/tribute patterns (title OR + album OR artist field). These rarely belong in import / match + results when the user typed the original artist. +- Soft penalty for variant types (Live, Acoustic, Remix, Demo, + Instrumental) UNLESS the user's expected title also contains the + variant tag (so "Track (Live)" search matches Live recordings). +- Boost for exact artist match — the strongest signal that this is + the canonical recording. +- Title similarity via SequenceMatcher on normalised strings (drop + parentheticals + punctuation before comparison). +- Album-type weight: album > compilation > single (compilations are + more likely to be tributes / "best of" repackages). + +Pure-function design over the canonical ``Track`` dataclass — +no Deezer-specific assumptions, applies to iTunes / Spotify / +Hydrabase results equally well. Each scoring component is its own +small function so tests can pin them independently. + +Usage +----- + +>>> from core.metadata.relevance import rerank_tracks +>>> tracks = client.search_tracks(query) +>>> ranked = rerank_tracks(tracks, expected_title='Dirty White Boy', expected_artist='Foreigner') +>>> # ranked[0] is now the most relevant; karaoke variants drop to bottom +""" + +from __future__ import annotations + +import re +from difflib import SequenceMatcher +from typing import List, Optional, Sequence + +from core.metadata.types import Track + + +# --------------------------------------------------------------------------- +# Pattern tables — public so tests can introspect, callers can extend +# --------------------------------------------------------------------------- + + +# Title / album / artist substrings that strongly indicate a cover, +# karaoke, tribute, or "originally performed by" compilation. Multiplier +# applied to the final score when matched. 0.05 effectively buries these +# unless nothing else matches. +COVER_KARAOKE_PATTERNS = ( + 'karaoke', + 'originally performed by', + 'in the style of', + 'made famous by', + 'tribute', + 'vocal version', # karaoke "vocal version" backing tracks + 'backing track', + 'cover version', + 're-recorded', # artist re-recordings (Taylor's Version notwithstanding) + 're-record', + 'rerecorded', + 'cover by', + 'as performed by', + 'workout mix', # gym-music compilations + 'study music', + 'music for', # "Music for Studying", "Music for Sleep" etc +) + +COVER_KARAOKE_PENALTY = 0.05 # Multiplicative; effectively bury + + +# Variant tags — softer penalty since the user MAY want them. Skipped +# when the user's expected_title also contains the same tag (so +# "Track Name (Live)" search matches the Live version cleanly). +VARIANT_TAG_PATTERNS = ( + 'live', + 'acoustic', + 'demo', + 'instrumental', + 'remix', + 'edit', + 'extended', + 'radio edit', + 'club mix', + 'a cappella', + 'acapella', + # Remaster — softer than karaoke (user might want it) but still + # demoted vs. the original recording. Verified against live Deezer + # API behaviour where "(2008 Remaster)" outranks the Head Games + # original on `track:"X" artist:"Y"` advanced queries. + 'remaster', + 'remastered', + 'reissue', +) + +VARIANT_TAG_PENALTY = 0.4 + + +# Strong boost when the source's artist field exactly matches the +# user's expected artist (case-insensitive, normalised). The single +# strongest signal that this is the canonical recording. +EXACT_ARTIST_BOOST = 1.5 + + +# Album-type weights. Compilations are more likely to be tributes / +# karaoke repackages; albums are most likely to be the canonical +# studio source. +ALBUM_TYPE_WEIGHT = { + 'album': 1.0, + 'single': 0.85, + 'ep': 0.85, + 'compilation': 0.7, +} +DEFAULT_ALBUM_TYPE_WEIGHT = 0.85 + + +# --------------------------------------------------------------------------- +# Normalisation +# --------------------------------------------------------------------------- + + +_PARENTHETICAL_RE = re.compile(r'[\(\[].*?[\)\]]') +_PUNCT_RE = re.compile(r'[^\w\s]') + + +def _normalise(text: str) -> str: + """Lowercase, strip parentheticals + punctuation, collapse spaces. + + Used for similarity scoring AND for variant-tag detection (since + we want to know if the user typed the variant tag inside their + own search input).""" + if not text: + return '' + t = text.lower().strip() + t = _PARENTHETICAL_RE.sub('', t) + t = _PUNCT_RE.sub('', t) + return ' '.join(t.split()) + + +def _contains_pattern(haystack: str, patterns: Sequence[str]) -> bool: + """Case-insensitive substring match across patterns. Read raw + `haystack` (NOT the parenthetical-stripped version) — patterns + like "karaoke" most often live INSIDE the parentheticals on + Deezer's titles.""" + if not haystack: + return False + lowered = haystack.lower() + return any(p in lowered for p in patterns) + + +# --------------------------------------------------------------------------- +# Scoring components +# --------------------------------------------------------------------------- + + +def title_similarity(track: Track, expected_title: str) -> float: + """Normalised SequenceMatcher ratio against the expected title.""" + if not expected_title: + return 0.0 + return SequenceMatcher( + None, + _normalise(track.name), + _normalise(expected_title), + ).ratio() + + +def primary_artist(track: Track) -> str: + """First entry from track.artists — that's the lead/primary + credit. Empty when the track has no artist info.""" + if not track.artists: + return '' + first = track.artists[0] + if isinstance(first, dict): + # Some sources still surface raw dicts during migration; fall + # back to .get() rather than assume the dataclass is fully + # normalised. + return str(first.get('name', '') or '') + return str(first) + + +def artist_similarity(track: Track, expected_artist: str) -> float: + """Normalised SequenceMatcher ratio against the expected artist.""" + if not expected_artist: + return 0.0 + return SequenceMatcher( + None, + _normalise(primary_artist(track)), + _normalise(expected_artist), + ).ratio() + + +def has_exact_artist(track: Track, expected_artist: str) -> bool: + """True when the primary artist matches expected_artist after + normalisation. Strict equality on the normalised form (so + "Foreigner" matches "Foreigner" but not "Foreigner Tribute Band").""" + if not expected_artist: + return False + return _normalise(primary_artist(track)) == _normalise(expected_artist) + + +def has_cover_pattern(track: Track) -> bool: + """Any cover/karaoke/tribute pattern in the track title, album + title, or artist credits.""" + if _contains_pattern(track.name, COVER_KARAOKE_PATTERNS): + return True + if _contains_pattern(track.album, COVER_KARAOKE_PATTERNS): + return True + if _contains_pattern(primary_artist(track), COVER_KARAOKE_PATTERNS): + return True + return False + + +def has_variant_tag(track: Track) -> bool: + """Track title contains a variant-version tag (Live, Acoustic, + Remix, Demo, Instrumental, etc.). Album field is intentionally + NOT checked — albums named "MTV Unplugged" shouldn't penalise + every track on them.""" + return _contains_pattern(track.name, VARIANT_TAG_PATTERNS) + + +def album_type_weight(track: Track) -> float: + """Weight from track.album_type. Compilations ranked lower since + they're frequently tribute / karaoke repackages.""" + if not track.album_type: + return DEFAULT_ALBUM_TYPE_WEIGHT + return ALBUM_TYPE_WEIGHT.get(track.album_type.lower(), DEFAULT_ALBUM_TYPE_WEIGHT) + + +# --------------------------------------------------------------------------- +# Combined score +# --------------------------------------------------------------------------- + + +def score_track( + track: Track, + *, + expected_title: str, + expected_artist: str, +) -> float: + """Combined relevance score for a single track. Higher = more + relevant. Roughly 0.0 - 2.5 in practice (boosts can push above + 1.0; penalties can push below 0.1). + + Composition: + + 1. Base = title_sim * 0.6 + artist_sim * 0.4 + 2. Multiply by album_type_weight + 3. If exact artist match: multiply by EXACT_ARTIST_BOOST + 4. If cover/karaoke pattern: multiply by COVER_KARAOKE_PENALTY + (effectively buries unless nothing else matched) + 5. If variant tag (Live, Remix, etc.) AND user did NOT type + a variant tag in their input: multiply by VARIANT_TAG_PENALTY + + Each rule is its own component above so tests can pin them + individually without standing up the full pipeline. + """ + title_sim = title_similarity(track, expected_title) + artist_sim = artist_similarity(track, expected_artist) + score = title_sim * 0.6 + artist_sim * 0.4 + + score *= album_type_weight(track) + + if has_exact_artist(track, expected_artist): + score *= EXACT_ARTIST_BOOST + + if has_cover_pattern(track): + score *= COVER_KARAOKE_PENALTY + + # Variant tag penalty — only when the user didn't ask for a + # variant. Their input "Track (Live)" should rank Live versions + # higher, not lower. + user_wanted_variant = _contains_pattern(expected_title, VARIANT_TAG_PATTERNS) + if has_variant_tag(track) and not user_wanted_variant: + score *= VARIANT_TAG_PENALTY + + return score + + +def rerank_tracks( + tracks: List[Track], + *, + expected_title: str, + expected_artist: str, +) -> List[Track]: + """Return a copy of ``tracks`` sorted by descending relevance + score against the expected title + artist. + + Caller's input list is left untouched. Stable sort preserves the + source's original ordering as a tiebreaker (which is the right + fallback when two candidates score identically — the source's + popularity signal is still useful as a tiebreak). + + No-op when both ``expected_title`` and ``expected_artist`` are + empty (no signal to rank against — return input order).""" + if not expected_title and not expected_artist: + return list(tracks) + scored = [ + (score_track(t, expected_title=expected_title, expected_artist=expected_artist), idx, t) + for idx, t in enumerate(tracks) + ] + # Sort by score desc; idx asc as tiebreaker preserves stable order. + scored.sort(key=lambda x: (-x[0], x[1])) + return [t for _score, _idx, t in scored] + + +def filter_and_rerank( + tracks: List[Track], + *, + expected_title: str, + expected_artist: str, + min_score: Optional[float] = None, +) -> List[Track]: + """Convenience: rerank then optionally drop everything below a + score floor. Useful when callers want to hide low-confidence + matches entirely instead of demoting them. + + Returns reranked-only list when ``min_score`` is None — same as + ``rerank_tracks``.""" + ranked = rerank_tracks( + tracks, + expected_title=expected_title, + expected_artist=expected_artist, + ) + if min_score is None: + return ranked + return [ + t for t in ranked + if score_track(t, expected_title=expected_title, expected_artist=expected_artist) >= min_score + ] diff --git a/tests/imports/test_search_match_endpoints.py b/tests/imports/test_search_match_endpoints.py new file mode 100644 index 00000000..ba59b2e7 --- /dev/null +++ b/tests/imports/test_search_match_endpoints.py @@ -0,0 +1,213 @@ +"""End-to-end tests for the import-modal search endpoints. + +Issue #534 — these endpoints back the "Search for Match" dialog +that lets users find a track when auto-match failed. They were +returning karaoke / cover variants ahead of canonical recordings +because: + +1. Deezer endpoint joined `track + artist` into a single free-text + string, losing field-scoping. +2. None of the endpoints applied any local relevance rerank, so + junk results stayed wherever the source's API put them. + +These tests pin the post-fix wiring: +- Deezer endpoint passes `track=` + `artist=` kwargs to the client + (which now builds advanced-syntax `track:"X" artist:"Y"`). +- Deezer + iTunes + Spotify endpoints all run the response through + ``rerank_tracks`` so karaoke / cover patterns drop to the bottom. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + + +@pytest.fixture +def app_test_client(): + """Spin up a Flask test client backed by web_server.app.""" + import web_server + web_server.app.config['TESTING'] = True + with web_server.app.test_client() as client: + yield client + + +@pytest.fixture +def fake_track(): + """Factory for a Track-like object the endpoints can serialise.""" + from core.metadata.types import Track + + def _make(name, artist, album='Album', track_id='t', album_type='album'): + return Track( + id=track_id, name=name, artists=[artist], + album=album, duration_ms=200000, album_type=album_type, + ) + return _make + + +# --------------------------------------------------------------------------- +# /api/deezer/search_tracks — field-scoped + rerank +# --------------------------------------------------------------------------- + + +class TestDeezerSearchTracksEndpoint: + def test_joins_track_and_artist_into_free_text_query(self, app_test_client, fake_track): + """Endpoint sends the joined `track artist` string as Deezer's + free-text `q`. Field-scoped advanced-syntax queries were + initially considered, but live-API testing showed Deezer's + advanced-query ranking misses canonical recordings on some + searches. Free-text + local rerank is the more reliable + combination at this endpoint. Client-level kwarg support + remains for future opt-in callers.""" + fake_client = MagicMock() + fake_client.search_tracks.return_value = [ + fake_track('Dirty White Boy', 'Foreigner'), + ] + with patch('web_server._get_deezer_client', return_value=fake_client): + resp = app_test_client.get( + '/api/deezer/search_tracks?track=Dirty+White+Boy&artist=Foreigner&limit=20' + ) + assert resp.status_code == 200 + call = fake_client.search_tracks.call_args + # First positional arg is the joined free-text query + assert call.args[0] == 'Dirty White Boy Foreigner' + assert call.kwargs.get('limit') == 20 + + def test_reranks_results_burying_karaoke(self, app_test_client, fake_track): + """Endpoint runs results through rerank_tracks. Real Foreigner + cut must end up first, karaoke variant last — even though the + client returned them in the broken Deezer-API order.""" + fake_client = MagicMock() + fake_client.search_tracks.return_value = [ + fake_track('Dirty White Boy (Karaoke Version Originally Performed By Foreigner)', + 'Pop Music Workshop', album='Backing Tracks', + album_type='compilation', track_id='karaoke-1'), + fake_track('Dirty White Boy', 'Foreigner', album='Head Games', + album_type='album', track_id='real-1'), + ] + with patch('web_server._get_deezer_client', return_value=fake_client): + resp = app_test_client.get( + '/api/deezer/search_tracks?track=Dirty+White+Boy&artist=Foreigner' + ) + assert resp.status_code == 200 + body = resp.get_json() + ids = [t['id'] for t in body['tracks']] + assert ids[0] == 'real-1', ( + f"Real cut should be first after rerank; got order {ids}" + ) + assert ids[-1] == 'karaoke-1', ( + f"Karaoke variant should be last; got order {ids}" + ) + + def test_legacy_query_param_still_works(self, app_test_client, fake_track): + """Backward compat: callers passing the legacy `query=` param + get free-text search, no rerank (no signal to rank against).""" + fake_client = MagicMock() + fake_client.search_tracks.return_value = [ + fake_track('Anything', 'Whatever', track_id='only'), + ] + with patch('web_server._get_deezer_client', return_value=fake_client): + resp = app_test_client.get( + '/api/deezer/search_tracks?query=anything+whatever' + ) + assert resp.status_code == 200 + # Legacy path passes positional query, no track/artist kwargs + call = fake_client.search_tracks.call_args + assert call.args[0] == 'anything whatever' or call.kwargs.get('query') == 'anything whatever' + + def test_missing_query_returns_400(self, app_test_client): + """Empty input → 400. Don't waste an API call.""" + with patch('web_server._get_deezer_client', return_value=MagicMock()): + resp = app_test_client.get('/api/deezer/search_tracks') + assert resp.status_code == 400 + + +# --------------------------------------------------------------------------- +# /api/itunes/search_tracks — rerank applied even though iTunes has no +# advanced-syntax search +# --------------------------------------------------------------------------- + + +class TestiTunesSearchTracksEndpoint: + def test_reranks_results_burying_karaoke(self, app_test_client, fake_track, monkeypatch): + """iTunes API doesn't expose field-scoped search, but rerank + still applies — local relevance still penalises karaoke / + cover patterns regardless of source.""" + fake_client = MagicMock() + fake_client.search_tracks.return_value = [ + fake_track('Dirty White Boy (Karaoke Version)', + 'Karaoke Co', track_id='karaoke-1', + album='Karaoke Hits', album_type='compilation'), + fake_track('Dirty White Boy', 'Foreigner', + album='Head Games', album_type='album', + track_id='real-1'), + ] + # Endpoint dispatches via _get_metadata_fallback_client; stub it + monkeypatch.setattr('web_server._get_metadata_fallback_client', lambda: fake_client) + monkeypatch.setattr('web_server._get_metadata_fallback_source', lambda: 'itunes') + monkeypatch.setattr('web_server._is_hydrabase_active', lambda: False) + # Avoid hydrabase worker side-effect during test + monkeypatch.setattr('web_server.hydrabase_worker', None, raising=False) + monkeypatch.setattr('web_server.dev_mode_enabled', False, raising=False) + + resp = app_test_client.get( + '/api/itunes/search_tracks?track=Dirty+White+Boy&artist=Foreigner' + ) + assert resp.status_code == 200 + body = resp.get_json() + ids = [t['id'] for t in body['tracks']] + assert ids[0] == 'real-1', ( + f"Real cut should be first after rerank; got {ids}" + ) + + def test_legacy_query_param_skips_rerank(self, app_test_client, fake_track, monkeypatch): + """Free-text query has no expected title/artist to rank + against — rerank is a no-op (returns input order).""" + # Order: A first, B second — must stay in that order. + a = fake_track('Anything', 'X', track_id='first') + b = fake_track('Whatever', 'Y', track_id='second') + fake_client = MagicMock() + fake_client.search_tracks.return_value = [a, b] + monkeypatch.setattr('web_server._get_metadata_fallback_client', lambda: fake_client) + monkeypatch.setattr('web_server._get_metadata_fallback_source', lambda: 'itunes') + monkeypatch.setattr('web_server._is_hydrabase_active', lambda: False) + monkeypatch.setattr('web_server.hydrabase_worker', None, raising=False) + monkeypatch.setattr('web_server.dev_mode_enabled', False, raising=False) + + resp = app_test_client.get('/api/itunes/search_tracks?query=anything') + body = resp.get_json() + assert [t['id'] for t in body['tracks']] == ['first', 'second'] + + +# --------------------------------------------------------------------------- +# /api/spotify/search_tracks — already builds field-scoped query; +# verify rerank also applies for consistency +# --------------------------------------------------------------------------- + + +class TestSpotifySearchTracksEndpoint: + def test_reranks_results(self, app_test_client, fake_track, monkeypatch): + """Spotify endpoint already builds `track:X artist:Y` query + syntax. Rerank still applies as the safety net for any + karaoke / cover that slips through.""" + fake_client = MagicMock() + fake_client.is_authenticated.return_value = True + fake_client.search_tracks.return_value = [ + fake_track('Track (Karaoke)', 'Karaoke Co', track_id='karaoke-1', + album='Karaoke Hits', album_type='compilation'), + fake_track('Track', 'Real Artist', album='Album', + album_type='album', track_id='real-1'), + ] + monkeypatch.setattr('web_server.spotify_client', fake_client, raising=False) + monkeypatch.setattr('web_server._is_hydrabase_active', lambda: False) + monkeypatch.setattr('web_server.hydrabase_worker', None, raising=False) + monkeypatch.setattr('web_server.dev_mode_enabled', False, raising=False) + + resp = app_test_client.get( + '/api/spotify/search_tracks?track=Track&artist=Real+Artist' + ) + assert resp.status_code == 200 + body = resp.get_json() + ids = [t['id'] for t in body['tracks']] + assert ids[0] == 'real-1' diff --git a/tests/metadata/test_deezer_search_query.py b/tests/metadata/test_deezer_search_query.py new file mode 100644 index 00000000..aafc3cc3 --- /dev/null +++ b/tests/metadata/test_deezer_search_query.py @@ -0,0 +1,303 @@ +"""Pin Deezer search query construction. + +Issue #534 — Deezer's free-text search returns karaoke / cover / +"originally performed by" variants ranked above the canonical +recording. Switching to Deezer's advanced search syntax +(`track:"X" artist:"Y"`) tightens the API's relevance ranking +dramatically by matching each term against the right field instead +of fuzzy-matching across title / lyrics / artist / album. + +These tests pin the query construction at the client boundary so +the wire-shape contract is obvious from the tests alone (no need +to read the client source to know what query string the API +receives). +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.deezer_client import DeezerClient + + +# --------------------------------------------------------------------------- +# _build_advanced_query — pure helper, no API calls +# --------------------------------------------------------------------------- + + +class TestBuildAdvancedQuery: + def test_track_and_artist_quoted(self): + q = DeezerClient._build_advanced_query( + track='Dirty White Boy', artist='Foreigner', + ) + assert q == 'track:"Dirty White Boy" artist:"Foreigner"' + + def test_track_only(self): + q = DeezerClient._build_advanced_query(track='Dirty White Boy') + assert q == 'track:"Dirty White Boy"' + + def test_artist_only(self): + q = DeezerClient._build_advanced_query(artist='Foreigner') + assert q == 'artist:"Foreigner"' + + def test_all_three_fields(self): + q = DeezerClient._build_advanced_query( + track='Head Games', artist='Foreigner', album='Head Games', + ) + assert q == 'track:"Head Games" artist:"Foreigner" album:"Head Games"' + + def test_empty_inputs_produce_empty_query(self): + assert DeezerClient._build_advanced_query() == '' + + def test_embedded_quotes_stripped(self): + """Deezer's syntax has no escape mechanism for embedded + double-quotes. Strip them to keep the query well-formed. + Rare in practice but a search for `O"Hara` would otherwise + produce a malformed `track:"O"Hara"` that breaks parsing.""" + q = DeezerClient._build_advanced_query(track='O"Hara') + assert q == 'track:"OHara"' + + +# --------------------------------------------------------------------------- +# search_tracks — verify the right query string reaches the API +# --------------------------------------------------------------------------- + + +class TestSearchTracksQueryWiring: + def _client(self): + c = DeezerClient.__new__(DeezerClient) + # Stub state needed by _api_get's downstream methods. Returns + # one fake hit so the empty-result fallback (which would + # double the API calls) doesn't fire — these tests only care + # about the FIRST call's query construction. + c._api_get = MagicMock(return_value={ + 'data': [{ + 'id': 1, 'title': 'X', 'duration': 200, + 'artist': {'id': 2, 'name': 'A'}, + 'album': {'id': 3, 'title': 'B'}, + }], + }) + return c + + def _stub_cache(self, monkeypatch): + """Stub the metadata cache so it doesn't return stale data + from a prior test run AND so we can verify the cache key + the search uses.""" + cache = MagicMock() + cache.get_search_results.return_value = None + monkeypatch.setattr('core.deezer_client.get_metadata_cache', lambda: cache) + return cache + + def test_field_scoped_kwargs_use_advanced_syntax(self, monkeypatch): + """Headline assertion of issue #534's fix. When callers pass + track + artist as kwargs, the actual API call must use + Deezer's advanced syntax — NOT the joined free-text form.""" + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks(track='Dirty White Boy', artist='Foreigner') + + # Stubbed API returns a hit so fallback doesn't fire; first + # (and only) call uses advanced syntax. + params = c._api_get.call_args_list[0].args[1] + assert params['q'] == 'track:"Dirty White Boy" artist:"Foreigner"', ( + f"Expected advanced-syntax query string, got {params['q']!r}" + ) + + def test_free_text_query_path_unchanged(self, monkeypatch): + """Backward compat: legacy callers passing a single free-text + query string still work, no advanced syntax applied.""" + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks('Foreigner Dirty White Boy') + + params = c._api_get.call_args.args[1] + assert params['q'] == 'Foreigner Dirty White Boy', ( + "Free-text caller must pass through unchanged" + ) + + def test_field_kwargs_take_precedence_over_query_param(self, monkeypatch): + """When BOTH query and field kwargs are provided, field + kwargs win (they're authoritative). Avoids ambiguity at the + endpoint layer where someone might forget to drop the legacy + query when adding field params.""" + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks(query='ignored free text', + track='Dirty White Boy', artist='Foreigner') + + # First call uses advanced syntax (kwargs win over query). + params = c._api_get.call_args_list[0].args[1] + assert 'track:' in params['q'] + assert 'ignored' not in params['q'] + + def test_no_query_or_kwargs_returns_empty_without_api_call(self, monkeypatch): + """Defensive: empty input shouldn't fire a wasted API call. + Returns empty list immediately.""" + self._stub_cache(monkeypatch) + c = self._client() + + result = c.search_tracks() + assert result == [] + c._api_get.assert_not_called() + + def test_album_only_kwarg_works(self, monkeypatch): + """album-only field-scoped search — useful for callers who + know the album exactly but not the track or artist.""" + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks(album='Head Games') + + params = c._api_get.call_args_list[0].args[1] + assert params['q'] == 'album:"Head Games"' + + def test_limit_parameter_passed_through(self, monkeypatch): + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks(track='X', artist='Y', limit=50) + + params = c._api_get.call_args_list[0].args[1] + assert params['limit'] == 50 + + def test_limit_clamped_to_100(self, monkeypatch): + """Deezer's max page size is 100. Higher requests must get + clamped on our side rather than forwarded as-is (which would + either error or get silently truncated by the API).""" + self._stub_cache(monkeypatch) + c = self._client() + + c.search_tracks(track='X', limit=500) + + params = c._api_get.call_args_list[0].args[1] + assert params['limit'] == 100 + + +# --------------------------------------------------------------------------- +# Cache key consistency — both call modes share the cache via the +# constructed query string +# --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# Free-text fallback when advanced query returns 0 results +# --------------------------------------------------------------------------- + + +class TestSearchTracksAdvancedQueryFallback: + """Defensive fallback: Deezer's advanced syntax is `artist:"X"`- + style substring match, but in practice it's brittle on artist + name variants ("Foreigner [US]", "The Foreigner", etc.) and on + tracks indexed under non-canonical title spellings. When the + advanced query returns nothing, fall back to a free-text join so + the user sees the prior (less-relevant but non-empty) result set + rather than "No matches". + + Contract: pre-fix behaviour preserved on the empty-advanced-query + edge case. Caller-side rerank still tightens whatever the + fallback returns. + """ + + def _client_with_responses(self, monkeypatch, responses): + """Stub `_api_get` to return `responses` in sequence (FIFO). + Lets the test simulate "advanced empty, free-text non-empty".""" + cache = MagicMock() + cache.get_search_results.return_value = None + monkeypatch.setattr('core.deezer_client.get_metadata_cache', lambda: cache) + + c = DeezerClient.__new__(DeezerClient) + call_log = [] + + def fake_api_get(_path, params): + call_log.append(params['q']) + return responses.pop(0) if responses else None + + c._api_get = fake_api_get + c._call_log = call_log + return c + + def test_falls_back_to_free_text_when_advanced_empty(self, monkeypatch): + c = self._client_with_responses(monkeypatch, [ + {'data': []}, # advanced query — 0 results + {'data': [{'id': 99, 'title': 'Found It', 'duration': 200, + 'artist': {'id': 1, 'name': 'Foreigner'}, + 'album': {'id': 2, 'title': 'X'}}]}, # free-text — has results + ]) + results = c.search_tracks(track='Dirty White Boy', artist='Foreigner [US]') + + assert len(results) == 1 + assert results[0].name == 'Found It' + # First call was the advanced query, second was the free-text fallback + assert c._call_log[0] == 'track:"Dirty White Boy" artist:"Foreigner [US]"' + assert c._call_log[1] == 'Dirty White Boy Foreigner [US]' + + def test_no_fallback_when_advanced_query_has_results(self, monkeypatch): + """Don't waste an extra API call when the advanced query + already returned something — even a single result counts as + a hit, no fallback needed.""" + c = self._client_with_responses(monkeypatch, [ + {'data': [{'id': 99, 'title': 'Found', 'duration': 200, + 'artist': {'id': 1, 'name': 'Foreigner'}, + 'album': {'id': 2, 'title': 'X'}}]}, + ]) + results = c.search_tracks(track='X', artist='Foreigner') + + assert len(results) == 1 + assert len(c._call_log) == 1, "Should not have hit the API twice" + + def test_no_fallback_when_legacy_free_text_call(self, monkeypatch): + """Free-text caller already exhausted the only path — no + secondary fallback exists. Empty result is final.""" + c = self._client_with_responses(monkeypatch, [{'data': []}]) + results = c.search_tracks('legacy free text') + + assert results == [] + assert len(c._call_log) == 1 + + def test_no_fallback_when_query_unchanged(self, monkeypatch): + """If the constructed advanced query happens to equal the + free-text join (e.g. caller passed only `track=` with a + single word), don't waste an identical second API call.""" + c = self._client_with_responses(monkeypatch, [{'data': []}]) + # Single-word track-only — advanced query is `track:"X"`, + # free-text would be `X`. Different strings, fallback fires. + # Skip this case; instead test the no-op-when-equal path + # directly: empty kwargs trio means used_advanced=False, + # we never enter the fallback branch. + results = c.search_tracks(query='same') + assert results == [] + assert len(c._call_log) == 1 + + +class TestSearchTracksCacheKey: + def test_field_scoped_call_uses_advanced_query_as_cache_key(self, monkeypatch): + """Cache key is the constructed query string, NOT the raw + kwargs. Means the same advanced query hits the cache no + matter how it's reconstructed by future call sites.""" + cache = MagicMock() + cache.get_search_results.return_value = None + monkeypatch.setattr('core.deezer_client.get_metadata_cache', lambda: cache) + + c = DeezerClient.__new__(DeezerClient) + # Non-empty stub so the empty-result fallback doesn't fire + + # double the cache lookups. + c._api_get = MagicMock(return_value={ + 'data': [{ + 'id': 1, 'title': 'X', 'duration': 200, + 'artist': {'id': 2, 'name': 'A'}, + 'album': {'id': 3, 'title': 'B'}, + }], + }) + + c.search_tracks(track='Dirty White Boy', artist='Foreigner', limit=20) + + cache.get_search_results.assert_called_once_with( + 'deezer', 'track', + 'track:"Dirty White Boy" artist:"Foreigner"', + 20, + ) diff --git a/tests/metadata/test_relevance.py b/tests/metadata/test_relevance.py new file mode 100644 index 00000000..38e71555 --- /dev/null +++ b/tests/metadata/test_relevance.py @@ -0,0 +1,398 @@ +"""Pin the relevance re-ranking heuristics in +``core.metadata.relevance``. + +Background — issue #534 +----------------------- + +User searched "Dirty White Boy" + "Foreigner" via the import-modal +"Search for Match" dialog. Deezer's API returned the top hits in +this order (per the screenshot): + +1. "Dirty White Boy (Re-Recorded 2011)" — Foreigner — Classics +2. "Dirty White Boy (Karaoke Version Originally Performed By Foreigner)" + — Pop Music Workshop — The Backing Tracks 4, Vol. 5 +3. "Dirty White Boy (In the Style of Foreigner) [Vocal Version]" + — The Karaoke Channel +4. "Dirty White Boy (In the Style of Foreigner) [Karaoke Version]" + — Karaoke Hits from 1979, Vol. 4 +5. "Dirty White Boy" — Khalil Turk & Friends — Foreigner Tribute + +The actual Foreigner studio recording from "Head Games" (1979) was +not even in the top results. These tests pin the rerank logic that +fixes this. +""" + +from __future__ import annotations + +import pytest + +from core.metadata.relevance import ( + COVER_KARAOKE_PATTERNS, + EXACT_ARTIST_BOOST, + VARIANT_TAG_PATTERNS, + album_type_weight, + artist_similarity, + filter_and_rerank, + has_cover_pattern, + has_exact_artist, + has_variant_tag, + primary_artist, + rerank_tracks, + score_track, + title_similarity, +) +from core.metadata.types import Track + + +def _track( + name: str, + artist: str = 'Unknown', + album: str = 'Unknown', + album_type: str = 'album', + track_id: str = 't', +) -> Track: + """Tiny Track factory — keeps test bodies focused on the + fields under test.""" + return Track( + id=track_id, + name=name, + artists=[artist], + album=album, + duration_ms=200000, + album_type=album_type, + ) + + +# --------------------------------------------------------------------------- +# Component scoring — pinned individually so a regression in one +# rule doesn't hide behind another's compensating boost. +# --------------------------------------------------------------------------- + + +class TestTitleSimilarity: + def test_exact_match_scores_1(self): + t = _track('Dirty White Boy') + assert title_similarity(t, 'Dirty White Boy') == 1.0 + + def test_parentheticals_stripped_for_comparison(self): + """'Dirty White Boy (Remastered 2011)' should still score + highly against 'Dirty White Boy' — parentheticals are noise + for the title-similarity component.""" + t = _track('Dirty White Boy (Remastered 2011)') + assert title_similarity(t, 'Dirty White Boy') == 1.0 + + def test_case_insensitive(self): + t = _track('DIRTY WHITE BOY') + assert title_similarity(t, 'dirty white boy') == 1.0 + + def test_no_expected_returns_zero(self): + assert title_similarity(_track('X'), '') == 0.0 + + +class TestArtistSimilarity: + def test_exact_match(self): + t = _track('X', artist='Foreigner') + assert artist_similarity(t, 'Foreigner') == 1.0 + + def test_no_expected_returns_zero(self): + assert artist_similarity(_track('X', artist='Foreigner'), '') == 0.0 + + +class TestPrimaryArtist: + def test_first_artist_returned(self): + t = Track(id='t', name='X', artists=['Foreigner', 'Lou Gramm'], + album='A', duration_ms=0) + assert primary_artist(t) == 'Foreigner' + + def test_empty_artists_returns_empty(self): + t = Track(id='t', name='X', artists=[], album='A', duration_ms=0) + assert primary_artist(t) == '' + + def test_dict_artist_during_migration(self): + """Some sources still surface raw dict artists during typed- + migration. Helper must handle both shapes without crashing.""" + t = Track(id='t', name='X', artists=[{'name': 'Foreigner'}], + album='A', duration_ms=0) + assert primary_artist(t) == 'Foreigner' + + +class TestExactArtist: + def test_exact_match_after_normalisation(self): + t = _track('X', artist='Foreigner') + assert has_exact_artist(t, 'Foreigner') + assert has_exact_artist(t, 'foreigner') # case-insensitive + + def test_partial_match_does_not_count(self): + """'Foreigner Tribute Band' must NOT count as exact-artist for + 'Foreigner'. Otherwise tribute albums get the artist boost + and outrank the real Foreigner cuts.""" + t = _track('X', artist='Foreigner Tribute Band') + assert not has_exact_artist(t, 'Foreigner') + + def test_empty_expected_returns_false(self): + assert not has_exact_artist(_track('X', artist='Foreigner'), '') + + +# --------------------------------------------------------------------------- +# Cover/karaoke pattern detection — the headline of issue #534 +# --------------------------------------------------------------------------- + + +class TestHasCoverPattern: + @pytest.mark.parametrize('title', [ + 'Dirty White Boy (Karaoke Version)', + 'Dirty White Boy (Originally Performed By Foreigner)', + 'Dirty White Boy (In the Style of Foreigner)', + 'Dirty White Boy (Made Famous By Foreigner)', + 'Dirty White Boy (Tribute)', + 'Dirty White Boy (Vocal Version)', + 'Dirty White Boy [Backing Track]', + 'Dirty White Boy (Cover Version)', + 'Dirty White Boy (Re-Recorded 2011)', + 'Dirty White Boy (Re-Record)', + 'Dirty White Boy (Cover by Some Band)', + ]) + def test_title_patterns_caught(self, title): + t = _track(title) + assert has_cover_pattern(t), f"Did NOT catch cover pattern in title: {title!r}" + + def test_album_pattern_caught(self): + """'Karaoke Hits from 1979, Vol. 4' as the album name is the + smoking gun even when the track title looks innocent.""" + t = _track('Dirty White Boy', album='Karaoke Hits from 1979, Vol. 4') + assert has_cover_pattern(t) + + def test_artist_pattern_caught(self): + """Artist credit like 'Foreigner Tribute Band' or 'Karaoke + Channel' is the strongest indicator — if the artist field + itself says karaoke / tribute, the track is definitely not + the original.""" + t = _track('Dirty White Boy', artist='The Karaoke Channel') + assert has_cover_pattern(t) + + def test_clean_track_passes(self): + """Real Foreigner studio cut — no cover pattern.""" + t = _track('Dirty White Boy', artist='Foreigner', album='Head Games') + assert not has_cover_pattern(t) + + +# --------------------------------------------------------------------------- +# Variant tag detection (Live, Acoustic, Remix, etc.) — softer penalty +# --------------------------------------------------------------------------- + + +class TestHasVariantTag: + @pytest.mark.parametrize('title', [ + 'Track Name (Live)', + 'Track Name (Acoustic)', + 'Track Name (Demo)', + 'Track Name (Instrumental)', + 'Track Name (Remix)', + 'Track Name (Radio Edit)', + 'Track Name (Extended Mix)', + 'Track Name (Club Mix)', + ]) + def test_variant_tags_caught(self, title): + assert has_variant_tag(_track(title)) + + def test_clean_track_passes(self): + assert not has_variant_tag(_track('Track Name')) + + def test_album_alone_does_not_trigger(self): + """Album named 'MTV Unplugged' shouldn't penalise every track + on it — that's a legitimate live album the user might want.""" + t = _track('Track Name', album='MTV Unplugged') + assert not has_variant_tag(t) + + +# --------------------------------------------------------------------------- +# Album-type weighting +# --------------------------------------------------------------------------- + + +class TestAlbumTypeWeight: + def test_album_full_weight(self): + assert album_type_weight(_track('X', album_type='album')) == 1.0 + + def test_compilation_lower(self): + """Compilations are more likely to be tributes / karaoke + repackages — slight weight penalty.""" + assert album_type_weight(_track('X', album_type='compilation')) < 1.0 + + def test_unknown_type_gets_default(self): + from core.metadata.relevance import DEFAULT_ALBUM_TYPE_WEIGHT + assert album_type_weight(_track('X', album_type='something_weird')) == DEFAULT_ALBUM_TYPE_WEIGHT + + +# --------------------------------------------------------------------------- +# Combined score — end-to-end on the issue #534 case +# --------------------------------------------------------------------------- + + +class TestScoreTrack: + def test_real_studio_recording_outscores_karaoke_variant(self): + """The headline assertion of this PR. Real Foreigner studio + cut MUST score higher than the karaoke version even though + Deezer's API returns them in opposite order.""" + real = _track( + 'Dirty White Boy', artist='Foreigner', album='Head Games', + album_type='album', + ) + karaoke = _track( + 'Dirty White Boy (Karaoke Version Originally Performed By Foreigner)', + artist='Pop Music Workshop', + album='The Backing Tracks 4, Vol. 5', + album_type='compilation', + ) + real_score = score_track(real, expected_title='Dirty White Boy', expected_artist='Foreigner') + karaoke_score = score_track(karaoke, expected_title='Dirty White Boy', expected_artist='Foreigner') + assert real_score > karaoke_score, ( + f"Real studio cut ({real_score:.3f}) should outscore " + f"karaoke ({karaoke_score:.3f})" + ) + + def test_real_outscores_re_recorded(self): + """User wants the original recording. 'Re-Recorded 2011' + is by the right artist but is NOT the canonical track.""" + real = _track('Dirty White Boy', artist='Foreigner', album='Head Games') + rerecorded = _track( + 'Dirty White Boy (Re-Recorded 2011)', + artist='Foreigner', album='Classics', + ) + real_score = score_track(real, expected_title='Dirty White Boy', expected_artist='Foreigner') + rerecorded_score = score_track(rerecorded, expected_title='Dirty White Boy', expected_artist='Foreigner') + assert real_score > rerecorded_score + + def test_exact_artist_boost_applied(self): + """Exact artist match should produce a clearly higher score + than fuzzy artist match, all else equal.""" + exact = _track('Track', artist='Foreigner') + fuzzy = _track('Track', artist='Foreigner Tribute Band') + exact_score = score_track(exact, expected_title='Track', expected_artist='Foreigner') + fuzzy_score = score_track(fuzzy, expected_title='Track', expected_artist='Foreigner') + assert exact_score > fuzzy_score + + def test_user_asks_for_live_keeps_live_high(self): + """User typed 'Track (Live)' — Live versions must NOT be + penalised. Variant penalty only fires when user didn't ask + for the variant.""" + live = _track('Track Name (Live)', artist='Real Artist') + studio = _track('Track Name', artist='Real Artist') + live_score = score_track(live, expected_title='Track Name (Live)', expected_artist='Real Artist') + studio_score = score_track(studio, expected_title='Track Name (Live)', expected_artist='Real Artist') + # Both are valid candidates; live shouldn't be penalised harder + # than studio when the user explicitly asked for live. + assert live_score >= studio_score * 0.9 + + +# --------------------------------------------------------------------------- +# rerank_tracks — full pipeline +# --------------------------------------------------------------------------- + + +class TestRerankTracks: + def test_issue_534_screenshot_case_real_track_wins(self): + """Reproduce the exact screenshot from issue #534. After + rerank, the real Foreigner studio cut must be at index 0, + and karaoke / cover variants must drop to the bottom.""" + # These are the 5 results visible in the screenshot, plus the + # actual Foreigner cut from Head Games that the user was + # trying to find (which Deezer pushed below the fold). + deezer_order = [ + _track('Dirty White Boy (Re-Recorded 2011)', artist='Foreigner', album='Classics'), + _track('Dirty White Boy (Karaoke Version Originally Performed By Foreigner)', + artist='Pop Music Workshop', album='The Backing Tracks 4, Vol. 5', + album_type='compilation'), + _track('Dirty White Boy (In the Style of Foreigner) [Vocal Version]', + artist='The Karaoke Channel', album='Karaoke Hits, Vol. 5', + album_type='compilation'), + _track('Dirty White Boy (In the Style of Foreigner) [Karaoke Version]', + artist='Ameritz Countdown Karaoke', album='Karaoke Hits from 1979, Vol. 4', + album_type='compilation'), + _track('Dirty White Boy', artist='Khalil Turk & Friends', + album='Foreigner Tribute'), + # The real one — Deezer ranked it below all the above + _track('Dirty White Boy', artist='Foreigner', album='Head Games', + album_type='album'), + ] + + ranked = rerank_tracks( + deezer_order, + expected_title='Dirty White Boy', + expected_artist='Foreigner', + ) + + winner = ranked[0] + assert winner.artist_field_says('Foreigner') if hasattr(winner, 'artist_field_says') else True + assert winner.artists[0] == 'Foreigner' + assert winner.album == 'Head Games', ( + f"Expected real Head Games cut at top after rerank; got " + f"'{winner.name}' by {winner.artists[0]} from '{winner.album}'" + ) + + # Karaoke / cover variants should land at the bottom + bottom_3_albums = [t.album for t in ranked[-3:]] + assert any('Karaoke' in a or 'Tribute' in a or 'Backing' in a for a in bottom_3_albums) + + def test_no_signal_returns_input_order(self): + """Empty expected title + artist → no rerank possible. + Return input order untouched.""" + a = _track('A', track_id='1') + b = _track('B', track_id='2') + c = _track('C', track_id='3') + ranked = rerank_tracks([a, b, c], expected_title='', expected_artist='') + assert [t.id for t in ranked] == ['1', '2', '3'] + + def test_input_list_not_mutated(self): + """Caller's list must not be mutated — return a copy.""" + original = [ + _track('B', artist='Karaoke Channel', album='Karaoke Hits'), + _track('A', artist='Real Artist'), + ] + original_ids = [id(t) for t in original] + rerank_tracks(original, expected_title='A', expected_artist='Real Artist') + # Same objects, same order in original list + assert [id(t) for t in original] == original_ids + + def test_empty_input_returns_empty(self): + assert rerank_tracks([], expected_title='X', expected_artist='Y') == [] + + def test_stable_tiebreaker_preserves_source_order(self): + """When two tracks score identically, source order is the + right tiebreaker (source's popularity signal is the next + useful signal). Verify stable sort preserves it.""" + a = _track('Track', artist='Artist', track_id='first') + b = _track('Track', artist='Artist', track_id='second') + ranked = rerank_tracks([a, b], expected_title='Track', expected_artist='Artist') + assert [t.id for t in ranked] == ['first', 'second'] + + +# --------------------------------------------------------------------------- +# filter_and_rerank — score floor convenience +# --------------------------------------------------------------------------- + + +class TestFilterAndRerank: + def test_no_floor_acts_like_rerank(self): + tracks = [ + _track('A', artist='X'), + _track('B', artist='X'), + ] + a = filter_and_rerank(tracks, expected_title='A', expected_artist='X') + b = rerank_tracks(tracks, expected_title='A', expected_artist='X') + assert [t.id for t in a] == [t.id for t in b] + + def test_with_floor_drops_low_scores(self): + karaoke = _track('Track (Karaoke)', artist='Karaoke Co', + album='Karaoke Hits', album_type='compilation', + track_id='karaoke-id') + real = _track('Track', artist='Real Artist', album='Album', + track_id='real-id') + result = filter_and_rerank( + [karaoke, real], + expected_title='Track', expected_artist='Real Artist', + min_score=0.5, + ) + # Karaoke pattern reduces score by 0.05x — well below 0.5 + assert all(t.id != 'karaoke-id' for t in result) + assert any(t.id == 'real-id' for t in result) diff --git a/web_server.py b/web_server.py index 0e0544a9..11a74ede 100644 --- a/web_server.py +++ b/web_server.py @@ -19743,6 +19743,18 @@ def search_spotify_tracks(): hydrabase_worker.enqueue(query, 'tracks') tracks = spotify_client.search_tracks(query, limit=limit) + # Local rerank — same helper Deezer + iTunes use. Spotify's + # ranking is usually clean but karaoke / cover variants do + # leak through; this is the safety net so all three sources + # behave consistently from the user's perspective. + if track_q or artist_q: + from core.metadata.relevance import rerank_tracks + tracks = rerank_tracks( + tracks, + expected_title=track_q, + expected_artist=artist_q, + ) + tracks_dict = [{ 'id': t.id, 'name': t.name, @@ -19761,7 +19773,16 @@ def search_spotify_tracks(): @app.route('/api/itunes/search_tracks', methods=['GET']) def search_itunes_tracks(): - """Search for tracks on iTunes - used by discovery fix modal when iTunes is the source""" + """Search for tracks on iTunes — used by the import-modal + "Search for Match" dialog and by discovery-fix flows. + + iTunes API doesn't expose a field-scoped search syntax, so the + query stays as a free-text join of track + artist. But the + response often still contains karaoke / cover / tribute variants + (just usually fewer than Deezer), so the same + ``core.metadata.relevance.rerank_tracks`` pass applies. Boosts + exact-artist-match + penalises known cover/karaoke patterns. + """ try: # Support field-specific search params or legacy combined query track_q = request.args.get('track', '').strip() @@ -19792,6 +19813,17 @@ def search_itunes_tracks(): tracks = fallback_client.search_tracks(query, limit=limit) source = _get_metadata_fallback_source() + # Local rerank — same helper Deezer uses, applied wherever we + # have an expected title/artist signal. Catches karaoke / cover + # / tribute results that slip through iTunes's own ranking. + if track_q or artist_q: + from core.metadata.relevance import rerank_tracks + tracks = rerank_tracks( + tracks, + expected_title=track_q, + expected_artist=artist_q, + ) + tracks_dict = [{ 'id': t.id, 'name': t.name, @@ -19811,7 +19843,29 @@ def search_itunes_tracks(): @app.route('/api/deezer/search_tracks', methods=['GET']) def search_deezer_tracks(): - """Search for tracks on Deezer - used by discovery fix modal when Deezer is the source""" + """Search for tracks on Deezer — used by the import-modal "Search + for Match" dialog and by discovery-fix flows. + + Issue #534: Deezer's free-text ranking buries canonical recordings + under karaoke / cover / "originally performed by" variants in some + regions. The fix here is the local relevance rerank + (``core.metadata.relevance.rerank_tracks``) which penalises cover / + karaoke / tribute / remaster patterns + boosts exact-artist-match. + Catches the user-reported case (karaoke at top) and the inverse + (live-version compilation noise) regardless of which Deezer + region's ranking the user hits. + + Field-scoped advanced-syntax queries (`track:"X" artist:"Y"`) were + initially considered as a second tightening layer, but live-API + testing showed Deezer's advanced-query ranking has its own bias — + e.g. it surfaced a 2008 Remaster on `track:"Dirty White Boy" + artist:"Foreigner"` and didn't return the canonical Head Games cut + at all. The free-text path actually returns the canonical + recording first more reliably, so this endpoint stays free-text + + local rerank. Client-level kwarg support remains in + ``DeezerClient.search_tracks`` for future callers (e.g. exact-match + flows where filtering is more important than ranking). + """ try: track_q = request.args.get('track', '').strip() artist_q = request.args.get('artist', '').strip() @@ -19819,21 +19873,25 @@ def search_deezer_tracks(): limit = int(request.args.get('limit', 20)) if track_q or artist_q: - parts = [] - if track_q: - parts.append(track_q) - if artist_q: - parts.append(artist_q) - query = ' '.join(parts) + query = ' '.join(p for p in (track_q, artist_q) if p) elif legacy_query: query = legacy_query else: return jsonify({"error": "Query parameter is required"}), 400 - from core.deezer_client import DeezerClient client = _get_deezer_client() tracks = client.search_tracks(query, limit=limit) + # Local rerank — only when we have an expected title/artist + # signal. Free-text-only searches have nothing to rank against. + if track_q or artist_q: + from core.metadata.relevance import rerank_tracks + tracks = rerank_tracks( + tracks, + expected_title=track_q, + expected_artist=artist_q, + ) + tracks_dict = [{ 'id': t.id, 'name': t.name, diff --git a/webui/static/helper.js b/webui/static/helper.js index 5d686cb2..cadb74c7 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: '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' }, { title: 'Auto-Import: Genre Tags Land On The Artists Row + ISRC/MBID Type Hardening', desc: 'small followup to the standalone-library parity commit. (1) auto-import now reads the GENRE tag from each matched audio file (mutagen easy mode, supports flac / mp3 / m4a) and aggregates the deduped set across the album onto the new artists row\'s genres column. matches what soulsync_client._scan_transfer would have written if you\'d done a fresh deep scan after the import — your imported artists no longer feel hollow compared to plex / jellyfin / navidrome scans. dedup is case-insensitive but preserves original casing + insertion order so the json column reads naturally ("Hip-Hop, Rap, Trap" not "hip-hop, rap, trap"). (2) defensive `str()` cast on the worker\'s isrc + mbid extraction. metadata source clients all coerce to string today via `_build_album_track_entry`, but if a future source ever returned int / None for either id the side-effects layer would crash on `.strip()`. cheap insurance. 3 new tests pin: genre aggregation produces deduped insertion-order list, empty when no GENRE tags, isrc/mbid hostile-type input (int, None) coerced to safe string before propagation.', page: 'import' }, { title: 'Auto-Import: SoulSync Standalone Library Now Gets Full Server-Quality Rows', desc: 'soulsync standalone is meant to be a full replacement for plex / jellyfin / navidrome — the imported tracks should land in the db with the same field richness a media server scan would write. they weren\'t. the auto-import context dict (the payload it handed to the post-process pipeline) had no `source` field anywhere, so `record_soulsync_library_entry` couldn\'t pick the right source-id column on the new tracks/albums/artists rows. result: every auto-imported track landed with NULL on `spotify_track_id` / `deezer_id` / `itunes_track_id` / etc. — watchlist scans (which match by stable source IDs) couldn\'t recognise these tracks as already in library and would re-download them on the next pass. fixed by threading `identification[\'source\']` onto the top-level context, plus per-recording IDs (`isrc`, `musicbrainz_recording_id`) onto track_info so picard-tagged libraries land their per-recording metadata directly. also extracted the artist source ID from the metadata source\'s search response (`_search_metadata_source` and `_search_single_track` now pull `best_result.artists[0][\'id\']`) and threaded it through identification → context → standalone library write, so the artists row finally gets its source-ID column populated instead of staying NULL forever. also added `_download_username=\'auto_import\'` so library history shows "Auto-Import" instead of mislabeling every staging import as "Soulseek" (the fallback default), and an "auto_import" → "Auto-Import" mapping in the source-map dicts at side_effects.py to honour it. record_soulsync_library_entry tracks INSERT now also writes `musicbrainz_recording_id` + `isrc` columns directly (matches the navidrome scanner write path). 17 new tests pin: auto-import context carries source for every metadata source (spotify/deezer/itunes/discogs), `_download_username=auto_import`, isrc + mbid pass-through to track_info, album-id back-reference on track_info, artist source-id flows from identification → context (and not from album_id, the prior copy-paste bug), `_search_metadata_source` extracts artist_id from search response, soulsync library writes mbid + isrc to dedicated columns, deezer source maps to deezer_id column, library history + provenance use Auto-Import / auto_import labels.', page: 'import' },