diff --git a/backend/endpoints/roms/__init__.py b/backend/endpoints/roms/__init__.py index 1df2a033a4..203f5dd402 100644 --- a/backend/endpoints/roms/__init__.py +++ b/backend/endpoints/roms/__init__.py @@ -27,6 +27,7 @@ from fastapi.responses import Response from fastapi_pagination import resolve_params from fastapi_pagination.limit_offset import LimitOffsetPage, LimitOffsetParams +from fastapi_pagination.types import GreaterEqualZero from pydantic import BaseModel, Field from sqlalchemy.exc import IntegrityError from starlette.responses import FileResponse @@ -324,6 +325,7 @@ class CustomLimitOffsetParams(LimitOffsetParams): class CustomLimitOffsetPage[T: BaseModel](LimitOffsetPage[T]): + total: GreaterEqualZero | None char_index: dict[str, int] rom_id_index: list[int] filter_values: RomFiltersDict @@ -348,6 +350,17 @@ def get_roms( ) ), ] = True, + with_total: Annotated[ + bool, + Query( + description=( + "Whether to count the full result set. Set to false when the caller" + " already knows the total, e.g. paging through a gallery it has" + " sized; total then comes back null, unless the rom id index is" + " being built and already carries it." + ) + ), + ] = True, search_term: Annotated[ str | None, Query(description="Search term to filter roms."), @@ -831,9 +844,20 @@ def _transform(items: Sequence[Rom]) -> list[SimpleRomSchema]: for item in items ] + def resolve_total() -> int | None: + if with_rom_id_index: + # The index already spans the result set, so the count is free. + return len(rom_id_index) + # Without the index the count is its own scan of the filtered set, + # so a caller scrolling a gallery it already sized opts out. + return ( + db_rom_handler.get_rom_count(query=query, session=session) + if with_total + else None + ) + params = resolve_params() if with_rom_id_index: - total = len(rom_id_index) page_ids = list(rom_id_index[params.offset : params.offset + params.limit]) if page_ids: page_rows = session.scalars(query.where(Rom.id.in_(page_ids))).all() @@ -847,12 +871,11 @@ def _transform(items: Sequence[Rom]) -> list[SimpleRomSchema]: page_items = list( session.scalars(query.offset(params.offset).limit(params.limit)).all() ) - total = db_rom_handler.get_rom_count(query=query, session=session) return CustomLimitOffsetPage.create( _transform(page_items), params, - total=total, + total=resolve_total(), char_index=char_index_dict, rom_id_index=list(rom_id_index), filter_values=filter_values, diff --git a/backend/tests/endpoints/roms/test_rom.py b/backend/tests/endpoints/roms/test_rom.py index eca6030855..ff191ef739 100644 --- a/backend/tests/endpoints/roms/test_rom.py +++ b/backend/tests/endpoints/roms/test_rom.py @@ -313,6 +313,63 @@ def test_get_roms_without_rom_id_index( assert items[0]["id"] == rom.id +def test_get_roms_without_total( + client: TestClient, access_token: str, rom: Rom, platform: Platform +): + params = { + "platform_id": platform.id, + "limit": 15, + "with_rom_id_index": False, + } + + with patch.object( + db_rom_handler, "get_rom_count", wraps=db_rom_handler.get_rom_count + ) as get_rom_count: + response = client.get( + "/api/roms", + headers={"Authorization": f"Bearer {access_token}"}, + params={**params, "with_total": False}, + ) + assert response.status_code == status.HTTP_200_OK + + # The point of the opt-out: no second scan of the filtered set. + get_rom_count.assert_not_called() + + body = response.json() + assert body["total"] is None + + items = body["items"] + assert len(items) == 1 + assert items[0]["id"] == rom.id + + # Control: the count still runs for callers that ask for it. + response = client.get( + "/api/roms", + headers={"Authorization": f"Bearer {access_token}"}, + params=params, + ) + assert response.status_code == status.HTTP_200_OK + assert response.json()["total"] == 1 + get_rom_count.assert_called_once() + + +def test_get_roms_keeps_total_from_the_rom_id_index( + client: TestClient, access_token: str, rom: Rom, platform: Platform +): + # The index already carries the count, so opting out of the separate + # count query costs the caller nothing there. + response = client.get( + "/api/roms", + headers={"Authorization": f"Bearer {access_token}"}, + params={"platform_id": platform.id, "with_total": False}, + ) + assert response.status_code == status.HTTP_200_OK + + body = response.json() + assert body["total"] == 1 + assert body["rom_id_index"] == [rom.id] + + def test_get_roms_filter_by_metadata_providers( client: TestClient, access_token: str, rom: Rom, platform: Platform ): diff --git a/frontend/src/__generated__/models/CustomLimitOffsetPage_SimpleRomSchema_.ts b/frontend/src/__generated__/models/CustomLimitOffsetPage_SimpleRomSchema_.ts index baea947bf9..ed16bac83a 100644 --- a/frontend/src/__generated__/models/CustomLimitOffsetPage_SimpleRomSchema_.ts +++ b/frontend/src/__generated__/models/CustomLimitOffsetPage_SimpleRomSchema_.ts @@ -6,7 +6,7 @@ import type { RomFiltersDict } from './RomFiltersDict'; import type { SimpleRomSchema } from './SimpleRomSchema'; export type CustomLimitOffsetPage_SimpleRomSchema_ = { items: Array; - total: number; + total: (number | null); limit: number; offset: number; char_index: Record; diff --git a/frontend/src/services/api/rom.ts b/frontend/src/services/api/rom.ts index 33e1dba52f..3bfb84cb55 100644 --- a/frontend/src/services/api/rom.ts +++ b/frontend/src/services/api/rom.ts @@ -183,10 +183,10 @@ export interface GetRomsParams { playerCountsLogic?: string | null; metadataProvidersLogic?: string | null; tagsLogic?: string | null; - // Skip the char index / filter-value / id-index aggregations server-side withCharIndex?: boolean; withFilterValues?: boolean; withRomIdIndex?: boolean; + withTotal?: boolean; // Cancel an in-flight request signal?: AbortSignal; } @@ -238,6 +238,7 @@ async function getRoms({ withCharIndex = undefined, withFilterValues = undefined, withRomIdIndex = undefined, + withTotal = undefined, signal = undefined, }: GetRomsParams) { const params = { @@ -351,6 +352,7 @@ async function getRoms({ ...(withRomIdIndex !== undefined ? { with_rom_id_index: withRomIdIndex } : {}), + ...(withTotal !== undefined ? { with_total: withTotal } : {}), }; return api.get(`/roms`, { diff --git a/frontend/src/v2/stores/galleryRoms.test.ts b/frontend/src/v2/stores/galleryRoms.test.ts index 7d629b8cb9..d4c7c6e28b 100644 --- a/frontend/src/v2/stores/galleryRoms.test.ts +++ b/frontend/src/v2/stores/galleryRoms.test.ts @@ -165,6 +165,7 @@ describe("galleryRoms windowed fetch", () => { expect(params.withCharIndex).toBeUndefined(); expect(params.withFilterValues).toBeUndefined(); expect(params.withRomIdIndex).toBeUndefined(); + expect(params.withTotal).toBeUndefined(); }); it("does not clobber the filter drawer when filter values are skipped", async () => { @@ -225,6 +226,48 @@ describe("galleryRoms windowed fetch", () => { expect(store.charIndex).toEqual({ A: 0, B: 10 }); }); + // Skipping the id index used to make the backend fall back to a full COUNT, + // so every scroll batch re-counted the library for a total the page already + // had (issue #4053). + it("skips the total on a window the bootstrap already sized", async () => { + getRoms.mockImplementation((params: { limit?: number }) => { + if (params.limit === 1) { + return Promise.resolve({ + data: { total: 500, items: [], char_index: {}, rom_id_index: [] }, + }); + } + // The backend returns a null total when the count is skipped. + return Promise.resolve({ + data: { total: null, items: [], char_index: {}, rom_id_index: [] }, + }); + }); + const store = storeGalleryRoms(); + + await store.fetchInitialMetadata(); + expect(store.total).toBe(500); + + store.syncVisibleWindows([72]); + await flushPromises(); + + const windowCall = getRoms.mock.calls.find((c) => c[0].offset === 72); + expect(windowCall?.[0].withTotal).toBe(false); + // The null total must not blank the size the bootstrap established. + expect(store.total).toBe(500); + }); + + // The very first window doubles as the bootstrap when nothing has loaded + // yet, so it still has to bring the total back with it. + it("asks for the total on the first window when no bootstrap ran", async () => { + getRoms.mockResolvedValue(windowResponse(0, 300)); + const store = storeGalleryRoms(); + + store.syncVisibleWindows([0]); + await flushPromises(); + + expect(getRoms.mock.calls[0][0].withTotal).toBeUndefined(); + expect(store.total).toBe(300); + }); + it("does not mark a window loaded when the context is invalidated mid-apply", async () => { // Controllable frame yield so we can interleave a context switch between // the batched-apply's frames. diff --git a/frontend/src/v2/stores/galleryRoms.ts b/frontend/src/v2/stores/galleryRoms.ts index 7f11eb6707..439fe45aae 100644 --- a/frontend/src/v2/stores/galleryRoms.ts +++ b/frontend/src/v2/stores/galleryRoms.ts @@ -523,7 +523,10 @@ export default defineStore("v2GalleryRoms", { // object when these are skipped, so re-applying it would wipe the // populated values and blank the AlphaStrip / filter drawer. The id // index is a full-library scan we already paid for in the bootstrap, so - // window fetches opt out of recomputing it. + // window fetches opt out of recomputing it. Dropping it makes the backend + // count the result set separately instead, which is the same scan under + // another name for a total the bootstrap already gave us, so opt out of + // that too and keep the window fetch to just its page of covers. const withAggregations = !this.metadataLoaded; try { @@ -535,6 +538,7 @@ export default defineStore("v2GalleryRoms", { withCharIndex: false, withFilterValues: false, withRomIdIndex: false, + withTotal: false, }), signal: controller.signal, }); @@ -547,9 +551,10 @@ export default defineStore("v2GalleryRoms", { const data = response.data; // Only apply the full metadata when this window actually fetched the - // aggregations (the very first window before the bootstrap resolved). + // aggregations (a window reached before the bootstrap resolved). // Otherwise char_index / filter_values come back empty and would - // clobber what the bootstrap populated, so just refresh `total`. + // clobber what the bootstrap populated; `total` comes back null and + // the guard below leaves the established size alone. if (offset === 0 && withAggregations) { this._applyMetadata(data, galleryFilter, platformsStore); } else if (data.total !== null && data.total !== undefined) {