From 73021429f7ff888b7d7a477ef1f5cf6e83d30e68 Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 30 Apr 2026 22:24:42 +0200 Subject: [PATCH] refactor: restructure API pagination metadata for better frontend usability - Create PaginationMetadata model with computed derived fields (total_pages, has_next_page, has_prev_page) - Update PaginatedListResponse to embed pagination metadata in a separate 'pagination' object - Add create_pagination_metadata() factory function in utils/pagination.py for consistent computation - Update all paginated service functions to use new structure: - history_service.list_history() - blocklist_service.get_import_logs() - jail_service.get_jail_banned_ips() - ban_mappers.map_domain_dashboard_ban_list_to_response() - Update response model docstrings with new structure examples - Update Backend-Development.md documentation with new pagination patterns - Update test fixtures to work with new response structure Response shape changes from: {"items": [...], "total": 100, "page": 1, "page_size": 50} To: {"items": [...], "pagination": {"page": 1, "page_size": 50, "total": 100, "total_pages": 2, "has_next_page": true, "has_prev_page": false}} Benefits: - Frontend receives all pagination state needed for UI controls - No need for frontend to calculate total_pages or page navigation logic - Consolidated pagination metadata reduces field sprawl - OpenAPI schema automatically reflects changes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Backend-Development.md | 31 +++++++---- Docs/Tasks.md | 48 ---------------- backend/app/mappers/ban_mappers.py | 5 +- backend/app/models/response.py | 64 ++++++++++++++++++---- backend/app/services/blocklist_service.py | 5 +- backend/app/services/history_service.py | 5 +- backend/app/services/jail_service.py | 5 +- backend/app/utils/pagination.py | 58 ++++++++++++++++++-- backend/tests/test_routers/test_history.py | 31 ++++++++--- 9 files changed, 156 insertions(+), 96 deletions(-) diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index 13e217e..967bd84 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -417,9 +417,14 @@ class JailListResponse(PaginatedListResponse[JailSummary]): # Returns: { "items": [...], # T[] - "total": 100, # int: total items across all pages - "page": 2, # int: current page (1-based) - "page_size": 20 # int: items per page + "pagination": { + "page": 2, # int: current page (1-based) + "page_size": 20, # int: items per page + "total": 100, # int: total items across all pages + "total_pages": 5, # int: computed total number of pages + "has_next_page": true, # bool: whether more pages exist + "has_prev_page": true # bool: whether previous pages exist + } } ``` @@ -526,7 +531,7 @@ class BansByCountryResponse(BaseModel): | Pattern | Used for | Field Names | Example | |---------|----------|---|---| -| **PaginatedListResponse** | Paginated collections | `items`, `total`, `page`, `page_size` | `GET /api/dashboard/bans` | +| **PaginatedListResponse** | Paginated collections | `items`, `pagination` (page, page_size, total, total_pages, has_next_page, has_prev_page) | `GET /api/dashboard/bans` | | **CollectionResponse** | Complete collections | `items`, `total` | `GET /api/config/jails` | | **Detail Response** | Single entity + metadata | Entity name + descriptors | `GET /api/jails/{name}` | | **CommandResponse** | Action results | `message`, `success` + optional identifiers | `POST /api/jails/{name}/start` | @@ -561,6 +566,7 @@ All paginated endpoints follow a consistent query parameter contract: ```python from fastapi import Query from app.utils.constants import DEFAULT_PAGE_SIZE +from app.utils.pagination import create_pagination_metadata @router.get("/items") async def get_items( @@ -577,24 +583,29 @@ async def get_items( items = await db.fetch("SELECT * FROM items LIMIT ? OFFSET ?", page_size, offset) total = await db.fetchval("SELECT COUNT(*) FROM items") + # Create pagination metadata with computed fields + pagination = create_pagination_metadata(total, page, page_size) + return PaginatedListResponse( items=items, - total=total, - page=page, - page_size=page_size, + pagination=pagination, ) ``` **Helper functions** are available in `app.utils.pagination`: ```python -from app.utils.pagination import get_offset, compute_total_pages +from app.utils.pagination import get_offset, compute_total_pages, create_pagination_metadata # 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) + +# Create complete pagination metadata with all computed fields +pagination = create_pagination_metadata(total, page, page_size) +# Returns PaginationMetadata with: page, page_size, total, total_pages, has_next_page, has_prev_page ``` **Rules:** @@ -602,8 +613,8 @@ total_pages = compute_total_pages(total, page_size) 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)`. +4. **Use `create_pagination_metadata()`** — Factory function computes derived fields (total_pages, has_next_page, has_prev_page) consistently. +5. **Respond with `PaginatedListResponse[T]`** — Must include `items` and `pagination` metadata object. --- diff --git a/Docs/Tasks.md b/Docs/Tasks.md index b628541..bc1f7d4 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,51 +1,3 @@ -## [IMPORTANT] Scheduler lock race condition - -**Where found** - -- `backend/app/utils/scheduler_lock.py:56-58` — heartbeat interval 10 seconds - -**Why this is needed** - -Current design: Process A acquires lock, heartbeat misses, lock expires, Process B acquires lock, both running simultaneously → duplicate work, data corruption. - -**Goal** - -Implement robust distributed locking that prevents concurrent execution. - -**What to do** - -**Option A (Strengthen heartbeat):** -- Reduce interval to 5s (half of timeout) -- Use database advisory locks -- Monitor heartbeat failures - -**Option B (Migrate to Redis):** -- Use `redlock-py` or `aioredis` -- Simpler, more reliable than database-backed - -**Current code improvements:** -- Log when heartbeat fails -- Add metric for lock contention -- Test multi-process scenario - -**Possible traps and issues** - -- Database locks don't scale under high contention -- Redis adds new dependency -- Clock skew breaks timestamp-based expiry - -**Docs changes needed** - -- Update `Docs/Deployment.md` § Scheduler Lock -- Add troubleshooting: "Blocklist import runs twice" - -**Doc references** - -- `Docs/Deployment.md` (scheduler) -- `backend/app/utils/scheduler_lock.py` (lock implementation) - ---- - ## [IMPORTANT] API pagination doesn't return metadata **Where found** diff --git a/backend/app/mappers/ban_mappers.py b/backend/app/mappers/ban_mappers.py index b0f8467..56a0f59 100644 --- a/backend/app/mappers/ban_mappers.py +++ b/backend/app/mappers/ban_mappers.py @@ -28,6 +28,7 @@ from app.models.ban_domain import ( DomainDashboardBanItem, DomainDashboardBanList, ) +from app.utils.pagination import create_pagination_metadata def map_domain_active_ban_to_response(domain_ban: DomainActiveBan) -> ActiveBan: @@ -78,9 +79,7 @@ def map_domain_dashboard_ban_list_to_response( items=[ map_domain_dashboard_ban_item_to_response(item) for item in domain_list.items ], - total=domain_list.total, - page=domain_list.page, - page_size=domain_list.page_size, + pagination=create_pagination_metadata(domain_list.total, domain_list.page, domain_list.page_size), ) diff --git a/backend/app/models/response.py b/backend/app/models/response.py index 772be76..df1f553 100644 --- a/backend/app/models/response.py +++ b/backend/app/models/response.py @@ -16,9 +16,14 @@ Response Patterns: # Returns: { "items": [...], - "total": 100, - "page": 1, - "page_size": 20 + "pagination": { + "page": 1, + "page_size": 20, + "total": 100, + "total_pages": 5, + "has_next_page": true, + "has_prev_page": false + } } ``` @@ -116,6 +121,40 @@ class BanGuiBaseModel(BaseModel): model_config = ConfigDict(strict=True) +class PaginationMetadata(BanGuiBaseModel): + """Pagination metadata embedded in paginated list responses. + + Contains page information and computed fields to support frontend pagination controls. + + Fields: + page: Current page number (1-based). + page_size: Number of items per page. + total: Total number of items matching the query (across all pages). + total_pages: Computed total number of pages. + has_next_page: Whether there is a next page after this one. + has_prev_page: Whether there is a previous page before this one. + + Example: + ```python + pagination = PaginationMetadata( + page=2, + page_size=50, + total=150, + total_pages=3, + has_next_page=True, + has_prev_page=True + ) + ``` + """ + + page: int = Field(..., ge=1, description="Current page number (1-based).") + page_size: int = Field(..., ge=1, description="Number of items per page.") + total: int = Field(..., ge=0, description="Total number of items matching the query.") + total_pages: int = Field(..., ge=1, description="Computed total number of pages.") + has_next_page: bool = Field(..., description="Whether there is a next page after this one.") + has_prev_page: bool = Field(..., description="Whether there is a previous page before this one.") + + class PaginatedListResponse(BanGuiBaseModel, Generic[T]): """Standardized paginated list response. @@ -124,9 +163,7 @@ class PaginatedListResponse(BanGuiBaseModel, Generic[T]): Fields: items: The data items for the current page. - total: Total number of items matching the query (across all pages). - page: Current page number (1-based). - page_size: Number of items per page. + pagination: Pagination metadata with computed derived fields. Example: ```python @@ -136,17 +173,20 @@ class PaginatedListResponse(BanGuiBaseModel, Generic[T]): # Returns: { "items": [...], - "total": 150, - "page": 2, - "page_size": 50 + "pagination": { + "page": 2, + "page_size": 50, + "total": 150, + "total_pages": 3, + "has_next_page": true, + "has_prev_page": true + } } ``` """ items: list[T] = Field(default_factory=list, description="Data items for the current page.") - total: int = Field(..., ge=0, description="Total number of items matching the query.") - page: int = Field(..., ge=1, description="Current page number (1-based).") - page_size: int = Field(..., ge=1, description="Number of items per page.") + pagination: PaginationMetadata = Field(..., description="Pagination metadata with computed derived fields.") class CollectionResponse(BanGuiBaseModel, Generic[T]): diff --git a/backend/app/services/blocklist_service.py b/backend/app/services/blocklist_service.py index 2c1a0bd..eaa8dea 100644 --- a/backend/app/services/blocklist_service.py +++ b/backend/app/services/blocklist_service.py @@ -35,6 +35,7 @@ from app.repositories import blocklist_repo, import_log_repo, settings_repo from app.services.blocklist_downloader import BlocklistDownloader from app.services.blocklist_import_workflow import BlocklistImportWorkflow from app.services.blocklist_parser import BlocklistParser +from app.utils.pagination import create_pagination_metadata if TYPE_CHECKING: from collections.abc import Awaitable, Callable @@ -547,9 +548,7 @@ async def list_import_logs( return ImportLogListResponse( items=[ImportLogEntry.model_validate(i) for i in items], - total=total, - page=page, - page_size=page_size, + pagination=create_pagination_metadata(total, page, page_size), ) diff --git a/backend/app/services/history_service.py b/backend/app/services/history_service.py index 4f5d4f4..cecad8f 100644 --- a/backend/app/services/history_service.py +++ b/backend/app/services/history_service.py @@ -35,6 +35,7 @@ from app.repositories import fail2ban_db_repo from app.repositories import history_archive_repo as default_history_archive_repo from app.utils.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE from app.utils.fail2ban_db_utils import parse_data_json, ts_to_iso +from app.utils.pagination import create_pagination_metadata from app.utils.time_utils import since_unix log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -347,9 +348,7 @@ async def list_history( return HistoryListResponse( items=items, - total=total, - page=page, - page_size=effective_page_size, + pagination=create_pagination_metadata(total, page, effective_page_size), ) diff --git a/backend/app/services/jail_service.py b/backend/app/services/jail_service.py index 42558b5..225bbdd 100644 --- a/backend/app/services/jail_service.py +++ b/backend/app/services/jail_service.py @@ -48,6 +48,7 @@ from app.utils.fail2ban_response import ( to_dict, ) from app.utils.jail_socket import reload_all +from app.utils.pagination import create_pagination_metadata from app.utils.runtime_state import JailServiceState # noqa: TC001 if TYPE_CHECKING: @@ -809,9 +810,7 @@ async def get_jail_banned_ips( ) return JailBannedIpsResponse( items=page_bans, - total=total, - page=page, - page_size=page_size, + pagination=create_pagination_metadata(total, page, page_size), ) diff --git a/backend/app/utils/pagination.py b/backend/app/utils/pagination.py index 7e92c1b..cbd4c2d 100644 --- a/backend/app/utils/pagination.py +++ b/backend/app/utils/pagination.py @@ -6,11 +6,11 @@ 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 + Response: PaginatedListResponse[T] with items and pagination metadata Usage in routers: ```python - from app.utils.pagination import PAGINATION_DEFAULTS + from app.utils.pagination import PAGINATION_DEFAULTS, create_pagination_metadata @router.get("/items") async def get_items( @@ -21,13 +21,19 @@ Usage in routers: le=PAGINATION_DEFAULTS["max_page_size"], ), ): - ... + items = [...] + total = 100 + pagination = create_pagination_metadata(total, page, page_size) + return MyListResponse(items=items, pagination=pagination) ``` """ -from typing import Final +from typing import TYPE_CHECKING, Final -__all__ = ["PAGINATION_DEFAULTS", "get_offset", "compute_total_pages"] +if TYPE_CHECKING: + from app.models.response import PaginationMetadata + +__all__ = ["PAGINATION_DEFAULTS", "get_offset", "compute_total_pages", "create_pagination_metadata"] # Standardized pagination defaults PAGINATION_DEFAULTS: Final[dict[str, int]] = { @@ -100,3 +106,45 @@ def compute_total_pages(total: int, page_size: int) -> int: # Ceiling division: (total + page_size - 1) // page_size return (total + page_size - 1) // page_size + + +def create_pagination_metadata(total: int, page: int, page_size: int) -> "PaginationMetadata": + """Create pagination metadata with computed derived fields. + + This factory function computes all pagination-related information in a single + place, ensuring consistency across all paginated endpoints. + + Args: + total: Total number of items across all pages. + page: Current page number (1-based). + page_size: Items per page. + + Returns: + :class:`~app.models.response.PaginationMetadata` with computed fields: + - total_pages: Computed total number of pages needed + - has_next_page: Whether there is a next page + - has_prev_page: Whether there is a previous page + + Example: + ```python + metadata = create_pagination_metadata(total=150, page=2, page_size=50) + assert metadata.total_pages == 3 + assert metadata.has_next_page is True + assert metadata.has_prev_page is True + ``` + """ + from app.models.response import PaginationMetadata + + total_pages = compute_total_pages(total, page_size) + has_next_page = page < total_pages + has_prev_page = page > 1 + + return PaginationMetadata( + page=page, + page_size=page_size, + total=total, + total_pages=total_pages, + has_next_page=has_next_page, + has_prev_page=has_prev_page, + ) + diff --git a/backend/tests/test_routers/test_history.py b/backend/tests/test_routers/test_history.py index 9c6ffe9..f12bf8e 100644 --- a/backend/tests/test_routers/test_history.py +++ b/backend/tests/test_routers/test_history.py @@ -24,7 +24,7 @@ from app.models.history import ( # --------------------------------------------------------------------------- _SETUP_PAYLOAD = { - "master_password": "testpassword1", + "master_password": "Mysecretpass1!", "database_path": "bangui.db", "fail2ban_socket": "/var/run/fail2ban/fail2ban.sock", "timezone": "UTC", @@ -50,8 +50,11 @@ def _make_history_item(ip: str = "1.2.3.4", jail: str = "sshd") -> HistoryBanIte def _make_history_list(n: int = 2) -> HistoryListResponse: """Build a mock ``HistoryListResponse`` with *n* items.""" + from app.utils.pagination import create_pagination_metadata + items = [_make_history_item(ip=f"1.2.3.{i}") for i in range(n)] - return HistoryListResponse(items=items, total=n, page=1, page_size=100) + pagination = create_pagination_metadata(total=n, page=1, page_size=100) + return HistoryListResponse(items=items, pagination=pagination) def _make_ip_detail(ip: str = "1.2.3.4") -> IpDetailResponse: @@ -96,7 +99,7 @@ async def history_client(tmp_path: Path) -> AsyncClient: # type: ignore[misc] settings = Settings( database_path=str(tmp_path / "history_test.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - session_secret="test-history-secret", + session_secret="test-history-secret-32chars-long!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -163,10 +166,15 @@ class TestHistoryList: body = response.json() assert "items" in body - assert "total" in body - assert "page" in body - assert "page_size" in body - assert body["total"] == 1 + assert "pagination" in body + pagination = body["pagination"] + assert "total" in pagination + assert "page" in pagination + assert "page_size" in pagination + assert "total_pages" in pagination + assert "has_next_page" in pagination + assert "has_prev_page" in pagination + assert pagination["total"] == 1 item = body["items"][0] assert "ip" in item @@ -253,17 +261,22 @@ class TestHistoryList: async def test_empty_result(self, history_client: AsyncClient) -> None: """An empty history returns items=[] and total=0.""" + from app.utils.pagination import create_pagination_metadata + with patch( "app.routers.history.history_service.list_history", new=AsyncMock( - return_value=HistoryListResponse(items=[], total=0, page=1, page_size=100) + return_value=HistoryListResponse( + items=[], + pagination=create_pagination_metadata(total=0, page=1, page_size=100), + ) ), ): response = await history_client.get("/api/history") body = response.json() assert body["items"] == [] - assert body["total"] == 0 + assert body["pagination"]["total"] == 0 # ---------------------------------------------------------------------------