Refactor backend to use request-scoped SQLite connections
This commit is contained in:
@@ -10,64 +10,71 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
|
||||
|
||||
---
|
||||
|
||||
### TASK-001 — WorldMap: filter companion table by selected country (server-side)
|
||||
### Backend Architecture
|
||||
|
||||
**Status:** Done
|
||||
**Priority:** Medium
|
||||
**Domain:** Full-stack (backend + frontend)
|
||||
**References:** `Docs/Features.md §4`, `Docs/Web-Development.md`
|
||||
- **Replace the single shared SQLite connection.** ✅
|
||||
- Current startup code opens one `aiosqlite.Connection` and reuses it for every request.
|
||||
- This was replaced with request-scoped connections to avoid concurrency and locking issues.
|
||||
- Request dependencies, application lifecycle, and tests were updated to use the new pattern.
|
||||
|
||||
#### Background
|
||||
- **Refactor dependency wiring and shared resource management.**
|
||||
- Remove hidden module-level import coupling between routers, services, and shared utilities.
|
||||
- Introduce explicit factories or providers for shared resources such as DB, HTTP client session, scheduler, and settings.
|
||||
- Ensure routers depend on injected providers rather than global state or dynamic imports.
|
||||
|
||||
The `GET /api/dashboard/bans/by-country` endpoint always returns the **200 most recent** ban rows in `bans` (constant `_MAX_COMPANION_BANS = 200` in `backend/app/services/ban_service.py`). `MapPage.tsx` stores a `selectedCountry` state and filters the returned rows client-side via `visibleBans`. This means the companion table can only show the fraction of a country's bans that fall within the global top-200 window. If the selected time range has, say, 1 500 bans and 300 are from China, but China's bans are not all in the top 200 overall, the table will silently display fewer than 300 rows.
|
||||
- **Harden fail2ban integration.**
|
||||
- Remove the `sys.path` hack that locates `fail2ban-master` at runtime.
|
||||
- Replace it with a deterministic packaging or configuration model so the backend does not depend on repository layout.
|
||||
- Refactor `Fail2BanClient` so concurrency control is instance-based and not backed by hidden module globals.
|
||||
|
||||
When a country is selected the companion table **must** return the complete set of bans for that country so the user sees an accurate picture.
|
||||
- **Improve startup / setup guard behavior.**
|
||||
- Convert `SetupRedirectMiddleware` from an on-demand DB check into a startup/initialisation guard where possible.
|
||||
- Cache setup completion in a safe way and provide an explicit invalidation path if the application state changes.
|
||||
- Reduce middleware responsibility and avoid DB access during normal request dispatch.
|
||||
|
||||
#### Desired behaviour
|
||||
- **Make deployment configuration explicit.**
|
||||
- Move hard-coded environment assumptions such as CORS origins into settings.
|
||||
- Ensure `fail2ban_socket`, `fail2ban_config_dir`, and startup commands are fully configurable via `Settings`.
|
||||
- Document production-ready defaults separately from development defaults.
|
||||
|
||||
- No country selected → companion table shows the 200 most recent bans across all countries (existing behaviour, no change).
|
||||
- Country selected → the server returns **all** ban entries for that country in the selected time window; no client-side row-count cap applies.
|
||||
- Deselecting a country (clicking the same country again, or the "Clear filter" button) reverts to the default 200-row unfiltered view.
|
||||
- The existing `visibleBans` client-side filter in `MapPage.tsx` can remain as a defensive guard but must not be the only filter.
|
||||
### Reliability and Resilience
|
||||
|
||||
#### Implementation steps
|
||||
- **Add backend lifecycle tests for resource cleanup.**
|
||||
- Verify startup opens and initialises DB, HTTP session, scheduler, and geo cache correctly.
|
||||
- Verify shutdown closes those resources cleanly.
|
||||
|
||||
1. **Backend — router** (`backend/app/routers/dashboard.py`)
|
||||
- Add `country_code: str | None = Query(default=None, description="ISO alpha-2 country code to filter companion rows.")` to `get_bans_by_country`.
|
||||
- Pass it to `ban_service.bans_by_country(..., country_code=country_code)`.
|
||||
- **Add concurrency/regression coverage for DB and fail2ban socket use.**
|
||||
- Add tests that simulate multiple concurrent requests using the same DB dependency.
|
||||
- Add tests around fail2ban socket retries, protocol errors, and rate limiting.
|
||||
|
||||
2. **Backend — service** (`backend/app/services/ban_service.py`)
|
||||
- Add `country_code: str | None = None` keyword argument to `bans_by_country`.
|
||||
- After `geo_map` is built (existing geo-resolution step), collect IPs whose resolved country matches `country_code`.
|
||||
- For the **fail2ban source**: call `fail2ban_db_repo.get_currently_banned` with `ip_filter=matched_ips` and no `limit` (remove the `_MAX_COMPANION_BANS` cap for filtered queries).
|
||||
- For the **archive source**: filter `all_rows` to those whose IP is in `matched_ips` and return all of them (skip the `page_size=_MAX_COMPANION_BANS` call).
|
||||
- When `country_code` is `None`, behaviour is identical to today.
|
||||
- **Improve state caching and invalidation.**
|
||||
- Add tests for session cache invalidation on logout.
|
||||
- Add tests for setup completion caching so stale state is never served.
|
||||
|
||||
3. **Backend — repository** (`backend/app/repositories/fail2ban_db_repo.py`)
|
||||
- Add `ip_filter: list[str] | None = None` to `get_currently_banned`.
|
||||
- When provided and non-empty, append `AND ip IN ({placeholders})` to the SQL `WHERE` clause, parameterised safely (never interpolated as a string).
|
||||
### Backend Feature Work
|
||||
|
||||
4. **Backend — repository (archive)** (`backend/app/repositories/history_archive_repo.py`)
|
||||
- Similarly add optional `ip_filter` to the archive companion-rows query used from `bans_by_country`.
|
||||
- **Document and implement backend-safe environment-driven CORS.**
|
||||
- Add support for production and local development origins through configuration.
|
||||
- Avoid a hardcoded Vite origin in the core app factory.
|
||||
|
||||
5. **Frontend — API client** (`frontend/src/api/map.ts`)
|
||||
- Add optional `countryCode?: string` parameter to `fetchBansByCountry`.
|
||||
- When set, append `country_code=<value>` to the query string.
|
||||
- **Centralise scheduler job registration.**
|
||||
- Refactor APScheduler registration so background tasks are registered through a common lifecycle helper.
|
||||
- Ensure jobs can be discovered, replaced, and tested without requiring implicit `app.state` side effects.
|
||||
|
||||
6. **Frontend — hook** (`frontend/src/hooks/useMapData.ts`)
|
||||
- Add `countryCode?: string` to the function signature.
|
||||
- Include it in the `useCallback` dependency array and pass it to `fetchBansByCountry`.
|
||||
- **Strengthen fail2ban error handling and reporting.**
|
||||
- Standardise `502` responses for connection/protocol failures across all endpoints.
|
||||
- Add structured logging for retries and fatal socket failures.
|
||||
- Ensure the UI can distinguish offline fail2ban from internal backend failures.
|
||||
|
||||
7. **Frontend — page** (`frontend/src/pages/MapPage.tsx`)
|
||||
- Pass `selectedCountry ?? undefined` as `countryCode` to `useMapData`.
|
||||
- The hook's effect will re-fetch automatically when `selectedCountry` changes; the existing `useEffect` that resets `page` to 1 already covers this.
|
||||
- **Improve documentation of backend responsibilities.**
|
||||
- Keep `Docs/Tasks.md` aligned with the backend architecture review.
|
||||
- Add references to the backend modules, resource lifecycle, and dependency model in the documentation.
|
||||
|
||||
#### Testing guidance
|
||||
### Priority Execution Plan
|
||||
|
||||
- Select a country that has > 200 bans in the chosen time window; confirm the companion table shows more than the previous cap would allow.
|
||||
- With no country selected, confirm only 200 rows are returned (no regression).
|
||||
- Deselect the country; confirm the unfiltered 200-row view is restored.
|
||||
- Test with the archive source as well as the fail2ban live source.
|
||||
- Verify the `ip_filter` SQL clause is parameterised and cannot be injected.
|
||||
|
||||
---
|
||||
1. Fix the global SQLite connection pattern and tests.
|
||||
2. Refactor dependency injection / explicit shared resources.
|
||||
3. Harden fail2ban client concurrency and packaging.
|
||||
4. Convert setup guard to a safer startup-driven model.
|
||||
5. Add deployment-safe configuration and production-ready CORS.
|
||||
6. Add lifecycle and concurrency regression tests.
|
||||
|
||||
Reference in New Issue
Block a user