From a2129bb9bde5c623244b59d00aaef73b3459c1e4 Mon Sep 17 00:00:00 2001 From: Lukas Date: Tue, 28 Apr 2026 21:40:22 +0200 Subject: [PATCH] Pagination contract is not standardized across endpoints --- Docs/Backend-Development.md | 58 ++++++++++ Docs/Tasks.md | 19 ---- Docs/Web-Development.md | 93 ++++++++++++++++ backend/app/models/blocklist.py | 16 +-- backend/app/models/history.py | 16 +-- backend/app/routers/blocklist.py | 7 +- backend/app/services/blocklist_service.py | 2 - backend/app/utils/pagination.py | 102 +++++++++++++++++ backend/tests/test_routers/test_blocklist.py | 6 +- .../test_services/test_blocklist_service.py | 2 - backend/tests/test_utils/test_pagination.py | 104 ++++++++++++++++++ frontend/src/types/blocklist.ts | 10 +- frontend/src/types/history.ts | 8 +- 13 files changed, 387 insertions(+), 56 deletions(-) create mode 100644 backend/app/utils/pagination.py create mode 100644 backend/tests/test_utils/test_pagination.py diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index 6a77bee..7d10404 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -461,6 +461,64 @@ class BansByCountryResponse(BaseModel): 5. **No ad-hoc wrapper objects** — Don't invent new response shapes. Use the patterns above. +### Standardized Pagination Query Parameters + +All paginated endpoints follow a consistent query parameter contract: + +| Parameter | Type | Constraints | Default | Notes | +|---|---|---|---|---| +| `page` | int | ≥ 1 | `1` | 1-based page number (not 0-based offset). | +| `page_size` | int | 1–500 | `100` | Items per page. Clients may request smaller pages for UI reasons. | + +**Implementation:** + +```python +from fastapi import Query +from app.utils.constants import DEFAULT_PAGE_SIZE + +@router.get("/items") +async def get_items( + page: int = Query(default=1, ge=1, description="1-based page number."), + page_size: int = Query( + default=DEFAULT_PAGE_SIZE, + ge=1, + le=500, + description="Items per page (max 500).", + ), +): + # Compute offset for database query + offset = (page - 1) * page_size + items = await db.fetch("SELECT * FROM items LIMIT ? OFFSET ?", page_size, offset) + total = await db.fetchval("SELECT COUNT(*) FROM items") + + return PaginatedListResponse( + items=items, + total=total, + page=page, + page_size=page_size, + ) +``` + +**Helper functions** are available in `app.utils.pagination`: + +```python +from app.utils.pagination import get_offset, compute_total_pages + +# Calculate database offset from page and page_size +offset = get_offset(page, page_size) # Equivalent to (page - 1) * page_size + +# Calculate total pages for rendering pagination UI (optional) +total_pages = compute_total_pages(total, page_size) +``` + +**Rules:** + +1. **Use 1-based pages** — Not 0-based offsets. Page 1 is always the first page. +2. **Always provide defaults** — Use `DEFAULT_PAGE_SIZE` (100) and initial page 1. +3. **Cap maximum page_size at 500** — Prevents accidental DoS from enormous requests. +4. **Respond with `PaginatedListResponse[T]`** — Must include `items`, `total`, `page`, `page_size`. +5. **Never include `total_pages` in responses** — The frontend can calculate it as `Math.ceil(total / page_size)`. + --- ## 5. Pydantic Models diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 2c8d655..14a9639 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,22 +1,3 @@ -## 25) No canonical snake_case/camelCase serialization policy -- Where found: - - [backend/app/models/server.py](backend/app/models/server.py) - - [frontend/src/types/server.ts](frontend/src/types/server.ts) -- Why this is needed: - - Naming convention drift causes brittle frontend-backend contracts. -- Goal: - - Enforce one API field naming policy. -- What to do: - - Configure model aliasing strategy and update frontend contracts. -- Possible traps and issues: - - Partial migration can leave mixed payload formats. -- Docs changes needed: - - Add naming convention section for API fields. -- Doc references: - - [Docs/Backend-Development.md](Docs/Backend-Development.md) - - https://docs.pydantic.dev/latest/concepts/alias/ - ---- ## 26) Pagination contract is not standardized across endpoints - Where found: diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index d84f312..7f7a3fc 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -246,6 +246,99 @@ const doBan = useCallback( - ❌ Passing API functions directly to components — couples components to API contract - ❌ Multiple domain hooks for the same data without deduplication — causes wasted requests and state desync +### Standardized Pagination + +All paginated endpoints follow a consistent contract to simplify frontend logic and reduce boilerplate: + +**Pagination Response Type:** + +```ts +// types/response.ts +export interface PaginatedListResponse extends CollectionResponse { + /** Current page number (1-based). */ + page: number; + /** Number of items per page. */ + page_size: number; +} + +// Extending interfaces for specific data types: +// types/ban.ts +export interface DashboardBanListResponse extends PaginatedListResponse {} + +// types/history.ts +export interface HistoryListResponse extends PaginatedListResponse {} + +// types/blocklist.ts +export interface ImportLogListResponse extends PaginatedListResponse {} +``` + +**Query Parameters:** + +All paginated endpoints accept these standardized parameters: + +| Parameter | Type | Range | Default | +|---|---|---|---| +| `page` | number | ≥ 1 | `1` | +| `page_size` | number | 1–500 | `100` | + +**Frontend Calculation of Total Pages:** + +Frontend code should calculate total pages from the response without relying on a `total_pages` field: + +```ts +// Compute the total number of pages +const totalPages = Math.ceil(response.total / response.page_size); +const isLastPage = response.page >= totalPages; +const isFirstPage = response.page === 1; + +// Determine next/previous pages (with bounds checking) +const nextPage = isLastPage ? response.page : response.page + 1; +const prevPage = isFirstPage ? 1 : response.page - 1; +``` + +**Example Hook:** + +```ts +// hooks/useHistoryPagination.ts +export function useHistoryPagination() { + const [page, setPage] = useState(1); + const pageSize = 100; + + const fetcher = useCallback( + (signal: AbortSignal) => + fetchHistory( + { page, pageSize, /* filters */ }, + signal, + ), + [page, pageSize], + ); + + const { items: historyItems, data: fullResponse, loading, error, refresh } = useListData({ + fetcher, + selector: (res: HistoryListResponse) => res.items, + }); + + // Calculate pagination UI state + const totalPages = Math.ceil((fullResponse?.total ?? 0) / pageSize); + const canGoPrevious = page > 1; + const canGoNext = page < totalPages; + + return { + historyItems, + page, + pageSize, + total: fullResponse?.total ?? 0, + totalPages, + canGoPrevious, + canGoNext, + setPage: (newPage: number) => setPage(Math.max(1, Math.min(newPage, totalPages))), + loading, + error, + refresh, + }; +} +``` + ### Hook Architecture & Reusable Primitives BanGUI's hooks are built on composable primitives to eliminate duplication and enforce consistent patterns. diff --git a/backend/app/models/blocklist.py b/backend/app/models/blocklist.py index b53ca9f..4cd65d3 100644 --- a/backend/app/models/blocklist.py +++ b/backend/app/models/blocklist.py @@ -10,7 +10,7 @@ from enum import StrEnum from pydantic import AnyHttpUrl, Field -from app.models.response import BanGuiBaseModel +from app.models.response import BanGuiBaseModel, PaginatedListResponse # --------------------------------------------------------------------------- @@ -69,14 +69,14 @@ class ImportLogEntry(BanGuiBaseModel): ips_skipped: int errors: str | None -class ImportLogListResponse(BanGuiBaseModel): - """Response for ``GET /api/blocklists/log``.""" +class ImportLogListResponse(PaginatedListResponse[ImportLogEntry]): + """Response for ``GET /api/blocklists/log``. - items: list[ImportLogEntry] = Field(default_factory=list) - total: int = Field(..., ge=0) - page: int = Field(default=1, ge=1) - page_size: int = Field(default=50, ge=1) - total_pages: int = Field(default=1, ge=1) + Paginated list of all blocklist import runs with timestamps, source info, + and per-source import/skip counts. + """ + + pass # --------------------------------------------------------------------------- # Schedule diff --git a/backend/app/models/history.py b/backend/app/models/history.py index c8dcd6f..266d3ba 100644 --- a/backend/app/models/history.py +++ b/backend/app/models/history.py @@ -7,7 +7,7 @@ from __future__ import annotations from pydantic import Field -from app.models.response import BanGuiBaseModel +from app.models.response import BanGuiBaseModel, PaginatedListResponse from app.models.ban import TimeRange @@ -56,13 +56,15 @@ class HistoryBanItem(BanGuiBaseModel): description="Organisation name associated with the IP.", ) -class HistoryListResponse(BanGuiBaseModel): - """Paginated history ban-list response.""" +class HistoryListResponse(PaginatedListResponse[HistoryBanItem]): + """Paginated history ban-list response. - items: list[HistoryBanItem] = Field(default_factory=list) - total: int = Field(..., ge=0, description="Total matching records.") - page: int = Field(..., ge=1) - page_size: int = Field(..., ge=1) + Request: ``GET /api/history`` with optional time-range, jail, IP, and + origin filters plus pagination parameters. + Response: Paginated collection of historical ban records with geolocation. + """ + + pass # --------------------------------------------------------------------------- # Per-IP timeline diff --git a/backend/app/routers/blocklist.py b/backend/app/routers/blocklist.py index a60faa6..12ee74d 100644 --- a/backend/app/routers/blocklist.py +++ b/backend/app/routers/blocklist.py @@ -46,6 +46,7 @@ from app.models.blocklist import ( ) from app.services import ban_service, blocklist_service from app.tasks.blocklist_import import run_import_with_resources +from app.utils.constants import DEFAULT_PAGE_SIZE router: APIRouter = APIRouter(prefix="/api/blocklists", tags=["Blocklists"]) @@ -221,8 +222,10 @@ async def get_import_log( blocklist_ctx: BlocklistServiceContextDep, _auth: AuthDep, source_id: int | None = Query(default=None, description="Filter by source id"), - page: int = Query(default=1, ge=1), - page_size: int = Query(default=50, ge=1, le=200), + page: int = Query(default=1, ge=1, description="1-based page number."), + page_size: int = Query( + default=DEFAULT_PAGE_SIZE, ge=1, le=500, description="Items per page (max 500)." + ), ) -> ImportLogListResponse: """Return a paginated log of all import runs. diff --git a/backend/app/services/blocklist_service.py b/backend/app/services/blocklist_service.py index abd2097..a976720 100644 --- a/backend/app/services/blocklist_service.py +++ b/backend/app/services/blocklist_service.py @@ -540,14 +540,12 @@ async def list_import_logs( items, total = await import_log_repo.list_logs( db, source_id=source_id, page=page, page_size=page_size ) - total_pages = import_log_repo.compute_total_pages(total, page_size) return ImportLogListResponse( items=[ImportLogEntry.model_validate(i) for i in items], total=total, page=page, page_size=page_size, - total_pages=total_pages, ) diff --git a/backend/app/utils/pagination.py b/backend/app/utils/pagination.py new file mode 100644 index 0000000..7e92c1b --- /dev/null +++ b/backend/app/utils/pagination.py @@ -0,0 +1,102 @@ +"""Pagination utilities and standardized query parameter handling. + +This module provides reusable utilities for implementing consistent pagination +across all endpoints. All paginated endpoints should use these utilities to +ensure a uniform API contract. + +Standard Pagination Contract: + Query parameters: page (1-based), page_size (1-500) + Response: PaginatedListResponse[T] with items, total, page, page_size + +Usage in routers: + ```python + from app.utils.pagination import PAGINATION_DEFAULTS + + @router.get("/items") + async def get_items( + page: int = Query(default=PAGINATION_DEFAULTS["page"], ge=1), + page_size: int = Query( + default=PAGINATION_DEFAULTS["page_size"], + ge=1, + le=PAGINATION_DEFAULTS["max_page_size"], + ), + ): + ... + ``` +""" + +from typing import Final + +__all__ = ["PAGINATION_DEFAULTS", "get_offset", "compute_total_pages"] + +# Standardized pagination defaults +PAGINATION_DEFAULTS: Final[dict[str, int]] = { + "page": 1, + "page_size": 100, + "max_page_size": 500, +} + + +def get_offset(page: int, page_size: int) -> int: + """Calculate the database offset for a given page and page size. + + Args: + page: 1-based page number. + page_size: Items per page. + + Returns: + 0-based database offset (number of items to skip). + + Raises: + ValueError: If page or page_size is invalid (< 1). + + Example: + ```python + # Page 1, size 10 → offset 0 + assert get_offset(1, 10) == 0 + + # Page 2, size 10 → offset 10 + assert get_offset(2, 10) == 10 + + # Page 3, size 50 → offset 100 + assert get_offset(3, 50) == 100 + ``` + """ + if page < 1: + raise ValueError(f"page must be >= 1, got {page}") + if page_size < 1: + raise ValueError(f"page_size must be >= 1, got {page_size}") + + return (page - 1) * page_size + + +def compute_total_pages(total: int, page_size: int) -> int: + """Calculate the total number of pages needed. + + Args: + total: Total number of items across all pages. + page_size: Items per page. + + Returns: + The number of pages required to hold all items. Always at least 1, + even when total is 0 (an empty page is still a page). + + Raises: + ValueError: If page_size is invalid (< 1). + + Example: + ```python + assert compute_total_pages(0, 10) == 1 + assert compute_total_pages(10, 10) == 1 + assert compute_total_pages(11, 10) == 2 + assert compute_total_pages(25, 5) == 5 + ``` + """ + if page_size < 1: + raise ValueError(f"page_size must be >= 1, got {page_size}") + + if total == 0: + return 1 + + # Ceiling division: (total + page_size - 1) // page_size + return (total + page_size - 1) // page_size diff --git a/backend/tests/test_routers/test_blocklist.py b/backend/tests/test_routers/test_blocklist.py index a0e9c94..3008534 100644 --- a/backend/tests/test_routers/test_blocklist.py +++ b/backend/tests/test_routers/test_blocklist.py @@ -85,7 +85,7 @@ def _make_import_result() -> ImportRunResult: def _make_log_response() -> ImportLogListResponse: return ImportLogListResponse( - items=[], total=0, page=1, page_size=50, total_pages=1 + items=[], total=0, page=1, page_size=50 ) @@ -457,10 +457,10 @@ class TestImportLog: assert resp.status_code == 200 async def test_log_response_shape(self, bl_client: AsyncClient) -> None: - """Log response has items, total, page, page_size, total_pages.""" + """Log response has items, total, page, page_size.""" resp = await bl_client.get("/api/blocklists/log") body = resp.json() - for key in ("items", "total", "page", "page_size", "total_pages"): + for key in ("items", "total", "page", "page_size"): assert key in body async def test_log_empty_when_no_runs(self, bl_client: AsyncClient) -> None: diff --git a/backend/tests/test_services/test_blocklist_service.py b/backend/tests/test_services/test_blocklist_service.py index 6f82749..8a1c75d 100644 --- a/backend/tests/test_services/test_blocklist_service.py +++ b/backend/tests/test_services/test_blocklist_service.py @@ -529,7 +529,6 @@ class TestImportLogPagination: assert resp.total == 0 assert resp.page == 1 assert resp.page_size == 10 - assert resp.total_pages == 1 async def test_list_import_logs_paginates(self, db: aiosqlite.Connection) -> None: """list_import_logs computes total pages and returns the correct subset.""" @@ -549,7 +548,6 @@ class TestImportLogPagination: db, source_id=None, page=2, page_size=2 ) assert resp.total == 3 - assert resp.total_pages == 2 assert resp.page == 2 assert resp.page_size == 2 assert len(resp.items) == 1 diff --git a/backend/tests/test_utils/test_pagination.py b/backend/tests/test_utils/test_pagination.py new file mode 100644 index 0000000..0cccc48 --- /dev/null +++ b/backend/tests/test_utils/test_pagination.py @@ -0,0 +1,104 @@ +"""Tests for pagination utilities. + +Validates the pagination helper functions used across all paginated endpoints. +""" + +import pytest + +from app.utils.pagination import get_offset, compute_total_pages, PAGINATION_DEFAULTS + + +class TestPaginationDefaults: + """Test pagination default constants.""" + + def test_pagination_defaults_structure(self) -> None: + """PAGINATION_DEFAULTS contains required keys.""" + required_keys = {"page", "page_size", "max_page_size"} + assert required_keys.issubset(PAGINATION_DEFAULTS.keys()) + + def test_pagination_defaults_values(self) -> None: + """PAGINATION_DEFAULTS have expected values.""" + assert PAGINATION_DEFAULTS["page"] == 1 + assert PAGINATION_DEFAULTS["page_size"] == 100 + assert PAGINATION_DEFAULTS["max_page_size"] == 500 + + +class TestGetOffset: + """Test get_offset calculation.""" + + def test_first_page(self) -> None: + """First page (page=1) produces offset=0.""" + assert get_offset(1, 10) == 0 + + def test_second_page(self) -> None: + """Second page (page=2) produces offset=page_size.""" + assert get_offset(2, 10) == 10 + + def test_arbitrary_page(self) -> None: + """Arbitrary pages produce correct offsets.""" + assert get_offset(3, 50) == 100 + assert get_offset(5, 25) == 100 + assert get_offset(10, 100) == 900 + + def test_page_size_variations(self) -> None: + """Offsets scale correctly with page_size.""" + assert get_offset(2, 1) == 1 + assert get_offset(2, 10) == 10 + assert get_offset(2, 100) == 100 + assert get_offset(2, 500) == 500 + + def test_invalid_page_raises(self) -> None: + """Invalid page numbers raise ValueError.""" + with pytest.raises(ValueError, match="page must be >= 1"): + get_offset(0, 10) + + with pytest.raises(ValueError, match="page must be >= 1"): + get_offset(-1, 10) + + def test_invalid_page_size_raises(self) -> None: + """Invalid page sizes raise ValueError.""" + with pytest.raises(ValueError, match="page_size must be >= 1"): + get_offset(1, 0) + + with pytest.raises(ValueError, match="page_size must be >= 1"): + get_offset(1, -1) + + +class TestComputeTotalPages: + """Test compute_total_pages calculation.""" + + def test_empty_collection(self) -> None: + """Empty collection (total=0) produces at least 1 page.""" + assert compute_total_pages(0, 10) == 1 + + def test_exact_page_fit(self) -> None: + """Totals that fit exactly produce expected page count.""" + assert compute_total_pages(10, 10) == 1 + assert compute_total_pages(20, 10) == 2 + assert compute_total_pages(50, 10) == 5 + + def test_partial_page(self) -> None: + """Totals that overflow a page produce ceil(total / page_size).""" + assert compute_total_pages(11, 10) == 2 + assert compute_total_pages(25, 5) == 5 + assert compute_total_pages(101, 10) == 11 + + def test_single_item(self) -> None: + """Single item produces 1 page regardless of page_size.""" + assert compute_total_pages(1, 1) == 1 + assert compute_total_pages(1, 10) == 1 + assert compute_total_pages(1, 500) == 1 + + def test_large_page_size(self) -> None: + """Large page sizes still compute correctly.""" + assert compute_total_pages(1, 500) == 1 + assert compute_total_pages(501, 500) == 2 + assert compute_total_pages(1000, 500) == 2 + + def test_invalid_page_size_raises(self) -> None: + """Invalid page sizes raise ValueError.""" + with pytest.raises(ValueError, match="page_size must be >= 1"): + compute_total_pages(100, 0) + + with pytest.raises(ValueError, match="page_size must be >= 1"): + compute_total_pages(100, -1) diff --git a/frontend/src/types/blocklist.ts b/frontend/src/types/blocklist.ts index c240ef8..b789f8a 100644 --- a/frontend/src/types/blocklist.ts +++ b/frontend/src/types/blocklist.ts @@ -2,6 +2,8 @@ * TypeScript types for the blocklist management feature. */ +import type { PaginatedListResponse } from "./response"; + // --------------------------------------------------------------------------- // Sources // --------------------------------------------------------------------------- @@ -45,13 +47,7 @@ export interface ImportLogEntry { errors: string | null; } -export interface ImportLogListResponse { - items: ImportLogEntry[]; - total: number; - page: number; - page_size: number; - total_pages: number; -} +export interface ImportLogListResponse extends PaginatedListResponse {} // --------------------------------------------------------------------------- // Schedule diff --git a/frontend/src/types/history.ts b/frontend/src/types/history.ts index 405a1a4..7521801 100644 --- a/frontend/src/types/history.ts +++ b/frontend/src/types/history.ts @@ -2,6 +2,7 @@ * TypeScript types for the ban history API. */ +import type { PaginatedListResponse } from "./response"; import type { TimeRange } from "./ban"; /** Optional time-range filter for history queries. */ @@ -22,12 +23,7 @@ export interface HistoryBanItem { } /** Paginated response from GET /api/history */ -export interface HistoryListResponse { - items: HistoryBanItem[]; - total: number; - page: number; - page_size: number; -} +export interface HistoryListResponse extends PaginatedListResponse {} /** A single ban event in a per-IP timeline. */ export interface IpTimelineEvent {