Invert blocklist scheduler dependency to task callback
This commit is contained in:
@@ -100,6 +100,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
|||||||
|
|
||||||
**Docs changes needed:** Update `Docs/Refactoring.md` and `Docs/Architekture.md` (task→service dependency direction).
|
**Docs changes needed:** Update `Docs/Refactoring.md` and `Docs/Architekture.md` (task→service dependency direction).
|
||||||
|
|
||||||
|
**Status:** Done ✅
|
||||||
|
|
||||||
**Why this is needed:** Circular layer dependencies (`service → task → service`) make it impossible to test services in isolation and create hidden initialisation-order coupling that can cause import errors or subtle bugs at startup.
|
**Why this is needed:** Circular layer dependencies (`service → task → service`) make it impossible to test services in isolation and create hidden initialisation-order coupling that can cause import errors or subtle bugs at startup.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|||||||
@@ -48,6 +48,7 @@ from app.models.blocklist import (
|
|||||||
ScheduleInfo,
|
ScheduleInfo,
|
||||||
)
|
)
|
||||||
from app.services import blocklist_service, geo_service, jail_service
|
from app.services import blocklist_service, geo_service, jail_service
|
||||||
|
from app.tasks.blocklist_import import run_import_with_resources
|
||||||
|
|
||||||
router: APIRouter = APIRouter(prefix="/api/blocklists", tags=["Blocklists"])
|
router: APIRouter = APIRouter(prefix="/api/blocklists", tags=["Blocklists"])
|
||||||
|
|
||||||
@@ -203,6 +204,7 @@ async def update_schedule(
|
|||||||
http_session,
|
http_session,
|
||||||
settings,
|
settings,
|
||||||
payload,
|
payload,
|
||||||
|
run_import_with_resources,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -525,9 +525,9 @@ def schedule_blocklist_job(
|
|||||||
settings: Settings,
|
settings: Settings,
|
||||||
http_session: aiohttp.ClientSession,
|
http_session: aiohttp.ClientSession,
|
||||||
config: ScheduleConfig,
|
config: ScheduleConfig,
|
||||||
|
run_import_callback: Callable[[Settings, aiohttp.ClientSession], Awaitable[None]],
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Register or replace the scheduled blocklist import job."""
|
"""Register or replace the scheduled blocklist import job."""
|
||||||
from app.tasks import blocklist_import as blocklist_import_task
|
|
||||||
|
|
||||||
if scheduler.get_job(JOB_ID):
|
if scheduler.get_job(JOB_ID):
|
||||||
scheduler.remove_job(JOB_ID)
|
scheduler.remove_job(JOB_ID)
|
||||||
@@ -555,7 +555,7 @@ def schedule_blocklist_job(
|
|||||||
}
|
}
|
||||||
|
|
||||||
scheduler.add_job(
|
scheduler.add_job(
|
||||||
blocklist_import_task._run_import_with_resources,
|
run_import_callback,
|
||||||
trigger=trigger_type,
|
trigger=trigger_type,
|
||||||
id=JOB_ID,
|
id=JOB_ID,
|
||||||
kwargs=kwargs,
|
kwargs=kwargs,
|
||||||
@@ -654,10 +654,17 @@ async def update_schedule(
|
|||||||
http_session: aiohttp.ClientSession,
|
http_session: aiohttp.ClientSession,
|
||||||
settings: Settings,
|
settings: Settings,
|
||||||
config: ScheduleConfig,
|
config: ScheduleConfig,
|
||||||
|
run_import_callback: Callable[[Settings, aiohttp.ClientSession], Awaitable[None]],
|
||||||
) -> ScheduleInfo:
|
) -> ScheduleInfo:
|
||||||
"""Persist a new schedule config and re-register the scheduled job."""
|
"""Persist a new schedule config and re-register the scheduled job."""
|
||||||
await set_schedule(db, config)
|
await set_schedule(db, config)
|
||||||
schedule_blocklist_job(scheduler, settings, http_session, config)
|
schedule_blocklist_job(
|
||||||
|
scheduler,
|
||||||
|
settings,
|
||||||
|
http_session,
|
||||||
|
config,
|
||||||
|
run_import_callback,
|
||||||
|
)
|
||||||
return await get_schedule_info(db, _get_job_next_run_at(scheduler))
|
return await get_schedule_info(db, _get_job_next_run_at(scheduler))
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -70,6 +70,9 @@ async def _run_import_with_resources(settings: Settings, http_session: ClientSes
|
|||||||
await db.close()
|
await db.close()
|
||||||
|
|
||||||
|
|
||||||
|
run_import_with_resources = _run_import_with_resources
|
||||||
|
|
||||||
|
|
||||||
async def _run_import(app: FastAPI) -> None:
|
async def _run_import(app: FastAPI) -> None:
|
||||||
await _run_import_with_resources(get_effective_settings(app), app.state.http_session)
|
await _run_import_with_resources(get_effective_settings(app), app.state.http_session)
|
||||||
|
|
||||||
@@ -136,4 +139,5 @@ def _apply_schedule(app: FastAPI, config: Any) -> None:
|
|||||||
get_effective_settings(app),
|
get_effective_settings(app),
|
||||||
app.state.http_session,
|
app.state.http_session,
|
||||||
config,
|
config,
|
||||||
|
run_import_with_resources,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -380,12 +380,15 @@ class TestSchedule:
|
|||||||
minute=15,
|
minute=15,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
run_import_callback = AsyncMock(return_value=None)
|
||||||
|
|
||||||
info = await blocklist_service.update_schedule(
|
info = await blocklist_service.update_schedule(
|
||||||
db,
|
db,
|
||||||
scheduler,
|
scheduler,
|
||||||
http_session,
|
http_session,
|
||||||
settings,
|
settings,
|
||||||
config,
|
config,
|
||||||
|
run_import_callback,
|
||||||
)
|
)
|
||||||
|
|
||||||
assert info.config.frequency == ScheduleFrequency.daily
|
assert info.config.frequency == ScheduleFrequency.daily
|
||||||
|
|||||||
@@ -9,7 +9,7 @@ registers the correct APScheduler trigger for each frequency preset.
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
from typing import Any
|
from typing import Any
|
||||||
from unittest.mock import AsyncMock, MagicMock, call, patch
|
from unittest.mock import ANY, AsyncMock, MagicMock, call, patch
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
@@ -42,6 +42,7 @@ def _make_app(
|
|||||||
fail2ban_socket="/var/run/fail2ban/fail2ban.sock",
|
fail2ban_socket="/var/run/fail2ban/fail2ban.sock",
|
||||||
database_path="/tmp/fake.db",
|
database_path="/tmp/fake.db",
|
||||||
)
|
)
|
||||||
|
app.state.runtime_settings = None
|
||||||
return app
|
return app
|
||||||
|
|
||||||
|
|
||||||
@@ -111,6 +112,7 @@ class TestRunImport:
|
|||||||
app.state.db,
|
app.state.db,
|
||||||
app.state.http_session,
|
app.state.http_session,
|
||||||
app.state.settings.fail2ban_socket,
|
app.state.settings.fail2ban_socket,
|
||||||
|
ban_ip=ANY,
|
||||||
)
|
)
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
@@ -196,6 +198,7 @@ class TestApplySchedule:
|
|||||||
"""
|
"""
|
||||||
app = MagicMock()
|
app = MagicMock()
|
||||||
app.state.scheduler = scheduler
|
app.state.scheduler = scheduler
|
||||||
|
app.state.runtime_settings = None
|
||||||
return app
|
return app
|
||||||
|
|
||||||
def test_apply_schedule_daily_registers_cron_trigger(self) -> None:
|
def test_apply_schedule_daily_registers_cron_trigger(self) -> None:
|
||||||
|
|||||||
Reference in New Issue
Block a user