fix(#945): Spotify backfill must disable cross-service search fallback
Double-checking the backfill logic found a real correctness bug. Spotify search_tracks defaults to allow_fallback=True, so when Spotify is rate-limited or in free mode it returns iTunes/Deezer tracks whose .id is an iTunes/Deezer id, NOT a Spotify id. The backfill took that .id as a Spotify track id and would push wrong/garbage tracks into the exported Spotify playlist. The unit tests used fake Track objects with hand-set ids, so they could never surface this cross-service contamination. Fix: the Spotify backfill search now passes allow_fallback=False — real Spotify hits or nothing (an unmatched track is left out, never replaced by a non-Spotify id). Deezer is unaffected: its search fallback is query-only and stays within Deezer, so its ids are always Deezer ids. Regression test asserts the Spotify backfill search is invoked with allow_fallback=False. 8 orchestration tests green, ruff clean.
This commit is contained in:
parent
050ce79d51
commit
9bb73c4cb4
2 changed files with 30 additions and 3 deletions
|
|
@ -110,3 +110,21 @@ def test_backfill_on_wires_search_fn(monkeypatch):
|
||||||
_FakeClient({'success': True, 'playlist_id': 'p', 'added': 1}),
|
_FakeClient({'success': True, 'playlist_id': 'p', 'added': 1}),
|
||||||
_fake_resolver(['sx'], seen))
|
_fake_resolver(['sx'], seen))
|
||||||
assert seen['search_id_fn'] == 'SEARCH_FN'
|
assert seen['search_id_fn'] == 'SEARCH_FN'
|
||||||
|
|
||||||
|
|
||||||
|
def test_spotify_backfill_search_disables_cross_service_fallback(monkeypatch):
|
||||||
|
"""REGRESSION: Spotify's search_tracks falls back to iTunes/Deezer (non-Spotify ids) under
|
||||||
|
rate-limit/free. The backfill MUST disable that or it pushes wrong ids into the Spotify
|
||||||
|
playlist. Assert the search is invoked with allow_fallback=False."""
|
||||||
|
seen = {}
|
||||||
|
|
||||||
|
class _FakeSpotify:
|
||||||
|
def search_tracks(self, q, limit=10, allow_fallback=True):
|
||||||
|
seen['allow_fallback'] = allow_fallback
|
||||||
|
return []
|
||||||
|
|
||||||
|
monkeypatch.setattr(ws, 'get_spotify_client', lambda: _FakeSpotify())
|
||||||
|
fn = ws._build_service_search_id_fn('spotify')
|
||||||
|
assert fn is not None
|
||||||
|
fn('Kendrick Lamar', 'Not Like Us') # drives the search
|
||||||
|
assert seen['allow_fallback'] is False
|
||||||
|
|
|
||||||
|
|
@ -27749,12 +27749,21 @@ def _build_service_search_id_fn(service):
|
||||||
try:
|
try:
|
||||||
if service == 'spotify':
|
if service == 'spotify':
|
||||||
client = get_spotify_client()
|
client = get_spotify_client()
|
||||||
|
if not client:
|
||||||
|
return None
|
||||||
|
# allow_fallback=False is CRITICAL: Spotify's search falls back to iTunes/Deezer
|
||||||
|
# when rate-limited/free, returning tracks whose .id is NOT a Spotify id — backfill
|
||||||
|
# would then push wrong ids into the Spotify playlist. Disable it: real Spotify hits
|
||||||
|
# or nothing.
|
||||||
|
search_fn = lambda q: client.search_tracks(q, limit=8, allow_fallback=False) # noqa: E731
|
||||||
else:
|
else:
|
||||||
from core.metadata.registry import get_deezer_client
|
from core.metadata.registry import get_deezer_client
|
||||||
client = get_deezer_client()
|
client = get_deezer_client()
|
||||||
if not client:
|
if not client:
|
||||||
return None
|
return None
|
||||||
search_fn = lambda q: client.search_tracks(q, limit=8) # noqa: E731
|
# Deezer's search stays within Deezer (query-only fallback), so its .id is always a
|
||||||
|
# Deezer id — no cross-service guard needed.
|
||||||
|
search_fn = lambda q: client.search_tracks(q, limit=8) # noqa: E731
|
||||||
return lambda artist, title: search_service_track_id(artist, title, search_fn=search_fn)
|
return lambda artist, title: search_service_track_id(artist, title, search_fn=search_fn)
|
||||||
except Exception:
|
except Exception:
|
||||||
return None
|
return None
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue