Add 5 plans

2026-06-24 15:30:47 +02:00
parent 5348bedb86
commit d97d8c6cf4
5 changed files with 644 additions and 0 deletions
@@ -0,0 +1,60 @@
+# Plan 1 — Quick Wins & Low Risk
+
+**Items covered:** #7 (updated_at bug), #8 (dead code), #9 (duplicate test file), #13 (unused dependency)
+
+---
+
+## Context
+
+You are working on a FastAPI backend for a baby monitor / sleep tracker app. The codebase lives under `app/`. The task is to make four small, isolated fixes. Each one is mechanical and carries no risk of breaking behaviour. Do them in order. After each change, re-read the file you edited to verify it looks correct before moving on.
+
+---
+
+## Tasks
+
+### 1. Fix the `updated_at` bug (`app/crud.py:106-113`)
+
+The update handler in `crud.py` never sets `updated_at` after a PATCH/PUT. Find the update function and add the following line before the session is committed:
+
+```python
+session.updated_at = datetime.utcnow()
+```
+
+Make sure `datetime` is already imported; add the import if it is not.
+
+### 2. Delete dead code
+
+The following symbols are defined but never called anywhere. Delete them entirely:
+
+- `ensure_aware()` — located in `app/crud.py:7-10`
+- `minutes_after()` — located in `app/utils/time_utils.py:60-64`
+
+After deleting each one, search the entire codebase (`grep -r "ensure_aware" .` and `grep -r "minutes_after" .`) to confirm nothing imports or calls the deleted symbol. If anything does, note it and do not delete the symbol — flag it for review instead.
+
+Also locate `day_start_pattern()` in `app/utils/time_utils.py`. It is only used internally inside `get_day_night_splits()` in the same file. Rename it to `_day_start_pattern` (leading underscore) to mark it as a private helper and remove it from any `__all__` export list if present.
+
+### 3. Delete the duplicate test file
+
+`tests/test_sessions_fixed.py` is an exact copy of `tests/test_sessions.py`. Verify they are identical (or note any differences), then delete `tests/test_sessions_fixed.py`.
+
+### 4. Remove the unused dependency
+
+`python-multipart` is listed in `pyproject.toml` but is not used (the app has no `Form` parameters). Remove it:
+
+```bash
+uv remove python-multipart
+```
+
+If `uv` is not available, remove the line manually from `pyproject.toml` and run `uv sync` or the appropriate install command to update the lockfile.
+
+---
+
+## Verification
+
+Run the full test suite after all changes:
+
+```bash
+pytest
+```
+
+All existing tests must pass. If any test fails, fix it before declaring this plan complete. Do not move on to Plan 2 until the test suite is green.
@@ -0,0 +1,136 @@
+# Plan 2 — Error Handling & Code Quality
+
+**Items covered:** #5 (magic return types), #6 (silent error suppression), #15 (hardcoded CORS warning)
+
+---
+
+## Context
+
+You are working on a FastAPI backend for a baby monitor / sleep tracker app. The codebase lives under `app/`. This plan improves how errors are communicated — both internally between modules and externally to API clients. Make one change at a time, re-read each modified file before moving to the next, and run the tests at the end.
+
+---
+
+## Tasks
+
+### 1. Replace magic return types with typed exceptions (`app/crud.py` and `app/routers/sessions.py`)
+
+#### Step 1 — Read the files first
+
+Read `app/crud.py` (specifically the session-end function around lines 57-82) and `app/routers/sessions.py` (specifically the caller around lines 64-73) before touching anything. Understand the full flow.
+
+#### Step 2 — Define typed exceptions
+
+Create a new file `app/exceptions.py` with the following exceptions:
+
+```python
+class SessionNotFoundError(Exception):
+    pass
+
+class SessionAlreadyEndedError(Exception):
+    pass
+
+class InvalidEndTimeError(Exception):
+    pass
+```
+
+#### Step 3 — Update `crud.py`
+
+In the session-end function in `app/crud.py`, replace each magic return value with the appropriate raise:
+
+- Where it currently `return None` (session not found) → `raise SessionNotFoundError`
+- Where it currently `return False` (session already ended) → `raise SessionAlreadyEndedError`
+- Where it currently `return "invalid"` → `raise InvalidEndTimeError`
+
+Change the function's return type annotation to `SleepSession` (it now only returns on success).
+
+Import the exceptions at the top of `crud.py`:
+```python
+from app.exceptions import SessionNotFoundError, SessionAlreadyEndedError, InvalidEndTimeError
+```
+
+#### Step 4 — Update `sessions.py`
+
+In `app/routers/sessions.py`, replace the chain of `if result is None` / `if result is False` / `if result == "invalid"` checks with try/except blocks that catch each exception and map it to the correct `HTTPException`:
+
+- `SessionNotFoundError` → `HTTPException(status_code=404, detail="Session not found")`
+- `SessionAlreadyEndedError` → `HTTPException(status_code=409, detail="Session already ended")`
+- `InvalidEndTimeError` → `HTTPException(status_code=422, detail="End time is invalid")`
+
+Import the exceptions at the top of `sessions.py`.
+
+---
+
+### 2. Replace silent error suppression with 422 responses (`app/routers/stats.py`)
+
+#### Step 1 — Read the file first
+
+Read `app/routers/stats.py` and locate both silent-suppression patterns (around lines 24-25 and 114-115). They look like:
+
+```python
+try:
+    day_start_time = time.fromisoformat(day_start)
+except ValueError:
+    return {"days": []}  # silent empty response
+```
+
+#### Step 2 — Replace each one
+
+Replace every such `except ValueError: return {...}` block with:
+
+```python
+except ValueError:
+    raise HTTPException(
+        status_code=422,
+        detail=f"Invalid format: {day_start!r}"
+    )
+```
+
+Adjust the `detail` message to name the specific parameter that was malformed. Make sure `HTTPException` is imported from `fastapi` at the top of the file.
+
+---
+
+### 3. Add a CORS misconfiguration warning (`app/config.py` and `app/main.py`)
+
+#### Step 1 — Read both files first
+
+Read `app/config.py` to understand the `Settings` model and how `CORS_ORIGINS` is defined. Read `app/main.py` to see where `settings` is used.
+
+#### Step 2 — Add a startup warning
+
+In `app/main.py`, add a startup event (or inline check at module load time) that logs a warning if `CORS_ORIGINS` still has the localhost default value. Example:
+
+```python
+import logging
+logger = logging.getLogger(__name__)
+
+@app.on_event("startup")
+async def warn_if_default_cors():
+    if "localhost" in settings.CORS_ORIGINS:
+        logger.warning(
+            "CORS_ORIGINS contains localhost (%s). "
+            "Set the CORS_ORIGINS environment variable in production.",
+            settings.CORS_ORIGINS,
+        )
+```
+
+If a startup event already exists, add the check inside it rather than creating a duplicate.
+
+Also add a comment directly above the `CORS_ORIGINS` field in `app/config.py`:
+
+```python
+# Override in production via the CORS_ORIGINS environment variable.
+# Leaving this as localhost will cause CORS errors in deployed environments.
+CORS_ORIGINS: str = "http://localhost:5173"
+```
+
+---
+
+## Verification
+
+Run the full test suite after all changes:
+
+```bash
+pytest
+```
+
+All existing tests must pass. If any test fails because it was checking for the old magic-return behaviour (e.g. asserting `result == "invalid"`), update those tests to expect the correct `HTTPException` status code instead. Do not suppress or skip failing tests. Do not move on to Plan 3 until the test suite is green.
@@ -0,0 +1,123 @@
+# Plan 3 — Performance
+
+**Items covered:** #4 (missing database indexes), #2 (O(n³) timetable loop), #3 (in-Python aggregation in other stats endpoints)
+
+---
+
+## Context
+
+You are working on a FastAPI backend for a baby monitor / sleep tracker app. The codebase lives under `app/`. This plan replaces expensive Python loops in `app/routers/stats.py` with SQL queries, and adds database indexes to make those queries fast. The index fix (#4) must be done first because the SQL queries depend on it being effective.
+
+Do one task at a time. After each file change, re-read the file to verify it is correct. Run the tests between each task.
+
+---
+
+## Tasks
+
+### 1. Add database indexes (`app/models.py`)
+
+#### Step 1 — Read the file first
+
+Read `app/models.py` in full to understand the `SleepSession` model structure and current field definitions.
+
+#### Step 2 — Add indexes
+
+Add indexes on `start_time` and `end_time`. The preferred approach is inline with `sa_column`:
+
+```python
+from sqlalchemy import Column, DateTime
+from sqlmodel import Field, SQLModel
+
+class SleepSession(SQLModel, table=True):
+    ...
+    start_time: datetime = Field(sa_column=Column(DateTime, index=True))
+    end_time: Optional[datetime] = Field(default=None, sa_column=Column(DateTime, index=True, nullable=True))
+```
+
+If the fields already use `sa_column`, just add `index=True` to the existing `Column(...)` calls. Do not duplicate field definitions.
+
+#### Step 3 — Verify
+
+Run the tests to make sure the model change doesn't break anything:
+
+```bash
+pytest
+```
+
+---
+
+### 2. Fix the O(n³) timetable query (`app/routers/stats.py:77-99`)
+
+#### Step 1 — Read the file first
+
+Read `app/routers/stats.py` in full before making any changes. Understand the full timetable endpoint: its parameters, what it returns, and the structure of the current triple-nested loop.
+
+#### Step 2 — Rewrite the loop
+
+The current code iterates over every day, then every 15-minute slot, then every session in memory — ~2.88 million comparisons for 30 days × 1,000 sessions.
+
+Replace it with a per-day SQL query approach:
+
+For each day in the date range:
+1. Query the database for sessions that overlap that day: `WHERE start_time < day_end AND (end_time IS NULL OR end_time > day_start)`
+2. Build the 96 fifteen-minute slots comparing only against the small set of sessions returned for that day.
+
+This keeps the slot-building logic in Python (which is fine) but moves the session filtering into SQL (which uses the index). The number of sessions compared per day drops from ALL sessions to only the ones active that day.
+
+Keep the return format identical to the current implementation — do not change the shape of the response.
+
+#### Step 3 — Verify
+
+Run the tests:
+
+```bash
+pytest
+```
+
+All timetable tests must pass. If there is only one happy-path test, that is expected — new tests are added in Plan 5.
+
+---
+
+### 3. Fix in-Python aggregation in the other two stats endpoints (`app/routers/stats.py:28-47` and `119-134`)
+
+Read the file again (it may have changed from the previous step) before editing.
+
+#### `daily-summary` endpoint (around lines 28-47)
+
+The current code loads all sessions for a date range and loops through them in Python to compute per-day totals. Replace with a single SQL `GROUP BY` query:
+
+```sql
+SELECT DATE(start_time), SUM(<duration_expression>), COUNT(*)
+FROM sleep_session
+WHERE start_time >= :from_date AND start_time < :to_date
+GROUP BY DATE(start_time)
+```
+
+Use SQLAlchemy's query API or a `text()` query to express this. Map the results back to the same response format the endpoint currently returns.
+
+#### `wake-sleep-times` endpoint (around lines 119-134)
+
+The current code loads all sessions and loops per day to find the first wakeup and last bedtime. Replace with:
+
+```sql
+SELECT DATE(start_time), MIN(start_time), MAX(end_time)
+FROM sleep_session
+WHERE start_time >= :from_date AND start_time < :to_date
+GROUP BY DATE(start_time)
+```
+
+Map the results back to the same response format.
+
+#### Important
+
+Do not change the response shape of either endpoint. The goal is identical output, produced more efficiently.
+
+#### Step — Verify
+
+Run the full test suite:
+
+```bash
+pytest
+```
+
+All tests must pass before declaring this plan complete. Do not move on to Plan 4 until the suite is green.
@@ -0,0 +1,162 @@
+# Plan 4 — Security
+
+**Items covered:** #1 (no authentication), #11 (no rate limiting), #14 (unbounded limit parameter)
+
+---
+
+## Context
+
+You are working on a FastAPI backend for a baby monitor / sleep tracker app. The codebase lives under `app/`. This plan adds three layers of security enforcement. Do them in order — auth first, then rate limiting, then the parameter bound. After each change, re-read the modified files to verify correctness, and run the tests.
+
+---
+
+## Tasks
+
+### 1. Add API key authentication to all endpoints (`app/`)
+
+#### Step 1 — Read the relevant files first
+
+Read `app/config.py`, `app/main.py`, and all router files under `app/routers/` before making any changes. Understand how the app is structured and how dependencies are currently shared.
+
+#### Step 2 — Add `API_KEY` to settings
+
+In `app/config.py`, add an `API_KEY` field to the `Settings` model:
+
+```python
+API_KEY: str = ""
+```
+
+The empty default means auth is effectively disabled unless the env var is set. This allows existing tests to keep working without modification (see Step 5 for the alternative if you prefer strict-by-default).
+
+#### Step 3 — Create the auth dependency
+
+Create a new file `app/dependencies.py`:
+
+```python
+from fastapi import Security, HTTPException
+from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials
+from app.config import settings
+
+bearer = HTTPBearer(auto_error=False)
+
+def verify_api_key(creds: HTTPAuthorizationCredentials = Security(bearer)):
+    if not settings.API_KEY:
+        # API_KEY not configured — auth is disabled (development mode)
+        return
+    if creds is None or creds.credentials != settings.API_KEY:
+        raise HTTPException(status_code=401, detail="Unauthorized")
+```
+
+#### Step 4 — Apply the dependency to all routers
+
+In each file under `app/routers/`, add `verify_api_key` as a dependency on the router so every endpoint in that file is protected:
+
+```python
+from fastapi import APIRouter, Depends
+from app.dependencies import verify_api_key
+
+router = APIRouter(dependencies=[Depends(verify_api_key)])
+```
+
+Apply this to every router file. Do not add it per-endpoint — the router-level dependency is cleaner and ensures nothing is accidentally missed.
+
+#### Step 5 — Update the tests
+
+Any test that calls a protected endpoint now needs an `Authorization: Bearer <key>` header — OR the `API_KEY` setting can be left empty (the default), which disables the check. Verify which approach your test setup uses. If tests use a test client with a settings override, ensure `API_KEY` is either empty (disables check) or consistently set to a test value and the header is passed.
+
+Run the tests:
+
+```bash
+pytest
+```
+
+Fix any 401 failures before proceeding.
+
+---
+
+### 2. Add rate limiting (`app/main.py` and `app/routers/`)
+
+#### Step 1 — Install slowapi
+
+```bash
+uv add slowapi
+```
+
+Or add `slowapi` to `pyproject.toml` dependencies and run `uv sync`.
+
+#### Step 2 — Configure the limiter in `main.py`
+
+```python
+from slowapi import Limiter, _rate_limit_exceeded_handler
+from slowapi.util import get_remote_address
+from slowapi.errors import RateLimitExceeded
+
+limiter = Limiter(key_func=get_remote_address)
+app.state.limiter = limiter
+app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)
+```
+
+#### Step 3 — Apply limits to endpoints
+
+Add rate limit decorators to the endpoints most at risk:
+
+- All write endpoints (`POST`, `PUT`, `PATCH`, `DELETE`): `@limiter.limit("10/minute")`
+- The timetable endpoint (expensive): `@limiter.limit("30/minute")`
+- Other read endpoints: `@limiter.limit("60/minute")`
+
+Example:
+
+```python
+from app.main import limiter
+from fastapi import Request
+
+@router.post("/sessions")
+@limiter.limit("10/minute")
+async def create_session(request: Request, ...):
+    ...
+```
+
+Note: `slowapi` requires the `request: Request` parameter to be present in the function signature for the decorator to work. Add it to any endpoint that doesn't already have it. It does not need to be used inside the function body.
+
+#### Step 4 — Verify
+
+Run the tests:
+
+```bash
+pytest
+```
+
+Rate limiting may interfere with tests that call endpoints in a tight loop. If so, configure the limiter to be disabled in the test environment (e.g. check for a `TESTING=true` env var and set `enabled=False` on the limiter).
+
+---
+
+### 3. Bound the `limit` query parameter (`app/routers/sessions.py`)
+
+#### Step 1 — Read the file first
+
+Find the `get_sessions` endpoint (or equivalent) and locate the `limit` parameter.
+
+#### Step 2 — Add the bound
+
+Replace the bare `limit: int = 200` with a constrained `Query`:
+
+```python
+from fastapi import Query
+
+async def get_sessions(
+    limit: int = Query(default=200, ge=1, le=1000),
+    ...
+):
+```
+
+This rejects any `limit` value below 1 or above 1000 with a FastAPI 422 validation error automatically — no extra code needed.
+
+#### Step 3 — Verify
+
+Run the full test suite one final time:
+
+```bash
+pytest
+```
+
+All tests must pass before declaring this plan complete. Do not move on to Plan 5 until the suite is green.
@@ -0,0 +1,163 @@
+# Plan 5 — Observability & Test Coverage
+
+**Items covered:** #10 (no logging), #12 (missing test coverage)
+
+---
+
+## Context
+
+You are working on a FastAPI backend for a baby monitor / sleep tracker app. The codebase lives under `app/`. This is the final plan. Plans 1–4 must already be complete — this plan assumes auth returns 401, malformed parameters return 422, and `updated_at` is updated on PATCH/PUT.
+
+Do the logging setup first (it is quick), then work through the missing tests methodically. Add tests one group at a time and run `pytest` after each group.
+
+---
+
+## Tasks
+
+### 1. Add structured logging (`app/main.py` and other files)
+
+#### Step 1 — Read `app/config.py` and `app/main.py` first
+
+Confirm that `LOG_LEVEL` already exists in `Settings` (it does — it is currently defined but never consumed). Note its default value.
+
+#### Step 2 — Configure logging in `main.py`
+
+At the top of `app/main.py`, before the app is created, add:
+
+```python
+import logging
+
+logging.basicConfig(
+    level=settings.LOG_LEVEL,
+    format="%(asctime)s %(levelname)s %(name)s %(message)s",
+)
+logger = logging.getLogger(__name__)
+```
+
+#### Step 3 — Add request logging middleware
+
+Add a simple middleware to log every request and response status:
+
+```python
+import time
+from fastapi import Request
+
+@app.middleware("http")
+async def log_requests(request: Request, call_next):
+    start = time.time()
+    response = await call_next(request)
+    duration_ms = (time.time() - start) * 1000
+    logger.info(
+        "%s %s → %s (%.1fms)",
+        request.method,
+        request.url.path,
+        response.status_code,
+        duration_ms,
+    )
+    return response
+```
+
+#### Step 4 — Add business event logging
+
+In the relevant router files, add a `logger = logging.getLogger(__name__)` at the module level and log the key business events:
+
+- Session started: `logger.info("Session started id=%s", session.id)`
+- Session ended: `logger.info("Session ended id=%s duration_min=%.1f", session.id, ...)`
+- Session deleted: `logger.info("Session deleted id=%s", session_id)`
+
+Keep log messages concise. Do not log sensitive user data.
+
+#### Step 5 — Verify
+
+Run the tests:
+
+```bash
+pytest
+```
+
+Logging must not break any existing tests.
+
+---
+
+### 2. Add missing test coverage (`tests/`)
+
+Read the existing test files before adding anything so you understand the test client setup, fixtures, and conventions already in use. Add all new tests to appropriate existing files (or a new file if clearly warranted — e.g. `tests/test_stats.py` if one does not exist).
+
+Work through each group below. After adding each group, run `pytest` and fix any failures before continuing.
+
+---
+
+#### Group A — Stats endpoints: empty date ranges
+
+For each stats endpoint (`/api/stats/daily-summary`, `/api/stats/timetable`, `/api/stats/wake-sleep-times`), add a test that:
+- Calls the endpoint with a valid date range that has no sessions
+- Asserts a 200 response
+- Asserts the response body represents an empty result (no days, empty list, etc.)
+
+---
+
+#### Group B — Stats endpoints: session spanning midnight
+
+Add a test that:
+- Creates a session with `start_time` on day N at 23:00 and `end_time` on day N+1 at 07:00
+- Calls `/api/stats/timetable` for a range covering both days
+- Asserts that the session appears in both days' slot data
+
+---
+
+#### Group C — Stats endpoints: session partially overlapping the query range
+
+Add a test that:
+- Creates a session that starts before the query range and ends inside it
+- Calls each stats endpoint with the range
+- Asserts the session is counted/included correctly (it should be, since it overlaps the range)
+
+---
+
+#### Group D — Stats endpoints: multiple sessions on the same day
+
+Add a test that:
+- Creates two or more sessions on the same day
+- Calls `/api/stats/daily-summary` for that day
+- Asserts that the totals correctly aggregate across both sessions
+
+---
+
+#### Group E — Invalid query parameters return 422
+
+These tests depend on Plan 2 (silent suppression replaced with 422). Add a parametrised test (or individual tests) for:
+- `/api/stats/daily-summary` with `day_start=not-a-time` → expect 422
+- `/api/stats/timetable` with `day_start=not-a-time` → expect 422
+- Any other endpoint that previously silently suppressed a `ValueError`
+
+---
+
+#### Group F — Bounded `limit` parameter
+
+This test depends on Plan 4 (Query bound added). Add tests for:
+- `GET /api/sessions?limit=0` → expect 422
+- `GET /api/sessions?limit=1000001` → expect 422
+- `GET /api/sessions?limit=1` → expect 200
+- `GET /api/sessions?limit=1000` → expect 200
+
+---
+
+#### Group G — `updated_at` regression
+
+This test depends on Plan 1 (updated_at bug fixed). Add a test that:
+- Creates a session
+- Records the `updated_at` value
+- Sends a PUT/PATCH request to update it
+- Asserts that `updated_at` in the response is strictly greater than the original value
+
+---
+
+## Final verification
+
+Run the complete test suite one last time:
+
+```bash
+pytest --tb=short
+```
+
+All tests must pass. Review the output for any warnings. This completes all 15 improvements from the backend improvement report.