Refactor backend services and routers
- Reorganized dashboard router with improved structure - Enhanced ban_service with better separation of concerns - Updated history service with cleaner logic - Improved constants and configuration handling - Updated documentation of completed tasks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -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
|
### 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`
|
**Where found:** `backend/app/routers/dashboard.py:45`, `routers/history.py:34`, `services/ban_service.py:70`, `services/history_service.py:49`
|
||||||
|
|||||||
@@ -35,6 +35,7 @@ from app.models.ban import (
|
|||||||
)
|
)
|
||||||
from app.models.server import ServerStatus, ServerStatusResponse
|
from app.models.server import ServerStatus, ServerStatusResponse
|
||||||
from app.services import ban_service, geo_service
|
from app.services import ban_service, geo_service
|
||||||
|
from app.utils.constants import DEFAULT_PAGE_SIZE
|
||||||
|
|
||||||
router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"])
|
router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"])
|
||||||
|
|
||||||
@@ -42,7 +43,6 @@ router: APIRouter = APIRouter(prefix="/api/dashboard", tags=["Dashboard"])
|
|||||||
# Default pagination constants
|
# Default pagination constants
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
_DEFAULT_PAGE_SIZE: int = 100
|
|
||||||
_DEFAULT_RANGE: TimeRange = "24h"
|
_DEFAULT_RANGE: TimeRange = "24h"
|
||||||
|
|
||||||
|
|
||||||
@@ -91,7 +91,7 @@ async def get_dashboard_bans(
|
|||||||
description="Data source: 'fail2ban' or 'archive'.",
|
description="Data source: 'fail2ban' or 'archive'.",
|
||||||
),
|
),
|
||||||
page: int = Query(default=1, ge=1, description="1-based page number."),
|
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(
|
origin: BanOrigin | None = Query(
|
||||||
default=None,
|
default=None,
|
||||||
description="Filter by ban origin: 'blocklist' or 'selfblock'. Omit for all.",
|
description="Filter by ban origin: 'blocklist' or 'selfblock'. Omit for all.",
|
||||||
|
|||||||
@@ -28,11 +28,10 @@ from app.dependencies import (
|
|||||||
from app.models.ban import BanOrigin, TimeRange
|
from app.models.ban import BanOrigin, TimeRange
|
||||||
from app.models.history import HistoryListResponse, IpDetailResponse
|
from app.models.history import HistoryListResponse, IpDetailResponse
|
||||||
from app.services import history_service
|
from app.services import history_service
|
||||||
|
from app.utils.constants import DEFAULT_PAGE_SIZE
|
||||||
|
|
||||||
router: APIRouter = APIRouter(prefix="/api/history", tags=["History"])
|
router: APIRouter = APIRouter(prefix="/api/history", tags=["History"])
|
||||||
|
|
||||||
_DEFAULT_PAGE_SIZE: int = 100
|
|
||||||
|
|
||||||
|
|
||||||
@router.get(
|
@router.get(
|
||||||
"",
|
"",
|
||||||
@@ -67,7 +66,7 @@ async def get_history(
|
|||||||
),
|
),
|
||||||
page: int = Query(default=1, ge=1, description="1-based page number."),
|
page: int = Query(default=1, ge=1, description="1-based page number."),
|
||||||
page_size: int = Query(
|
page_size: int = Query(
|
||||||
default=_DEFAULT_PAGE_SIZE,
|
default=DEFAULT_PAGE_SIZE,
|
||||||
ge=1,
|
ge=1,
|
||||||
le=500,
|
le=500,
|
||||||
description="Items per page (max 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."),
|
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."),
|
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: 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:
|
) -> HistoryListResponse:
|
||||||
|
|
||||||
return await history_service.list_history(
|
return await history_service.list_history(
|
||||||
|
|||||||
@@ -43,6 +43,7 @@ from app.models.ban import (
|
|||||||
from app.repositories import fail2ban_db_repo
|
from app.repositories import fail2ban_db_repo
|
||||||
from app.repositories import history_archive_repo as default_history_archive_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.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 (
|
from app.utils.fail2ban_client import (
|
||||||
Fail2BanClient,
|
Fail2BanClient,
|
||||||
)
|
)
|
||||||
@@ -72,8 +73,6 @@ async def get_fail2ban_db_path(socket_path: str) -> str:
|
|||||||
# Constants
|
# Constants
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
_DEFAULT_PAGE_SIZE: int = 100
|
|
||||||
_MAX_PAGE_SIZE: int = 500
|
|
||||||
_SOCKET_TIMEOUT: float = 5.0
|
_SOCKET_TIMEOUT: float = 5.0
|
||||||
|
|
||||||
|
|
||||||
@@ -357,7 +356,7 @@ async def list_bans(
|
|||||||
*,
|
*,
|
||||||
source: str = "fail2ban",
|
source: str = "fail2ban",
|
||||||
page: int = 1,
|
page: int = 1,
|
||||||
page_size: int = _DEFAULT_PAGE_SIZE,
|
page_size: int = DEFAULT_PAGE_SIZE,
|
||||||
http_session: aiohttp.ClientSession | None = None,
|
http_session: aiohttp.ClientSession | None = None,
|
||||||
app_db: aiosqlite.Connection | None = None,
|
app_db: aiosqlite.Connection | None = None,
|
||||||
geo_batch_lookup: GeoBatchLookup | None = None,
|
geo_batch_lookup: GeoBatchLookup | None = None,
|
||||||
@@ -385,7 +384,7 @@ async def list_bans(
|
|||||||
range_: Time-range preset (``"24h"``, ``"7d"``, ``"30d"``, or
|
range_: Time-range preset (``"24h"``, ``"7d"``, ``"30d"``, or
|
||||||
``"365d"``).
|
``"365d"``).
|
||||||
page: 1-based page number (default: ``1``).
|
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``).
|
(default: ``100``).
|
||||||
http_session: Optional shared :class:`aiohttp.ClientSession`. When
|
http_session: Optional shared :class:`aiohttp.ClientSession`. When
|
||||||
provided, :func:`~app.services.geo_service.lookup_batch` is used
|
provided, :func:`~app.services.geo_service.lookup_batch` is used
|
||||||
@@ -403,7 +402,7 @@ async def list_bans(
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
since: int = _since_unix(range_)
|
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
|
offset: int = (page - 1) * effective_page_size
|
||||||
|
|
||||||
if source not in ("fail2ban", "archive"):
|
if source not in ("fail2ban", "archive"):
|
||||||
|
|||||||
@@ -19,8 +19,8 @@ from app.models.ban import TIME_RANGE_SECONDS, BanOrigin, TimeRange
|
|||||||
from app.services import geo_service
|
from app.services import geo_service
|
||||||
|
|
||||||
if TYPE_CHECKING:
|
if TYPE_CHECKING:
|
||||||
import aiosqlite
|
|
||||||
import aiohttp
|
import aiohttp
|
||||||
|
import aiosqlite
|
||||||
|
|
||||||
from app.models.geo import GeoEnricher, GeoInfo
|
from app.models.geo import GeoEnricher, GeoInfo
|
||||||
from app.repositories.protocols import HistoryArchiveRepository
|
from app.repositories.protocols import HistoryArchiveRepository
|
||||||
@@ -30,8 +30,10 @@ from app.models.history import (
|
|||||||
IpDetailResponse,
|
IpDetailResponse,
|
||||||
IpTimelineEvent,
|
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.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
|
from app.utils.fail2ban_db_utils import parse_data_json, ts_to_iso
|
||||||
|
|
||||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||||
@@ -46,9 +48,6 @@ async def get_fail2ban_db_path(socket_path: str) -> str:
|
|||||||
# Constants
|
# Constants
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
_DEFAULT_PAGE_SIZE: int = 100
|
|
||||||
_MAX_PAGE_SIZE: int = 500
|
|
||||||
|
|
||||||
|
|
||||||
def _since_unix(range_: TimeRange) -> int:
|
def _since_unix(range_: TimeRange) -> int:
|
||||||
"""Return the Unix timestamp for the start of the given time window.
|
"""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(
|
async def _resolve_geo_info(
|
||||||
ip: str,
|
ip: str,
|
||||||
*,
|
*,
|
||||||
http_session: "aiohttp.ClientSession" | None = None,
|
http_session: aiohttp.ClientSession | None = None,
|
||||||
geo_enricher: GeoEnricher | None = None,
|
geo_enricher: GeoEnricher | None = None,
|
||||||
) -> GeoInfo | None:
|
) -> GeoInfo | None:
|
||||||
"""Resolve geolocation information for a single IP address.
|
"""Resolve geolocation information for a single IP address.
|
||||||
@@ -167,8 +166,8 @@ async def list_history(
|
|||||||
origin: BanOrigin | None = None,
|
origin: BanOrigin | None = None,
|
||||||
source: str = "fail2ban",
|
source: str = "fail2ban",
|
||||||
page: int = 1,
|
page: int = 1,
|
||||||
page_size: int = _DEFAULT_PAGE_SIZE,
|
page_size: int = DEFAULT_PAGE_SIZE,
|
||||||
http_session: "aiohttp.ClientSession" | None = None,
|
http_session: aiohttp.ClientSession | None = None,
|
||||||
geo_enricher: GeoEnricher | None = None,
|
geo_enricher: GeoEnricher | None = None,
|
||||||
db: aiosqlite.Connection | None = None,
|
db: aiosqlite.Connection | None = None,
|
||||||
history_archive_repo: HistoryArchiveRepository = default_history_archive_repo,
|
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
|
ip_filter: If given, restrict results to bans for this exact IP
|
||||||
(or a prefix — the query uses ``LIKE ip_filter%``).
|
(or a prefix — the query uses ``LIKE ip_filter%``).
|
||||||
page: 1-based page number (default: ``1``).
|
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
|
http_session: Optional shared :class:`aiohttp.ClientSession` used for
|
||||||
geo lookups when no explicit *geo_enricher* is provided.
|
geo lookups when no explicit *geo_enricher* is provided.
|
||||||
geo_enricher: Optional async callable ``(ip: str) -> GeoInfo | None``.
|
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
|
:class:`~app.models.history.HistoryListResponse` with paginated items
|
||||||
and the total matching count.
|
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.
|
# Build WHERE clauses dynamically.
|
||||||
since: int | None = None
|
since: int | None = None
|
||||||
@@ -333,7 +332,7 @@ async def get_ip_detail(
|
|||||||
socket_path: str,
|
socket_path: str,
|
||||||
ip: str,
|
ip: str,
|
||||||
*,
|
*,
|
||||||
http_session: "aiohttp.ClientSession" | None = None,
|
http_session: aiohttp.ClientSession | None = None,
|
||||||
geo_enricher: GeoEnricher | None = None,
|
geo_enricher: GeoEnricher | None = None,
|
||||||
) -> IpDetailResponse | None:
|
) -> IpDetailResponse | None:
|
||||||
"""Return the full historical record for a single IP address.
|
"""Return the full historical record for a single IP address.
|
||||||
|
|||||||
@@ -66,7 +66,7 @@ TIME_RANGE_HOURS: Final[dict[str, int]] = {
|
|||||||
# Pagination
|
# Pagination
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
DEFAULT_PAGE_SIZE: Final[int] = 50
|
DEFAULT_PAGE_SIZE: Final[int] = 100
|
||||||
MAX_PAGE_SIZE: Final[int] = 500
|
MAX_PAGE_SIZE: Final[int] = 500
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user