137 lines
4.7 KiB
Markdown
137 lines
4.7 KiB
Markdown
# 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.
|