diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 2ad5153..f08392e 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,28 +1,3 @@ -### T-02 · Remove duplicate router-level exception helpers — use global handlers only - -**Where found:** `backend/app/routers/jails.py`, `bans.py`, `jail_config.py`, `server.py`, `config_misc.py` each define `_bad_gateway()`, `_not_found()`, `_conflict()`. Global handlers for the same exceptions exist in `backend/app/main.py`. - -**Why this is needed:** Two parallel error-mapping systems produce inconsistent error bodies. A `Fail2BanConnectionError` caught by a router produces `"Cannot reach fail2ban: {exc}"` while one that escapes produces `"{exc}"` (just the exception string). Adds dead code and maintenance burden. - -**Goal:** All domain exceptions propagate to the global handlers in `main.py`. Routers contain zero HTTP error construction. - -**What to do:** -1. Remove all `_bad_gateway`, `_not_found`, `_conflict` helpers from routers. -2. Remove the `try/except` blocks in router handlers that convert domain exceptions to `HTTPException` — let them propagate. -3. Verify that all domain exception types are covered in `main.py`'s `add_exception_handler` registrations. -4. Confirm error body format is consistent across all affected endpoints. - -**Possible traps and issues:** -- A few routers catch `ValueError` from service layer and re-raise as `JailOperationError` — ensure those paths still reach the correct global handler. -- FastAPI evaluates exception handlers in registration order. Verify the order in `create_app` is most-specific first. -- Integration tests that assert on specific error message strings may need updates. - -**Docs changes needed:** `Docs/Backend-Development.md` — document that routers must not construct `HTTPException` for domain errors; they should raise or let domain exceptions propagate. - -**Doc references:** `Docs/Backend-Development.md` - ---- - ### T-03 · Centralise `_DEFAULT_PAGE_SIZE` constant **Where found:** `backend/app/routers/dashboard.py:45`, `routers/history.py:34`, `services/ban_service.py:70`, `services/history_service.py:49` diff --git a/backend/app/routers/dashboard.py b/backend/app/routers/dashboard.py index 0fa2be6..860d913 100644 --- a/backend/app/routers/dashboard.py +++ b/backend/app/routers/dashboard.py @@ -35,6 +35,7 @@ from app.models.ban import ( ) from app.models.server import ServerStatus, ServerStatusResponse from app.services import ban_service, geo_service +from app.utils.constants import DEFAULT_PAGE_SIZE router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"]) @@ -42,7 +43,6 @@ router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"]) # Default pagination constants # --------------------------------------------------------------------------- -_DEFAULT_PAGE_SIZE: int = 100 _DEFAULT_RANGE: TimeRange = "24h" @@ -91,7 +91,7 @@ async def get_dashboard_bans( description="Data source: 'fail2ban' or 'archive'.", ), 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."), + page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=500, description="Items per page."), origin: BanOrigin | None = Query( default=None, description="Filter by ban origin: 'blocklist' or 'selfblock'. Omit for all.", diff --git a/backend/app/routers/history.py b/backend/app/routers/history.py index 617ab0b..73823f6 100644 --- a/backend/app/routers/history.py +++ b/backend/app/routers/history.py @@ -28,11 +28,10 @@ from app.dependencies import ( from app.models.ban import BanOrigin, TimeRange from app.models.history import HistoryListResponse, IpDetailResponse from app.services import history_service +from app.utils.constants import DEFAULT_PAGE_SIZE router: APIRouter = APIRouter(prefix="/api/history", tags=["History"]) -_DEFAULT_PAGE_SIZE: int = 100 - @router.get( "", @@ -67,7 +66,7 @@ async def get_history( ), page: int = Query(default=1, ge=1, description="1-based page number."), page_size: int = Query( - default=_DEFAULT_PAGE_SIZE, + default=DEFAULT_PAGE_SIZE, ge=1, le=500, description="Items per page (max 500).", @@ -124,7 +123,7 @@ async def get_history_archive( jail: str | None = Query(default=None, description="Restrict results to this jail name."), ip: str | None = Query(default=None, description="Restrict results to IPs matching this prefix."), 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)."), + page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=500, description="Items per page (max 500)."), ) -> HistoryListResponse: return await history_service.list_history( diff --git a/backend/app/services/ban_service.py b/backend/app/services/ban_service.py index 486a848..035f288 100644 --- a/backend/app/services/ban_service.py +++ b/backend/app/services/ban_service.py @@ -43,6 +43,7 @@ from app.models.ban import ( from app.repositories import fail2ban_db_repo from app.repositories import history_archive_repo as default_history_archive_repo from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service +from app.utils.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE from app.utils.fail2ban_client import ( Fail2BanClient, ) @@ -72,8 +73,6 @@ async def get_fail2ban_db_path(socket_path: str) -> str: # Constants # --------------------------------------------------------------------------- -_DEFAULT_PAGE_SIZE: int = 100 -_MAX_PAGE_SIZE: int = 500 _SOCKET_TIMEOUT: float = 5.0 @@ -357,7 +356,7 @@ async def list_bans( *, source: str = "fail2ban", page: int = 1, - page_size: int = _DEFAULT_PAGE_SIZE, + page_size: int = DEFAULT_PAGE_SIZE, http_session: aiohttp.ClientSession | None = None, app_db: aiosqlite.Connection | None = None, geo_batch_lookup: GeoBatchLookup | None = None, @@ -385,7 +384,7 @@ async def list_bans( range_: Time-range preset (``"24h"``, ``"7d"``, ``"30d"``, or ``"365d"``). page: 1-based page number (default: ``1``). - page_size: Maximum items per page, capped at ``_MAX_PAGE_SIZE`` + page_size: Maximum items per page, capped at ``MAX_PAGE_SIZE`` (default: ``100``). http_session: Optional shared :class:`aiohttp.ClientSession`. When provided, :func:`~app.services.geo_service.lookup_batch` is used @@ -403,7 +402,7 @@ async def list_bans( """ since: int = _since_unix(range_) - effective_page_size: int = min(page_size, _MAX_PAGE_SIZE) + effective_page_size: int = min(page_size, MAX_PAGE_SIZE) offset: int = (page - 1) * effective_page_size if source not in ("fail2ban", "archive"): diff --git a/backend/app/services/history_service.py b/backend/app/services/history_service.py index 50eb4ee..5e45038 100644 --- a/backend/app/services/history_service.py +++ b/backend/app/services/history_service.py @@ -19,8 +19,8 @@ from app.models.ban import TIME_RANGE_SECONDS, BanOrigin, TimeRange from app.services import geo_service if TYPE_CHECKING: - import aiosqlite import aiohttp + import aiosqlite from app.models.geo import GeoEnricher, GeoInfo from app.repositories.protocols import HistoryArchiveRepository @@ -30,8 +30,10 @@ from app.models.history import ( IpDetailResponse, IpTimelineEvent, ) -from app.repositories import fail2ban_db_repo, history_archive_repo as default_history_archive_repo +from app.repositories import fail2ban_db_repo +from app.repositories import history_archive_repo as default_history_archive_repo from app.services.fail2ban_metadata_service import default_fail2ban_metadata_service +from app.utils.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE from app.utils.fail2ban_db_utils import parse_data_json, ts_to_iso log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -46,9 +48,6 @@ async def get_fail2ban_db_path(socket_path: str) -> str: # Constants # --------------------------------------------------------------------------- -_DEFAULT_PAGE_SIZE: int = 100 -_MAX_PAGE_SIZE: int = 500 - def _since_unix(range_: TimeRange) -> int: """Return the Unix timestamp for the start of the given time window. @@ -66,7 +65,7 @@ def _since_unix(range_: TimeRange) -> int: async def _resolve_geo_info( ip: str, *, - http_session: "aiohttp.ClientSession" | None = None, + http_session: aiohttp.ClientSession | None = None, geo_enricher: GeoEnricher | None = None, ) -> GeoInfo | None: """Resolve geolocation information for a single IP address. @@ -167,8 +166,8 @@ async def list_history( origin: BanOrigin | None = None, source: str = "fail2ban", page: int = 1, - page_size: int = _DEFAULT_PAGE_SIZE, - http_session: "aiohttp.ClientSession" | None = None, + page_size: int = DEFAULT_PAGE_SIZE, + http_session: aiohttp.ClientSession | None = None, geo_enricher: GeoEnricher | None = None, db: aiosqlite.Connection | None = None, history_archive_repo: HistoryArchiveRepository = default_history_archive_repo, @@ -186,7 +185,7 @@ async def list_history( ip_filter: If given, restrict results to bans for this exact IP (or a prefix — the query uses ``LIKE ip_filter%``). page: 1-based page number (default: ``1``). - page_size: Maximum items per page, capped at ``_MAX_PAGE_SIZE``. + page_size: Maximum items per page, capped at ``MAX_PAGE_SIZE``. http_session: Optional shared :class:`aiohttp.ClientSession` used for geo lookups when no explicit *geo_enricher* is provided. geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``. @@ -195,7 +194,7 @@ async def list_history( :class:`~app.models.history.HistoryListResponse` with paginated items and the total matching count. """ - effective_page_size: int = min(page_size, _MAX_PAGE_SIZE) + effective_page_size: int = min(page_size, MAX_PAGE_SIZE) # Build WHERE clauses dynamically. since: int | None = None @@ -333,7 +332,7 @@ async def get_ip_detail( socket_path: str, ip: str, *, - http_session: "aiohttp.ClientSession" | None = None, + http_session: aiohttp.ClientSession | None = None, geo_enricher: GeoEnricher | None = None, ) -> IpDetailResponse | None: """Return the full historical record for a single IP address. diff --git a/backend/app/utils/constants.py b/backend/app/utils/constants.py index f2f371f..ca1dce8 100644 --- a/backend/app/utils/constants.py +++ b/backend/app/utils/constants.py @@ -66,7 +66,7 @@ TIME_RANGE_HOURS: Final[dict[str, int]] = { # Pagination # --------------------------------------------------------------------------- -DEFAULT_PAGE_SIZE: Final[int] = 50 +DEFAULT_PAGE_SIZE: Final[int] = 100 MAX_PAGE_SIZE: Final[int] = 500 # ---------------------------------------------------------------------------