# 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 |