Improvements todo created and small bugfixes
This commit is contained in:
@@ -0,0 +1,320 @@
|
||||
# Backend Improvement Report
|
||||
|
||||
## 1. No Authentication — Security
|
||||
|
||||
**Severity: Critical**
|
||||
|
||||
There is zero authentication on any endpoint. Any caller with network access can create, read, update, and delete all sleep session data. CORS configuration in `app/main.py:18-24` restricts browser origins but does nothing to protect against direct HTTP clients (curl, scripts, etc.).
|
||||
|
||||
**Recommended fix (incremental):**
|
||||
1. **Immediate:** Add a single shared API key checked via an `Authorization: Bearer <key>` header using a FastAPI dependency. The key is stored in an environment variable.
|
||||
2. **Longer term:** Proper user authentication if the app ever becomes multi-user.
|
||||
|
||||
Example dependency:
|
||||
```python
|
||||
from fastapi import Security, HTTPException
|
||||
from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials
|
||||
|
||||
bearer = HTTPBearer()
|
||||
|
||||
def verify_api_key(creds: HTTPAuthorizationCredentials = Security(bearer)):
|
||||
if creds.credentials != settings.API_KEY:
|
||||
raise HTTPException(status_code=401, detail="Unauthorized")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 2. O(days × slots × sessions) Timetable Query — Performance
|
||||
|
||||
**Severity: Critical**
|
||||
|
||||
`app/routers/stats.py:77-99` contains a triple-nested Python loop to build the timetable:
|
||||
|
||||
```python
|
||||
while current_date <= to_date.date():
|
||||
for i in range(96): # 96 fifteen-minute slots per day
|
||||
for s in sessions: # ALL sessions loaded into memory
|
||||
if s.start_time <= slot_end and (s.end_time is None or s.end_time >= slot_start):
|
||||
is_sleeping = True
|
||||
break
|
||||
```
|
||||
|
||||
With 30 days and 1,000 sessions this is ~2.88 million comparisons executed in Python. The comparison work should happen in the database.
|
||||
|
||||
**Fix:** For each day, run a single SQL query that returns the sessions overlapping that day's range, then build the 96 slots against only the relevant sessions. Alternatively, generate a CTE of slot boundaries and join against `sleep_session` in a single query. Even the naive "one query per day" approach is far faster than the current Python loop because the database uses the index (see item 4).
|
||||
|
||||
---
|
||||
|
||||
## 3. In-Python Aggregation in Other Stats Endpoints — Performance
|
||||
|
||||
**Severity: High**
|
||||
|
||||
The same load-everything-then-loop pattern appears in the other two stats endpoints:
|
||||
|
||||
- `/api/stats/daily-summary` (`stats.py:28-47`): loads all sessions for the date range, then loops through each one in Python to compute per-day totals.
|
||||
- `/api/stats/wake-sleep-times` (`stats.py:119-134`): loads all sessions, then loops per day to find the first wakeup and last bedtime.
|
||||
|
||||
Both of these can be replaced with a single SQL GROUP BY query per endpoint:
|
||||
- `daily-summary`: `SELECT DATE(start_time), SUM(...) FROM sleep_session WHERE ... GROUP BY DATE(start_time)`
|
||||
- `wake-sleep-times`: `SELECT DATE(start_time), MIN(start_time), MAX(end_time) FROM sleep_session WHERE ... GROUP BY DATE(start_time)`
|
||||
|
||||
This removes the O(sessions × days) Python loop entirely.
|
||||
|
||||
---
|
||||
|
||||
## 4. Missing Database Indexes — Performance
|
||||
|
||||
**Severity: High**
|
||||
|
||||
Every stats query and the session list query filter and sort on `start_time` and `end_time`, but neither column has an index in `app/models.py`.
|
||||
|
||||
**Fix:** Add indexes to the model:
|
||||
```python
|
||||
from sqlmodel import Field, SQLModel, Index
|
||||
|
||||
class SleepSession(SQLModel, table=True):
|
||||
...
|
||||
|
||||
# after the class definition:
|
||||
Index("ix_sleep_session_start_time", SleepSession.__table__.c.start_time)
|
||||
Index("ix_sleep_session_end_time", SleepSession.__table__.c.end_time)
|
||||
```
|
||||
|
||||
Or declare them inline with `sa_column`:
|
||||
```python
|
||||
start_time: datetime = Field(sa_column=Column(DateTime, index=True))
|
||||
```
|
||||
|
||||
This is especially important for the timetable query once it is pushed into SQL.
|
||||
|
||||
---
|
||||
|
||||
## 5. Magic Return Types on Session End — Code Quality / Simplification
|
||||
|
||||
**Severity: High**
|
||||
|
||||
`app/crud.py:57-82` returns `Union[SleepSession, bool, str, None]` where the string value is the magic sentinel `"invalid"`:
|
||||
|
||||
```python
|
||||
# crud.py
|
||||
if session is None:
|
||||
return None
|
||||
if session.end_time is not None:
|
||||
return False
|
||||
if ...:
|
||||
return "invalid"
|
||||
return session
|
||||
```
|
||||
|
||||
The caller in `app/routers/sessions.py:64-73` then has to chain `if result is None`, `if result is False`, `if result == "invalid"` checks. This is fragile, untestable without string matching, and breaks static analysis.
|
||||
|
||||
**Fix:** Define typed exceptions and raise them:
|
||||
```python
|
||||
class SessionNotFoundError(Exception): pass
|
||||
class SessionAlreadyEndedError(Exception): pass
|
||||
class InvalidEndTimeError(Exception): pass
|
||||
```
|
||||
|
||||
`crud.py` raises, `sessions.py` catches and maps to the appropriate `HTTPException`. The function return type becomes simply `SleepSession`.
|
||||
|
||||
---
|
||||
|
||||
## 6. Silent Error Suppression in Stats — Code Quality / Documentation
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
`app/routers/stats.py:24-25` and `114-115` catch invalid query parameter values and silently return empty data:
|
||||
|
||||
```python
|
||||
try:
|
||||
day_start_time = time.fromisoformat(day_start)
|
||||
except ValueError:
|
||||
return {"days": []} # client gets empty data with no explanation
|
||||
```
|
||||
|
||||
The client has no way to distinguish "no data for this range" from "your parameters were malformed". This makes debugging difficult.
|
||||
|
||||
**Fix:** Return a `422 Unprocessable Entity` with a descriptive message:
|
||||
```python
|
||||
from fastapi import HTTPException
|
||||
|
||||
try:
|
||||
day_start_time = time.fromisoformat(day_start)
|
||||
except ValueError:
|
||||
raise HTTPException(status_code=422, detail=f"Invalid day_start format: {day_start!r}")
|
||||
```
|
||||
|
||||
Alternatively, use FastAPI's `Query()` with a regex validator so malformed values are rejected before the handler runs.
|
||||
|
||||
---
|
||||
|
||||
## 7. `updated_at` Field Never Updated — Bug
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
`app/models.py` declares an `updated_at` column, but `app/crud.py:106-113` (the update handler) never sets it. After any PATCH, `updated_at` retains the creation timestamp, making it misleading.
|
||||
|
||||
**Fix:** In the update function, add:
|
||||
```python
|
||||
session.updated_at = datetime.utcnow()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 8. Dead Code — Code Slop
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
Three functions are defined but never called anywhere in the application:
|
||||
|
||||
| Function | File | Lines |
|
||||
|----------|------|-------|
|
||||
| `ensure_aware()` | `app/crud.py` | 7-10 |
|
||||
| `minutes_after()` | `app/utils/time_utils.py` | 60-64 |
|
||||
|
||||
Additionally, `app/utils/time_utils.py` exports `day_start_pattern()` which is only used internally within `get_day_night_splits()` — it does not need to be a module-level export.
|
||||
|
||||
**Fix:** Delete `ensure_aware()` and `minutes_after()`. Make `day_start_pattern()` a local helper (leading underscore or inline).
|
||||
|
||||
---
|
||||
|
||||
## 9. Duplicate Test File — Code Slop
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
`tests/test_sessions_fixed.py` is an exact copy of `tests/test_sessions.py`. It appears to be a temporary file that was never cleaned up.
|
||||
|
||||
**Fix:** Delete `tests/test_sessions_fixed.py`.
|
||||
|
||||
---
|
||||
|
||||
## 10. No Application Logging — Observability
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
There are zero log statements anywhere in the backend. No requests, errors, slow queries, or business events are recorded. This makes diagnosing production issues entirely dependent on guesswork.
|
||||
|
||||
**Fix:** Add structured logging using Python's `logging` module (or `structlog` for structured output). At minimum log:
|
||||
- Every request with method, path, and response status (can use a FastAPI middleware or Starlette's built-in `ServerErrorMiddleware`)
|
||||
- Unhandled exceptions with stack traces
|
||||
- Slow database queries (via SQLAlchemy event listeners if needed)
|
||||
- Business events: session started, session ended, session deleted
|
||||
|
||||
Example setup in `main.py`:
|
||||
```python
|
||||
import logging
|
||||
logging.basicConfig(level=settings.LOG_LEVEL, format="%(asctime)s %(levelname)s %(name)s %(message)s")
|
||||
logger = logging.getLogger(__name__)
|
||||
```
|
||||
|
||||
Note: `app/config.py` already has a `LOG_LEVEL` setting — it is currently configured but never consumed.
|
||||
|
||||
---
|
||||
|
||||
## 11. No Rate Limiting — Security
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
There is no rate limiting on any endpoint. A client could spam POST requests to `/api/sessions` flooding the database, or hammer the expensive timetable endpoint.
|
||||
|
||||
**Fix:** Add `slowapi` (a FastAPI-compatible rate limiter):
|
||||
```python
|
||||
from slowapi import Limiter
|
||||
from slowapi.util import get_remote_address
|
||||
|
||||
limiter = Limiter(key_func=get_remote_address)
|
||||
|
||||
@router.post("/sessions")
|
||||
@limiter.limit("10/minute")
|
||||
async def create_session(...): ...
|
||||
```
|
||||
|
||||
A reasonable starting limit: 10 writes/minute per IP, 60 reads/minute per IP.
|
||||
|
||||
---
|
||||
|
||||
## 12. Missing Test Coverage — Testing
|
||||
|
||||
**Severity: High**
|
||||
|
||||
Current test coverage has significant gaps:
|
||||
|
||||
| Area | Current state |
|
||||
|------|--------------|
|
||||
| `/api/stats/timetable` | 1 basic happy-path test |
|
||||
| `/api/stats/daily-summary` | 1 basic happy-path test |
|
||||
| `/api/stats/wake-sleep-times` | No tests |
|
||||
| Date boundary conditions | No tests |
|
||||
| Concurrent session creation | No tests |
|
||||
| Invalid query parameters | No tests |
|
||||
|
||||
**Recommended additions:**
|
||||
- Parametrised tests for all stats endpoints covering: empty date ranges, single session spanning midnight, session starting before range and ending inside it, multiple sessions on the same day.
|
||||
- Test that invalid `day_start`/`day_end` values return 422 (once fix #6 is applied).
|
||||
- Test that `limit` parameter is bounded (once fix #14 is applied).
|
||||
- Regression test asserting `updated_at` changes after a PUT request (covers fix #7).
|
||||
|
||||
---
|
||||
|
||||
## 13. Unused Dependency — Code Slop
|
||||
|
||||
**Severity: Low**
|
||||
|
||||
`python-multipart` is listed in `pyproject.toml:13` but is not used by this application (FastAPI only requires it if you use `Form` parameters, which this API does not).
|
||||
|
||||
**Fix:** Remove the dependency:
|
||||
```
|
||||
uv remove python-multipart
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 14. Unbounded `limit` Parameter — Security / Reliability
|
||||
|
||||
**Severity: Medium**
|
||||
|
||||
`app/routers/sessions.py` accepts a `limit` query parameter with default 200 and no upper bound. A caller can pass `limit=10000000` and force a full table scan returning all rows.
|
||||
|
||||
**Fix:** Use FastAPI's `Query` with a `le` constraint:
|
||||
```python
|
||||
from fastapi import Query
|
||||
|
||||
async def get_sessions(limit: int = Query(default=200, ge=1, le=1000)):
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 15. Hardcoded CORS Origin — Configuration
|
||||
|
||||
**Severity: Low**
|
||||
|
||||
`app/config.py:7` hardcodes the CORS allowed origin:
|
||||
```python
|
||||
CORS_ORIGINS: str = "http://localhost:5173"
|
||||
```
|
||||
|
||||
This is already an env-var-overridable `Settings` field (via Pydantic's `BaseSettings`), so the mechanism is correct. But the default value means local development works without configuration while production requires the env var to be set — if it is forgotten, the production frontend gets CORS errors.
|
||||
|
||||
**Documentation fix:** Add a comment or a startup check that warns if the running environment looks like production but `CORS_ORIGINS` still has the localhost default.
|
||||
|
||||
---
|
||||
|
||||
## Summary Table
|
||||
|
||||
| # | Category | File(s) | Severity |
|
||||
|---|----------|---------|----------|
|
||||
| 1 | Security — no auth | All routers | Critical |
|
||||
| 2 | Performance — O(n³) loop | `stats.py:77-99` | Critical |
|
||||
| 3 | Performance — in-Python aggregation | `stats.py:28-47`, `119-134` | High |
|
||||
| 4 | Performance — missing indexes | `models.py` | High |
|
||||
| 5 | Code quality — magic return types | `crud.py:57-82`, `sessions.py:64-73` | High |
|
||||
| 6 | Code quality — silent error suppression | `stats.py:24-25`, `114-115` | Medium |
|
||||
| 7 | Bug — `updated_at` not updated | `crud.py:106-113` | Medium |
|
||||
| 8 | Code slop — dead functions | `crud.py:7-10`, `time_utils.py:60-64` | Medium |
|
||||
| 9 | Code slop — duplicate test file | `tests/test_sessions_fixed.py` | Medium |
|
||||
| 10 | Observability — no logging | All files | Medium |
|
||||
| 11 | Security — no rate limiting | All routers | Medium |
|
||||
| 12 | Testing — missing coverage | `tests/` | High |
|
||||
| 13 | Code slop — unused dependency | `pyproject.toml:13` | Low |
|
||||
| 14 | Security — unbounded limit param | `sessions.py` | Medium |
|
||||
| 15 | Configuration — hardcoded CORS | `config.py:7` | Low |
|
||||
Reference in New Issue
Block a user