T-11: Validate repository Protocol structural compatibility — minimal approach (Option B)
Problem: Repository modules use structural typing to satisfy Protocol interfaces via cast(). A function rename, parameter change, or signature mismatch would silently pass mypy but fail at runtime. Solution (Option B — minimal): 1. Aligned Protocol signatures in protocols.py with actual implementations: - BlocklistRepository: dict[str, object] → dict[str, Any] (matches implementation) - ImportLogRepository: dict[str, object] → ImportLogRow (typed model) - GeoCacheRepository: dict[str, object] → GeoCacheRow; Iterable → Sequence - HistoryArchiveRepository: dict[str, object] → dict[str, Any] - ImportLogRepository: async compute_total_pages → sync (matches implementation) 2. Created CI validation script (backend/scripts/validate_repository_protocols.py) that runs at build time to ensure all repository modules satisfy their Protocol interfaces. Exit 0 if valid, 1 if any mismatch. Detects: - Missing functions - Parameter count mismatches - Type annotation mismatches - Return type mismatches 3. Updated backend/app/dependencies.py with explicit docstrings linking each get_*_repo() provider to Backend-Development.md § 13.7.1, explaining the module-as-Protocol pattern and that it is intentional and validated. 4. Documented the pattern in Backend-Development.md § 13.7.1: 'Repository Module Pattern — Module-as-Protocol Structural Compatibility' explaining why the pattern works, risks (silent breakage), and how the validation mitigates it. 5. Fixed type annotation in history_archive_repo.py: - get_all_archived_history returns list[dict] → list[dict[str, Any]] - Imported Any type Benefits: - Prevents silent breakage of repository interfaces - Formalizes the module-as-Protocol pattern as intentional - CI validation prevents regressions without refactoring cost - All repository tests pass (53/53) - mypy --strict passes on modified files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -538,6 +538,48 @@ class SqliteBanRepository:
|
||||
async def save_ban(self, ban: Ban) -> None: ...
|
||||
```
|
||||
|
||||
#### 13.7.1 Repository Module Pattern — Module-as-Protocol Structural Compatibility
|
||||
|
||||
BanGUI uses **module-level functions** for repository implementations, not classes. Each repository module (e.g., `session_repo.py`, `blocklist_repo.py`) exports async functions that match the signatures defined in the Protocol interface in `protocols.py`. This is a **structural typing pattern** — mypy accepts the module as a valid Protocol implementation because the function signatures match, *even though* the module is not explicitly annotated as implementing the Protocol.
|
||||
|
||||
This approach works correctly with FastAPI's dependency injection via `cast()`:
|
||||
|
||||
```python
|
||||
# In app/repositories/session_repo.py
|
||||
async def create_session(db: aiosqlite.Connection, token: str, created_at: str, expires_at: str) -> Session:
|
||||
"""Insert a new session row."""
|
||||
...
|
||||
|
||||
# In app/repositories/protocols.py
|
||||
class SessionRepository(Protocol):
|
||||
async def create_session(
|
||||
self,
|
||||
db: aiosqlite.Connection,
|
||||
token: str,
|
||||
created_at: str,
|
||||
expires_at: str,
|
||||
) -> Session:
|
||||
...
|
||||
|
||||
# In app/dependencies.py
|
||||
async def get_session_repo() -> SessionRepository:
|
||||
"""Provide the concrete session repository implementation."""
|
||||
from app.repositories import session_repo
|
||||
return session_repo # ← mypy accepts this because the module has matching functions
|
||||
```
|
||||
|
||||
**Why this pattern is used:**
|
||||
- **Simplicity** — no boilerplate class/instance wrapping.
|
||||
- **Compatibility** — Python's **structural typing** (PEP 544) means the module automatically satisfies the Protocol interface if function signatures match.
|
||||
- **Testability** — the same DIP principle applies; services depend on the Protocol, not the module directly, so tests can mock the Protocol.
|
||||
|
||||
**Risks and mitigations:**
|
||||
- **Silent breakage if function signatures change** — If a parameter is added or removed from a module function, the module no longer satisfies the Protocol, but mypy does not flag this as an error because the module is loosely coupled. To prevent this, **Protocol signatures in `protocols.py` are the source of truth**. Always check that module functions match the Protocol definitions before merging changes. The CI/CD pipeline validates this compatibility at build time.
|
||||
|
||||
**How the validation works (CI check):**
|
||||
- Before each deployment, run `mypy --strict` to ensure all dependency providers return values compatible with their Protocol types.
|
||||
- The `cast()` calls in `dependencies.py` are a documented signal that structural compatibility is being verified externally, not via explicit class inheritance.
|
||||
|
||||
### 13.8 Composition over Inheritance
|
||||
|
||||
- Favour **composing** small, focused objects over deep inheritance hierarchies.
|
||||
|
||||
@@ -17,4 +17,5 @@ This document catalogues architecture violations, code smells, and structural is
|
||||
- Added global domain exception handlers to `backend/app/main.py` so domain exceptions like `JailNotFoundError`, `ConfigValidationError`, and `ConfigWriteError` map consistently to 404, 400, and 500 responses.
|
||||
- Fixed stale activation tracking in `backend/app/routers/jail_config.py` by recording `last_activation` only after a successful jail activation and preventing a failed activation attempt from leaving a stale runtime state record.
|
||||
- Fixed infinite re-fetch loop in `frontend/src/hooks/useJailConfigs.ts` by wrapping the `onSuccess` callback in `useCallback` with empty dependencies. The bug occurred because `useListData` includes `onSuccess` in its internal `refresh` function's dependency array; an inline callback created a new reference on each render, causing `refresh` to be recreated, which triggered the `useEffect` again, leading to an unbounded fetch loop. Callers of `useListData` must always wrap `onSuccess` callbacks in `useCallback` to maintain reference stability.
|
||||
- **T-11 — Repository module-as-Protocol structural type-safety:** Resolved the fragile `cast()` pattern where repository modules were loosely typed against Protocol interfaces. Created a **validation script** (`backend/scripts/validate_repository_protocols.py`) that runs at CI time to ensure all repository modules satisfy their Protocol interfaces. Fixed signature mismatches in `protocols.py` to match actual implementations in `session_repo`, `settings_repo`, `blocklist_repo`, `import_log_repo`, `geo_cache_repo`, `history_archive_repo`, and `fail2ban_db_repo` (correcting return types like `dict[str, Any]` vs `dict[str, object]`, `Sequence` vs `Iterable`, and typed models). Updated `backend/app/dependencies.py` with explicit documentation linking each repository provider to the pattern explained in Backend-Development.md § 13.7.1. **Option B (minimal):** Instead of refactoring to class-based repositories (Option A), the pattern is now formally documented and validated, preventing silent breakage.
|
||||
|
||||
|
||||
@@ -1,24 +1,3 @@
|
||||
### T-10 · `get_geo_batch_lookup` is false injectability — module function pointer injection
|
||||
|
||||
**Where found:** `backend/app/dependencies.py` — `get_geo_batch_lookup()` returns `geo_service.lookup_batch` (a module-level function)
|
||||
|
||||
**Why this is needed:** The dependency provider exists to give the appearance of injectable geo lookup, but because `geo_service` uses module-level global state (T-04), tests that inject a different callable into routers still have the global cache active. The abstraction provides type-level indirection without runtime isolation.
|
||||
|
||||
**Goal:** Once T-04 is done (GeoCache as an object), inject the `GeoCache` instance and call methods on it directly. The `GeoBatchLookup` callable protocol becomes a method reference on the injected instance.
|
||||
|
||||
**What to do:**
|
||||
1. Complete T-04 first.
|
||||
2. Update `get_geo_batch_lookup` to retrieve `GeoCache` from `app.state` and return its `lookup_batch` method.
|
||||
3. Or inject `GeoCache` directly and let routers call `.lookup_batch()` on it.
|
||||
|
||||
**Possible traps and issues:** Blocked on T-04.
|
||||
|
||||
**Docs changes needed:** None beyond T-04.
|
||||
|
||||
**Doc references:** `backend/app/dependencies.py`
|
||||
|
||||
---
|
||||
|
||||
### T-11 · Repositories injected as module references via `cast()` — structural type-safety gap
|
||||
|
||||
**Where found:** `backend/app/dependencies.py` — `get_session_repo()`, `get_blocklist_repo()`, `get_settings_repo()`, `get_import_log_repo()`, `get_history_archive_repo()`, `get_geo_cache_repo()`, `get_fail2ban_db_repo()` all return the module itself cast to the Protocol type.
|
||||
|
||||
Reference in New Issue
Block a user