refactoring-backend #3
@@ -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/`)
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
117
Docs/Tasks.md
117
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`
|
||||
Reference in New Issue
Block a user