Implement API response contract and custom exception handling#35
Open
saudzahirr wants to merge 2 commits intomainfrom
Open
Implement API response contract and custom exception handling#35saudzahirr wants to merge 2 commits intomainfrom
saudzahirr wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR adds a standardized API response contract, Pydantic response schemas, an APIException hierarchy, request-ID middleware, centralized exception handlers, DB lifecycle and deps, auth/security utilities, health/login routers, a pre-start DB checker, and a broadened .gitignore entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as add_request_id
participant Route as Handler
participant ExceptionH as ExceptionHandler
participant Logger
participant DB as DB/Deps
Client->>Middleware: HTTP Request (may include X-Request-ID)
Middleware->>Middleware: ensure/generate X-Request-ID
Middleware->>Route: call_next(request) with request_id
alt Handler normal flow
Route->>DB: acquire Cursor via get_db
DB-->>Route: Cursor
Route-->>Client: 200 Success (SuccessResponse + X-Request-ID)
else Handler raises APIException
Route->>ExceptionH: APIException
ExceptionH->>Logger: log warning with request_id
ExceptionH-->>Client: error payload (code, message, http_status, details, request_id)
else Handler raises RequestValidationError
Route->>ExceptionH: RequestValidationError
ExceptionH->>Logger: log validation details with request_id
ExceptionH-->>Client: 422 Validation Error (details + request_id)
else Unhandled Exception
Route->>ExceptionH: Exception
ExceptionH->>Logger: log traceback with request_id
ExceptionH-->>Client: 500 Internal Server Error (generic code + request_id)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/API_RESPONSE_CONTRACT.md`:
- Around line 145-165: The fenced code blocks under the "Example
Request/Response with Request ID" section (and the other flagged ranges) are
missing surrounding blank lines and language tags; update each request/response
fence to have a blank line before and after the triple-backticks and add an
appropriate language tag (e.g., ```http for the request/response examples and
```json for JSON bodies) so that all examples follow markdownlint rules and
render correctly.
In `@backend/app/core/exceptions.py`:
- Around line 26-35: The ValidationException currently constructs the base error
with http_status=400 while using ErrorCode.VALIDATION_FAILED; change the
http_status passed in ValidationException.__init__ to 422 so the class aligns
with the API contract and existing Pydantic handler; also update the example
entry in the response schema (in backend/app/schemas/response.py) that documents
VALIDATION_FAILED to show http_status: 422 instead of 400 to keep docs
consistent with the exception class.
In `@backend/app/main.py`:
- Around line 91-94: Replace eager f-string interpolation in logging calls with
lazy %-style formatting: change the logger.warning call that currently uses
f"API Exception: {exc.error_code} - {exc.message}" to use a format string and
pass exc.error_code and exc.message as separate positional args (keeping the
extra={"request_id": request_id, "error_code": exc.error_code} argument). Do the
same refactor for the other logging statements mentioned (the similar logger.*
calls at the other occurrences around the API exception handling blocks) so all
logging uses format strings like "API Exception: %s - %s" with parameters
instead of f-strings.
- Line 24: The code passes settings.DATABASE_DSN (a PostgresDsn object) directly
into ConnectionPool; convert it to a plain string before use. Update the
instantiation of ConnectionPool so it receives str(settings.DATABASE_DSN) (same
fix applied in pre_start.py), ensuring any code that constructs the pool (the
ConnectionPool call in main.py) receives a string DSN rather than the
PostgresDsn object.
In `@build/lib/backend/app/api/deps.py`:
- Around line 11-12: get_db currently raises a plain HTTPException which
bypasses the new standardized error handlers; replace the HTTPException raise in
get_db with raising APIException(status_code=500, detail="Database pool not
initialized") (or equivalent constructor used by your APIException), and
add/import APIException into deps.py so the custom exception middleware will
format the response consistently.
In `@build/lib/backend/app/main.py`:
- Line 3: The imports typing.Any and http_exception_handler are unused in
main.py; remove the unused import statement "from typing import Any" and the
import of http_exception_handler (where it's imported) so the file only imports
symbols that are actually used (e.g., delete or trim the import lines
referencing Any and http_exception_handler and run the linter/type-check to
confirm no references remain).
- Line 26: settings.DATABASE_DSN is a PostgresDsn object, not a plain string, so
passing it directly to ConnectionPool causes a type/serialization issue; update
the ConnectionPool instantiation in main.py to pass str(settings.DATABASE_DSN)
(i.e., convert the PostgresDsn to a string) so
ConnectionPool(settings.DATABASE_DSN) becomes
ConnectionPool(str(settings.DATABASE_DSN)), ensuring ConnectionPool receives a
proper DSN string.
- Around line 88-97: Remove the dead stub function get_request_id_from_context()
from the module: it only imports fastapi/Headers inside itself, catches all
exceptions, and always returns a new UUID, so delete the entire function
definition (def get_request_id_from_context(...)) to eliminate non-functional
code; ensure no remaining references to get_request_id_from_context() exist in
the file before committing.
---
Duplicate comments:
In `@backend/app/schemas/response.py`:
- Around line 56-67: The example in Config.json_schema_extra is inconsistent:
the "error.code" ("VALIDATION_FAILED") doesn't match the canonical HTTP status
chosen elsewhere; update this example in backend/app/schemas/response.py (Config
-> json_schema_extra -> example) so the "error.code", "error.message" and
"error.http_status" reflect the canonical status decided in the exception
hierarchy (e.g., if the canonical status for validation is 422 change
http_status to 422 and keep code "VALIDATION_FAILED" and a validation message;
if the canonical status is 400 change code to "BAD_REQUEST" and adjust message
accordingly), and ensure "details" and "request_id" remain representative.
In `@build/lib/backend/app/core/exceptions.py`:
- Around line 26-35: The ValidationException currently sets http_status=400 in
__init__ (class ValidationException) which conflicts with the API contract;
update ValidationException to use the contract-approved status (e.g.,
http_status=422) and ensure the change is applied both in the source
ValidationException implementation and in this built artifact
(build/lib/backend/app/core/exceptions.py) so they stay in sync, then regenerate
the build artifacts so the compiled module reflects the corrected http_status.
In `@build/lib/backend/app/schemas/response.py`:
- Around line 57-67: The json_schema_extra example in response.py currently
shows an error object with code "VALIDATION_FAILED" but the http_status value is
inconsistent with the canonical status chosen; update the example inside
json_schema_extra so the "http_status" matches the officially chosen status for
VALIDATION_FAILED (e.g., set to 400 if validation errors are 400, or change the
"code" to the status-aligned code), and ensure the "code" and "http_status"
inside the example error object are consistent with the canonical exception
handling used elsewhere.
Member
Author
|
@laraibg786 Please take a look! |
Member
Author
|
Resolves #5 |
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Chores