From 826ac0b3664e556365ddf53a02537d602830d6f3 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 11 Jun 2026 15:35:36 -0700 Subject: [PATCH] #853 follow-up: don't cache a partial Deezer discography on mid-pagination error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/deezer_client.py | 15 +++++- ...test_artist_album_list_cache_nonspotify.py | 47 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/core/deezer_client.py b/core/deezer_client.py index 5df8c078..6f9d96ec 100644 --- a/core/deezer_client.py +++ b/core/deezer_client.py @@ -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] diff --git a/tests/test_artist_album_list_cache_nonspotify.py b/tests/test_artist_album_list_cache_nonspotify.py index a10d0855..87f4ff17 100644 --- a/tests/test_artist_album_list_cache_nonspotify.py +++ b/tests/test_artist_album_list_cache_nonspotify.py @@ -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