Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions backend/endpoints/roms/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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."),
Expand Down Expand Up @@ -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()
Expand 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,
Expand Down
57 changes: 57 additions & 0 deletions backend/tests/endpoints/roms/test_rom.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
):
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion frontend/src/services/api/rom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -238,6 +238,7 @@ async function getRoms({
withCharIndex = undefined,
withFilterValues = undefined,
withRomIdIndex = undefined,
withTotal = undefined,
signal = undefined,
}: GetRomsParams) {
const params = {
Expand Down Expand Up @@ -351,6 +352,7 @@ async function getRoms({
...(withRomIdIndex !== undefined
? { with_rom_id_index: withRomIdIndex }
: {}),
...(withTotal !== undefined ? { with_total: withTotal } : {}),
};

return api.get<GetRomsResponse>(`/roms`, {
Expand Down
43 changes: 43 additions & 0 deletions frontend/src/v2/stores/galleryRoms.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 8 additions & 3 deletions frontend/src/v2/stores/galleryRoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -535,6 +538,7 @@ export default defineStore("v2GalleryRoms", {
withCharIndex: false,
withFilterValues: false,
withRomIdIndex: false,
withTotal: false,
}),
signal: controller.signal,
});
Expand All @@ -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) {
Expand Down
Loading