#853 follow-up: don't cache a partial Deezer discography on mid-pagination error
PR #853 added artist album-list caching to Deezer, but unlike the Spotify path it had no equivalent of the truncated-fetch guard: Deezer paginates, and a transient/ malformed response on page 2+ (artist with >100 albums) broke the loop and cached the PARTIAL list as the full discography — serving an incomplete album list from cache until TTL. Fix: track whether pagination finished cleanly. A malformed/empty-of-data response mid-walk now clears a `complete` flag and the artist→album-LIST is cached only when complete. Individual album entities still cache regardless (each is complete; we just have fewer). A clean end (short page / empty page / reached limit) still caches as before. Tests: a page-1-ok / page-2-errors walk no longer caches (second call refetches instead of serving a permanently-incomplete list); a clean two-page walk still caches (happy path intact). 181 deezer/metadata tests green.
This commit is contained in:
parent
814af2cdfb
commit
826ac0b366
2 changed files with 60 additions and 2 deletions
|
|
@ -896,12 +896,19 @@ class DeezerClient:
|
|||
requested_types = [t.strip() for t in album_type.split(',')]
|
||||
offset = 0
|
||||
page_size = 100 # Deezer API max per request
|
||||
complete = True # cleared if pagination breaks on a transient/malformed error
|
||||
|
||||
while offset < limit:
|
||||
fetch_limit = min(page_size, limit - offset)
|
||||
data = self._api_get(f'artist/{artist_id}/albums', {'limit': fetch_limit, 'index': offset})
|
||||
if not data or 'data' not in data or len(data['data']) == 0:
|
||||
if not data or 'data' not in data:
|
||||
# Malformed/transient response mid-pagination — what we have is a
|
||||
# PARTIAL discography. Don't cache it as the full list (mirrors the
|
||||
# Spotify truncated-fetch guard). #853 follow-up.
|
||||
complete = False
|
||||
break
|
||||
if len(data['data']) == 0:
|
||||
break # No more albums — a clean end of pagination.
|
||||
|
||||
for album_data in data['data']:
|
||||
all_raw.append(album_data)
|
||||
|
|
@ -931,7 +938,11 @@ class DeezerClient:
|
|||
entries.append((str(ad['id']), ad))
|
||||
if entries:
|
||||
cache.store_entities_bulk('deezer', 'album', entries, skip_if_exists=True)
|
||||
store_artist_album_items(cache, 'deezer', artist_id, all_raw, album_type=album_type, limit=limit)
|
||||
# Only cache the artist→album-LIST when pagination finished cleanly; a
|
||||
# partial list would otherwise serve an incomplete discography until TTL.
|
||||
# (Individual album entities above are complete, so they cache regardless.)
|
||||
if complete:
|
||||
store_artist_album_items(cache, 'deezer', artist_id, all_raw, album_type=album_type, limit=limit)
|
||||
|
||||
logger.info(f"Retrieved {len(albums)} albums for artist {artist_id}")
|
||||
return albums[:limit]
|
||||
|
|
|
|||
|
|
@ -116,3 +116,50 @@ def test_discogs_artist_albums_reuses_list_cache(monkeypatch):
|
|||
assert [album.name for album in first] == ['Cached Discogs Album']
|
||||
assert [album.name for album in second] == ['Cached Discogs Album']
|
||||
assert client._api_get.call_count == 2
|
||||
|
||||
|
||||
def _deezer_album(i):
|
||||
return {
|
||||
'id': i, 'title': f'Album {i}', 'record_type': 'album',
|
||||
'release_date': '2024-01-01', 'nb_tracks': 10,
|
||||
'artist': {'id': 7, 'name': 'Artist'},
|
||||
}
|
||||
|
||||
|
||||
def test_deezer_partial_pagination_not_cached(monkeypatch):
|
||||
"""#853 follow-up: a transient/malformed error mid-pagination must NOT cache a
|
||||
partial discography (mirrors Spotify's truncated-fetch guard) — otherwise an
|
||||
incomplete album list serves from cache until TTL."""
|
||||
cache = MemoryCache()
|
||||
monkeypatch.setattr(deezer_mod, 'get_metadata_cache', lambda: cache)
|
||||
client = deezer_mod.DeezerClient.__new__(deezer_mod.DeezerClient)
|
||||
|
||||
full_page = {'data': [_deezer_album(i) for i in range(100)]} # full → forces page 2
|
||||
# page 1 ok, page 2 errors (None) → incomplete, on BOTH attempts
|
||||
client._api_get = MagicMock(side_effect=[full_page, None, full_page, None])
|
||||
|
||||
first = client.get_artist_albums('7', limit=200)
|
||||
assert len(first) == 100 # page-1 albums still returned
|
||||
|
||||
second = client.get_artist_albums('7', limit=200)
|
||||
assert len(second) == 100
|
||||
# No partial cache → the second call refetched (4 api calls total), instead of
|
||||
# serving a permanently-incomplete discography from cache (which would be 2).
|
||||
assert client._api_get.call_count == 4
|
||||
|
||||
|
||||
def test_deezer_complete_multipage_is_cached(monkeypatch):
|
||||
"""A clean multi-page pagination still caches (guard didn't break the happy path)."""
|
||||
cache = MemoryCache()
|
||||
monkeypatch.setattr(deezer_mod, 'get_metadata_cache', lambda: cache)
|
||||
client = deezer_mod.DeezerClient.__new__(deezer_mod.DeezerClient)
|
||||
|
||||
page1 = {'data': [_deezer_album(i) for i in range(100)]} # full
|
||||
page2 = {'data': [_deezer_album(i) for i in range(100, 150)]} # short → clean end
|
||||
client._api_get = MagicMock(side_effect=[page1, page2])
|
||||
|
||||
first = client.get_artist_albums('7', limit=200)
|
||||
assert len(first) == 150
|
||||
second = client.get_artist_albums('7', limit=200) # served from cache
|
||||
assert len(second) == 150
|
||||
assert client._api_get.call_count == 2 # no refetch → cached
|
||||
|
|
|
|||
Loading…
Reference in a new issue