Fix HTTP exception handler detail normalization #625
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a helper to normalize various HTTPException detail shapes into (message, type, details); refactored http_exception_handler to use it for consistent chat and non-chat error payloads; added unit tests covering normalization, merging, and preservation of structured detail and outer metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as App/Router
participant H as http_exception_handler
participant N as _normalize_http_exception_detail
participant R as Response
C->>A: Request triggers HTTPException
A->>H: Pass HTTPException (exc)
H->>N: Normalize exc.detail
N-->>H: (message, type, extra_details)
alt Chat completions endpoint
H->>H: Build chat error payload using message/type
H->>H: Attach extra_details under `error` when present
else Standard endpoint
H->>H: Build non-chat error payload with status_code
H->>H: Merge/preserve extra_details into `detail.error`
end
H-->>R: Return JSON response
R-->>C: Error payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| default_type = "HttpError" | ||
| if isinstance(detail, Mapping): | ||
| # Handle FastAPI-style payloads where the detail already contains an | ||
| # ``error`` structure with message/type/details fields. | ||
| nested_error = detail.get("error") | ||
| if isinstance(nested_error, Mapping): | ||
| message = nested_error.get("message") | ||
| error_type = nested_error.get("type", default_type) | ||
| details = nested_error.get("details") | ||
| if message is None: | ||
| message = str({k: v for k, v in detail.items() if k != "error"}) | ||
| return str(message), str(error_type), details |
There was a problem hiding this comment.
When the detail payload already contains an error object, _normalize_http_exception_detail returns only the nested message/type/details and ignores any sibling keys in the original mapping. For a payload like {"error": {...}, "trace_id": "abc"} the returned tuple drops trace_id, so the standard and chat error responses no longer contain that metadata at all. Prior to this change the full dictionary was stringified and the metadata remained visible, so this regression strips useful context (request IDs, trace data, etc.) from error responses. Consider merging keys outside of error into the returned details just like the generic mapping branch does so structured extras are preserved.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/app/error_handlers.py (1)
64-110: LGTM! Solid normalization logic with good fallback handling.The
_normalize_http_exception_detailfunction effectively handles three distinct payload shapes:
- Nested error structure (lines 70-80): Extracts from
detail.error.{message,type,details}- Generic mapping (lines 83-106): Extracts common keys and merges extra fields into details
- Fallback (lines 108-109): Treats non-mapping details as strings
The merge strategy at line 99 (
{**extras, **details}) correctly prioritizes explicitdetailsfields over top-level extras, ensuring structured context is preserved without loss.Edge cases are handled gracefully:
- None message triggers stringification of remaining fields (lines 78-79, 103-104)
- Non-mapping details with extras wraps both (line 101)
- String/primitive details fall through to string conversion (line 109)
Consider enhancing the docstring to document the three input formats and the merge precedence rules for better maintainability:
def _normalize_http_exception_detail( detail: Any, ) -> tuple[str, str, Any | None]: - """Extract a stable message/type/details triple from HTTPException detail.""" + """Extract a stable message/type/details triple from HTTPException detail. + + Handles three payload formats: + 1. Nested: {"error": {"message": str, "type": str, "details": dict}} + 2. Flat mapping: {"message": str, "type": str, "details": dict, ...extras} + - Extra fields are merged into details (details takes precedence) + 3. Fallback: Any non-mapping value is stringified + + Returns: + Tuple of (message, error_type, details_or_none) + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/app/error_handlers.py(4 hunks)tests/unit/core/app/test_error_handlers.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/app/error_handlers.pytests/unit/core/app/test_error_handlers.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/app/error_handlers.py
🧬 Code graph analysis (1)
tests/unit/core/app/test_error_handlers.py (1)
src/core/app/error_handlers.py (1)
http_exception_handler(112-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.10)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
tests/unit/core/app/test_error_handlers.py (2)
142-171: LGTM! Comprehensive test for detail normalization and merging.The test correctly validates that
http_exception_handlernormalizes structured detail payloads by:
- Extracting
messageandtypefrom the top level- Merging extra fields (like
hint) into thedetailsdict- Preserving the original
detailsfield contentsThe assertion correctly expects that
hintandfieldare both present in the mergeddetailspayload, withdetailsvalues taking precedence in case of conflicts.
193-220: LGTM! Well-structured test for nested error payloads.The test validates that the handler correctly normalizes nested error structures (where
detail.errorcontains the actual error fields) and preserves the structured information in the chat completion response format. The assertions properly verify both the error message in the chat content and the complete error payload structure.src/core/app/error_handlers.py (2)
5-5: LGTM! Appropriate import for type checking.The
Mappingimport fromcollections.abcis correctly placed in the standard library import section and is used appropriately for type checking in the normalization function.
130-168: LGTM! Clean refactor with consistent error payload construction.The handler correctly uses the normalization helper to derive
message,error_type, andextra_details, then applies them consistently for both chat and non-chat responses:
- Chat completions (lines 146, 153-154, 158-159): Uses normalized values in both the error message content and the structured error payload
- Standard responses (lines 162-168): Builds a unified error payload structure with conditional details inclusion
The refactor eliminates duplication and ensures structured detail information is preserved across both response formats.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/app/error_handlers.py (2)
64-118: Consider extracting duplicate merging logic.The function correctly handles three cases: nested error structures, generic mappings, and fallback. However, the merging logic (lines 79-85 and lines 102-109) is duplicated. While the duplication is minimal, extracting it to a helper function would improve maintainability.
Apply this diff to extract the merging logic:
+def _merge_details_with_extras( + details: Any | None, + extras: dict[str, Any], +) -> Any | None: + """Merge extras into details, handling various detail types.""" + if not extras: + return details + if details is None: + return extras + elif isinstance(details, Mapping): + return {**extras, **details} + else: + return {"value": details, "extras": extras} + + def _normalize_http_exception_detail( detail: Any, ) -> tuple[str, str, Any | None]: """Extract a stable message/type/details triple from HTTPException detail.""" default_type = "HttpError" if isinstance(detail, Mapping): # Handle FastAPI-style payloads where the detail already contains an # ``error`` structure with message/type/details fields. nested_error = detail.get("error") if isinstance(nested_error, Mapping): message = nested_error.get("message") error_type = nested_error.get("type", default_type) details = nested_error.get("details") extras = {k: v for k, v in detail.items() if k != "error"} - if extras: - if details is None: - details = extras - elif isinstance(details, Mapping): - details = {**extras, **details} - else: - details = {"value": details, "extras": extras} + details = _merge_details_with_extras(details, extras) if message is None: message = str({k: v for k, v in detail.items() if k != "error"}) return str(message), str(error_type), details # Generic mapping: look for common keys first. message = detail.get("message") error_type = detail.get("type", default_type) details = detail.get("details") # Preserve any remaining fields as part of the details payload so that # callers do not lose structured context. extras = { key: value for key, value in detail.items() if key not in {"message", "type", "details"} } - if extras: - if details is None: - details = extras - elif isinstance(details, Mapping): - # Merge extras with details when both are mapping-like. - details = {**extras, **details} - else: - details = {"value": details, "extras": extras} + details = _merge_details_with_extras(details, extras) if message is None: message = str({k: v for k, v in detail.items() if k != "details"}) return str(message), str(error_type), details # Fallback: treat the detail as a simple string payload. return str(detail), default_type, None
64-118: Consider enhancing the docstring.The one-line docstring is functional but could be expanded to document the three cases the function handles: nested error structures, generic mappings, and fallback for non-mapping details. This would improve code maintainability.
Apply this diff:
def _normalize_http_exception_detail( detail: Any, ) -> tuple[str, str, Any | None]: - """Extract a stable message/type/details triple from HTTPException detail.""" + """Extract a stable message/type/details triple from HTTPException detail. + + Handles three cases: + 1. FastAPI-style payloads with nested "error" structure + 2. Generic mappings with message/type/details at top level + 3. Fallback for non-mapping details (returns string representation) + + Args: + detail: The detail payload from HTTPException + + Returns: + Tuple of (message, type, details) where extras are merged into details + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/app/error_handlers.py(4 hunks)tests/unit/core/app/test_error_handlers.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/app/error_handlers.pytests/unit/core/app/test_error_handlers.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/app/error_handlers.py
🔇 Additional comments (5)
tests/unit/core/app/test_error_handlers.py (3)
142-171: LGTM! Comprehensive test coverage for mapping normalization.The test correctly verifies that when HTTPException detail is a mapping with message, type, details, and extra fields, they are properly normalized and merged. The assertion confirms that "hint" is merged into details alongside "field", and status_code is added.
193-220: LGTM! Validates chat completions with nested error structure.The test properly verifies that structured detail payloads with nested error objects are correctly extracted and normalized for chat completion endpoints. The assertions confirm that message content, error payload, and details are all preserved.
222-258: LGTM! Confirms outer metadata preservation.This test validates the fix for the past review comment, ensuring that outer metadata (trace_id, correlation) is preserved and merged into details alongside the nested error's details. The assertions confirm all fields are properly merged.
src/core/app/error_handlers.py (2)
5-5: LGTM! Appropriate import for type checking.The import of
Mappingfromcollections.abcis correct for usingisinstance(detail, Mapping)checks in the normalization logic.
138-188: LGTM! Normalization properly integrated.The handler correctly uses the normalized message, error_type, and extra_details for both chat completion and non-chat endpoints. The logic properly constructs error payloads with the normalized values, and conditionally includes details when present.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ec25e4aee08333a1a09066d47a3acf
Summary by CodeRabbit