refactoring-backend #3
@@ -224,6 +224,10 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
||||
|
||||
**Goal:** Make a deliberate decision and enforce it consistently. Option A: adopt the protocol injection pattern everywhere — update all 16 non-compliant routers to accept their service via the typed `Dep` alias. Option B: acknowledge the pattern is unused overhead and remove the protocol aliases and `cast()` wrappers from `dependencies.py`, letting all routers import concrete services directly (the current de facto standard). Option B is cheaper; Option A makes service substitution in tests easier.
|
||||
|
||||
**Decision:** Option B applied — service protocol aliases and provider wrappers have been removed in favor of direct concrete service imports.
|
||||
|
||||
**Status:** Completed ✅
|
||||
|
||||
**Possible traps and issues:**
|
||||
- Option A requires updating all 16 routers and their test files simultaneously; this is a broad change with high regression risk. Stage it one router at a time.
|
||||
- Option B means deleting `services/protocols.py` (398 lines) and all `cast("…Service", …)` calls in `dependencies.py`. Ensure nothing outside this layer references the Protocol classes (check test files).
|
||||
|
||||
@@ -29,16 +29,6 @@ from app.repositories.protocols import (
|
||||
ImportLogRepository,
|
||||
SessionRepository,
|
||||
)
|
||||
from app.services.protocols import (
|
||||
AuthService,
|
||||
BlocklistService,
|
||||
ConfigService,
|
||||
GeoService,
|
||||
HealthService,
|
||||
HistoryService,
|
||||
JailService,
|
||||
ServerService,
|
||||
)
|
||||
from app.utils.constants import SESSION_COOKIE_NAME
|
||||
from app.utils.runtime_state import RuntimeState
|
||||
from app.utils.session_cache import InMemorySessionCache, NoOpSessionCache, SessionCache
|
||||
@@ -240,62 +230,6 @@ async def get_session_cache(app_context: Annotated[ApplicationContext, Depends(g
|
||||
return app_context.session_cache
|
||||
|
||||
|
||||
async def get_auth_service() -> AuthService:
|
||||
"""Provide the concrete authentication service implementation."""
|
||||
from app.services import auth_service # noqa: PLC0415
|
||||
|
||||
return cast("AuthService", auth_service)
|
||||
|
||||
|
||||
async def get_jail_service() -> JailService:
|
||||
"""Provide the concrete jail service implementation."""
|
||||
from app.services import jail_service # noqa: PLC0415
|
||||
|
||||
return cast("JailService", jail_service)
|
||||
|
||||
|
||||
async def get_blocklist_service() -> BlocklistService:
|
||||
"""Provide the concrete blocklist service implementation."""
|
||||
from app.services import blocklist_service # noqa: PLC0415
|
||||
|
||||
return cast("BlocklistService", blocklist_service)
|
||||
|
||||
|
||||
async def get_config_service() -> ConfigService:
|
||||
"""Provide the concrete configuration service implementation."""
|
||||
from app.services import config_service # noqa: PLC0415
|
||||
|
||||
return cast("ConfigService", config_service)
|
||||
|
||||
|
||||
async def get_history_service() -> HistoryService:
|
||||
"""Provide the concrete history service implementation."""
|
||||
from app.services import history_service # noqa: PLC0415
|
||||
|
||||
return cast("HistoryService", history_service)
|
||||
|
||||
|
||||
async def get_geo_service() -> GeoService:
|
||||
"""Provide the concrete geo service implementation."""
|
||||
from app.services import geo_service # noqa: PLC0415
|
||||
|
||||
return cast("GeoService", geo_service)
|
||||
|
||||
|
||||
async def get_health_service() -> HealthService:
|
||||
"""Provide the concrete health service implementation."""
|
||||
from app.services import health_service # noqa: PLC0415
|
||||
|
||||
return cast("HealthService", health_service)
|
||||
|
||||
|
||||
async def get_server_service() -> ServerService:
|
||||
"""Provide the concrete server service implementation."""
|
||||
from app.services import server_service # noqa: PLC0415
|
||||
|
||||
return cast("ServerService", server_service)
|
||||
|
||||
|
||||
async def get_session_repo() -> SessionRepository:
|
||||
"""Provide the concrete session repository implementation."""
|
||||
from app.repositories import session_repo # noqa: PLC0415
|
||||
@@ -357,7 +291,6 @@ async def require_auth(
|
||||
db: Annotated[aiosqlite.Connection, Depends(get_db)],
|
||||
settings: Annotated[Settings, Depends(get_settings)],
|
||||
session_cache: Annotated[SessionCache, Depends(get_session_cache)],
|
||||
auth_service: Annotated[AuthService, Depends(get_auth_service)],
|
||||
session_repo: Annotated[SessionRepository, Depends(get_session_repo)],
|
||||
) -> Session:
|
||||
"""Validate the session token and return the active session.
|
||||
@@ -403,6 +336,8 @@ async def require_auth(
|
||||
if cached is not None:
|
||||
return cached
|
||||
|
||||
from app.services import auth_service # noqa: PLC0415
|
||||
|
||||
try:
|
||||
session = await auth_service.validate_session(
|
||||
db,
|
||||
@@ -434,14 +369,6 @@ GeoBatchLookupDep = Annotated[GeoBatchLookup, Depends(get_geo_batch_lookup)]
|
||||
ServerStatusDep = Annotated[ServerStatus, Depends(get_server_status)]
|
||||
PendingRecoveryDep = Annotated[PendingRecovery | None, Depends(get_pending_recovery)]
|
||||
SessionCacheDep = Annotated[SessionCache, Depends(get_session_cache)]
|
||||
AuthServiceDep = Annotated[AuthService, Depends(get_auth_service)]
|
||||
JailServiceDep = Annotated[JailService, Depends(get_jail_service)]
|
||||
BlocklistServiceDep = Annotated[BlocklistService, Depends(get_blocklist_service)]
|
||||
ConfigServiceDep = Annotated[ConfigService, Depends(get_config_service)]
|
||||
HistoryServiceDep = Annotated[HistoryService, Depends(get_history_service)]
|
||||
GeoServiceDep = Annotated[GeoService, Depends(get_geo_service)]
|
||||
HealthServiceDep = Annotated[HealthService, Depends(get_health_service)]
|
||||
ServerServiceDep = Annotated[ServerService, Depends(get_server_service)]
|
||||
SessionRepoDep = Annotated[SessionRepository, Depends(get_session_repo)]
|
||||
BlocklistRepositoryDep = Annotated[BlocklistRepository, Depends(get_blocklist_repo)]
|
||||
ImportLogRepositoryDep = Annotated[ImportLogRepository, Depends(get_import_log_repo)]
|
||||
|
||||
@@ -12,14 +12,9 @@ from __future__ import annotations
|
||||
import structlog
|
||||
from fastapi import APIRouter, HTTPException, Request, Response, status
|
||||
|
||||
from app.dependencies import (
|
||||
AuthServiceDep,
|
||||
DbDep,
|
||||
SessionCacheDep,
|
||||
SessionRepoDep,
|
||||
SettingsDep,
|
||||
)
|
||||
from app.dependencies import DbDep, SessionCacheDep, SessionRepoDep, SettingsDep
|
||||
from app.models.auth import LoginRequest, LoginResponse, LogoutResponse
|
||||
from app.services import auth_service
|
||||
from app.utils.constants import SESSION_COOKIE_NAME
|
||||
|
||||
log: structlog.stdlib.BoundLogger = structlog.get_logger()
|
||||
@@ -37,7 +32,6 @@ async def login(
|
||||
response: Response,
|
||||
db: DbDep,
|
||||
settings: SettingsDep,
|
||||
auth_service: AuthServiceDep,
|
||||
session_repo: SessionRepoDep,
|
||||
) -> LoginResponse:
|
||||
"""Verify the master password and return a session token.
|
||||
@@ -93,7 +87,6 @@ async def logout(
|
||||
db: DbDep,
|
||||
settings: SettingsDep,
|
||||
session_cache: SessionCacheDep,
|
||||
auth_service: AuthServiceDep,
|
||||
session_repo: SessionRepoDep,
|
||||
) -> LogoutResponse:
|
||||
"""Invalidate the active session.
|
||||
|
||||
@@ -29,9 +29,12 @@ from app.dependencies import (
|
||||
Fail2BanSocketDep,
|
||||
GeoBatchLookupDep,
|
||||
HttpSessionDep,
|
||||
JailServiceDep,
|
||||
)
|
||||
from app.exceptions import JailNotFoundError, JailOperationError
|
||||
from app.exceptions import (
|
||||
Fail2BanConnectionError,
|
||||
JailNotFoundError,
|
||||
JailOperationError,
|
||||
)
|
||||
from app.models.ban import JailBannedIpsResponse
|
||||
from app.models.jail import (
|
||||
IgnoreIpRequest,
|
||||
@@ -40,7 +43,6 @@ from app.models.jail import (
|
||||
JailListResponse,
|
||||
)
|
||||
from app.services import jail_service
|
||||
from app.exceptions import Fail2BanConnectionError
|
||||
|
||||
router: APIRouter = APIRouter(prefix="/api/jails", tags=["Jails"])
|
||||
|
||||
@@ -109,7 +111,6 @@ def _conflict(message: str) -> HTTPException:
|
||||
async def get_jails(
|
||||
_auth: AuthDep,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailListResponse:
|
||||
"""Return a summary of every active fail2ban jail.
|
||||
|
||||
@@ -124,7 +125,7 @@ async def get_jails(
|
||||
:class:`~app.models.jail.JailListResponse` with all active jails.
|
||||
"""
|
||||
try:
|
||||
return await jail_service_dep.list_jails(socket_path)
|
||||
return await jail_service.list_jails(socket_path)
|
||||
except Fail2BanConnectionError as exc:
|
||||
raise _bad_gateway(exc) from exc
|
||||
|
||||
@@ -138,7 +139,6 @@ async def get_jail(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailDetailResponse:
|
||||
"""Return the complete configuration and runtime state for one jail.
|
||||
|
||||
@@ -158,7 +158,7 @@ async def get_jail(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
return await jail_service_dep.get_jail(socket_path, name)
|
||||
return await jail_service.get_jail(socket_path, name)
|
||||
except JailNotFoundError:
|
||||
raise _not_found(name) from None
|
||||
except Fail2BanConnectionError as exc:
|
||||
@@ -178,7 +178,6 @@ async def get_jail(
|
||||
async def reload_all_jails(
|
||||
_auth: AuthDep,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Reload every fail2ban jail to apply configuration changes.
|
||||
|
||||
@@ -196,7 +195,7 @@ async def reload_all_jails(
|
||||
HTTPException: 409 when fail2ban reports the operation failed.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.reload_all(socket_path)
|
||||
await jail_service.reload_all(socket_path)
|
||||
return JailCommandResponse(message="All jails reloaded successfully.", jail="*")
|
||||
except JailOperationError as exc:
|
||||
raise _conflict(str(exc)) from exc
|
||||
@@ -213,7 +212,6 @@ async def start_jail(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Start a fail2ban jail that is currently stopped.
|
||||
|
||||
@@ -230,7 +228,7 @@ async def start_jail(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.start_jail(socket_path, name)
|
||||
await jail_service.start_jail(socket_path, name)
|
||||
return JailCommandResponse(message=f"Jail {name!r} started.", jail=name)
|
||||
except JailNotFoundError:
|
||||
raise _not_found(name) from None
|
||||
@@ -249,7 +247,6 @@ async def stop_jail(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Stop a running fail2ban jail.
|
||||
|
||||
@@ -269,7 +266,7 @@ async def stop_jail(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.stop_jail(socket_path, name)
|
||||
await jail_service.stop_jail(socket_path, name)
|
||||
return JailCommandResponse(message=f"Jail {name!r} stopped.", jail=name)
|
||||
except JailOperationError as exc:
|
||||
raise _conflict(str(exc)) from exc
|
||||
@@ -286,7 +283,6 @@ async def toggle_idle(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
on: bool = Body(..., description="``true`` to enable idle, ``false`` to disable."),
|
||||
) -> JailCommandResponse:
|
||||
"""Enable or disable idle mode for a fail2ban jail.
|
||||
@@ -309,7 +305,7 @@ async def toggle_idle(
|
||||
"""
|
||||
state_str = "on" if on else "off"
|
||||
try:
|
||||
await jail_service_dep.set_idle(socket_path, name, on=on)
|
||||
await jail_service.set_idle(socket_path, name, on=on)
|
||||
return JailCommandResponse(
|
||||
message=f"Jail {name!r} idle mode turned {state_str}.",
|
||||
jail=name,
|
||||
@@ -331,7 +327,6 @@ async def reload_jail(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Reload a single fail2ban jail to pick up configuration changes.
|
||||
|
||||
@@ -348,7 +343,7 @@ async def reload_jail(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.reload_jail(socket_path, name)
|
||||
await jail_service.reload_jail(socket_path, name)
|
||||
return JailCommandResponse(message=f"Jail {name!r} reloaded.", jail=name)
|
||||
except JailNotFoundError:
|
||||
raise _not_found(name) from None
|
||||
@@ -380,7 +375,6 @@ async def get_ignore_list(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> list[str]:
|
||||
"""Return the current ignore list (IP whitelist) for a fail2ban jail.
|
||||
|
||||
@@ -396,7 +390,7 @@ async def get_ignore_list(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
return await jail_service_dep.get_ignore_list(socket_path, name)
|
||||
return await jail_service.get_ignore_list(socket_path, name)
|
||||
except JailNotFoundError:
|
||||
raise _not_found(name) from None
|
||||
except Fail2BanConnectionError as exc:
|
||||
@@ -414,7 +408,6 @@ async def add_ignore_ip(
|
||||
name: _NamePath,
|
||||
body: IgnoreIpRequest,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Add an IP address or CIDR network to a jail's ignore list.
|
||||
|
||||
@@ -436,7 +429,7 @@ async def add_ignore_ip(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.add_ignore_ip(socket_path, name, body.ip)
|
||||
await jail_service.add_ignore_ip(socket_path, name, body.ip)
|
||||
return JailCommandResponse(
|
||||
message=f"IP {body.ip!r} added to ignore list of jail {name!r}.",
|
||||
jail=name,
|
||||
@@ -464,7 +457,6 @@ async def del_ignore_ip(
|
||||
name: _NamePath,
|
||||
body: IgnoreIpRequest,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
) -> JailCommandResponse:
|
||||
"""Remove an IP address or CIDR network from a jail's ignore list.
|
||||
|
||||
@@ -482,7 +474,7 @@ async def del_ignore_ip(
|
||||
HTTPException: 502 when fail2ban is unreachable.
|
||||
"""
|
||||
try:
|
||||
await jail_service_dep.del_ignore_ip(socket_path, name, body.ip)
|
||||
await jail_service.del_ignore_ip(socket_path, name, body.ip)
|
||||
return JailCommandResponse(
|
||||
message=f"IP {body.ip!r} removed from ignore list of jail {name!r}.",
|
||||
jail=name,
|
||||
@@ -504,7 +496,6 @@ async def toggle_ignore_self(
|
||||
_auth: AuthDep,
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
on: bool = Body(..., description="``true`` to enable ignoreself, ``false`` to disable."),
|
||||
) -> JailCommandResponse:
|
||||
"""Toggle the ``ignoreself`` flag for a fail2ban jail.
|
||||
@@ -527,7 +518,7 @@ async def toggle_ignore_self(
|
||||
"""
|
||||
state_str = "enabled" if on else "disabled"
|
||||
try:
|
||||
await jail_service_dep.set_ignore_self(socket_path, name, on=on)
|
||||
await jail_service.set_ignore_self(socket_path, name, on=on)
|
||||
return JailCommandResponse(
|
||||
message=f"ignoreself {state_str} for jail {name!r}.",
|
||||
jail=name,
|
||||
@@ -556,7 +547,6 @@ async def get_jail_banned_ips(
|
||||
name: _NamePath,
|
||||
socket_path: Fail2BanSocketDep,
|
||||
http_session: HttpSessionDep,
|
||||
jail_service_dep: JailServiceDep,
|
||||
geo_batch_lookup: GeoBatchLookupDep,
|
||||
page: int = 1,
|
||||
page_size: int = 25,
|
||||
@@ -595,7 +585,7 @@ async def get_jail_banned_ips(
|
||||
)
|
||||
|
||||
try:
|
||||
return await jail_service_dep.get_jail_banned_ips(
|
||||
return await jail_service.get_jail_banned_ips(
|
||||
socket_path=socket_path,
|
||||
jail_name=name,
|
||||
page=page,
|
||||
|
||||
Reference in New Issue
Block a user