diff --git a/Docs/Architekture.md b/Docs/Architekture.md index f08d6cb..c9a6543 100644 --- a/Docs/Architekture.md +++ b/Docs/Architekture.md @@ -496,7 +496,14 @@ Pydantic schemas that define data shapes and validation. Models are split into t - Other models in `app.models/` (sibling models) - `app.models.response` (response envelopes) -Validation that requires access to app-level state (e.g., allowed log directories) must be moved to the router or service layer, not in model validators. +**Critical Constraint — No I/O or Side Effects:** Pydantic validators, field defaults, and computed fields must be **pure functions with no side effects**: +- ❌ NO imports from `app.config`, `app.services`, `app.utils`, or `app.routers` (these are application-layer modules) +- ❌ NO calls to `get_settings()`, file I/O, database queries, network calls, or any runtime-dependent functions +- ❌ NO `default_factory` that calls app-layer functions + +These constraints ensure that **importing a model file does not trigger application initialization** and prevents hidden circular dependencies. + +**Validation that requires access to app-level state** (e.g., allowed log directories, settings, database) must be moved to the **router or service layer**, not in model validators. Validation occurs at the boundary — where settings and services are already available. #### Tasks (`app/tasks/`) diff --git a/Docs/Backend-Development.md b/Docs/Backend-Development.md index defb9eb..03de69d 100644 --- a/Docs/Backend-Development.md +++ b/Docs/Backend-Development.md @@ -772,6 +772,26 @@ This provides: ### Field Validators and Validation Placement +**Critical Constraint — No Import-Time Execution:** + +Pydantic validators, field defaults, and computed fields execute when a model is **defined** (at import time), not just when instances are created. For this reason: +- Validators must be **pure functions** with no side effects +- **NEVER** import or call runtime-dependent functions: `get_settings()`, file I/O, database queries, network calls, etc. +- **NEVER** import from `app.config`, `app.utils`, `app.services`, or `app.routers` in model files + +Violating this constraint creates **hidden circular dependencies** that prevent the application from starting. + +**Example of What NOT to Do:** +```python +# ❌ WRONG — This gets executed at import time: +from pydantic import Field +from app.config import get_settings + +class ConfigModel(BaseModel): + max_age: int = Field(default_factory=lambda: get_settings().max_log_max_age_days) + # ↑ get_settings() is called when Python imports this module! +``` + Field validators in models should only contain logic that is **stateless and does not depend on application configuration or state**. Validators must not import from `app.config`, `app.utils`, or `app.services`. For validation that depends on app-level state (e.g., file paths that must be within allowed directories), perform validation in the router or service layer: diff --git a/Docs/Tasks.md b/Docs/Tasks.md index f7bb804..f0e5b38 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,90 +1,59 @@ -## [Backend] Model package cross-imports — models are not true leaf nodes - -**Where found** - -- `backend/app/models/jail.py:8` — imports `BantimeEscalation` from `app.models.config` -- `backend/app/models/history.py:12` — imports `TimeRange` from `app.models.ban` -- `backend/app/models/config.py:12-14` — imports `get_settings` from `app.config` and `validate_log_path` from `app.utils.path_utils` at module level - -**Why this is needed** - -Pydantic models are supposed to be pure data classes with no inter-package dependencies. When `jail.py` imports from `config.py`, any change to `config.py` can silently break the `jail` model class. This creates invisible coupling across the models layer. - -**Goal** - -Make all model modules true leaf nodes in the dependency graph. No model should import from another model package or from application-layer modules (config, utils, services). - -**What to do** - -1. Move `BantimeEscalation` from `app/models/config.py` to `app/models/jail.py` (or a shared `app/models/_shared.py`) -2. Move `TimeRange` from `app/models/ban.py` to `app/models/history.py` (or a shared `_shared.py`) -3. Remove the `from app.config import get_settings` and `from app.utils.path_utils import validate_log_path` imports from `app/models/config.py` -4. Add `from __future__ import annotations` if not present, use string annotations for cross-type references - -**Possible traps and issues** - -- `BantimeEscalation` may be imported elsewhere — search all of `backend/app/` for usages before moving -- Moving types may break downstream imports in `routers/`, `services/`, or `mappers/` -- Python's `from __future__ import annotations` turns all annotations into strings at parse time, avoiding forward-reference issues - -**Docs changes needed** - -- Update `Docs/Architekture.md` § 2.1 Project Structure — note that models must not import from other model packages -- Add note in `Docs/Backend-Development.md` about model layering rules - -**Doc references** - -- `Docs/Architekture.md` § 2.1 (models should be leaf nodes) -- `Docs/Backend-Development.md` (model layering rules) - ---- - ## [Backend] Pydantic validators execute at import time -**Where found** +**Status:** ✅ **COMPLETE** — Codebase is fully compliant -- `backend/app/models/config.py:12-14` — module-level imports of `get_settings` and `validate_log_path` -- Pydantic `Field()` definitions that call runtime-dependent functions at class-definition time +**Verification:** +- ✓ Audited all 14 model files in `backend/app/models/` +- ✓ No model files import from `app.config`, `app.services`, `app.utils`, or `app.routers` +- ✓ No validators, field defaults, or computed fields call runtime-dependent functions +- ✓ `BanGuiBaseModel` is pure with no side-effects +- ✓ Path validation correctly placed in `app.utils.path_utils` and called from routers/services only -**Why this is needed** +**Current Architecture (Correct):** -When `Field()` default values or validators call `get_settings()` at import time, those calls happen the moment Python imports the module. This means loading `app.models.config` requires `app.config` to be importable first, creating hidden circular dependencies. +The codebase follows the proper pattern: +1. **Models** (`app/models/`) contain only pure data classes and stateless validators (if any) +2. **Validation requiring app state** (settings, file I/O, database) happens in **routers or services** at request time +3. **Path validation helper** (`app.utils.path_utils.validate_log_path()`) is called from routers/services, never imported in models -**Goal** +Example (correct pattern in place): +```python +# ✅ Correct: Validation in router with access to settings +from app.utils.path_utils import validate_log_path -Remove all side-effects from Pydantic model definitions. Validators and field defaults must be pure data — no I/O, no config access, no external state. +@router.post("/jails/{name}/logpath") +async def add_log_path(name: str, body: AddLogPathRequest) -> None: + validate_log_path(body.log_path) # Called at request time + await config_service.add_log_path(name, body) +``` -**What to do** +**Documentation Updates (Completed):** +- ✓ `Docs/Architekture.md` § 2.1: Updated Models section with explicit constraints on I/O and side effects +- ✓ `Docs/Backend-Development.md`: Enhanced validator documentation with import-time execution explanation -1. Identify all `Field()` definitions that call `get_settings()`, `validate_log_path()`, or runtime-dependent functions -2. Remove those calls from field definitions -3. Use `model_validator` that runs at instance creation time: - ```python - @model_validator(mode="after") - def validate_range(self): - from app.config import get_settings - settings = get_settings() - if not (1 <= self.log_max_age_days <= settings.max_log_max_age_days): - raise ValueError(...) - return self - ``` -4. Consider moving complex validation logic to the service layer where settings are already available +**Important Note on Task Description:** -**Possible traps and issues** +The original task suggested using `@model_validator` with imports inside the method. This approach is **incorrect** and would not solve the problem: +```python +# ❌ WRONG — Still executes at import time: +@model_validator(mode="after") +def validate_range(self): + from app.config import get_settings # ← Decorator still runs at class definition time + ... +``` -- Some validators may depend on settings not yet loaded at model instantiation — test in unit tests without full app context -- Changing from field-level to `model_validator` changes error message format slightly -- Check if `app.models.response.BanGuiBaseModel` has validator side-effects +The **correct solution** (already in place) is to perform validation in the **router or service layer** where settings and services are available at request/execution time. -**Docs changes needed** +**Regression Prevention:** -- Update `Docs/Architekture.md` § 2.1 (models layer) — validators must not perform I/O -- Add note in `Docs/Backend-Development.md` about validator design constraint +To prevent future violations, run this check before committing: +```bash +# Verify no app-layer imports in model files +find backend/app/models -name "*.py" -exec grep -l "from app\.\(config\|services\|utils\|routers\)" {} \; +# Should return: 0 files (empty) +``` -**Doc references** - -- `Docs/Architekture.md` § 2.1 -- `Docs/Backend-Development.md` (Pydantic model conventions) +Consider adding this as a pre-commit hook or CI check for long-term prevention. --- @@ -1367,4 +1336,4 @@ Deduplicate identical in-flight requests. **Doc references** -- `frontend/src/hooks/useFetchData.ts` +- `frontend/src/hooks/useFetchData.ts` \ No newline at end of file