feat: Implement uniform error handling and dynamic versioning#35
feat: Implement uniform error handling and dynamic versioning#35aniebietafia merged 4 commits intomainfrom
Conversation
- Create centralized exception hierarchy in app/core/exceptions.py - Standardize API error response schema in app/core/error_responses.py - Implement global FastAPI exception handlers for custom/validation/HTTP errors - Dynamically read project version from pyproject.toml in Settings - Add comprehensive test suite for error handling scenarios - Configure Pydantic to ignore extra environment variables Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
- Create centralized exception hierarchy in app/core/exceptions.py - Standardize API error response schema in app/core/error_responses.py - Implement global FastAPI exception handlers for custom/validation/HTTP errors - Dynamically read project version from pyproject.toml in Settings - Add comprehensive test suite for error handling scenarios - Configure Pydantic to ignore extra environment variables Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a centralized error-handling system (custom exceptions, response models, and global handlers), makes application version dynamic from pyproject.toml, raises CI pytest coverage threshold, updates tests to use dynamic version, and removes an old linting guidance doc. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant Handler as ExceptionHandler
participant Creator as create_error_response
participant JSON as JSONResponse
Client->>FastAPI: Send HTTP request
FastAPI->>FastAPI: Process request
alt exception thrown
FastAPI->>Handler: Route to registered handler
Handler->>Creator: Build ErrorResponse (code,message,details,status)
Creator->>JSON: Serialize model_dump()
JSON->>Client: Return structured error JSON (status code)
else no exception
FastAPI->>Client: Return success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/test_error_handling.py (1)
20-62: Consider test route isolation.These test routes are added directly to the production
appinstance at module import time. While this works for testing, it means these/test/*routes will exist in the app whenever this module is imported.This is acceptable for a test-only module, but for better isolation, consider using a fixture that creates a fresh app instance or removes routes after tests. Low priority given the routes are only exposed during test execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_error_handling.py` around lines 20 - 62, The test routes (trigger_bad_request, trigger_unauthorized, trigger_forbidden, trigger_not_found, trigger_conflict, trigger_http_exception, trigger_unhandled_exception, trigger_validation_error) are being registered on the shared production app at import time; refactor tests to avoid polluting the global app by creating a fresh app instance or removing routes after each test: implement a test fixture that constructs a new FastAPI app, registers these test endpoints on that local app (or clones the production app then mounts test routes), and use that fixture's TestClient in tests, or alternatively capture and unregister these route handlers at teardown to ensure isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/core/config.py`:
- Line 14: Replace the built-in open(...) call with Path.open(...) to follow
pathlib usage: locate the context where pyproject_path is used (the with
open(pyproject_path, "rb") as f block in app/core/config.py) and change it to
use pyproject_path.open("rb") so that the code calls Path.open on the
pyproject_path Path object (ensure mode "rb" is preserved).
- Around line 3-6: The fallback import block in app/core/config.py (the
try/except that imports tomllib or falls back to tomli) requires adding tomli to
project dependencies and silencing mypy's redefinition warning: add tomli to
requirements.txt or pyproject.toml with a Python constraint (e.g., only for
python_version < "3.11") and append a mypy suppression comment to the fallback
import line (the `import tomli as tomllib` branch) to suppress the
redefinition/import warning so the type checker does not complain.
In `@app/core/exception_handlers.py`:
- Around line 57-66: The function unhandled_exception_handler lacks a return
type and uses an unused parameter; update its signature to include an explicit
Response return annotation (e.g., -> Response) and prefix the unused request
parameter with an underscore (rename request to _request) so linters know it's
intentionally unused; keep exc as-is for logging and ensure imports for Response
are present if needed.
- Around line 46-54: Update the http_exception_handler signature to annotate its
return type and mark the unused request param as intentionally unused: rename
the first parameter from request to _request and add an explicit return type
(e.g., -> Response or -> JSONResponse depending on what create_error_response
returns). Ensure you import the matching Response/JSONResponse type used by
create_error_response and update the function signature for
http_exception_handler(_request: Request, exc: StarletteHTTPException) ->
Response (or -> JSONResponse).
- Around line 25-43: Update the exception handler signature to annotate the
return type and mark the unused request parameter as private: change the
function signature of validation_exception_handler(request: Request, exc:
RequestValidationError) to validation_exception_handler(_request: Request, exc:
RequestValidationError) -> JSONResponse (or the appropriate Response type that
create_error_response returns) and ensure the corresponding
Response/JSONResponse is imported so the type annotation resolves; leave the
body and create_error_response usage unchanged.
- Around line 13-22: Update the FastAPI exception handler signature to include
an explicit return type and mark the unused request parameter as intentionally
unused: rename request to _request in the function definition of
fluentmeet_exception_handler and add the return type annotation (e.g., ->
Response). Also import the Response type from fastapi.responses (or
starlette.responses) so the annotation resolves; no other logic changes required
because only exc is used inside create_error_response.
In `@tests/test_error_handling.py`:
- Around line 1-2: Remove the unused imports causing test pipeline failures:
delete the pytest and FastAPI import names from the top of the file so only
actually used symbols remain (e.g., keep HTTPException if it is referenced
elsewhere); check for any references to pytest or FastAPI in the test file and
remove or replace them as needed so imports match usage.
---
Nitpick comments:
In `@tests/test_error_handling.py`:
- Around line 20-62: The test routes (trigger_bad_request, trigger_unauthorized,
trigger_forbidden, trigger_not_found, trigger_conflict, trigger_http_exception,
trigger_unhandled_exception, trigger_validation_error) are being registered on
the shared production app at import time; refactor tests to avoid polluting the
global app by creating a fresh app instance or removing routes after each test:
implement a test fixture that constructs a new FastAPI app, registers these test
endpoints on that local app (or clones the production app then mounts test
routes), and use that fixture's TestClient in tests, or alternatively capture
and unregister these route handlers at teardown to ensure isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b681b5ea-cd4f-4f04-9d69-a077bf3fd703
📒 Files selected for processing (10)
.github/workflows/ci.ymlapp/core/config.pyapp/core/error_responses.pyapp/core/exception_handlers.pyapp/core/exceptions.pyapp/main.pyinfra/docker-compose.ymllinting_issue.mdtests/test_error_handling.pytests/test_main.py
💤 Files with no reviewable changes (1)
- linting_issue.md
app/core/config.py
Outdated
| try: | ||
| import tomllib | ||
| except ImportError: | ||
| import tomli as tomllib |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if tomli is already in requirements
rg -i "tomli" requirements*.txt pyproject.toml 2>/dev/null || echo "tomli not found in dependencies"Repository: Brints/FluentMeet
Length of output: 92
Add tomli to project dependencies and suppress type checker warnings.
The tomli module is not listed in your project dependencies, which will cause import failures on Python < 3.11. Add it as a dependency with Python version constraint, and suppress the mypy redefinition warning:
Suggested changes
In app/core/config.py:
try:
import tomllib
except ImportError:
- import tomli as tomllib
+ import tomli as tomllib # type: ignore[import-not-found,no-redef]In requirements.txt (or pyproject.toml):
+tomli>=2.0.0;python_version<"3.11"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| import tomllib | |
| except ImportError: | |
| import tomli as tomllib | |
| try: | |
| import tomllib | |
| except ImportError: | |
| import tomli as tomllib # type: ignore[import-not-found,no-redef] |
🧰 Tools
🪛 GitHub Actions: CI
[error] 6-6: Cannot find implementation or library stub for module named 'tomli' [import-not-found]
[error] 6-6: Name 'tomllib' already defined (by an import) [no-redef]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/core/config.py` around lines 3 - 6, The fallback import block in
app/core/config.py (the try/except that imports tomllib or falls back to tomli)
requires adding tomli to project dependencies and silencing mypy's redefinition
warning: add tomli to requirements.txt or pyproject.toml with a Python
constraint (e.g., only for python_version < "3.11") and append a mypy
suppression comment to the fallback import line (the `import tomli as tomllib`
branch) to suppress the redefinition/import warning so the type checker does not
complain.
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
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 `@app/core/exception_handlers.py`:
- Around line 14-47: Change the broad Any annotations on the exception handler
signatures to the concrete exception types already imported: update
fluentmeet_exception_handler to accept FluentMeetException,
validation_exception_handler to accept RequestValidationError, and
http_exception_handler to accept StarletteHTTPException; this requires editing
the function parameters in the definitions (fluentmeet_exception_handler,
validation_exception_handler, http_exception_handler) to use those specific
types so linting/import checks pass and typing is stronger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1d309e3-933e-4f84-9b49-a320796f0ae4
📒 Files selected for processing (4)
README.mdapp/core/config.pyapp/core/exception_handlers.pytests/test_error_handling.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_error_handling.py
- app/core/config.py
| async def fluentmeet_exception_handler(_request: Request, exc: Any) -> JSONResponse: | ||
| """ | ||
| Handler for all custom FluentMeetException exceptions. | ||
| """ | ||
| return create_error_response( | ||
| status_code=exc.status_code, | ||
| code=exc.code, | ||
| message=exc.message, | ||
| details=exc.details, | ||
| ) | ||
|
|
||
|
|
||
| async def validation_exception_handler(_request: Request, exc: Any) -> JSONResponse: | ||
| """ | ||
| Handler for Pydantic validation errors (422 -> 400). | ||
| """ | ||
| details = [] | ||
| for error in exc.errors(): | ||
| details.append( | ||
| { | ||
| "field": ".".join(str(loc) for loc in error["loc"]), | ||
| "msg": error["msg"], | ||
| } | ||
| ) | ||
|
|
||
| return create_error_response( | ||
| status_code=400, | ||
| code="VALIDATION_ERROR", | ||
| message="Request validation failed", | ||
| details=details, | ||
| ) | ||
|
|
||
|
|
||
| async def http_exception_handler(_request: Request, exc: Any) -> JSONResponse: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unresolved Any usage vs imports in the reviewed file.
rg -n 'from typing import Any|\bAny\b' app/core/exception_handlers.pyRepository: Brints/FluentMeet
Length of output: 318
🏁 Script executed:
head -n 50 app/core/exception_handlers.py | cat -nRepository: Brints/FluentMeet
Length of output: 1796
Fix undefined Any annotations to avoid module import/lint failures.
Lines 14, 26, and 47 use Any without importing it, which causes F821 pipeline failures and blocks CI.
🔧 Proposed fix
import logging
from fastapi import FastAPI, Request
-async def fluentmeet_exception_handler(_request: Request, exc: Any) -> JSONResponse:
+async def fluentmeet_exception_handler(
+ _request: Request, exc: FluentMeetException
+) -> JSONResponse:-async def validation_exception_handler(_request: Request, exc: Any) -> JSONResponse:
+async def validation_exception_handler(
+ _request: Request, exc: RequestValidationError
+) -> JSONResponse:-async def http_exception_handler(_request: Request, exc: Any) -> JSONResponse:
+async def http_exception_handler(
+ _request: Request, exc: StarletteHTTPException
+) -> JSONResponse:Replace Any with the concrete exception types that are already imported (FluentMeetException, RequestValidationError, StarletteHTTPException). This provides stronger typing and is already available in the module.
🧰 Tools
🪛 GitHub Actions: Code Quality
[error] 14-14: F821 Undefined name 'Any' in type annotation.
[error] 26-26: F821 Undefined name 'Any' in type annotation.
[error] 47-47: F821 Undefined name 'Any' in type annotation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/core/exception_handlers.py` around lines 14 - 47, Change the broad Any
annotations on the exception handler signatures to the concrete exception types
already imported: update fluentmeet_exception_handler to accept
FluentMeetException, validation_exception_handler to accept
RequestValidationError, and http_exception_handler to accept
StarletteHTTPException; this requires editing the function parameters in the
definitions (fluentmeet_exception_handler, validation_exception_handler,
http_exception_handler) to use those specific types so linting/import checks
pass and typing is stronger.
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation