Refactor Exceptions - #285
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughCentralizes API error handling by adding two marker exceptions and a new exception_handlers module, moves middleware to pure-ASGI implementations, removes per-route try/except error-to-HTTP conversions (letting exceptions propagate), and adds tests exercising the centralized handlers and middleware headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ASGI as ASGI Middleware (SecurityHeaders / RequestId)
participant App as FastAPI App / Router
participant Service as Business Service
participant Handlers as Exception Handlers
Client->>ASGI: HTTP request
ASGI->>App: forward scope/receive/send
App->>Service: invoke route handler / service call
Service-->>App: raises Exception
App->>Handlers: dispatch to registered exception handler
Handlers-->>ASGI: JSONResponse (status + body)
ASGI-->>Client: HTTP response (headers + body)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@simulation/api/exception_handlers.py`:
- Around line 28-35: The global_exception_handler currently defined as def
global_exception_handler(request: Request, exc: Exception) loses traceback
because sync handlers run in a threadpool; change it to an async function (async
def global_exception_handler(...)) so the exception context is preserved and
logger.exception() can capture the full traceback, keeping the same return
(JSONResponse via error_response) and parameters (request, exc) and retaining
the detail logic (is_local_mode()) unchanged.
In `@simulation/api/main.py`:
- Line 23: There are two implementations of the security-headers middleware
causing inconsistency: the ASGI SecurityHeadersMiddleware in this module and the
old BaseHTTPMiddleware variant still in lib.security_headers; keep a single
implementation to avoid accidental re-import of the broken path. Remove the
duplicate middleware here and instead import and use the canonical
SecurityHeadersMiddleware (or move the canonical implementation back into
lib.security_headers), ensure you still import _hsts_enabled from
lib.security_headers, update any app.add_middleware/app.mount calls to reference
the single SecurityHeadersMiddleware symbol, and delete the old
BaseHTTPMiddleware-based class from lib.security_headers (or vice versa) so only
one SecurityHeadersMiddleware implementation exists project-wide.
In `@tests/api/test_exception_handlers.py`:
- Around line 33-97: Add a real RequestValidationError regression by adding an
endpoint that fails during FastAPI request parsing rather than raising inside
the handler body; for example implement a route function named
trigger_request_validation_error (or similar) that declares a typed
path/query/body parameter (e.g., id: int or a Pydantic model) and does nothing
else, so sending an invalid type or malformed/missing body triggers FastAPI's
RequestValidationError before the handler runs; add this alongside the other
trigger routes (refer to existing trigger_* functions) and ensure the test
invokes it with an invalid request to exercise the RequestValidationError
handler (also add corresponding coverage in the later section referenced
109-211).
- Around line 100-102: Don't mutate the global FastAPI app at import: stop
calling app.include_router(_router, prefix="/test-errors") and creating a
module-level TestClient(app, ...) during import. Instead, move the router
registration and TestClient creation into a pytest fixture (e.g., a
function-scoped fixture that registers _router, yields a TestClient, and then on
teardown restores the original app.router.routes and app.openapi_schema); use
app.include_router(_router, prefix="/test-errors") inside the fixture and after
yielding revert app.router.routes back to the saved original list and reset
app.openapi_schema to the saved value so the global simulation.api.main.app
remains unchanged across tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65a6c061-481e-478e-a102-9a397aafd64c
📒 Files selected for processing (8)
simulation/api/errors.pysimulation/api/exception_handlers.pysimulation/api/main.pysimulation/api/routes/agents.pysimulation/api/routes/metadata.pysimulation/api/routes/posts.pysimulation/api/routes/runs.pytests/api/test_exception_handlers.py
Comment on lines
+33
to
+97
| @_router.get("/trigger/run-not-found") | ||
| async def trigger_run_not_found(): | ||
| raise ApiRunNotFoundError(run_id="run-abc") | ||
|
|
||
|
|
||
| @_router.get("/trigger/run-creation-failed") | ||
| async def trigger_run_creation_failed(): | ||
| raise ApiRunCreationFailedError(message="engine returned no run") | ||
|
|
||
|
|
||
| @_router.get("/trigger/handle-already-exists") | ||
| async def trigger_handle_already_exists(): | ||
| raise ApiHandleAlreadyExistsError(handle="alice") | ||
|
|
||
|
|
||
| @_router.get("/trigger/agent-not-found") | ||
| async def trigger_agent_not_found(): | ||
| raise ApiAgentNotFoundError(handle="alice") | ||
|
|
||
|
|
||
| @_router.get("/trigger/target-agent-not-found") | ||
| async def trigger_target_agent_not_found(): | ||
| raise ApiTargetAgentNotFoundError(handle="bob") | ||
|
|
||
|
|
||
| @_router.get("/trigger/follow-edge-already-exists") | ||
| async def trigger_follow_edge_already_exists(): | ||
| raise ApiAgentFollowEdgeAlreadyExistsError( | ||
| follower_handle="alice", target_handle="bob" | ||
| ) | ||
|
|
||
|
|
||
| @_router.get("/trigger/follow-edge-not-found") | ||
| async def trigger_follow_edge_not_found(): | ||
| raise ApiAgentFollowEdgeNotFoundError(follower_handle="alice", target_handle="bob") | ||
|
|
||
|
|
||
| @_router.get("/trigger/self-follow-not-allowed") | ||
| async def trigger_self_follow_not_allowed(): | ||
| raise ApiSelfFollowNotAllowedError(handle="alice") | ||
|
|
||
|
|
||
| @_router.get("/trigger/unauthorized") | ||
| async def trigger_unauthorized(): | ||
| raise UnauthorizedError(message="Token expired") | ||
|
|
||
|
|
||
| @_router.get("/trigger/value-error") | ||
| async def trigger_value_error(): | ||
| raise ValueError("generic value error") | ||
|
|
||
|
|
||
| @_router.get("/trigger/api-validation-error") | ||
| async def trigger_api_validation_error(): | ||
| raise ApiValidationError("specific validation failed") | ||
|
|
||
|
|
||
| @_router.get("/trigger/api-invalid-input") | ||
| async def trigger_api_invalid_input(): | ||
| raise ApiInvalidInputError("malformed ID format") | ||
|
|
||
|
|
||
| @_router.get("/trigger/internal-error") | ||
| async def trigger_internal_error(): | ||
| raise RuntimeError("unexpected crash") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a real RequestValidationError regression.
simulation/api/exception_handlers.py also replaces FastAPI's RequestValidationError handler, but every test here raises from inside the endpoint body. A broken registration or response shape for request-parsing failures would currently go unnoticed.
Also applies to: 109-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/test_exception_handlers.py` around lines 33 - 97, Add a real
RequestValidationError regression by adding an endpoint that fails during
FastAPI request parsing rather than raising inside the handler body; for example
implement a route function named trigger_request_validation_error (or similar)
that declares a typed path/query/body parameter (e.g., id: int or a Pydantic
model) and does nothing else, so sending an invalid type or malformed/missing
body triggers FastAPI's RequestValidationError before the handler runs; add this
alongside the other trigger routes (refer to existing trigger_* functions) and
ensure the test invokes it with an invalid request to exercise the
RequestValidationError handler (also add corresponding coverage in the later
section referenced 109-211).
Collaborator
Author
|
I ended up refactoring a lot more than the original issue description (which ended up being a pretty comprehensive refactor), so I'll document my whole process:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
simulation/api/exception_handlers.py (1)
148-155: 🧹 Nitpick | 🔵 TrivialRemove redundant
ApiValidationErrorcheck.The explicit
ApiValidationErrorbranch (lines 151-154) returns the same response as the fallback (line 155). Consider simplifying:♻️ Proposed simplification
def value_error_handler(_request: Request, exc: ValueError) -> JSONResponse: if isinstance(exc, ApiInvalidInputError): return error_response(status_code=400, code="INVALID_INPUT", message=str(exc)) - if isinstance(exc, ApiValidationError): - return error_response( - status_code=422, code="VALIDATION_ERROR", message=str(exc) - ) + # ApiValidationError and other ValueError subclasses → 422 return error_response(status_code=422, code="VALIDATION_ERROR", message=str(exc))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/api/exception_handlers.py` around lines 148 - 155, In value_error_handler, remove the redundant explicit ApiValidationError branch and let the final fallback handle it; keep the ApiInvalidInputError check (returning INVALID_INPUT via error_response) and then return the generic VALIDATION_ERROR fallback using error_response for all other ValueError types (including ApiValidationError) so you can delete the if isinstance(exc, ApiValidationError) block and its return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@simulation/api/exception_handlers.py`:
- Around line 158-171: The EXCEPTION_HANDLERS mapping currently types values as
object which loses handler signatures; update the annotation for
EXCEPTION_HANDLERS to a specific callable type (e.g., use typing.Callable with
appropriate parameters/return like Callable[[Request, Exception], Response] or
define an ExceptionHandler alias) and apply it to the mapping so entries such as
global_exception_handler, value_error_handler, validation_exception_handler,
unauthorized_handler, run_not_found_handler, run_creation_failed_handler,
handle_already_exists_handler, agent_not_found_handler,
target_agent_not_found_handler, follow_edge_already_exists_handler,
follow_edge_not_found_handler, and self_follow_not_allowed_handler are typed
correctly for IDE/type-checker support.
---
Duplicate comments:
In `@simulation/api/exception_handlers.py`:
- Around line 148-155: In value_error_handler, remove the redundant explicit
ApiValidationError branch and let the final fallback handle it; keep the
ApiInvalidInputError check (returning INVALID_INPUT via error_response) and then
return the generic VALIDATION_ERROR fallback using error_response for all other
ValueError types (including ApiValidationError) so you can delete the if
isinstance(exc, ApiValidationError) block and its return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 385f10bd-db23-495a-b722-320d0014f639
📒 Files selected for processing (1)
simulation/api/exception_handlers.py
Comment on lines
+158
to
+171
| EXCEPTION_HANDLERS: dict[type[Exception], object] = { | ||
| Exception: global_exception_handler, | ||
| ValueError: value_error_handler, | ||
| RequestValidationError: validation_exception_handler, | ||
| UnauthorizedError: unauthorized_handler, | ||
| ApiRunNotFoundError: run_not_found_handler, | ||
| ApiRunCreationFailedError: run_creation_failed_handler, | ||
| ApiHandleAlreadyExistsError: handle_already_exists_handler, | ||
| ApiAgentNotFoundError: agent_not_found_handler, | ||
| ApiTargetAgentNotFoundError: target_agent_not_found_handler, | ||
| ApiAgentFollowEdgeAlreadyExistsError: follow_edge_already_exists_handler, | ||
| ApiAgentFollowEdgeNotFoundError: follow_edge_not_found_handler, | ||
| ApiSelfFollowNotAllowedError: self_follow_not_allowed_handler, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a more specific type annotation for handler mapping.
Using object for handler values loses type information. A more precise type would improve IDE support and type checking:
♻️ Proposed type improvement
+from collections.abc import Awaitable, Callable
+from typing import TypeAlias
+
+ExceptionHandler: TypeAlias = Callable[
+ [Request, Exception], JSONResponse | Awaitable[JSONResponse]
+]
+
-EXCEPTION_HANDLERS: dict[type[Exception], object] = {
+EXCEPTION_HANDLERS: dict[type[Exception], ExceptionHandler] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simulation/api/exception_handlers.py` around lines 158 - 171, The
EXCEPTION_HANDLERS mapping currently types values as object which loses handler
signatures; update the annotation for EXCEPTION_HANDLERS to a specific callable
type (e.g., use typing.Callable with appropriate parameters/return like
Callable[[Request, Exception], Response] or define an ExceptionHandler alias)
and apply it to the mapping so entries such as global_exception_handler,
value_error_handler, validation_exception_handler, unauthorized_handler,
run_not_found_handler, run_creation_failed_handler,
handle_already_exists_handler, agent_not_found_handler,
target_agent_not_found_handler, follow_edge_already_exists_handler,
follow_edge_not_found_handler, and self_follow_not_allowed_handler are typed
correctly for IDE/type-checker support.
Collaborator
There was a problem hiding this comment.
CodeRabbit's comment is a bit nitpicky, OK to keep as is.
Contributor
There was a problem hiding this comment.
@mark-torres10, understood — no worries, I'll drop this one! 🐇
(´･ω･`)
🧠 Learnings used
Learnt from: mark-torres10
Repo: METResearchGroup/social_agent_simulation_platform PR: 172
File: simulation/core/models/posts.py:0-0
Timestamp: 2026-03-08T16:24:56.751Z
Learning: Guideline: Rename Post model field from uri to source_id across the codebase. Apply to all layers impacted: update canonical_post_id parameter, create a new Alembic migration to rename the DB column, adjust adapter SQL, update API schemas (PostSchema) where this is a breaking change, migrate UI types to camelCase (sourceId), rename validators referencing uri, and update all factories and tests accordingly. Do not allow post.uri in reviews going forward; the field now is post.source_id. This change affects Python files under simulation/, SQL migrations, and any API/UI/validation code that references the field.
… added after done testing
mark-torres10
approved these changes
Mar 24, 2026
mark-torres10
left a comment
mark-torres10
left a comment
Collaborator
There was a problem hiding this comment.
few comments, but otherwise great work! Will merge once those are addressed.
| title="Agent Simulation Platform API", | ||
| lifespan=lifespan, | ||
| ) | ||
| class SecurityHeadersMiddleware: |
Collaborator
There was a problem hiding this comment.
could you delete the SecurityHeadersMiddleware to the lib/security_headers.py file as we don't use it anymore, and then move this SecurityHeadersMIiddleware and the RequestIdMiddleware classes to a simulation/api/middleware.py file?
| app.state.limiter = limiter | ||
| app.add_exception_handler(RateLimitExceeded, rate_limit_exceeded_handler) # type: ignore[reportArgumentType] | ||
|
|
||
| for exc_class, handler_func in EXCEPTION_HANDLERS.items(): |
Collaborator
There was a problem hiding this comment.
Great job on defining the exception handlers in EXCEPTION_HANDLERS and just looping through it, you're right in that this greatly cleans up the number of lines and logic.
| return error_response(status_code=422, code="VALIDATION_ERROR", message=str(exc)) | ||
|
|
||
|
|
||
| EXCEPTION_HANDLERS: dict[type[Exception], object] = { |
Collaborator
There was a problem hiding this comment.
If I'm understanding this correctly, defining these exception handlers will mean something like:
- The application throws an error.
- The exception is caught by the application.
- Based on the exception raised, the appropriate handler is thrown.
Is that correct? Just want to make sure that I follow the intention based on the code and your description.
Collaborator
Author
There was a problem hiding this comment.
Yep, exactly that! The exception is caught by FastAPI's ExceptionMiddleware, which will match the Exception thrown to one of the exceptions added to the middleware (if it exists) and falls back to the basic Exception handler when it doesn't match any.
Comment on lines
+158
to
+171
| EXCEPTION_HANDLERS: dict[type[Exception], object] = { | ||
| Exception: global_exception_handler, | ||
| ValueError: value_error_handler, | ||
| RequestValidationError: validation_exception_handler, | ||
| UnauthorizedError: unauthorized_handler, | ||
| ApiRunNotFoundError: run_not_found_handler, | ||
| ApiRunCreationFailedError: run_creation_failed_handler, | ||
| ApiHandleAlreadyExistsError: handle_already_exists_handler, | ||
| ApiAgentNotFoundError: agent_not_found_handler, | ||
| ApiTargetAgentNotFoundError: target_agent_not_found_handler, | ||
| ApiAgentFollowEdgeAlreadyExistsError: follow_edge_already_exists_handler, | ||
| ApiAgentFollowEdgeNotFoundError: follow_edge_not_found_handler, | ||
| ApiSelfFollowNotAllowedError: self_follow_not_allowed_handler, | ||
| } |
Collaborator
There was a problem hiding this comment.
CodeRabbit's comment is a bit nitpicky, OK to keep as is.
Collaborator
|
could you also merge the latest main into this? That'll merge in some tests that will verify that this implementation will work even in production. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@simulation/api/middleware.py`:
- Around line 48-60: The middleware currently accepts client-supplied
X-Request-ID verbatim (request_id) and echoes/logs it via log_request_start and
send_with_request_id; harden this by validating/sanitizing request_id after
reading request.headers.get("X-Request-ID"): enforce an allowed charset (e.g.,
hex or printable ASCII), strip/control character check, and cap length (e.g., 64
chars); if validation fails, replace with a freshly generated id
(uuid.uuid4().hex), set request.state.request_id to the final safe value, and
use that sanitized id for log_request_start and when adding the X-Request-ID
header in send_with_request_id so only validated IDs are logged/echoed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66b208bb-daaa-45d8-a321-fbd7ff99a3c4
📒 Files selected for processing (3)
lib/security_headers.pysimulation/api/main.pysimulation/api/middleware.py
💤 Files with no reviewable changes (1)
- lib/security_headers.py
Comment on lines
+48
to
+60
| request_id = request.headers.get("X-Request-ID") or uuid.uuid4().hex | ||
| request.state.request_id = request_id | ||
| log_request_start( | ||
| request_id=request_id, | ||
| method=request.method, | ||
| path=request.url.path, | ||
| ) | ||
|
|
||
| async def send_with_request_id(message: Message) -> None: | ||
| if message["type"] == "http.response.start": | ||
| headers = MutableHeaders(scope=message) | ||
| headers["X-Request-ID"] = request_id | ||
| await send(message) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden inbound X-Request-ID before echoing/logging.
The current flow trusts client-supplied X-Request-ID verbatim. Consider validating charset/length and regenerating when invalid to avoid malformed or high-cardinality IDs in logs/headers.
Suggested patch
import uuid
+import re
@@
from lib.request_logging import log_request_start
from lib.security_headers import _hsts_enabled
+REQUEST_ID_RE = re.compile(r"^[A-Za-z0-9._-]{1,128}$")
@@
- request_id = request.headers.get("X-Request-ID") or uuid.uuid4().hex
+ incoming_request_id = request.headers.get("X-Request-ID")
+ request_id = (
+ incoming_request_id
+ if incoming_request_id and REQUEST_ID_RE.fullmatch(incoming_request_id)
+ else uuid.uuid4().hex
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simulation/api/middleware.py` around lines 48 - 60, The middleware currently
accepts client-supplied X-Request-ID verbatim (request_id) and echoes/logs it
via log_request_start and send_with_request_id; harden this by
validating/sanitizing request_id after reading
request.headers.get("X-Request-ID"): enforce an allowed charset (e.g., hex or
printable ASCII), strip/control character check, and cap length (e.g., 64
chars); if validation fails, replace with a freshly generated id
(uuid.uuid4().hex), set request.state.request_id to the final safe value, and
use that sanitized id for log_request_start and when adding the X-Request-ID
header in send_with_request_id so only validated IDs are logged/echoed.
Collaborator
|
LGTM! Great work @dudu-theman ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Having to write try except blocks in the api endpoint functions can become very repetitive as many endpoints end up using the same errors, but with slightly different messages.
Fixes #236.
Solution
Create custom exception handlers to deal with exceptions. Utilize the custom exceptions for this.
Changes
simulation/api/exception_handlers.pycontaining exception handlers for ALL exceptions (not just 500). My reasoning for doing this was that there were so many excepts for each try that it seemed cleaner to just have an exception handler for every single exception instead of still having to deal with a bunch of try excepts.EXCEPTION_HANDLERS, a dictionary that maps exception to exception handler object.simulation/api/errors.pyto deal with ValueErrors since ValueErrors could either return 400 or 422.simulation/api/main.pyfrom class-based middleware to fully asynchronous. Using the class-based caused us to lose the error message when having custom exception handlers. This is because with try except, the endpoint would return a JSONResponse object in the try except regardless, whereas with custom exception handlers (and no try except blocks) I have to add all of the exceptions handler into FastAPI's ExceptionMiddleware, which catches my Exceptions.tests/api/test_exception_handlers.py(which uses the same app object assimulation/api/main.pyto preserve the middleware) with fault injection to test all exceptions.Manual Verification
PYTHONPATH=. uv run pytest tests/api/test_exception_handlers.py -v
Summary by CodeRabbit
Refactor
New Features
Tests