From fd2be0ddd561f4b144a19d55a879848f4c660da9 Mon Sep 17 00:00:00 2001 From: Harvey <64276030+HabiRabbu@users.noreply.github.com> Date: Tue, 14 Apr 2026 09:59:08 +0100 Subject: [PATCH] new backend pagination approach (#49) * new backend pagination approach * prettier --- backend/api/v1/schemas/artist.py | 9 +- backend/services/artist_service.py | 148 +++++++-- .../routes/test_artist_releases_route.py | 14 +- .../test_artist_release_pagination.py | 313 ++++++++++++++++++ .../queries/artist/ArtistQueries.svelte.ts | 11 +- frontend/src/lib/types.ts | 6 +- frontend/src/routes/artist/[id]/+page.svelte | 5 +- 7 files changed, 458 insertions(+), 48 deletions(-) create mode 100644 backend/tests/services/test_artist_release_pagination.py diff --git a/backend/api/v1/schemas/artist.py b/backend/api/v1/schemas/artist.py index 3f5a065..871f52f 100644 --- a/backend/api/v1/schemas/artist.py +++ b/backend/api/v1/schemas/artist.py @@ -15,9 +15,16 @@ class ArtistReleases(AppStruct): albums: list[ReleaseItem] = [] singles: list[ReleaseItem] = [] eps: list[ReleaseItem] = [] - total_count: int = 0 + + offset: int = 0 + limit: int = 50 + returned_count: int = 0 + + next_offset: int | None = None has_more: bool = False + source_total_count: int | None = None + class LastFmSimilarArtistSchema(AppStruct): name: str diff --git a/backend/services/artist_service.py b/backend/services/artist_service.py index 915f3c8..2fb67d4 100644 --- a/backend/services/artist_service.py +++ b/backend/services/artist_service.py @@ -543,46 +543,126 @@ class ArtistService: included_primary_types = set(t.lower() for t in prefs.primary_types) included_secondary_types = set(t.lower() for t in prefs.secondary_types) - if in_library and offset == 0: - lidarr_albums = await self._lidarr_repo.get_artist_albums(artist_id) - albums, singles, eps = self._categorize_lidarr_albums(lidarr_albums, album_mbids, requested_mbids=requested_mbids) - - total_count = len(albums) + len(singles) + len(eps) - + if in_library: + if offset == 0: + lidarr_albums = await self._lidarr_repo.get_artist_albums(artist_id) + albums, singles, eps = self._categorize_lidarr_albums(lidarr_albums, album_mbids, requested_mbids=requested_mbids) + total_count = len(albums) + len(singles) + len(eps) + return ArtistReleases( + albums=albums, + singles=singles, + eps=eps, + offset=0, + limit=limit, + returned_count=total_count, + next_offset=None, + has_more=False, + source_total_count=None, + ) return ArtistReleases( - albums=albums, - singles=singles, - eps=eps, - total_count=total_count, - has_more=False + albums=[], singles=[], eps=[], + offset=offset, limit=limit, returned_count=0, + next_offset=None, has_more=False, source_total_count=None, ) - release_groups, total_count = await self._mb_repo.get_artist_release_groups( - artist_id, offset, limit - ) - - temp_artist = {"release-group-list": release_groups} - - albums, singles, eps = categorize_release_groups( - temp_artist, - album_mbids, - included_primary_types, - included_secondary_types, - requested_mbids - ) - - has_more = (offset + len(release_groups)) < total_count - - return ArtistReleases( - albums=albums, - singles=singles, - eps=eps, - total_count=total_count, - has_more=has_more + return await self._filter_aware_release_page( + artist_id, offset, limit, album_mbids, requested_mbids, + included_primary_types, included_secondary_types, ) except Exception as e: # noqa: BLE001 logger.error(f"Error fetching releases for artist {artist_id} at offset {offset}: {e}") - return ArtistReleases(albums=[], singles=[], eps=[], total_count=0, has_more=False) + return ArtistReleases( + albums=[], singles=[], eps=[], + offset=offset, limit=limit, returned_count=0, + next_offset=None, has_more=False, source_total_count=None, + ) + + async def _filter_aware_release_page( + self, + artist_id: str, + offset: int, + limit: int, + album_mbids: set[str], + requested_mbids: set[str], + included_primary_types: set[str], + included_secondary_types: set[str], + ) -> ArtistReleases: + if not included_primary_types: + return ArtistReleases( + albums=[], singles=[], eps=[], + offset=offset, limit=limit, returned_count=0, + next_offset=None, has_more=False, source_total_count=None, + ) + + _SCAN_BATCH = 100 + _MAX_SCAN_BATCHES = 20 + seen_mbids: set[str] = set() + all_albums: list[ReleaseItem] = [] + all_singles: list[ReleaseItem] = [] + all_eps: list[ReleaseItem] = [] + + raw_offset = offset + source_total: int | None = None + batches_scanned = 0 + + while batches_scanned < _MAX_SCAN_BATCHES: + release_groups, mb_total = await self._mb_repo.get_artist_release_groups( + artist_id, raw_offset, _SCAN_BATCH + ) + if source_total is None: + source_total = mb_total + + if not release_groups: + break + + temp_artist = {"release-group-list": release_groups} + page_albums, page_singles, page_eps = categorize_release_groups( + temp_artist, album_mbids, included_primary_types, + included_secondary_types, requested_mbids, + ) + + for item in page_albums: + if item.id and item.id not in seen_mbids: + seen_mbids.add(item.id) + all_albums.append(item) + for item in page_singles: + if item.id and item.id not in seen_mbids: + seen_mbids.add(item.id) + all_singles.append(item) + for item in page_eps: + if item.id and item.id not in seen_mbids: + seen_mbids.add(item.id) + all_eps.append(item) + + raw_offset += len(release_groups) + batches_scanned += 1 + + total_collected = len(all_albums) + len(all_singles) + len(all_eps) + if total_collected >= limit: + break + if raw_offset >= mb_total: + break + + for lst in (all_albums, all_singles, all_eps): + lst.sort(key=lambda x: (x.year is None, -(x.year or 0))) + + returned_count = len(all_albums) + len(all_singles) + len(all_eps) + + has_more = raw_offset < (source_total or 0) + + next_offset = raw_offset if has_more else None + + return ArtistReleases( + albums=all_albums, + singles=all_singles, + eps=all_eps, + offset=offset, + limit=limit, + returned_count=returned_count, + next_offset=next_offset, + has_more=has_more, + source_total_count=source_total, + ) async def _fetch_artist_data( self, diff --git a/backend/tests/routes/test_artist_releases_route.py b/backend/tests/routes/test_artist_releases_route.py index 134bb38..eb6de32 100644 --- a/backend/tests/routes/test_artist_releases_route.py +++ b/backend/tests/routes/test_artist_releases_route.py @@ -26,8 +26,12 @@ def mock_artist_service(): albums=[ReleaseItem(id="rg-2", title="Album Two", type="Album", year=2023)], singles=[], eps=[], - total_count=120, + offset=0, + limit=50, + returned_count=1, + next_offset=100, has_more=True, + source_total_count=120, ) ) mock.get_artist_info_basic = AsyncMock() @@ -61,13 +65,17 @@ class TestGetArtistReleasesRoute: assert response.status_code == 200 mock_artist_service.get_artist_releases.assert_awaited_once_with(VALID_MBID, 50, 50) - def test_has_more_flag_propagated(self, client): + def test_new_pagination_fields_propagated(self, client): response = client.get(f"/api/v1/artists/{VALID_MBID}/releases?offset=0&limit=50") body = response.json() assert response.status_code == 200 assert body["has_more"] is True - assert body["total_count"] == 120 + assert body["offset"] == 0 + assert body["limit"] == 50 + assert body["returned_count"] == 1 + assert body["next_offset"] == 100 + assert body["source_total_count"] == 120 def test_default_params(self, client, mock_artist_service): response = client.get(f"/api/v1/artists/{VALID_MBID}/releases") diff --git a/backend/tests/services/test_artist_release_pagination.py b/backend/tests/services/test_artist_release_pagination.py new file mode 100644 index 0000000..ddde951 --- /dev/null +++ b/backend/tests/services/test_artist_release_pagination.py @@ -0,0 +1,313 @@ +"""Service-level tests for filter-aware artist release pagination.""" +import os +import tempfile + +os.environ.setdefault("ROOT_APP_DIR", tempfile.mkdtemp()) + +import pytest +from unittest.mock import AsyncMock, MagicMock + +from services.artist_service import ArtistService + + +ARTIST_MBID = "f4a31f0a-51dd-4fa7-986d-3095c40c5ed9" + + +def _make_release_group(rg_id: str, title: str, primary_type: str, date: str = "2020-01-01") -> dict: + return { + "id": rg_id, + "title": title, + "primary-type": primary_type, + "secondary-types": [], + "first-release-date": date, + } + + +def _make_prefs(primary_types: list[str] | None = None, secondary_types: list[str] | None = None) -> MagicMock: + p = MagicMock() + p.get_preferences.return_value = MagicMock( + primary_types=primary_types if primary_types is not None else ["Album", "Single", "EP"], + secondary_types=secondary_types if secondary_types is not None else ["Studio", "Live", "Compilation"], + ) + p.get_advanced_settings.return_value = MagicMock( + cache_ttl_artist_library=21600, + cache_ttl_artist_non_library=3600, + ) + return p + + +def _make_service( + *, + mb_release_pages: list[tuple[list[dict], int]] | None = None, + lidarr_artist: dict | None = None, + prefs: MagicMock | None = None, +) -> ArtistService: + mb_repo = AsyncMock() + if mb_release_pages is not None: + mb_repo.get_artist_release_groups = AsyncMock(side_effect=mb_release_pages) + else: + mb_repo.get_artist_release_groups = AsyncMock(return_value=([], 0)) + + lidarr_repo = MagicMock() + lidarr_repo.is_configured.return_value = False + lidarr_repo.get_artist_details = AsyncMock(return_value=lidarr_artist) + lidarr_repo.get_library_mbids = AsyncMock(return_value=set()) + lidarr_repo.get_requested_mbids = AsyncMock(return_value=set()) + lidarr_repo.get_artist_mbids = AsyncMock(return_value=set()) + + wikidata_repo = AsyncMock() + + memory_cache = AsyncMock() + memory_cache.get = AsyncMock(return_value=None) + memory_cache.set = AsyncMock() + + disk_cache = AsyncMock() + disk_cache.get_artist = AsyncMock(return_value=None) + disk_cache.set_artist = AsyncMock() + + return ArtistService( + mb_repo=mb_repo, + lidarr_repo=lidarr_repo, + wikidata_repo=wikidata_repo, + preferences_service=prefs or _make_prefs(), + memory_cache=memory_cache, + disk_cache=disk_cache, + ) + + +class TestFilterAwarePagination: + @pytest.mark.asyncio + async def test_single_page_fits_filter(self): + rg1 = _make_release_group("rg-1", "Album A", "Album") + rg2 = _make_release_group("rg-2", "Single B", "Single") + svc = _make_service(mb_release_pages=[([rg1, rg2], 2)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert len(result.albums) == 1 + assert result.albums[0].title == "Album A" + assert len(result.singles) == 1 + assert result.singles[0].title == "Single B" + assert result.has_more is False + assert result.next_offset is None + assert result.returned_count == 2 + assert result.source_total_count == 2 + + @pytest.mark.asyncio + async def test_sparse_filter_scans_multiple_batches(self): + batch1 = [_make_release_group(f"rg-{i}", f"Broadcast {i}", "Broadcast") for i in range(5)] + batch2 = [_make_release_group("rg-album", "Real Album", "Album")] + svc = _make_service( + mb_release_pages=[ + (batch1, 6), + (batch2, 6), + ] + ) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.albums[0].title == "Real Album" + assert result.returned_count == 1 + assert result.source_total_count == 6 + + @pytest.mark.asyncio + async def test_has_more_true_when_unscanned_raw_data_remains(self): + rgs = [_make_release_group("rg-1", "Album 1", "Album")] + svc = _make_service(mb_release_pages=[(rgs, 200), ([], 200), ([], 200)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.has_more is True + assert result.next_offset is not None + assert result.returned_count == 1 + + @pytest.mark.asyncio + async def test_next_offset_is_scan_position(self): + batch1 = [_make_release_group(f"rg-{i}", f"Album {i}", "Album") for i in range(100)] + svc = _make_service(mb_release_pages=[(batch1, 200)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=10) + + assert result.has_more is True + assert result.next_offset == 100 + + @pytest.mark.asyncio + async def test_no_duplicates_within_scan(self): + batch1 = [_make_release_group(f"rg-{i}", f"Album {i}", "Album") for i in range(5)] + batch2 = [_make_release_group(f"rg-{i}", f"Album {i}", "Album") for i in range(3, 8)] + svc = _make_service( + mb_release_pages=[ + (batch1, 8), + (batch2, 8), + ] + ) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + all_ids = [r.id for r in result.albums] + assert len(all_ids) == len(set(all_ids)) + + @pytest.mark.asyncio + async def test_no_drops_across_sequential_pages(self): + rgs = [_make_release_group(f"rg-{i}", f"Album {i}", "Album") for i in range(10)] + svc = _make_service(mb_release_pages=[(rgs, 10)]) + + page1 = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=3) + assert page1.returned_count == 10 + assert page1.has_more is False + assert page1.next_offset is None + + @pytest.mark.asyncio + async def test_empty_result_set(self): + svc = _make_service(mb_release_pages=[([], 0)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 0 + assert result.has_more is False + assert result.next_offset is None + assert result.source_total_count == 0 + + @pytest.mark.asyncio + async def test_lidarr_library_artist_single_page(self): + lidarr_albums = [ + {"mbid": "rg-1", "title": "Lib Album", "album_type": "Album", "release_date": "2020-01-01", "year": 2020, "track_file_count": 5, "secondary_types": []}, + ] + svc = _make_service(lidarr_artist={"monitored": True}) + + lidarr_repo = svc._lidarr_repo + lidarr_repo.get_artist_albums = AsyncMock(return_value=lidarr_albums) + lidarr_repo.get_library_mbids = AsyncMock(return_value={"rg-1"}) + lidarr_repo.get_requested_mbids = AsyncMock(return_value=set()) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 1 + assert result.has_more is False + assert result.next_offset is None + + @pytest.mark.asyncio + async def test_lidarr_library_artist_offset_gt_zero_returns_empty(self): + svc = _make_service(lidarr_artist={"monitored": True}) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=50, limit=50) + + assert result.returned_count == 0 + assert result.has_more is False + assert result.next_offset is None + + @pytest.mark.asyncio + async def test_offset_reflects_client_param(self): + rg = _make_release_group("rg-1", "Album 1", "Album") + svc = _make_service(mb_release_pages=[([rg], 1)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=200, limit=50) + + assert result.offset == 200 + + @pytest.mark.asyncio + async def test_returned_count_across_categories(self): + rgs = [ + _make_release_group("rg-a1", "Album 1", "Album"), + _make_release_group("rg-a2", "Album 2", "Album"), + _make_release_group("rg-s1", "Single 1", "Single"), + _make_release_group("rg-e1", "EP 1", "EP"), + ] + svc = _make_service(mb_release_pages=[(rgs, 4)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 4 + assert len(result.albums) == 2 + assert len(result.singles) == 1 + assert len(result.eps) == 1 + + @pytest.mark.asyncio + async def test_limit_controls_scan_termination_all_items_returned(self): + rgs = [ + _make_release_group("rg-a1", "Album 1", "Album"), + _make_release_group("rg-a2", "Album 2", "Album"), + _make_release_group("rg-a3", "Album 3", "Album"), + _make_release_group("rg-s1", "Single 1", "Single"), + _make_release_group("rg-e1", "EP 1", "EP"), + ] + svc = _make_service(mb_release_pages=[(rgs, 5)]) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=3) + + assert result.returned_count == 5 + assert len(result.albums) == 3 + assert len(result.singles) == 1 + assert len(result.eps) == 1 + assert result.has_more is False + + @pytest.mark.asyncio + async def test_exception_returns_empty_page(self): + svc = _make_service() + svc._lidarr_repo.get_artist_details = AsyncMock(side_effect=RuntimeError("boom")) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 0 + assert result.has_more is False + assert result.next_offset is None + + @pytest.mark.asyncio + async def test_empty_filter_types_returns_immediately(self): + svc = _make_service( + prefs=_make_prefs(primary_types=[], secondary_types=[]), + ) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 0 + assert result.has_more is False + assert result.next_offset is None + svc._mb_repo.get_artist_release_groups.assert_not_awaited() + + @pytest.mark.asyncio + async def test_all_types_filtered_out_except_one(self): + batch = ( + [_make_release_group(f"rg-b{i}", f"Broadcast {i}", "Broadcast") for i in range(5)] + + [_make_release_group("rg-album", "Found Album", "Album")] + ) + svc = _make_service( + mb_release_pages=[ + (batch, 6), + ] + ) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.returned_count == 1 + assert result.albums[0].title == "Found Album" + assert result.source_total_count == 6 + assert result.has_more is False + + @pytest.mark.asyncio + async def test_global_sort_across_batches(self): + batch1 = [_make_release_group("rg-a", "Old Album", "Album", "2010-01-01")] + batch2 = [_make_release_group("rg-b", "New Album", "Album", "2020-01-01")] + svc = _make_service( + mb_release_pages=[ + (batch1, 2), + (batch2, 2), + ] + ) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert len(result.albums) == 2 + assert result.albums[0].title == "New Album" + assert result.albums[1].title == "Old Album" + + @pytest.mark.asyncio + async def test_scan_batch_cap_stops_early(self): + batches = [([_make_release_group(f"rg-{i}", f"Album {i}", "Album")], 5000) for i in range(25)] + svc = _make_service(mb_release_pages=batches) + + result = await svc.get_artist_releases(ARTIST_MBID, offset=0, limit=50) + + assert result.has_more is True + assert result.next_offset is not None + assert result.next_offset == 20 diff --git a/frontend/src/lib/queries/artist/ArtistQueries.svelte.ts b/frontend/src/lib/queries/artist/ArtistQueries.svelte.ts index 0616bd6..883c18f 100644 --- a/frontend/src/lib/queries/artist/ArtistQueries.svelte.ts +++ b/frontend/src/lib/queries/artist/ArtistQueries.svelte.ts @@ -115,13 +115,14 @@ export const getArtistReleasesInfiniteQuery = (getArtistId: Getter) => ); return response; }, - getNextPageParam: (lastPage, allPages) => { - const wasLastPageEmpty = - lastPage.albums.length === 0 && lastPage.singles.length === 0 && lastPage.eps.length === 0; - if (!lastPage.has_more || wasLastPageEmpty) { + getNextPageParam: (lastPage) => { + if (!lastPage.has_more) { return undefined; } - return allPages.length * BATCH_SIZE; + if (lastPage.next_offset != null) { + return lastPage.next_offset; + } + return undefined; } })); diff --git a/frontend/src/lib/types.ts b/frontend/src/lib/types.ts index 2bbbd31..454bff3 100644 --- a/frontend/src/lib/types.ts +++ b/frontend/src/lib/types.ts @@ -162,8 +162,12 @@ export type ArtistReleases = { albums: ReleaseGroup[]; singles: ReleaseGroup[]; eps: ReleaseGroup[]; - total_count: number; + offset: number; + limit: number; + returned_count: number; + next_offset: number | null; has_more: boolean; + source_total_count: number | null; }; export type UserPreferences = { diff --git a/frontend/src/routes/artist/[id]/+page.svelte b/frontend/src/routes/artist/[id]/+page.svelte index 54577b5..1c7c53d 100644 --- a/frontend/src/routes/artist/[id]/+page.svelte +++ b/frontend/src/routes/artist/[id]/+page.svelte @@ -128,8 +128,6 @@ releasesQuery.data?.pages.flatMap((page) => [...page.albums, ...page.singles, ...page.eps]) .length || 0 ); - // Total count is not reliable, since it contains items that are filtered out - const totalReleaseCount = $derived(releasesQuery.data?.pages[0].total_count || 0); $effect(() => { if (hasMoreReleases && !releasesQuery.isFetchingNextPage) { @@ -340,8 +338,7 @@ Loading releases... - Loaded {loadedReleaseCount} of {totalReleaseCount} potential releasesLoading {loadedReleaseCount} releases