brut/bincio-activity
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

refactoring.md: update Step 4 to reflect as-built (post-split routers)

Browse Source
This commit is contained in:
Davide Scaini
2026-05-14 00:04:52 +02:00
parent 27f6d141f7
commit 7ec91b0e6a
1 changed files with 32 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
refactoring.md
+32 -50
View File
@@ -426,62 +426,44 @@ def test_get_current_user_raises_without_cookie(tmp_path):
## Step 4 — Narrow broad `except Exception:` catches ## Step 4 — Narrow broad `except Exception:` catches
### Problem ### Problem
`bincio/serve/server.py` has ~20 bare `except Exception:` or `except Exception as exc:` clauses. Most are in route handlers, not background threads. Broad catches hide bugs and prevent the right exception type from propagating to logs. After the Step 3 split, the router files inherited ~35 bare `except Exception:` clauses from the original monolith. Most were in route handlers where a specific narrow type is knowable and preferable — broad catches hide bugs and let surprising failures silently produce wrong results.
### Inventory (file: `bincio/serve/server.py`) ### Classification rule
| Line | Context | Acceptable? | Proposed fix | | Situation | Decision |
|---|---|---|---| |---|---|
| 204 | Reading per-user strava creds JSON from disk | No | `(OSError, json.JSONDecodeError, KeyError, ValueError)` | | Background thread top-level guard (calls `log.exception`) | **Keep** — last-resort, full traceback essential |
| 228 | Cleanup of orphaned `tmp*.zip` files at startup | Yes (best-effort) | Keep, add inline comment | | SSE stream generator top-level | **Keep** — must convert any error to a client event |
| 407 | Background site-rebuild thread top-level | Yes (last-resort guard) | Keep — already calls `log.exception()` | | Per-item batch loop (must not abort on one failure) | **Keep** — `log.warning/error` already present |
| 443 | Background rebuild-for-handle thread | Yes (same) | Keep | | Explicitly non-fatal post-upload merge step | **Keep** — `log.warning` present; upload already succeeded |
| 554 | Extracting preview coords from geojson | No | `(KeyError, TypeError, IndexError)` | | Route handler: reading/writing JSON files | `(OSError, json.JSONDecodeError)` |
| 652 | Background BAS upload processing | Yes (background task) | Keep, ensure `log.exception()` is called | | Route handler: datetime parsing | `ValueError` |
| 694 | Background raw upload processing | Yes | Keep, ensure `log.exception()` | | Route handler: base64 decoding | `ValueError` |
| 753 | Subprocess result parsing in raw upload | No | `(subprocess.SubprocessError, OSError, json.JSONDecodeError)` | | Route handler: YAML parsing | `(OSError, yaml.YAMLError)` |
| 767 | Same function, inner block | No | Same as above | | Route handler: GeoJSON coord extraction | `(TypeError, IndexError, AttributeError)` |
| 1287 | Reading activity JSON in admin diag endpoint | No | `(OSError, json.JSONDecodeError)` | | Startup cleanup (`Path.unlink`) | `OSError` |
| 1346 | Parsing timeseries JSON in admin diag | No | `(OSError, json.JSONDecodeError, KeyError)` | | JSON line parsing inside SSE batch | `json.JSONDecodeError` |
| 1354 | Parsing merged JSON in admin diag | No | Same |
| 1486 | Strava admin sync — per-user iteration | Yes (don't abort whole sync) | Keep, ensure `log.exception()` |
| 1512 | Strava admin sync — inner loop | No | `StravaError, requests.RequestException` |
| 1529 | Same | No | Same |
| 1567 | Garmin sync iteration | Yes (don't abort) | Keep, ensure `log.exception()` |
| 1678 | Deleting user directory | No | `OSError` |
| 1741 | Reading strava creds file in user endpoint | No | `(OSError, json.JSONDecodeError)` |
| 1769 | Writing strava creds file | No | `OSError` |
| 1782 | Same | No | `OSError` |
**Rule of thumb applied here**: background thread top-level handlers and "don't abort whole batch" iterators keep `except Exception` but must call `log.exception()`. All route-handler catches should be narrowed to the specific exception types that the called code can raise. ### What was changed (28 catches narrowed across 8 files)
### Test plan - `server.py` — startup `tmp*.zip` cleanup → `OSError`
- `segments.py` — file-scan loops (6 catches) → `(OSError, json.JSONDecodeError, ValueError)` / `ValueError`
- `me.py` — credential file reads, manifest write (4 catches) → `(OSError, json.JSONDecodeError)`
- `activities.py` — index/cache reads (2 catches) → `(OSError, json.JSONDecodeError)`; YAML enrichment (1) → `(OSError, yaml.YAMLError)` with `import yaml` moved above the `try`
- `admin.py` — diag index reads, strava-status loop reads (5 catches) → `(OSError, json.JSONDecodeError)` / `json.JSONDecodeError`
- `ideas.py` — idea file reads (3 catches) → `(OSError, json.JSONDecodeError)`
- `strava.py` — index parse in reset endpoint → `(OSError, json.JSONDecodeError, ValueError)`
- `uploads.py` — GeoJSON coords, base64 decode, cache update (3 catches)
For each narrowed catch, write a targeted test that injects the specific failure and verifies the correct HTTP response code is returned (not a 500). ### What was kept (11 catches, all intentional)
```python `tasks.py:97`, `tasks.py:133` — background thread tops with `log.exception`
# tests/serve/test_error_handling.py `admin.py:579` — admin strava-sync background thread top with `log.exception`
`admin.py:630` — per-activity batch loop in recompute-elevation with `log.warning`
def test_strava_creds_file_malformed_json(client, tmp_path): `garmin.py:112`, `strava.py:164`, `uploads.py:491` — SSE stream tops
"""Malformed strava_credentials.json is silently ignored; global creds used.""" `uploads.py:143`, `uploads.py:259` — non-fatal post-upload merge with `log.warning`
user_dir = tmp_path / "alice" `uploads.py:245` — extraction failure → 422 (any parser failure must surface as 422)
user_dir.mkdir() `uploads.py:404` — per-file batch loop in upload event stream
(user_dir / "strava_credentials.json").write_text("not json")
# Should not 500; should fall back to global (empty) creds
# Test via /api/strava/status once auth is in place
...
def test_activity_geojson_missing_geometry(client, tmp_path, authenticated_session):
"""Activity with malformed geojson does not crash the upload endpoint."""
...
```
### Implementation steps
1. Audit each line in the inventory table above.
2. For each "No" row: replace `except Exception` with the specific types listed.
3. For each "Yes — keep" row: verify `log.exception()` (not `log.error()` or bare `pass`) is called so the full traceback appears in logs.
4. Run full test suite + new `test_error_handling.py` tests.
--- ---