fix: let HTTPException pass through @handle_db_errors #104

Merged

cal merged 2 commits from fix/handle-db-errors-passthrough-http into main 2026-04-02 16:57:14 +00:00

Conversation 1 Commits 2 Files Changed 2 +18 -15

cal commented 2026-04-02 13:30:47 +00:00

Owner

Copy Link

Summary

• @handle_db_errors was catching all exceptions including intentional HTTPException (401, 404, etc.) and re-wrapping them as 500 "Database error"
• This masked auth failures on POST /transactions/ — the bot saw a 500 instead of the actual 401
• Fix: add an except HTTPException: raise clause before the generic except Exception handler

Root cause

Discovered during /ilmove failure investigation — an API_TOKEN mismatch caused a 401 on POST /transactions/, but the decorator re-wrapped it as a 500, making the error undiagnosable from the bot side.

Test plan

• test_query_limits.py — 5/5 pass
• test_base_service.py — 14/14 pass (1 pre-existing failure unrelated)
• Verify 401 responses are returned as-is (not wrapped as 500)

🤖 Generated with Claude Code

## Summary - `@handle_db_errors` was catching **all** exceptions including intentional `HTTPException` (401, 404, etc.) and re-wrapping them as `500 "Database error"` - This masked auth failures on `POST /transactions/` — the bot saw a 500 instead of the actual 401 - Fix: add an `except HTTPException: raise` clause before the generic `except Exception` handler ## Root cause Discovered during `/ilmove` failure investigation — an API_TOKEN mismatch caused a 401 on `POST /transactions/`, but the decorator re-wrapped it as a 500, making the error undiagnosable from the bot side. ## Test plan - [x] `test_query_limits.py` — 5/5 pass - [x] `test_base_service.py` — 14/14 pass (1 pre-existing failure unrelated) - [ ] Verify 401 responses are returned as-is (not wrapped as 500) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-04-02 13:30:48 +00:00

fix: let HTTPException pass through @handle_db_errors unchanged ... 215085b326

The decorator was catching all exceptions including intentional
HTTPException (401, 404, etc.) and re-wrapping them as 500 "Database
error". This masked auth failures and other deliberate HTTP errors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Claude added the

ai-reviewing

label 2026-04-02 13:46:58 +00:00

Claude requested changes 2026-04-02 13:48:54 +00:00

Dismissed

Claude left a comment

Collaborator

Copy Link

Review: fix: let HTTPException pass through @handle_db_errors

The fix itself is correct and necessary — HTTPException is a subclass of Exception, so the generic except Exception handler was catching intentional 401/404 responses and re-wrapping them as 500. The except HTTPException: raise clause before the generic handler is the right fix, and HTTPException is already imported at dependencies.py:10.

However, there is one blocking issue before merging.

---

Blocking: Integration test asserts the old broken behavior

tests/integration/test_stratplay_routes.py:499 was written to document the (now-fixed) broken behavior:

def test_get_nonexistent_play(self, api):
    """GET /plays/999999999 returns an error (wrapped by handle_db_errors)."""
    r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10)
    # handle_db_errors wraps HTTPException as 500 with detail message
    assert r.status_code == 500   # ← will fail after this PR
    assert "not found" in r.json().get("detail", "").lower()

crud.py:24 raises HTTPException(status_code=404, ...) for a missing play. Before this PR, @handle_db_errors caught it and re-raised as 500. After this fix, it correctly returns 404. The assertion on line 499 will fail.

Fix needed:

def test_get_nonexistent_play(self, api):
    """GET /plays/999999999 returns 404 Not Found."""
    r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10)
    assert r.status_code == 404
    assert "not found" in r.json().get("detail", "").lower()

The adjacent test_week_and_stype_conflict (line 517) asserts r.status_code in (400, 500) — that assertion is already permissive and will continue to pass (returns 400 after the fix).

---

Non-blocking notes

• The except Exception handler calls db.close() for actual DB errors. For the new except HTTPException: raise path, no explicit close is called — that's fine because the middleware from PR #87 closes the connection in its finally block regardless.
• The PR test plan doesn't mention the integration test suite — worth adding test_stratplay_routes.py to the manual checklist.

## Review: fix: let HTTPException pass through @handle_db_errors The fix itself is **correct and necessary** — `HTTPException` is a subclass of `Exception`, so the generic `except Exception` handler was catching intentional 401/404 responses and re-wrapping them as 500. The `except HTTPException: raise` clause before the generic handler is the right fix, and `HTTPException` is already imported at `dependencies.py:10`. However, there is one **blocking issue** before merging. --- ### Blocking: Integration test asserts the old broken behavior `tests/integration/test_stratplay_routes.py:499` was written to document the (now-fixed) broken behavior: ```python def test_get_nonexistent_play(self, api): """GET /plays/999999999 returns an error (wrapped by handle_db_errors).""" r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10) # handle_db_errors wraps HTTPException as 500 with detail message assert r.status_code == 500 # ← will fail after this PR assert "not found" in r.json().get("detail", "").lower() ``` `crud.py:24` raises `HTTPException(status_code=404, ...)` for a missing play. Before this PR, `@handle_db_errors` caught it and re-raised as 500. After this fix, it correctly returns 404. The assertion on line 499 will fail. **Fix needed:** ```python def test_get_nonexistent_play(self, api): """GET /plays/999999999 returns 404 Not Found.""" r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10) assert r.status_code == 404 assert "not found" in r.json().get("detail", "").lower() ``` The adjacent `test_week_and_stype_conflict` (line 517) asserts `r.status_code in (400, 500)` — that assertion is already permissive and will continue to pass (returns 400 after the fix). --- ### Non-blocking notes - The `except Exception` handler calls `db.close()` for actual DB errors. For the new `except HTTPException: raise` path, no explicit close is called — that's fine because the middleware from PR #87 closes the connection in its `finally` block regardless. - The PR test plan doesn't mention the integration test suite — worth adding `test_stratplay_routes.py` to the manual checklist.

tests/integration/test_stratplay_routes.py

Claude commented 2026-04-02 13:48:54 +00:00

Collaborator

Copy Link

This assertion documents the old broken behavior and will fail after this fix. GET /plays/999999999 now correctly returns 404 (from crud.py:24's HTTPException(status_code=404, ...)), not 500.

Change to:

assert r.status_code == 404

and update the docstring/comment accordingly.

This assertion documents the old broken behavior and will fail after this fix. `GET /plays/999999999` now correctly returns 404 (from `crud.py:24`'s `HTTPException(status_code=404, ...)`), not 500. Change to: ```python assert r.status_code == 404 ``` and update the docstring/comment accordingly.

Claude removed the

ai-reviewing

label 2026-04-02 13:49:16 +00:00

Claude added the

ai-reviewing

label 2026-04-02 14:15:07 +00:00

Claude requested changes 2026-04-02 14:16:30 +00:00

Claude left a comment

Collaborator

Copy Link

AI Code Review

Files Reviewed

• app/dependencies.py (modified)

Findings

Correctness

• Fix is correct. HTTPException subclasses Exception, so without this change it was caught by the generic handler and re-wrapped as a 500. The new except HTTPException: raise clause is placed correctly before except Exception as e: and will intercept all intentional HTTP errors (401, 403, 404, 400, etc.).
• HTTPException already imported at app/dependencies.py:10 — no new import needed.
• Connection cleanup on HTTPException path: the db.close() inside except Exception is no longer reached for HTTP errors. This is safe — the middleware added in PR #87 (now in main.py) has a finally: db.close() that covers this path.

Security

• No issues found.

Style & Conventions

• No issues found. The comment on the new clause is clear and appropriate.

Blocker — Integration test documents old (broken) behavior

tests/integration/test_stratplay_routes.py:499 must be updated before merging:

def test_get_nonexistent_play(self, api):
    """GET /plays/999999999 returns an error (wrapped by handle_db_errors)."""
    r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10)
    # handle_db_errors wraps HTTPException as 500 with detail message
    assert r.status_code == 500   # ← WILL FAIL after this fix
    assert "not found" in r.json().get("detail", "").lower()

After this fix, GET /plays/999999999 raises a 404 HTTPException inside the route, which now passes through the decorator unchanged. The assertion must change to == 404, and the docstring/comment should be updated to reflect the corrected behavior.

Required change:

def test_get_nonexistent_play(self, api):
    """GET /plays/999999999 returns 404 Not Found."""
    r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10)
    assert r.status_code == 404
    assert "not found" in r.json().get("detail", "").lower()

Non-blocking

• test_week_and_stype_conflict (line 517) asserts r.status_code in (400, 500) — this will continue to pass after the fix (400 now passes through correctly). No change needed.

Verdict: REQUEST_CHANGES

The core fix is correct and addresses a real observability bug (auth failures silently becoming 500s). One integration test at tests/integration/test_stratplay_routes.py:499 explicitly tests the old broken wrapping behavior and will fail after deployment. Update the assertion to == 404 before merging.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified) ### Findings #### Correctness - **Fix is correct.** `HTTPException` subclasses `Exception`, so without this change it was caught by the generic handler and re-wrapped as a 500. The new `except HTTPException: raise` clause is placed correctly before `except Exception as e:` and will intercept all intentional HTTP errors (401, 403, 404, 400, etc.). - **`HTTPException` already imported** at `app/dependencies.py:10` — no new import needed. - **Connection cleanup on HTTPException path**: the `db.close()` inside `except Exception` is no longer reached for HTTP errors. This is safe — the middleware added in PR #87 (now in `main.py`) has a `finally: db.close()` that covers this path. #### Security - No issues found. #### Style & Conventions - No issues found. The comment on the new clause is clear and appropriate. #### Blocker — Integration test documents old (broken) behavior `tests/integration/test_stratplay_routes.py:499` **must be updated** before merging: ```python def test_get_nonexistent_play(self, api): """GET /plays/999999999 returns an error (wrapped by handle_db_errors).""" r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10) # handle_db_errors wraps HTTPException as 500 with detail message assert r.status_code == 500 # ← WILL FAIL after this fix assert "not found" in r.json().get("detail", "").lower() ``` After this fix, `GET /plays/999999999` raises a 404 `HTTPException` inside the route, which now passes through the decorator unchanged. The assertion must change to `== 404`, and the docstring/comment should be updated to reflect the corrected behavior. **Required change:** ```python def test_get_nonexistent_play(self, api): """GET /plays/999999999 returns 404 Not Found.""" r = requests.get(f"{api}/api/v3/plays/999999999", timeout=10) assert r.status_code == 404 assert "not found" in r.json().get("detail", "").lower() ``` #### Non-blocking - `test_week_and_stype_conflict` (line 517) asserts `r.status_code in (400, 500)` — this will continue to pass after the fix (400 now passes through correctly). No change needed. ### Verdict: REQUEST_CHANGES The core fix is correct and addresses a real observability bug (auth failures silently becoming 500s). One integration test at `tests/integration/test_stratplay_routes.py:499` explicitly tests the old broken wrapping behavior and will fail after deployment. Update the assertion to `== 404` before merging. --- *Automated review by Claude PR Reviewer*

Claude added

ai-changes-requested

and removed

ai-reviewing

labels 2026-04-02 14:16:36 +00:00

Claude added the

ai-working

label 2026-04-02 14:30:06 +00:00

cal added 1 commit 2026-04-02 14:30:47 +00:00

test: update test_get_nonexistent_play to expect 404 after HTTPException fix ... c49f91cc19

After handle_db_errors no longer catches HTTPException, GET /plays/999999999
correctly returns 404 instead of 500. Update the assertion and docstring
to reflect the fixed behavior.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude commented 2026-04-02 14:30:56 +00:00

Collaborator

Copy Link

Addressed the blocking review feedback in commit c49f91c:

tests/integration/test_stratplay_routes.py:499 — updated test_get_nonexistent_play to assert r.status_code == 404 (was 500) and removed the stale docstring/comment that described the old broken wrapping behavior. The test now correctly documents the fixed behavior: HTTPException(404) passes through @handle_db_errors unchanged.

test_week_and_stype_conflict (line 517) was not changed — the in (400, 500) assertion is already permissive and passes with the fix.

Addressed the blocking review feedback in commit c49f91c: **`tests/integration/test_stratplay_routes.py:499`** — updated `test_get_nonexistent_play` to assert `r.status_code == 404` (was `500`) and removed the stale docstring/comment that described the old broken wrapping behavior. The test now correctly documents the fixed behavior: `HTTPException(404)` passes through `@handle_db_errors` unchanged. `test_week_and_stype_conflict` (line 517) was not changed — the `in (400, 500)` assertion is already permissive and passes with the fix.

Claude removed the

ai-working

ai-changes-requested

labels 2026-04-02 14:31:00 +00:00

Claude added the

ai-reviewing

label 2026-04-02 14:45:08 +00:00

Claude reviewed 2026-04-02 14:46:06 +00:00

Claude left a comment

Collaborator

Copy Link

AI Code Review

Files Reviewed

• app/dependencies.py (modified)
• tests/integration/test_stratplay_routes.py (modified)

Findings

Correctness

• The fix is correct. HTTPException is a subclass of Exception, so the original except Exception clause caught it and re-wrapped it as a 500. Adding except HTTPException: raise immediately before except Exception intercepts intentional HTTP errors and re-raises them unchanged. Placement is sound — it comes after the return result success path and before the generic error handler.
• HTTPException is already imported at line 10 (from fastapi import HTTPException, Response) — no new import required.
• The previous blocker from this PR's earlier review iteration has been resolved: tests/integration/test_stratplay_routes.py line 499 now correctly asserts r.status_code == 404 (was 500), and the docstring and comment have been updated to reflect the fixed behavior.
• The additional test changes (assert message reformatting) are cosmetic ruff/black style fixes — no behavioral change.

Security

• No issues introduced by this change. The fix improves security observability: 401 auth failures from valid_token() checks will now propagate correctly to callers instead of being masked as opaque 500 errors.

Style & Conventions

• The new except clause is clean and idiomatic. The inline comment (# Let intentional HTTP errors (401, 404, etc.) pass through unchanged) is appropriately explanatory.
• Reformatted assert statements in the test file follow the assert expr, msg pattern preferred by ruff — consistent with project formatting.

Suggestions

• No blocking suggestions. The hardcoded Discord webhook URL at app/dependencies.py:519 is a pre-existing issue tracked separately — out of scope for this PR.

Verdict: COMMENT

The fix is correct and the previously-flagged blocker (integration test asserting the old broken 500 behavior) has been addressed in this PR. The implementation is minimal, well-placed, and the test update accurately documents the new correct behavior. Approving as COMMENT per convention (Gitea blocks self-approval).

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified) - `tests/integration/test_stratplay_routes.py` (modified) ### Findings #### Correctness - The fix is correct. `HTTPException` is a subclass of `Exception`, so the original `except Exception` clause caught it and re-wrapped it as a 500. Adding `except HTTPException: raise` immediately before `except Exception` intercepts intentional HTTP errors and re-raises them unchanged. Placement is sound — it comes after the `return result` success path and before the generic error handler. - `HTTPException` is already imported at line 10 (`from fastapi import HTTPException, Response`) — no new import required. - The previous blocker from this PR's earlier review iteration has been resolved: `tests/integration/test_stratplay_routes.py` line 499 now correctly asserts `r.status_code == 404` (was `500`), and the docstring and comment have been updated to reflect the fixed behavior. - The additional test changes (assert message reformatting) are cosmetic ruff/black style fixes — no behavioral change. #### Security - No issues introduced by this change. The fix improves security observability: 401 auth failures from `valid_token()` checks will now propagate correctly to callers instead of being masked as opaque 500 errors. #### Style & Conventions - The new `except` clause is clean and idiomatic. The inline comment (`# Let intentional HTTP errors (401, 404, etc.) pass through unchanged`) is appropriately explanatory. - Reformatted assert statements in the test file follow the `assert expr, msg` pattern preferred by ruff — consistent with project formatting. #### Suggestions - No blocking suggestions. The hardcoded Discord webhook URL at `app/dependencies.py:519` is a pre-existing issue tracked separately — out of scope for this PR. ### Verdict: COMMENT The fix is correct and the previously-flagged blocker (integration test asserting the old broken `500` behavior) has been addressed in this PR. The implementation is minimal, well-placed, and the test update accurately documents the new correct behavior. Approving as COMMENT per convention (Gitea blocks self-approval). --- *Automated review by Claude PR Reviewer*

Claude removed the

ai-reviewing

label 2026-04-02 14:46:33 +00:00

Claude added the

ai-reviewing

label 2026-04-02 15:15:07 +00:00

Claude reviewed 2026-04-02 15:16:12 +00:00

Claude left a comment

Collaborator

Copy Link

AI Code Review

Files Reviewed

• app/dependencies.py (modified)
• tests/integration/test_stratplay_routes.py (modified)

Findings

Correctness

• Fix is correct. HTTPException (Starlette) is a subclass of Exception, so without this clause it was caught by the broad except Exception handler and re-wrapped as a 500. Adding except HTTPException: raise before except Exception restores the intended semantics.
• HTTPException is already imported at dependencies.py:10 — no new import needed.
• DB rollback/close is correctly skipped for HTTPException: if an HTTPException is raised, it was raised intentionally by the handler (auth check, not found, etc.) with no DB transaction to roll back. The connection middleware (main.py) handles db.close() in its finally block regardless.
• Test update is accurate: crud.py:24 raises HTTPException(status_code=404, detail=f"Play ID {play_id} not found") for nonexistent plays. Both the == 404 assertion and the "not found" in detail check are correct.
• All other test changes are ruff/black assert-message reformats — no functional change.

Security

• No issues. This fix improves debuggability of auth failures (401s now surface correctly instead of being masked as 500s).

Style & Conventions

• No issues. Minimal change, clean comment, follows project patterns.

Suggestions

• None.

Verdict: COMMENT

Approved — Gitea blocks self-review. Fix is minimal, correct, and well-tested. The blocker from the earlier review (test asserting == 500) has been resolved.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified) - `tests/integration/test_stratplay_routes.py` (modified) ### Findings #### Correctness - Fix is correct. `HTTPException` (Starlette) is a subclass of `Exception`, so without this clause it was caught by the broad `except Exception` handler and re-wrapped as a 500. Adding `except HTTPException: raise` before `except Exception` restores the intended semantics. - `HTTPException` is already imported at `dependencies.py:10` — no new import needed. - DB rollback/close is correctly skipped for `HTTPException`: if an `HTTPException` is raised, it was raised intentionally by the handler (auth check, not found, etc.) with no DB transaction to roll back. The connection middleware (`main.py`) handles `db.close()` in its `finally` block regardless. - Test update is accurate: `crud.py:24` raises `HTTPException(status_code=404, detail=f"Play ID {play_id} not found")` for nonexistent plays. Both the `== 404` assertion and the `"not found" in detail` check are correct. - All other test changes are ruff/black assert-message reformats — no functional change. #### Security - No issues. This fix improves debuggability of auth failures (401s now surface correctly instead of being masked as 500s). #### Style & Conventions - No issues. Minimal change, clean comment, follows project patterns. #### Suggestions - None. ### Verdict: COMMENT Approved — Gitea blocks self-review. Fix is minimal, correct, and well-tested. The blocker from the earlier review (test asserting `== 500`) has been resolved. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-02 15:16:20 +00:00

cal merged commit 0d8e666a75 into main 2026-04-02 16:57:14 +00:00

cal deleted branch fix/handle-db-errors-passthrough-http 2026-04-02 16:57:15 +00:00

cal referenced this issue from a commit 2026-04-02 16:57:18 +00:00

Merge pull request 'fix: let HTTPException pass through @handle_db_errors' (#104) from fix/handle-db-errors-passthrough-http into main

Sign in to join this conversation.

Reviewers

No reviewers

Claude

Labels

Clear labels   

ai-changes-requested

  

ai-pr-opened

  

ai-reviewed

  

ai-reviewing

  

ai-reviewing

  

ai-working

No Label

ai-changes-requested

ai-pr-opened

ai-reviewed

ai-reviewing

ai-reviewing

ai-working

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

2 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-database#104

No description provided.