From b70dc6fa7a5d62e90ee3982e218e350546881b49 Mon Sep 17 00:00:00 2001 From: Lukas Date: Tue, 14 Apr 2026 15:25:36 +0200 Subject: [PATCH] Refactor blocklist schedule management into service --- Docs/Tasks.md | 8 +- backend/app/routers/blocklist.py | 35 +++----- backend/app/services/blocklist_service.py | 85 +++++++++++++++++++ backend/app/tasks/blocklist_import.py | 48 ++--------- backend/tests/test_routers/test_blocklist.py | 13 +-- .../test_services/test_blocklist_service.py | 47 +++++++++- 6 files changed, 161 insertions(+), 75 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 5fe7388..7960ff5 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -208,7 +208,11 @@ Lazy imports mask circular dependencies at import time but surface as confusing --- -### TASK-07 — Move schedule management out of `routers/blocklist.py` into a service 🟠 +### TASK-07 — Move schedule management out of `routers/blocklist.py` into a service ✅ + +**Status:** Completed ✅ + +**Summary:** Router schedule endpoints now delegate schedule persistence and APScheduler job management to `blocklist_service`, removing the router's direct dependency on `app.tasks.blocklist_import`. **Where:** `backend/app/routers/blocklist.py` — line 51: @@ -418,4 +422,4 @@ Additionally note in section 6 (Authentication): the `_session_cache` in `depend - Cross-reference TASK-05 once it is complete (the task should delegate to a service). **Why:** -The architecture document is a contract between contributors. Undocumented modules lead to inconsistent patterns and duplicate implementations when new developers add features that the undocumented module already handles. +The architecture document is a contract between contributors. Undocumented modules lead to inconsistent patterns and duplicate implementations when new developers add features that the undocumented module already handlzttttttttt nbbbbbbbbbbes. diff --git a/backend/app/routers/blocklist.py b/backend/app/routers/blocklist.py index 04c4d4b..df2742d 100644 --- a/backend/app/routers/blocklist.py +++ b/backend/app/routers/blocklist.py @@ -28,12 +28,12 @@ import aiosqlite from fastapi import APIRouter, Depends, HTTPException, Query, status from app.dependencies import ( - AppDep, AuthDep, Fail2BanSocketDep, GeoBatchLookupDep, HttpSessionDep, SchedulerDep, + SettingsDep, get_db, ) from app.models.blocklist import ( @@ -48,7 +48,6 @@ from app.models.blocklist import ( ScheduleInfo, ) from app.services import blocklist_service, geo_service, jail_service -from app.tasks import blocklist_import as blocklist_import_task router: APIRouter = APIRouter(prefix="/api/blocklists", tags=["Blocklists"]) @@ -162,7 +161,6 @@ async def get_schedule( The ``next_run_at`` field is read from APScheduler if the job is active. Args: - request: Incoming request (used to query the scheduler). db: Application database connection (injected). _auth: Validated session — enforces authentication. @@ -170,12 +168,7 @@ async def get_schedule( :class:`~app.models.blocklist.ScheduleInfo` with config and run times. """ - job = scheduler.get_job(blocklist_import_task.JOB_ID) - next_run_at: str | None = None - if job is not None and job.next_run_time is not None: - next_run_at = job.next_run_time.isoformat() - - return await blocklist_service.get_schedule_info(db, next_run_at) + return await blocklist_service.get_schedule_info_with_runtime(db, scheduler) @router.put( @@ -188,29 +181,29 @@ async def update_schedule( db: DbDep, _auth: AuthDep, scheduler: SchedulerDep, - app: AppDep, + http_session: HttpSessionDep, + settings: SettingsDep, ) -> ScheduleInfo: """Persist a new schedule configuration and reschedule the import job. Args: payload: New :class:`~app.models.blocklist.ScheduleConfig`. - app: FastAPI application instance used to reschedule the import job. db: Application database connection (injected). _auth: Validated session — enforces authentication. + scheduler: Shared APScheduler instance (injected). + http_session: Shared HTTP session used by the scheduler job. + settings: Current application settings used by the scheduler job. Returns: Updated :class:`~app.models.blocklist.ScheduleInfo`. """ - await blocklist_service.set_schedule(db, payload) - # Reschedule the background job immediately. - blocklist_import_task.reschedule(app) - - job = scheduler.get_job(blocklist_import_task.JOB_ID) - next_run_at: str | None = None - if job is not None and job.next_run_time is not None: - next_run_at = job.next_run_time.isoformat() - - return await blocklist_service.get_schedule_info(db, next_run_at) + return await blocklist_service.update_schedule( + db, + scheduler, + http_session, + settings, + payload, + ) @router.get( diff --git a/backend/app/services/blocklist_service.py b/backend/app/services/blocklist_service.py index f95d47f..53835e3 100644 --- a/backend/app/services/blocklist_service.py +++ b/backend/app/services/blocklist_service.py @@ -30,6 +30,7 @@ from app.models.blocklist import ( ImportSourceResult, PreviewResponse, ScheduleConfig, + ScheduleFrequency, ScheduleInfo, ) from app.repositories import blocklist_repo, import_log_repo, settings_repo @@ -40,7 +41,9 @@ if TYPE_CHECKING: import aiohttp import aiosqlite + from apscheduler.schedulers.asyncio import AsyncIOScheduler + from app.config import Settings from app.models.geo import GeoBatchLookup log: structlog.stdlib.BoundLogger = structlog.get_logger() @@ -505,6 +508,66 @@ async def import_all( _DEFAULT_SCHEDULE = ScheduleConfig() +#: Stable APScheduler job id for the blocklist import job. +JOB_ID: str = "blocklist_import" + + +def _get_job_next_run_at(scheduler: AsyncIOScheduler) -> str | None: + """Return the next scheduled run time as an ISO 8601 string.""" + job = scheduler.get_job(JOB_ID) + if job is None or job.next_run_time is None: + return None + return job.next_run_time.isoformat() + + +def schedule_blocklist_job( + scheduler: AsyncIOScheduler, + settings: Settings, + http_session: aiohttp.ClientSession, + config: ScheduleConfig, +) -> None: + """Register or replace the scheduled blocklist import job.""" + from app.tasks import blocklist_import as blocklist_import_task + + if scheduler.get_job(JOB_ID): + scheduler.remove_job(JOB_ID) + + kwargs: dict[str, object] = { + "settings": settings, + "http_session": http_session, + } + + if config.frequency == ScheduleFrequency.hourly: + trigger_type = "interval" + trigger_kwargs = {"hours": config.interval_hours} + elif config.frequency == ScheduleFrequency.weekly: + trigger_type = "cron" + trigger_kwargs = { + "day_of_week": config.day_of_week, + "hour": config.hour, + "minute": config.minute, + } + else: + trigger_type = "cron" + trigger_kwargs = { + "hour": config.hour, + "minute": config.minute, + } + + scheduler.add_job( + blocklist_import_task._run_import_with_resources, + trigger=trigger_type, + id=JOB_ID, + kwargs=kwargs, + **trigger_kwargs, + ) + log.info( + "blocklist_import_scheduled", + frequency=config.frequency, + trigger=trigger_type, + trigger_kwargs=trigger_kwargs, + ) + async def get_schedule(db: aiosqlite.Connection) -> ScheduleConfig: """Read the import schedule config from the settings table. @@ -576,6 +639,28 @@ async def get_schedule_info( ) +async def get_schedule_info_with_runtime( + db: aiosqlite.Connection, + scheduler: AsyncIOScheduler, +) -> ScheduleInfo: + """Return schedule info enriched with runtime scheduler metadata.""" + next_run_at = _get_job_next_run_at(scheduler) + return await get_schedule_info(db, next_run_at) + + +async def update_schedule( + db: aiosqlite.Connection, + scheduler: AsyncIOScheduler, + http_session: aiohttp.ClientSession, + settings: Settings, + config: ScheduleConfig, +) -> ScheduleInfo: + """Persist a new schedule config and re-register the scheduled job.""" + await set_schedule(db, config) + schedule_blocklist_job(scheduler, settings, http_session, config) + return await get_schedule_info(db, _get_job_next_run_at(scheduler)) + + async def list_import_logs( db: aiosqlite.Connection, *, diff --git a/backend/app/tasks/blocklist_import.py b/backend/app/tasks/blocklist_import.py index 9f57c45..e472033 100644 --- a/backend/app/tasks/blocklist_import.py +++ b/backend/app/tasks/blocklist_import.py @@ -18,7 +18,6 @@ from typing import TYPE_CHECKING, Any import structlog from app.db import open_db -from app.models.blocklist import ScheduleFrequency from app.services import blocklist_service, jail_service from app.utils.runtime_state import get_effective_settings @@ -130,46 +129,11 @@ def _apply_schedule(app: FastAPI, config: Any) -> None: app: FastAPI application instance. config: :class:`~app.models.blocklist.ScheduleConfig` to apply. """ - scheduler = app.state.scheduler + from app.services import blocklist_service - kwargs: dict[str, Any] = { - "settings": get_effective_settings(app), - "http_session": app.state.http_session, - } - trigger_type: str - trigger_kwargs: dict[str, Any] - - if config.frequency == ScheduleFrequency.hourly: - trigger_type = "interval" - trigger_kwargs = {"hours": config.interval_hours} - elif config.frequency == ScheduleFrequency.weekly: - trigger_type = "cron" - trigger_kwargs = { - "day_of_week": config.day_of_week, - "hour": config.hour, - "minute": config.minute, - } - else: # daily (default) - trigger_type = "cron" - trigger_kwargs = { - "hour": config.hour, - "minute": config.minute, - } - - # Remove existing job if it exists, then add new one. - if scheduler.get_job(JOB_ID): - scheduler.remove_job(JOB_ID) - - scheduler.add_job( - _run_import_with_resources, - trigger=trigger_type, - id=JOB_ID, - kwargs=kwargs, - **trigger_kwargs, - ) - log.info( - "blocklist_import_scheduled", - frequency=config.frequency, - trigger=trigger_type, - trigger_kwargs=trigger_kwargs, + blocklist_service.schedule_blocklist_job( + app.state.scheduler, + get_effective_settings(app), + app.state.http_session, + config, ) diff --git a/backend/tests/test_routers/test_blocklist.py b/backend/tests/test_routers/test_blocklist.py index 354bfcc..9b1d1cb 100644 --- a/backend/tests/test_routers/test_blocklist.py +++ b/backend/tests/test_routers/test_blocklist.py @@ -361,7 +361,7 @@ class TestGetSchedule: async def test_schedule_returns_200(self, bl_client: AsyncClient) -> None: """GET /api/blocklists/schedule returns 200.""" with patch( - "app.routers.blocklist.blocklist_service.get_schedule_info", + "app.routers.blocklist.blocklist_service.get_schedule_info_with_runtime", new=AsyncMock(return_value=_make_schedule_info()), ): resp = await bl_client.get("/api/blocklists/schedule") @@ -370,7 +370,7 @@ class TestGetSchedule: async def test_schedule_response_has_config(self, bl_client: AsyncClient) -> None: """Schedule response includes the config sub-object.""" with patch( - "app.routers.blocklist.blocklist_service.get_schedule_info", + "app.routers.blocklist.blocklist_service.get_schedule_info_with_runtime", new=AsyncMock(return_value=_make_schedule_info()), ): resp = await bl_client.get("/api/blocklists/schedule") @@ -396,7 +396,7 @@ class TestGetSchedule: last_run_errors=True, ) with patch( - "app.routers.blocklist.blocklist_service.get_schedule_info", + "app.routers.blocklist.blocklist_service.get_schedule_info_with_runtime", new=AsyncMock(return_value=info_with_errors), ): resp = await bl_client.get("/api/blocklists/schedule") @@ -425,13 +425,8 @@ class TestUpdateSchedule: last_run_at=None, ) with patch( - "app.routers.blocklist.blocklist_service.set_schedule", - new=AsyncMock(), - ), patch( - "app.routers.blocklist.blocklist_service.get_schedule_info", + "app.routers.blocklist.blocklist_service.update_schedule", new=AsyncMock(return_value=new_info), - ), patch( - "app.routers.blocklist.blocklist_import_task.reschedule", ): resp = await bl_client.put( "/api/blocklists/schedule", diff --git a/backend/tests/test_services/test_blocklist_service.py b/backend/tests/test_services/test_blocklist_service.py index 04eedac..9a98490 100644 --- a/backend/tests/test_services/test_blocklist_service.py +++ b/backend/tests/test_services/test_blocklist_service.py @@ -9,7 +9,11 @@ import aiosqlite import pytest from app.db import init_db -from app.models.blocklist import BlocklistSource, ScheduleConfig, ScheduleFrequency +from app.models.blocklist import ( + BlocklistSource, + ScheduleConfig, + ScheduleFrequency, +) from app.services import blocklist_service # --------------------------------------------------------------------------- @@ -346,6 +350,47 @@ class TestSchedule: info = await blocklist_service.get_schedule_info(db, None) assert info.last_run_errors is True + async def test_get_schedule_info_with_runtime_uses_scheduler_metadata( + self, db: aiosqlite.Connection + ) -> None: + """get_schedule_info_with_runtime derives next_run_at from the scheduler.""" + next_run = MagicMock() + next_run.isoformat.return_value = "2099-01-01T00:00:00+00:00" + scheduler = MagicMock() + scheduler.get_job.return_value = MagicMock(next_run_time=next_run) + + info = await blocklist_service.get_schedule_info_with_runtime(db, scheduler) + assert info.next_run_at == "2099-01-01T00:00:00+00:00" + + async def test_update_schedule_persists_and_schedules_job( + self, db: aiosqlite.Connection + ) -> None: + """update_schedule must persist the config and schedule a job.""" + settings = MagicMock( + fail2ban_socket="/var/run/fail2ban/fail2ban.sock", + database_path=":memory:", + ) + http_session = MagicMock() + scheduler = MagicMock() + scheduler.get_job.return_value = None + + config = ScheduleConfig( + frequency=ScheduleFrequency.daily, + hour=4, + minute=15, + ) + + info = await blocklist_service.update_schedule( + db, + scheduler, + http_session, + settings, + config, + ) + + assert info.config.frequency == ScheduleFrequency.daily + scheduler.add_job.assert_called_once() + # --------------------------------------------------------------------------- # Geo prewarm cache filtering