lukas.pupkalipinski/BanGUI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: complete Task 2/3 geo decouple + exceptions centralization; mark as done

Browse Source
This commit is contained in:
Lukas
2026-03-21 17:15:02 +01:00
parent 3aba2b6446
commit a442836c5c
28 changed files with 803 additions and 571 deletions
Download Patch File Download Diff File Expand all files Collapse all files

365
Docs/Refactoring.md
View File

@@ -1,238 +1,195 @@
# BanGUI — Refactoring Instructions for AI Agents
# BanGUI — Architecture Issues & Refactoring Plan
This document is the single source of truth for any AI agent performing a refactoring task on the BanGUI codebase.
Read it in full before writing a single line of code.
The authoritative description of every module, its responsibilities, and the allowed dependency direction is in [Architekture.md](Architekture.md). Always cross-reference it.
This document catalogues architecture violations, code smells, and structural issues found during a full project review. Issues are grouped by category and prioritised.
---
## 0. Golden Rules
## 1. Service-to-Service Coupling (Backend)
1. **Architecture first.** Every change must comply with the layered architecture defined in [Architekture.md §2](Architekture.md). Dependencies flow inward: `routers → services → repositories`. Never add an import that reverses this direction.
2. **One concern per file.** Each module has an explicitly stated purpose in [Architekture.md](Architekture.md). Do not add responsibilities to a module that do not belong there.
3. **No behaviour change.** Refactoring must preserve all existing behaviour. If a function's public signature, return value, or side-effects must change, that is a feature — create a separate task for it.
4. **Tests stay green.** Run the full test suite (`pytest backend/`) before and after every change. Do not submit work that introduces new failures.
5. **Smallest diff wins.** Prefer targeted edits. Do not rewrite a file when a few lines suffice.
The architecture mandates that dependencies flow **routers → services → repositories**, yet **15 service-to-service imports** exist, with 7 using lazy imports to work around circular dependencies.
| Source Service | Imports From | Line | Mechanism |
|---|---|---|---|
| `history_service` | `ban_service` | L29 | Direct import of 3 **private** functions: `_get_fail2ban_db_path`, `_parse_data_json`, `_ts_to_iso` |
| `auth_service` | `setup_service` | L23 | Top-level import |
| `config_service` | `setup_service` | L47 | Top-level import |
| `config_service` | `health_service` | L891 | Lazy import inside function |
| `config_file_service` | `jail_service` | L57–58 | Top-level import + re-export of `JailNotFoundError` |
| `blocklist_service` | `jail_service` | L299 | Lazy import |
| `blocklist_service` | `geo_service` | L343 | Lazy import |
| `jail_service` | `geo_service` | L860, L1047 | Lazy import (2 sites) |
| `ban_service` | `geo_service` | L251, L392 | Lazy import (2 sites) |
| `history_service` | `geo_service` | L19 | TYPE_CHECKING import |
**Impact**: Circular dependency risk; lazy imports hide coupling; private function imports create fragile links between services.
**Recommendation**:
- Extract `_get_fail2ban_db_path()`, `_parse_data_json()`, `_ts_to_iso()` from `ban_service` to `app/utils/fail2ban_db_utils.py` (shared utility).
- Pass geo-enrichment as a callback parameter instead of each service importing `geo_service` directly. The router or dependency layer should wire this.
- Where services depend on another service's domain exceptions (e.g., `JailNotFoundError`), move exceptions to `app/models/` or a shared `app/exceptions.py`.
---
## 1. Before You Start
## 2. God Modules (Backend)
### 1.1 Understand the project
Several service files far exceed a reasonable size for a single-domain module:
Read the following documents in order:
| File | Lines | Functions | Issue |
|---|---|---|---|
| `config_file_service.py` | **3105** | **73** | Handles jails, filters, and actions — three distinct domains crammed into one file |
| `jail_service.py` | **1382** | **34** | Manages jail listing, status, controls, banned-IP queries, and geo enrichment |
| `config_service.py` | **921** | ~20 | Socket-based config, log preview, regex testing, and service status |
| `file_config_service.py` | **1011** | ~20 | Raw file I/O for jails, filters, and actions |
1. [Architekture.md](Architekture.md) — full system overview, component map, module purposes, dependency rules.
2. [Docs/Backend-Development.md](Backend-Development.md) — coding conventions, testing strategy, environment setup.
3. [Docs/Tasks.md](Tasks.md) — open issues and planned work; avoid touching areas that have pending conflicting changes.
**Recommendation**:
- Split `config_file_service.py` into `filter_config_service.py`, `action_config_service.py`, and a slimmed-down `jail_config_service.py`.
- Extract log-preview / regex-test functionality from `config_service.py` into a dedicated `log_service.py`.
### 1.2 Map the code to the architecture
---
Before editing, locate every file that is in scope:
## 3. Confusing Config Service Naming (Backend)
```
backend/app/
routers/ HTTP layer — zero business logic
services/ Business logic — orchestrates repositories + clients
repositories/ Data access — raw SQL only
models/ Pydantic schemas
tasks/ APScheduler jobs
utils/ Pure helpers, no framework deps
main.py App factory, lifespan, middleware
config.py Pydantic settings
dependencies.py FastAPI Depends() wiring
Three services with overlapping names handle different aspects of configuration, causing developer confusion:
frontend/src/
api/ Typed fetch wrappers + endpoint constants
components/ Presentational UI, no API calls
hooks/ All state, side-effects, API calls
pages/ Route components — orchestration only
providers/ React context
types/ TypeScript interfaces
utils/ Pure helpers
| Current Name | Purpose |
|---|---|
| `config_service` | Read/write via the fail2ban **socket** |
| `config_file_service` | Parse/activate/deactivate jails from **files on disk** |
| `file_config_service` | **Raw file I/O** for jail/filter/action `.conf` files |
`config_file_service` vs `file_config_service` differ only by word order, making it easy to import the wrong one.
**Recommendation**: Rename for clarity:
- `config_service` → keep (socket-based)
- `config_file_service` → `jail_activation_service` (its main job is activating/deactivating jails)
- `file_config_service` → `raw_config_io_service` or merge into `config_file_service`
---
## 4. Architecture Doc Drift
The architecture doc does not fully reflect the current codebase:
| Category | In Architecture Doc | Actually Exists | Notes |
|---|---|---|---|
| Repositories | 4 listed | **6 files** | `fail2ban_db_repo.py` and `geo_cache_repo.py` are missing from the doc |
| Utils | 4 listed | **8 files** | `conffile_parser.py`, `config_parser.py`, `config_writer.py`, `jail_config.py` are undocumented |
| Tasks | 3 listed | **4 files** | `geo_re_resolve.py` is missing from the doc |
| Services | `conffile_parser` listed as a service | Actually in `app/utils/` | Doc says "Services" but the file is in `utils/` |
| Routers | `file_config.py` not listed | Exists | Missing from router table |
**Recommendation**: Update the Architecture doc to reflect the actual file inventory.
---
## 5. Shared Private Functions Cross Service Boundary (Backend)
`history_service.py` imports three **underscore-prefixed** ("private") functions from `ban_service.py`:
```python
from app.services.ban_service import _get_fail2ban_db_path, _parse_data_json, _ts_to_iso
```
Confirm which layer every file you intend to touch belongs to. If unsure, consult [Architekture.md §2.2](Architekture.md) (backend) or [Architekture.md §3.2](Architekture.md) (frontend).
These are implementation details of `ban_service` that should not be consumed externally. Their `_` prefix signals they are not part of the public API.
### 1.3 Run the baseline
```bash
# Backend
pytest backend/ -x --tb=short
# Frontend
cd frontend && npm run test
```
Record the number of passing tests. After refactoring, that number must be equal or higher.
**Recommendation**: Move these to `app/utils/fail2ban_db_utils.py` as public functions and import from there in both services.
---
## 2. Backend Refactoring
## 6. Missing Error Boundaries (Frontend)
### 2.1 Routers (`app/routers/`)
No React Error Boundary component exists anywhere in the frontend. A single unhandled exception in any component will crash the entire application with a white screen.
**Allowed content:** request parsing, response serialisation, dependency injection via `Depends()`, delegation to a service, HTTP error mapping.
**Forbidden content:** SQL queries, business logic, direct use of `fail2ban_client`, any logic that would also make sense in a unit test without an HTTP request.
Checklist:
- [ ] Every handler calls exactly one service method per logical operation.
- [ ] No `if`/`elif` chains that implement business rules — move these to the service.
- [ ] No raw SQL or repository imports.
- [ ] All response models are Pydantic schemas from `app/models/`.
- [ ] HTTP status codes are consistent with API conventions (200 OK, 201 Created, 204 No Content, 400/422 for client errors, 404 for missing resources, 500 only for unexpected failures).
### 2.2 Services (`app/services/`)
**Allowed content:** business rules, coordination between repositories and external clients, validation that goes beyond Pydantic, fail2ban command orchestration.
**Forbidden content:** raw SQL, direct aiosqlite calls, FastAPI `HTTPException` (raise domain exceptions instead and let the router or exception handler convert them).
Checklist:
- [ ] Service classes / functions accept plain Python types or domain models — not `Request` or `Response` objects.
- [ ] No direct `aiosqlite` usage — go through a repository.
- [ ] No `HTTPException` — raise a custom domain exception or a plain `ValueError`/`RuntimeError` with a clear message.
- [ ] No circular imports between services — if two services need each other's logic, extract the shared logic to a utility or a third service.
### 2.3 Repositories (`app/repositories/`)
**Allowed content:** SQL queries, result mapping to domain models, transaction management.
**Forbidden content:** business logic, fail2ban calls, HTTP concerns, logging beyond debug-level traces.
Checklist:
- [ ] Every public method accepts a `db: aiosqlite.Connection` parameter — sessions are not managed internally.
- [ ] Methods return typed domain models or plain Python primitives, never raw `aiosqlite.Row` objects exposed to callers.
- [ ] No business rules (e.g., no "if this setting is missing, create a default" logic — that belongs in the service).
### 2.4 Models (`app/models/`)
- Keep **Request**, **Response**, and **Domain** model types clearly separated (see [Architekture.md §2.2](Architekture.md)).
- Do not use response models as function arguments inside service or repository code.
- Validators (`@field_validator`, `@model_validator`) belong in models only when they concern data shape, not business rules.
### 2.5 Tasks (`app/tasks/`)
- Tasks must be thin: fetch inputs → call one service method → log result.
- Error handling must be inside the task (APScheduler swallows unhandled exceptions — log them explicitly).
- No direct repository or `fail2ban_client` use; go through a service.
### 2.6 Utils (`app/utils/`)
- Must have zero framework dependencies (no FastAPI, no aiosqlite imports).
- Must be pure or near-pure functions.
- `fail2ban_client.py` is the single exception — it wraps the socket protocol but still has no service-layer logic.
### 2.7 Dependencies (`app/dependencies.py`)
- This file is the **only** place where service constructors are called and injected.
- Do not construct services inside router handlers; always receive them via `Depends()`.
**Recommendation**: Add an `<ErrorBoundary>` wrapper in `MainLayout.tsx` or `App.tsx` with a fallback UI that shows the error and offers a retry/reload.
---
## 3. Frontend Refactoring
## 7. Duplicated Formatting Functions (Frontend)
### 3.1 Pages (`src/pages/`)
Several formatting functions are independently defined in multiple files instead of being shared:
**Allowed content:** composing components and hooks, layout decisions, routing.
**Forbidden content:** direct `fetch`/`axios` calls, inline business logic, state management beyond what is needed to coordinate child components.
Checklist:
- [ ] All data fetching goes through a hook from `src/hooks/`.
- [ ] No API function from `src/api/` is called directly inside a page component.
### 3.2 Components (`src/components/`)
**Allowed content:** rendering, styling, event handlers that call prop callbacks.
**Forbidden content:** API calls, hook-level state (prefer lifting state to the page or a dedicated hook), direct use of `src/api/`.
Checklist:
- [ ] Components receive all data via props.
- [ ] Components emit changes via callback props (`onXxx`).
- [ ] No `useEffect` that calls an API function — that belongs in a hook.
### 3.3 Hooks (`src/hooks/`)
**Allowed content:** `useState`, `useEffect`, `useCallback`, `useRef`; calls to `src/api/`; local state derivation.
**Forbidden content:** JSX rendering, Fluent UI components.
Checklist:
- [ ] Each hook has a single, focused concern matching its name (e.g., `useBans` only manages ban data).
- [ ] Hooks return a stable interface: `{ data, loading, error, refetch }` or equivalent.
- [ ] Shared logic between hooks is extracted to `src/utils/` (pure) or a parent hook (stateful).
### 3.4 API layer (`src/api/`)
- `client.ts` is the only place that calls `fetch`. All other api files call `client.ts`.
- `endpoints.ts` is the single source of truth for URL strings.
- API functions must be typed: explicit request and response TypeScript interfaces from `src/types/`.
### 3.5 Types (`src/types/`)
- Interfaces must match the backend Pydantic response schemas exactly (field names, optionality).
- Do not use `any`. Use `unknown` and narrow with type guards when the shape is genuinely unknown.
---
## 4. General Code Quality Rules
### Naming
- Python: `snake_case` for variables/functions, `PascalCase` for classes.
- TypeScript: `camelCase` for variables/functions, `PascalCase` for components and types.
- File names must match the primary export they contain.
### Error handling
- Backend: raise typed exceptions; map them to HTTP status codes in `main.py` exception handlers or in the router — nowhere else.
- Frontend: all API call error states are represented in hook return values; never swallow errors silently.
### Logging (backend)
- Use `structlog` with bound context loggers — never bare `print()`.
- Log at `debug` in repositories, `info` in services for meaningful events, `warning`/`error` in tasks and exception handlers.
- Never log sensitive data (passwords, session tokens, raw IP lists larger than a handful of entries).
### Async correctness (backend)
- Every function that touches I/O (database, fail2ban socket, HTTP) must be `async def`.
- Never call `asyncio.run()` inside a running event loop.
- Do not use `time.sleep()` — use `await asyncio.sleep()`.
---
## 5. Refactoring Workflow
Follow this sequence for every refactoring task:
1. **Read** the relevant section of [Architekture.md](Architekture.md) for the files you will touch.
2. **Run** the full test suite to confirm the baseline.
3. **Identify** the violation or smell: which rule from this document does it break?
4. **Plan** the minimal change: what is the smallest edit that fixes the violation?
5. **Edit** the code. One logical change per commit.
6. **Verify** imports: nothing new violates the dependency direction.
7. **Run** the full test suite. All previously passing tests must still pass.
8. **Update** any affected docstrings or inline comments to reflect the new structure.
9. **Do not** update `Architekture.md` unless the refactor changes the documented structure — that requires a separate review.
---
## 6. Common Violations to Look For
| Violation | Where it typically appears | Fix |
| Function | Defined In | Also In |
|---|---|---|
| Business logic in a router handler | `app/routers/*.py` | Extract logic to the corresponding service |
| Direct `aiosqlite` calls in a service | `app/services/*.py` | Move the query into the matching repository |
| `HTTPException` raised inside a service | `app/services/*.py` | Raise a domain exception; catch and convert it in the router or exception handler |
| API call inside a React component | `src/components/*.tsx` | Move to a hook; pass data via props |
| Hardcoded URL string in a hook or component | `src/hooks/*.ts`, `src/components/*.tsx` | Use the constant from `src/api/endpoints.ts` |
| `any` type in TypeScript | anywhere in `src/` | Replace with a concrete interface from `src/types/` |
| `print()` statements in production code | `backend/app/**/*.py` | Replace with `structlog` logger |
| Synchronous I/O in an async function | `backend/app/**/*.py` | Use the async equivalent |
| A repository method that contains an `if` with a business rule | `app/repositories/*.py` | Move the rule to the service layer |
| `formatTimestamp()` | `BanTable.tsx` (L103) | — (but `fmtTime()` in `BannedIpsSection.tsx` does the same thing) |
| `fmtSeconds()` | `JailDetailPage.tsx` (L152) | `JailsPage.tsx` (L147) — identical |
| `fmtTime()` | `BannedIpsSection.tsx` (L139) | — |
**Recommendation**: Consolidate into `src/utils/formatDate.ts` and import from there.
---
## 7. Out of Scope
## 8. Duplicated Hook Logic (Frontend)
Do not make the following changes unless explicitly instructed in a separate task:
Three hooks follow an identical fetch-then-save pattern with near-identical code:
- Adding new API endpoints or pages.
- Changing database schema or migration files.
- Upgrading dependencies.
- Altering Docker or CI configuration.
- Modifying `Architekture.md` or `Tasks.md`.
| Hook | Lines | Pattern |
|---|---|---|
| `useFilterConfig.ts` | 91 | Load item → expose save → handle abort |
| `useActionConfig.ts` | 89 | Load item → expose save → handle abort |
| `useJailFileConfig.ts` | 76 | Load item → expose save → handle abort |
**Recommendation**: Create a generic `useConfigItem<T>()` hook that takes `fetchFn` and `saveFn` parameters and eliminates the triplication.
---
## 9. Inconsistent Error Handling in Hooks (Frontend)
Hooks handle errors differently:
- Some filter out `AbortError` (e.g., `useHistory`, `useMapData`)
- Others catch all errors indiscriminately (e.g., `useBans`, `useBlocklist`)
This means some hooks surface spurious "request aborted" errors to the UI while others don't.
**Recommendation**: Standardise a shared error-catching pattern, e.g. a `handleFetchError(err, setError)` utility that always filters `AbortError`.
---
## 10. No Global Request State / Caching (Frontend)
Each hook manages its own loading/error/data state independently. There is:
- No request deduplication (two components fetching the same data trigger two requests)
- No stale-while-revalidate caching
- No automatic background refetching
**Recommendation**: Consider adopting React Query (TanStack Query) or SWR for data-fetching hooks. This would eliminate boilerplate in every hook (abort handling, loading state, error state, caching) and provide automatic deduplication.
---
## 11. Large Frontend Components
| Component | Lines | Issue |
|---|---|---|
| `BlocklistsPage.tsx` | 968 | Page does a lot: source list, add/edit dialogs, import log, schedule config |
| `JailsTab.tsx` | 939 | Combines jail list, config editing, raw config, validation, activate/deactivate |
| `JailsPage.tsx` | 691 | Mixes jail table, detail drawer, ban/unban forms |
| `JailDetailPage.tsx` | 663 | Full detail view with multiple sections |
**Recommendation**: Extract sub-sections into focused child components. For example, `JailsTab.tsx` could delegate to `<JailConfigEditor>`, `<JailValidation>`, and `<JailActivateDialog>`.
---
## 12. Duplicated Section Styles (Frontend)
The same card/section styling pattern (`backgroundColor`, `borderRadius`, `border`, `padding` using Fluent UI tokens) is repeated across 13+ files. Each page recreates it in its own `makeStyles` block.
**Recommendation**: Define a shared `useCardStyles()` or export a `sectionStyle` in `src/theme/commonStyles.ts` and import it.
---
## Summary by Priority
| Priority | Issue | Section |
|---|---|---|
| **High** | Service-to-service coupling / circular deps | §1 |
| **High** | God module `config_file_service.py` (3105 lines, 73 functions) | §2 |
| **High** | Shared private function imports across services | §5 |
| **Medium** | Confusing config service naming | §3 |
| **Medium** | Architecture doc drift | §4 |
| **Medium** | Missing error boundaries (frontend) | §6 |
| **Medium** | No global request state / caching (frontend) | §10 |
| **Low** | Duplicated formatting functions (frontend) | §7 |
| **Low** | Duplicated hook logic (frontend) | §8 |
| **Low** | Inconsistent error handling in hooks (frontend) | §9 |
| **Low** | Large frontend components | §11 |
| **Low** | Duplicated section styles (frontend) | §12 |