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
This commit is contained in:
@@ -190,7 +190,7 @@ Paginated list of recent bans with geo enrichment.
|
|||||||
| `range` | `TimeRange` | `24h` | Time window: `24h`, `7d`, `30d`, `365d` |
|
| `range` | `TimeRange` | `24h` | Time window: `24h`, `7d`, `30d`, `365d` |
|
||||||
| `source` | string | `fail2ban` | Data source: `fail2ban` or `archive` |
|
| `source` | string | `fail2ban` | Data source: `fail2ban` or `archive` |
|
||||||
| `page` | int | `1` | 1-based page number |
|
| `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` |
|
| `origin` | string | null | Filter: `blocklist` or `selfblock` |
|
||||||
|
|
||||||
**Response `200`**
|
**Response `200`**
|
||||||
@@ -209,7 +209,7 @@ Paginated list of recent bans with geo enrichment.
|
|||||||
],
|
],
|
||||||
"total": 150,
|
"total": 150,
|
||||||
"page": 1,
|
"page": 1,
|
||||||
"page_size": 25
|
"page_size": 100
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
@@ -384,7 +384,7 @@ Paginated historical ban records.
|
|||||||
| `origin` | string | null | Filter: `blocklist` or `selfblock` |
|
| `origin` | string | null | Filter: `blocklist` or `selfblock` |
|
||||||
| `source` | string | `fail2ban` | `fail2ban` or `archive` |
|
| `source` | string | `fail2ban` | `fail2ban` or `archive` |
|
||||||
| `page` | int | `1` | 1-based page number |
|
| `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`**
|
**Response `200`**
|
||||||
```json
|
```json
|
||||||
@@ -401,7 +401,7 @@ Paginated historical ban records.
|
|||||||
],
|
],
|
||||||
"total": 500,
|
"total": 500,
|
||||||
"page": 1,
|
"page": 1,
|
||||||
"page_size": 25
|
"page_size": 100
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
@@ -629,7 +629,7 @@ Paginated currently-banned IPs for a specific jail.
|
|||||||
| Param | Type | Default | Description |
|
| Param | Type | Default | Description |
|
||||||
|---|---|---|---|
|
|---|---|---|---|
|
||||||
| `page` | int | `1` | 1-based page number |
|
| `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 |
|
| `search` | string | null | Case-insensitive substring filter on IP |
|
||||||
|
|
||||||
**Response `200`**
|
**Response `200`**
|
||||||
@@ -647,7 +647,7 @@ Paginated currently-banned IPs for a specific jail.
|
|||||||
],
|
],
|
||||||
"total": 12,
|
"total": 12,
|
||||||
"page": 1,
|
"page": 1,
|
||||||
"page_size": 25
|
"page_size": 100
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
@@ -1208,7 +1208,7 @@ Paginated import log.
|
|||||||
|---|---|---|---|
|
|---|---|---|---|
|
||||||
| `source_id` | int | null | Filter by source |
|
| `source_id` | int | null | Filter by source |
|
||||||
| `page` | int | `1` | 1-based page |
|
| `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`**
|
**Response `200`**
|
||||||
```json
|
```json
|
||||||
@@ -1228,7 +1228,7 @@ Paginated import log.
|
|||||||
],
|
],
|
||||||
"total": 50,
|
"total": 50,
|
||||||
"page": 1,
|
"page": 1,
|
||||||
"page_size": 25
|
"page_size": 100
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
### Issue #67: LOW - Default Page Size Inconsistently Applied Across Routers
|
||||||
|
|
||||||
**Where found**:
|
**Where found**:
|
||||||
|
|||||||
@@ -22,7 +22,7 @@ from __future__ import annotations
|
|||||||
import asyncio
|
import asyncio
|
||||||
from typing import Annotated
|
from typing import Annotated
|
||||||
|
|
||||||
from fastapi import APIRouter, Body, Path, status
|
from fastapi import APIRouter, Body, Path, Query, status
|
||||||
|
|
||||||
from app.dependencies import (
|
from app.dependencies import (
|
||||||
AuthDep,
|
AuthDep,
|
||||||
@@ -43,6 +43,7 @@ from app.models.jail import (
|
|||||||
JailListResponse,
|
JailListResponse,
|
||||||
)
|
)
|
||||||
from app.services import jail_service
|
from app.services import jail_service
|
||||||
|
from app.utils.constants import DEFAULT_PAGE_SIZE
|
||||||
|
|
||||||
router: APIRouter = APIRouter(prefix="/api/v1/jails", tags=["Jails"])
|
router: APIRouter = APIRouter(prefix="/api/v1/jails", tags=["Jails"])
|
||||||
|
|
||||||
@@ -521,8 +522,8 @@ async def get_jail_banned_ips(
|
|||||||
socket_path: Fail2BanSocketDep,
|
socket_path: Fail2BanSocketDep,
|
||||||
http_session: HttpSessionDep,
|
http_session: HttpSessionDep,
|
||||||
geo_cache: GeoCacheDep,
|
geo_cache: GeoCacheDep,
|
||||||
page: int = 1,
|
page: int = Query(default=1, ge=1, description="1-based page number."),
|
||||||
page_size: int = 25,
|
page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=100, description="Items per page (max 100)."),
|
||||||
search: str | None = None,
|
search: str | None = None,
|
||||||
) -> JailBannedIpsResponse:
|
) -> JailBannedIpsResponse:
|
||||||
"""Return a paginated list of IPs currently banned by a specific jail.
|
"""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.
|
http_session: Shared HTTP session for geolocation.
|
||||||
geo_cache: Geolocation cache instance.
|
geo_cache: Geolocation cache instance.
|
||||||
page: 1-based page number (default 1, min 1).
|
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.
|
search: Optional case-insensitive substring filter on the IP address.
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
|
|||||||
90
backend/tests/test_utils/test_router_page_size_lint.py
Normal file
90
backend/tests/test_utils/test_router_page_size_lint.py
Normal file
@@ -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 = <int>" 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)
|
||||||
@@ -14,6 +14,7 @@ Hooks that expose a `refresh()` callback must use a long-lived `AbortController`
|
|||||||
- Pass `controller.signal` to the API function.
|
- Pass `controller.signal` to the API function.
|
||||||
- In the cleanup effect, abort the controller when the hook unmounts.
|
- In the cleanup effect, abort the controller when the hook unmounts.
|
||||||
- After each `await`, check `signal.aborted` before updating state.
|
- 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.
|
This prevents stale responses from overwriting newer results and avoids React state updates after unmount.
|
||||||
|
|
||||||
|
|||||||
@@ -72,8 +72,8 @@ export interface UseFetchDataResult<TData> {
|
|||||||
loading: boolean;
|
loading: boolean;
|
||||||
/** Typed error or null. Check `error?.type` to handle specific failure modes. */
|
/** Typed error or null. Check `error?.type` to handle specific failure modes. */
|
||||||
error: FetchError | null;
|
error: FetchError | null;
|
||||||
/** Trigger a fresh fetch. Cancels any in-flight request first. */
|
/** Trigger a fresh fetch. Cancels any in-flight request first. Returns null if aborted, otherwise the selected data. */
|
||||||
refresh: () => void;
|
refresh: () => Promise<TData | null>;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -107,7 +107,7 @@ export function useFetchData<TResponse, TData>(
|
|||||||
/** Unique ID for this instance, used to track its subscription to deduplicated requests. */
|
/** Unique ID for this instance, used to track its subscription to deduplicated requests. */
|
||||||
const subscriberIdRef = useRef<string | null>(null);
|
const subscriberIdRef = useRef<string | null>(null);
|
||||||
|
|
||||||
const refresh = useCallback((): void => {
|
const refresh = useCallback(async (): Promise<TData | null> => {
|
||||||
// Abort any previous request from this hook instance
|
// Abort any previous request from this hook instance
|
||||||
abortRef.current?.abort();
|
abortRef.current?.abort();
|
||||||
localControllerRef.current = new AbortController();
|
localControllerRef.current = new AbortController();
|
||||||
@@ -165,31 +165,41 @@ export function useFetchData<TResponse, TData>(
|
|||||||
.finally(() => {
|
.finally(() => {
|
||||||
setLoading(false);
|
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);
|
setLoading(true);
|
||||||
setError(null);
|
setError(null);
|
||||||
|
|
||||||
const controller = localControllerRef.current;
|
const controller = localControllerRef.current;
|
||||||
const responsePromise = fetcher(controller.signal)
|
|
||||||
.then((response) => {
|
// Raw promise for deduplication storage - stores BEFORE transformation
|
||||||
if (controller.signal.aborted) return response;
|
const rawPromise = fetcher(controller.signal).catch((err: unknown) => {
|
||||||
setData(selector(response));
|
|
||||||
if (onSuccess) {
|
|
||||||
onSuccess(response);
|
|
||||||
}
|
|
||||||
return response;
|
|
||||||
})
|
|
||||||
.catch((err: unknown) => {
|
|
||||||
if (controller.signal.aborted) throw err;
|
if (controller.signal.aborted) throw err;
|
||||||
handleFetchError(err, setError, errorMessage);
|
handleFetchError(err, setError, errorMessage);
|
||||||
throw err;
|
throw err;
|
||||||
|
});
|
||||||
|
|
||||||
|
// Store in-flight BEFORE transformation for correct subscriber type
|
||||||
|
if (requestKey) {
|
||||||
|
inFlightRequests.set(requestKey, {
|
||||||
|
promise: rawPromise as Promise<TResponse>,
|
||||||
|
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 null;
|
||||||
|
const data = selector(response);
|
||||||
|
setData(data);
|
||||||
|
if (onSuccess) {
|
||||||
|
onSuccess(response);
|
||||||
|
}
|
||||||
|
return data;
|
||||||
})
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
if (!controller.signal.aborted) {
|
if (!controller.signal.aborted) {
|
||||||
@@ -200,15 +210,7 @@ export function useFetchData<TResponse, TData>(
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
// Store in-flight request for deduplication
|
return responsePromise;
|
||||||
if (requestKey) {
|
|
||||||
inFlightRequests.set(requestKey, {
|
|
||||||
promise: responsePromise,
|
|
||||||
controller,
|
|
||||||
subscribers: new Map(),
|
|
||||||
initiatorDone: false,
|
|
||||||
});
|
|
||||||
}
|
|
||||||
}, [fetcher, selector, errorMessage, onSuccess, requestKey]);
|
}, [fetcher, selector, errorMessage, onSuccess, requestKey]);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|||||||
@@ -195,7 +195,7 @@ export function usePolledData<TResponse, TData>(
|
|||||||
if (!refetchOnWindowFocus) return;
|
if (!refetchOnWindowFocus) return;
|
||||||
|
|
||||||
const onFocus = (): void => {
|
const onFocus = (): void => {
|
||||||
refreshRef.current();
|
refreshRef.current?.();
|
||||||
};
|
};
|
||||||
|
|
||||||
window.addEventListener("focus", onFocus);
|
window.addEventListener("focus", onFocus);
|
||||||
|
|||||||
Reference in New Issue
Block a user