Spotify: enrichment can prefer Free, and the budget→Free bridge actually diverts
Two related fixes at one root cause. Every catalog method gated the official API on `use_spotify = is_spotify_authenticated()` and only fell to Free *after* official failed. So when the client was authed but should defer to Free — specifically when the worker's daily real-API budget was spent — it kept hitting the official API anyway (just stopped *counting* it). The budget "bridge" never actually diverted; it only stopped pausing. Root-cause fix: the official gate is now `is_spotify_authenticated() and not self._free_active()` across all 8 catalog methods (search_artists/albums/tracks, get_artist/album/album_tracks/track_details/artist_albums). No-auth and rate-limited are unchanged (auth is already False there); the change only affects the cases where auth is True but we deliberately defer to Free. The user-account methods and the metadata-availability helper are untouched. New opt-in: metadata.spotify_free_enrichment. When set, the worker puts `_prefer_free` on its OWN client and _free_active() honors it (needs only the package installed — the flag is the opt-in — not the 'Spotify Free' source choice). So a connected user can run bulk enrichment on the no-creds source to spare their official quota, while interactive search/resolve stay official-first (they use a different client that never sets the flag). Default off. Tests: _free_active honors prefer_free (and is inert without the package); search_albums defers to Free — official .sp raises if touched — both under prefer_free AND under budget-exhaustion (the divert that previously never happened). 215 Spotify tests pass.
This commit is contained in:
parent
4a6d6fc17b
commit
38461295c2
3 changed files with 136 additions and 13 deletions
|
|
@ -646,13 +646,21 @@ class SpotifyClient:
|
|||
term covers the brief window before the auth cache refreshes. When authed
|
||||
+ healthy the official path returns first, so this never opens.
|
||||
|
||||
Three activations fall out of this: a no-auth user who chose Spotify
|
||||
Activations that fall out of this: a no-auth user who chose Spotify
|
||||
Free (free is their source), a connected user mid-rate-limit (free
|
||||
bridges the ban), and a connected user who has spent the enrichment
|
||||
worker's real-API daily budget (``_budget_exhausted_use_free``, set by
|
||||
the worker) — so a Spotify-Free user is never paused by the budget, it
|
||||
just switches to the uncapped free source. See _free_wanted()."""
|
||||
bridges the ban), a connected user who has spent the enrichment worker's
|
||||
real-API daily budget (``_budget_exhausted_use_free``, set by the worker)
|
||||
— so a Spotify-Free user is never paused by the budget — and the worker
|
||||
opt-in below. See _free_wanted()."""
|
||||
from core.spotify_free_metadata import should_use_free_fallback
|
||||
# Worker opt-in (metadata.spotify_free_enrichment): prefer the no-creds
|
||||
# source for enrichment even while authed + healthy + under budget, to
|
||||
# spare the official quota for interactive use. The flag IS the explicit
|
||||
# opt-in, so it only needs the package installed — not the 'Spotify Free'
|
||||
# metadata-source choice — and it's set only on the enrichment worker's
|
||||
# own client, so interactive search/resolve stay official-first.
|
||||
if getattr(self, '_prefer_free', False) and self._free_installed():
|
||||
return True
|
||||
if not self._free_available():
|
||||
return False
|
||||
try:
|
||||
|
|
@ -1426,7 +1434,11 @@ class SpotifyClient:
|
|||
if tracks:
|
||||
return tracks
|
||||
|
||||
use_spotify = self.is_spotify_authenticated()
|
||||
# Skip the official API when the no-creds free source should serve this
|
||||
# (no-auth / rate-limited — where auth is already False — plus the
|
||||
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||
# but we deliberately defer to free). The free branch below then runs.
|
||||
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||
|
||||
if use_spotify:
|
||||
try:
|
||||
|
|
@ -1492,7 +1504,11 @@ class SpotifyClient:
|
|||
artists.sort(key=lambda a: (0 if a.name.lower().strip() == query_lower else 1))
|
||||
return artists
|
||||
|
||||
use_spotify = self.is_spotify_authenticated()
|
||||
# Skip the official API when the no-creds free source should serve this
|
||||
# (no-auth / rate-limited — where auth is already False — plus the
|
||||
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||
# but we deliberately defer to free). The free branch below then runs.
|
||||
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||
|
||||
if use_spotify:
|
||||
try:
|
||||
|
|
@ -1574,7 +1590,11 @@ class SpotifyClient:
|
|||
if albums:
|
||||
return albums
|
||||
|
||||
use_spotify = self.is_spotify_authenticated()
|
||||
# Skip the official API when the no-creds free source should serve this
|
||||
# (no-auth / rate-limited — where auth is already False — plus the
|
||||
# budget-bridge and the worker's prefer-free opt-in, where auth is True
|
||||
# but we deliberately defer to free). The free branch below then runs.
|
||||
use_spotify = self.is_spotify_authenticated() and not self._free_active()
|
||||
|
||||
if use_spotify:
|
||||
try:
|
||||
|
|
@ -1643,7 +1663,7 @@ class SpotifyClient:
|
|||
# Fallback cache hit — delegate to fallback client which reconstructs enhanced format
|
||||
return self._fallback.get_track_details(track_id)
|
||||
|
||||
if self.is_spotify_authenticated():
|
||||
if self.is_spotify_authenticated() and not self._free_active():
|
||||
try:
|
||||
track_data = self.sp.track(track_id)
|
||||
|
||||
|
|
@ -1745,7 +1765,7 @@ class SpotifyClient:
|
|||
# Fallback cache hit — delegate to fallback client
|
||||
return self._fallback.get_album(album_id)
|
||||
|
||||
if self.is_spotify_authenticated():
|
||||
if self.is_spotify_authenticated() and not self._free_active():
|
||||
try:
|
||||
album_data = self.sp.album(album_id)
|
||||
if album_data:
|
||||
|
|
@ -1788,7 +1808,7 @@ class SpotifyClient:
|
|||
if cached:
|
||||
return cached
|
||||
|
||||
if self.is_spotify_authenticated():
|
||||
if self.is_spotify_authenticated() and not self._free_active():
|
||||
try:
|
||||
# Get first page of tracks
|
||||
first_page = self.sp.album_tracks(album_id)
|
||||
|
|
@ -1882,7 +1902,7 @@ class SpotifyClient:
|
|||
except Exception as e:
|
||||
logger.debug("artist albums cache reuse: %s", e)
|
||||
|
||||
if self.is_spotify_authenticated():
|
||||
if self.is_spotify_authenticated() and not self._free_active():
|
||||
try:
|
||||
albums = []
|
||||
raw_items = []
|
||||
|
|
@ -2000,7 +2020,7 @@ class SpotifyClient:
|
|||
return self._fallback.get_artist(artist_id)
|
||||
return None
|
||||
|
||||
if self.is_spotify_authenticated():
|
||||
if self.is_spotify_authenticated() and not self._free_active():
|
||||
try:
|
||||
result = self.sp.artist(artist_id)
|
||||
if result:
|
||||
|
|
|
|||
|
|
@ -218,6 +218,21 @@ class SpotifyWorker:
|
|||
# protect the REAL authenticated API from bans — they don't apply
|
||||
# to free (a different, anonymous path). Computed once and reused
|
||||
# below; the loop already probes auth, so no extra quota cost.
|
||||
# Worker opt-in: when the user enables Spotify Free for enrichment
|
||||
# (metadata.spotify_free_enrichment), prefer the no-creds source
|
||||
# even while authed + under budget — bulk enrichment is the work
|
||||
# that bans the real API, so this spares the official quota for
|
||||
# interactive use. _free_active() honors this flag (and still falls
|
||||
# back to official only if free can't serve); set only on the
|
||||
# worker's own client, so interactive paths stay official-first.
|
||||
# Cheap config read; harmless when Spotify Free isn't installed.
|
||||
try:
|
||||
from config.settings import config_manager as _cfg
|
||||
self.client._prefer_free = bool(
|
||||
_cfg.get('metadata.spotify_free_enrichment', False))
|
||||
except Exception: # noqa: S110 — prefer-free toggle is best-effort
|
||||
self.client._prefer_free = False
|
||||
|
||||
budget_exhausted = self._is_daily_budget_exhausted()
|
||||
|
||||
# Daily budget is a REAL-API ban protection. When it's spent, if
|
||||
|
|
|
|||
|
|
@ -305,3 +305,91 @@ def test_client_search_albums_uses_free_via_artist_when_active():
|
|||
|
||||
assert len(results) == 1 and results[0].name == 'GNX'
|
||||
assert results[0].id == 'al2'
|
||||
|
||||
|
||||
# ── prefer-free (enrichment opt-in) + the use_spotify root-cause fix ─────────
|
||||
|
||||
def _free_active_with(prefer_free, installed, authed=True, rate_limited=False,
|
||||
selected=False, budget=False):
|
||||
c = SpotifyClient.__new__(SpotifyClient)
|
||||
if prefer_free:
|
||||
c._prefer_free = True
|
||||
if budget:
|
||||
c._budget_exhausted_use_free = True
|
||||
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=authed), \
|
||||
patch('core.spotify_client.config_manager') as cm, \
|
||||
patch('core.spotify_client._is_globally_rate_limited', return_value=rate_limited), \
|
||||
patch.object(_sfm, 'spotify_free_installed', return_value=installed):
|
||||
cm.get.side_effect = lambda k, d=None: selected if k == 'metadata.spotify_free' else d
|
||||
return c._free_active()
|
||||
|
||||
|
||||
def test_prefer_free_activates_even_when_authed_healthy_under_budget():
|
||||
# The enrichment opt-in: free serves even though official is perfectly usable.
|
||||
assert _free_active_with(prefer_free=True, installed=True) is True
|
||||
|
||||
|
||||
def test_prefer_free_inert_without_package():
|
||||
# Graceful: opt-in set but SpotipyFree missing -> stays on official.
|
||||
assert _free_active_with(prefer_free=True, installed=False) is False
|
||||
|
||||
|
||||
def test_no_prefer_free_authed_healthy_stays_official():
|
||||
assert _free_active_with(prefer_free=False, installed=True, selected=True) is False
|
||||
|
||||
|
||||
def _client_that_forbids_official(prefer_free=False, budget=False):
|
||||
"""A SpotifyClient whose official .sp blows up if touched + a Free client that
|
||||
serves albums — so a passing search proves official was skipped."""
|
||||
c = SpotifyClient.__new__(SpotifyClient)
|
||||
if prefer_free:
|
||||
c._prefer_free = True
|
||||
if budget:
|
||||
c._budget_exhausted_use_free = True
|
||||
|
||||
class _Sp:
|
||||
def search(self, *a, **k):
|
||||
raise AssertionError("official Spotify API must not be called when deferring to Free")
|
||||
c.sp = _Sp()
|
||||
fake_free = SpotifyFreeMetadataClient()
|
||||
fake_free.search_albums_via_artist = lambda artist, album, limit: [
|
||||
{'id': 'al2', 'name': 'GNX', 'artists': [{'name': 'Kendrick Lamar', 'id': 'art_k'}]}]
|
||||
c._free_meta_client = fake_free
|
||||
return c
|
||||
|
||||
|
||||
def test_search_albums_prefers_free_when_authed_and_prefer_free_set():
|
||||
"""Root-cause regression: prefer_free makes an AUTHED, healthy client defer to
|
||||
Spotify Free instead of the official API. Previously the use_spotify gate
|
||||
(= is_spotify_authenticated()) ignored _free_active() and hit official."""
|
||||
c = _client_that_forbids_official(prefer_free=True)
|
||||
fake_cache = type('C', (), {'get_search_results': lambda *a, **k: None})()
|
||||
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=True), \
|
||||
patch('core.spotify_client.config_manager') as cm, \
|
||||
patch('core.spotify_client._is_globally_rate_limited', return_value=False), \
|
||||
patch('core.spotify_client.get_metadata_cache', return_value=fake_cache), \
|
||||
patch.object(_sfm, 'spotify_free_installed', return_value=True):
|
||||
cm.get_spotify_config.return_value = {'client_id': 'x', 'client_secret': 'y'}
|
||||
cm.get.side_effect = lambda k, d=None: d
|
||||
results = c.search_albums('Kendrick Lamar GNX', limit=5,
|
||||
artist='Kendrick Lamar', album='GNX')
|
||||
assert len(results) == 1 and results[0].id == 'al2' # Free served; official untouched
|
||||
|
||||
|
||||
def test_search_albums_diverts_to_free_when_budget_exhausted_and_authed():
|
||||
"""Budget→Free bridge regression: an AUTHED client that has spent the daily
|
||||
budget defers to Free instead of hammering the official API (which the budget
|
||||
exists to protect). This is the divert that previously never happened."""
|
||||
c = _client_that_forbids_official(budget=True)
|
||||
fake_cache = type('C', (), {'get_search_results': lambda *a, **k: None})()
|
||||
with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=True), \
|
||||
patch('core.spotify_client.config_manager') as cm, \
|
||||
patch('core.spotify_client._is_globally_rate_limited', return_value=False), \
|
||||
patch('core.spotify_client.get_metadata_cache', return_value=fake_cache), \
|
||||
patch.object(_sfm, 'spotify_free_installed', return_value=True):
|
||||
cm.get_spotify_config.return_value = {'client_id': 'x', 'client_secret': 'y'}
|
||||
# metadata.spotify_free=True so the budget path's _free_available() holds
|
||||
cm.get.side_effect = lambda k, d=None: True if k == 'metadata.spotify_free' else d
|
||||
results = c.search_albums('Kendrick Lamar GNX', limit=5,
|
||||
artist='Kendrick Lamar', album='GNX')
|
||||
assert len(results) == 1 and results[0].id == 'al2'
|
||||
|
|
|
|||
Loading…
Reference in a new issue