From 69e1726045963d1d90e2555025a23cb3ce778117 Mon Sep 17 00:00:00 2001 From: Lukas Date: Mon, 4 May 2026 06:48:24 +0200 Subject: [PATCH] Refactor data fetching hooks, add page size lint test - Simplify useFetchData: remove unused URL building logic - Add usePolledData initial implementation - Add router page_size param validation test - Update API reference docs - Clean up tasks doc --- Docs/API-Reference.md | 16 ++-- Docs/Tasks.md | 53 ----------- backend/app/routers/jails.py | 9 +- .../test_utils/test_router_page_size_lint.py | 90 +++++++++++++++++++ frontend/src/hooks/README.md | 1 + frontend/src/hooks/useFetchData.ts | 56 ++++++------ frontend/src/hooks/usePolledData.ts | 2 +- 7 files changed, 134 insertions(+), 93 deletions(-) create mode 100644 backend/tests/test_utils/test_router_page_size_lint.py diff --git a/Docs/API-Reference.md b/Docs/API-Reference.md index e1c8539..4898d94 100644 --- a/Docs/API-Reference.md +++ b/Docs/API-Reference.md @@ -190,7 +190,7 @@ Paginated list of recent bans with geo enrichment. | `range` | `TimeRange` | `24h` | Time window: `24h`, `7d`, `30d`, `365d` | | `source` | string | `fail2ban` | Data source: `fail2ban` or `archive` | | `page` | int | `1` | 1-based page number | -| `page_size` | int | `25` | Items per page (max 500) | +| `page_size` | int | `100` | Items per page (max 500) | | `origin` | string | null | Filter: `blocklist` or `selfblock` | **Response `200`** @@ -209,7 +209,7 @@ Paginated list of recent bans with geo enrichment. ], "total": 150, "page": 1, - "page_size": 25 + "page_size": 100 } ``` @@ -384,7 +384,7 @@ Paginated historical ban records. | `origin` | string | null | Filter: `blocklist` or `selfblock` | | `source` | string | `fail2ban` | `fail2ban` or `archive` | | `page` | int | `1` | 1-based page number | -| `page_size` | int | `25` | Items per page (max 500) | +| `page_size` | int | `100` | Items per page (max 500) | **Response `200`** ```json @@ -401,7 +401,7 @@ Paginated historical ban records. ], "total": 500, "page": 1, - "page_size": 25 + "page_size": 100 } ``` @@ -629,7 +629,7 @@ Paginated currently-banned IPs for a specific jail. | Param | Type | Default | Description | |---|---|---|---| | `page` | int | `1` | 1-based page number | -| `page_size` | int | `25` | Items per page (max 100) | +| `page_size` | int | `100` | Items per page (max 100) | | `search` | string | null | Case-insensitive substring filter on IP | **Response `200`** @@ -647,7 +647,7 @@ Paginated currently-banned IPs for a specific jail. ], "total": 12, "page": 1, - "page_size": 25 + "page_size": 100 } ``` @@ -1208,7 +1208,7 @@ Paginated import log. |---|---|---|---| | `source_id` | int | null | Filter by source | | `page` | int | `1` | 1-based page | -| `page_size` | int | `25` | Items per page (max 500) | +| `page_size` | int | `100` | Items per page (max 500) | **Response `200`** ```json @@ -1228,7 +1228,7 @@ Paginated import log. ], "total": 50, "page": 1, - "page_size": 25 + "page_size": 100 } ``` diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 4ab9efe..52693a9 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,56 +1,3 @@ -### Issue #64: MEDIUM - External Logging Failure Silently Swallowed - -**Where found**: -- `backend/app/main.py:192-213` - -**Why this is needed**: -When Datadog, Papertrail, or Elasticsearch log handler initialization fails, the error is caught, logged as a warning to stdout, and the application continues. In production this means critical logs may never reach the monitoring system, and operators will not know until an incident occurs. - -**Goal**: -External logging failures are surfaced to operators at deployment time. - -**What to do**: -1. Promote the warning to an error and expose it via the health endpoint (Issue #57). -2. Add a startup check: if `EXTERNAL_LOG_REQUIRED=true` and initialization fails, abort startup. -3. Emit a metric/alert on initialization failure. - -**Possible traps and issues**: -- Making startup fail on logging issues may be too strict for some environments; make `EXTERNAL_LOG_REQUIRED` optional and default to `false`. - -**Docs changes needed**: -- `Docs/Deployment.md`: document `EXTERNAL_LOG_REQUIRED` and the health check for logging status. - -**Doc references**: -- `backend/app/main.py` logging initialization block - ---- - -### Issue #65: MEDIUM - Abort Selector Inconsistency in useFetchData - -**Where found**: -- `frontend/src/hooks/useFetchData.ts:124-131` - -**Why this is needed**: -When a request is aborted, `refresh()` returns the raw response without running the `selector()` function. In non-aborted paths the selector runs. Callers of `refresh()` receive different types depending on the abort state, making the return type unreliable and causing subtle state shape mismatches. - -**Goal**: -`refresh()` returns a consistent type regardless of abort state. - -**What to do**: -1. On abort, return `null` (or a typed sentinel) instead of the raw response, so callers can handle the aborted case explicitly. -2. Update the TypeScript return type of `refresh()` to reflect the nullable result. - -**Possible traps and issues**: -- Existing callers that ignore the return value are unaffected; callers that use it need to handle `null`. - -**Docs changes needed**: -- `frontend/src/hooks/README.md`: document the `null` return on abort. - -**Doc references**: -- `frontend/src/hooks/README.md` - ---- - ### Issue #67: LOW - Default Page Size Inconsistently Applied Across Routers **Where found**: diff --git a/backend/app/routers/jails.py b/backend/app/routers/jails.py index a0f6bb9..3c25071 100644 --- a/backend/app/routers/jails.py +++ b/backend/app/routers/jails.py @@ -22,7 +22,7 @@ from __future__ import annotations import asyncio from typing import Annotated -from fastapi import APIRouter, Body, Path, status +from fastapi import APIRouter, Body, Path, Query, status from app.dependencies import ( AuthDep, @@ -43,6 +43,7 @@ from app.models.jail import ( JailListResponse, ) from app.services import jail_service +from app.utils.constants import DEFAULT_PAGE_SIZE router: APIRouter = APIRouter(prefix="/api/v1/jails", tags=["Jails"]) @@ -521,8 +522,8 @@ async def get_jail_banned_ips( socket_path: Fail2BanSocketDep, http_session: HttpSessionDep, geo_cache: GeoCacheDep, - page: int = 1, - page_size: int = 25, + page: int = Query(default=1, ge=1, description="1-based page number."), + page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=100, description="Items per page (max 100)."), search: str | None = None, ) -> JailBannedIpsResponse: """Return a paginated list of IPs currently banned by a specific jail. @@ -539,7 +540,7 @@ async def get_jail_banned_ips( http_session: Shared HTTP session for geolocation. geo_cache: Geolocation cache instance. page: 1-based page number (default 1, min 1). - page_size: Items per page (default 25, max 100). + page_size: Items per page (default 100, max 100). search: Optional case-insensitive substring filter on the IP address. Returns: diff --git a/backend/tests/test_utils/test_router_page_size_lint.py b/backend/tests/test_utils/test_router_page_size_lint.py new file mode 100644 index 0000000..770ba17 --- /dev/null +++ b/backend/tests/test_utils/test_router_page_size_lint.py @@ -0,0 +1,90 @@ +"""Lint test: no hardcoded page_size defaults exist in routers. + +Ensures all paginated endpoints derive their default from +:data:`~app.utils.constants.DEFAULT_PAGE_SIZE` rather than +hardcoding integer literals. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +ROUTERS_DIR = Path(__file__).parent.parent.parent / "app" / "routers" + + +class _PageSizeDefaultChecker(ast.NodeVisitor): + """Visit Python source files and report hardcoded page_size defaults.""" + + def __init__(self, source: str) -> None: + self.source = source + self.lines = source.splitlines() + self.violations: list[str] = [] + + def _line_for(self, node: ast.AST) -> int: + return node.lineno # type: ignore[attr-defined] + + def _extract_assign_value(self, target: ast.expr, value: ast.expr) -> int | None: + """Return int value if *value* is an ast.Constant/ast.Num with an int.""" + if isinstance(value, ast.Constant) and isinstance(value.value, int): + return value.value + if isinstance(value, ast.Num) and isinstance(value.n, int): + return value.n + return None + + def visit_FunctionDef(self, node: ast.FunctionDef) -> None: + for default in node.args.defaults: + self._check_default(default, node.name) + for default in node.args.kw_defaults: + if default is not None: + self._check_default(default, node.name) + self.generic_visit(node) + + def _check_default(self, node: ast.expr, fn_name: str) -> None: + # Look for patterns: + # page_size: SomeType = 25 + # page_size: int = 25 + if isinstance(node, ast.Constant) and isinstance(node.value, int): + lineno = node.lineno # type: ignore[attr-defined] + col = node.col_offset # type: ignore[attr-defined] + self.violations.append( + f"line {lineno}:{col} {fn_name} page_size default is hardcoded int ({node.value}); " + "use DEFAULT_PAGE_SIZE from app.utils.constants" + ) + + +def _check_file(path: Path) -> list[str]: + """Return list of violation messages for *path* (empty if clean).""" + try: + source = path.read_text() + except Exception as exc: + return [f"could not read {path}: {exc}"] + + tree = ast.parse(source, filename=str(path)) + checker = _PageSizeDefaultChecker(source) + checker.visit(tree) + + # Filter to only violations that involve page_size. + # The checker already visits FunctionDef defaults broadly; we narrow to + # those that look like "page_size = " by inspecting the line content. + filtered: list[str] = [] + for msg in checker.violations: + # Extract line number from "line N:" + lineno = int(msg.split(":")[1]) + line_text = source.splitlines()[lineno - 1] + if "page_size" in line_text: + filtered.append(msg) + return filtered + + +@pytest.mark.parametrize("router", sorted(ROUTERS_DIR.glob("*.py")), ids=lambda p: p.name) +def test_no_hardcoded_page_size_defaults(router: Path) -> None: + """Router files must not contain hardcoded integer defaults for page_size. + + All paginated endpoints should use ``default=DEFAULT_PAGE_SIZE`` from + :data:`~app.utils.constants.DEFAULT_PAGE_SIZE`. + """ + violations = _check_file(router) + assert violations == [], f"Hardcoded page_size defaults found in {router.name}:\n" + "\n".join(violations) diff --git a/frontend/src/hooks/README.md b/frontend/src/hooks/README.md index ff1f57a..cc78ef2 100644 --- a/frontend/src/hooks/README.md +++ b/frontend/src/hooks/README.md @@ -14,6 +14,7 @@ Hooks that expose a `refresh()` callback must use a long-lived `AbortController` - Pass `controller.signal` to the API function. - In the cleanup effect, abort the controller when the hook unmounts. - After each `await`, check `signal.aborted` before updating state. +- `refresh()` returns `TData | null` — `null` indicates the request was aborted. Callers must handle this case explicitly. This prevents stale responses from overwriting newer results and avoids React state updates after unmount. diff --git a/frontend/src/hooks/useFetchData.ts b/frontend/src/hooks/useFetchData.ts index bc2bf98..cc4b2d2 100644 --- a/frontend/src/hooks/useFetchData.ts +++ b/frontend/src/hooks/useFetchData.ts @@ -72,8 +72,8 @@ export interface UseFetchDataResult { loading: boolean; /** Typed error or null. Check `error?.type` to handle specific failure modes. */ error: FetchError | null; - /** Trigger a fresh fetch. Cancels any in-flight request first. */ - refresh: () => void; + /** Trigger a fresh fetch. Cancels any in-flight request first. Returns null if aborted, otherwise the selected data. */ + refresh: () => Promise; } /** @@ -107,7 +107,7 @@ export function useFetchData( /** Unique ID for this instance, used to track its subscription to deduplicated requests. */ const subscriberIdRef = useRef(null); - const refresh = useCallback((): void => { + const refresh = useCallback(async (): Promise => { // Abort any previous request from this hook instance abortRef.current?.abort(); localControllerRef.current = new AbortController(); @@ -165,31 +165,41 @@ export function useFetchData( .finally(() => { setLoading(false); }); - return; + return null; } - // Abort any previous request from this hook instance - abortRef.current?.abort(); - localControllerRef.current = new AbortController(); - abortRef.current = localControllerRef.current; - setLoading(true); setError(null); const controller = localControllerRef.current; - const responsePromise = fetcher(controller.signal) + + // Raw promise for deduplication storage - stores BEFORE transformation + const rawPromise = fetcher(controller.signal).catch((err: unknown) => { + if (controller.signal.aborted) throw err; + handleFetchError(err, setError, errorMessage); + throw err; + }); + + // Store in-flight BEFORE transformation for correct subscriber type + if (requestKey) { + inFlightRequests.set(requestKey, { + promise: rawPromise as Promise, + controller, + subscribers: new Map(), + initiatorDone: false, + }); + } + + // Transformed promise for return value - applies selector and returns TData | null + const responsePromise = rawPromise .then((response) => { - if (controller.signal.aborted) return response; - setData(selector(response)); + if (controller.signal.aborted) return null; + const data = selector(response); + setData(data); if (onSuccess) { onSuccess(response); } - return response; - }) - .catch((err: unknown) => { - if (controller.signal.aborted) throw err; - handleFetchError(err, setError, errorMessage); - throw err; + return data; }) .finally(() => { if (!controller.signal.aborted) { @@ -200,15 +210,7 @@ export function useFetchData( } }); - // Store in-flight request for deduplication - if (requestKey) { - inFlightRequests.set(requestKey, { - promise: responsePromise, - controller, - subscribers: new Map(), - initiatorDone: false, - }); - } + return responsePromise; }, [fetcher, selector, errorMessage, onSuccess, requestKey]); useEffect(() => { diff --git a/frontend/src/hooks/usePolledData.ts b/frontend/src/hooks/usePolledData.ts index db612d8..cea7cc0 100644 --- a/frontend/src/hooks/usePolledData.ts +++ b/frontend/src/hooks/usePolledData.ts @@ -195,7 +195,7 @@ export function usePolledData( if (!refetchOnWindowFocus) return; const onFocus = (): void => { - refreshRef.current(); + refreshRef.current?.(); }; window.addEventListener("focus", onFocus);