Feat/fastapi loggin route error detail#179
Conversation
WalkthroughReplaced Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (1)
agave/fastapi/middlewares/loggin_route.py (1)
31-40: Consider consolidating withget_status_code_exceptionto reduce duplication.The new
get_error_detailfunction mirrors the structure ofget_status_code_exception(lines 19-28), checking the same exception types in the same order. While each function has a clear single responsibility, consider refactoring to reduce the duplicated conditional logic.For example, you could create a generic helper that takes a mapping of exception types to attribute extractors, or combine both functions into a single one that returns a tuple.
Current implementation is correct and functional, so this refactor can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agave/fastapi/middlewares/loggin_route.py(2 hunks)agave/version.py(1 hunks)tests/fastapi/test_loggin_route.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format code with Black and sort imports with isort (make format)
Pass linting with flake8 and formatting checks (isort --check-only, black --check)
Maintain type correctness with mypy (make lint runs mypy)
Files:
tests/fastapi/test_loggin_route.pyagave/version.pyagave/fastapi/middlewares/loggin_route.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
tests/fastapi/test_loggin_route.pyagave/version.pyagave/fastapi/middlewares/loggin_route.py
agave/fastapi/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
agave/fastapi/**/*.py: Implement FastAPI-specific functionality under agave/fastapi/
In FastAPI modules, use async/await and mongoengine_plus.aio.AsyncDocument for MongoDB models
Files:
agave/fastapi/middlewares/loggin_route.py
agave/{fastapi,chalice}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
agave/{fastapi,chalice}/**/*.py: Define REST resources as classes with model, validation, and CRUD methods; apply @app.resource('/path') to generate endpoints
Use Pydantic models for request/response validation with automatic error handling
Files:
agave/fastapi/middlewares/loggin_route.py
🧬 Code graph analysis (1)
agave/fastapi/middlewares/loggin_route.py (1)
agave/core/exc.py (1)
AgaveError(6-8)
🔇 Additional comments (3)
agave/version.py (1)
1-1: LGTM! Version bump is appropriate.The version bump from 1.5.1 to 1.5.2 correctly reflects the addition of new error detail logging functionality in this release.
agave/fastapi/middlewares/loggin_route.py (1)
81-82: LGTM! Error detail logging correctly implemented.The exception handler now properly captures both status code and error detail, providing more comprehensive logging for debugging.
tests/fastapi/test_loggin_route.py (1)
100-103: LGTM! Test assertions properly validate error detail logging.The new assertions comprehensively verify that error details are correctly captured and logged for various exception types:
- ValidationError with detailed field information
- HTTPException messages (Not found, Bad request, Unauthorized)
- Generic Exception messages (Internal server error)
This ensures the new
get_error_detailfunction works correctly across all supported exception types.Also applies to: 125-125, 286-286, 302-303, 319-320
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 19
Lines 710 711 +1
Branches 74 74
=========================================
+ Hits 710 711 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
agave/fastapi/middlewares/loggin_route.py (1)
23-23: Usejson.dumpsfor cleaner validation error logging.Currently,
str(exc.errors())produces Python's repr-like output[{'loc': ..., 'msg': ...}]which is harder to read in logs. Since this string gets stored inlog_data['response']['error']and the entire structure is JSON-serialized at line 99, usingjson.dumps(exc.errors())will produce properly formatted JSON.Apply this diff:
- return 422, str(exc.errors()) + return 422, json.dumps(exc.errors())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agave/fastapi/middlewares/loggin_route.py(2 hunks)agave/version.py(1 hunks)tests/fastapi/test_loggin_route.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- agave/version.py
- tests/fastapi/test_loggin_route.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Format code with Black and sort imports with isort (make format)
Pass linting with flake8 and formatting checks (isort --check-only, black --check)
Maintain type correctness with mypy (make lint runs mypy)
Files:
agave/fastapi/middlewares/loggin_route.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
agave/fastapi/middlewares/loggin_route.py
agave/fastapi/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
agave/fastapi/**/*.py: Implement FastAPI-specific functionality under agave/fastapi/
In FastAPI modules, use async/await and mongoengine_plus.aio.AsyncDocument for MongoDB models
Files:
agave/fastapi/middlewares/loggin_route.py
agave/{fastapi,chalice}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
agave/{fastapi,chalice}/**/*.py: Define REST resources as classes with model, validation, and CRUD methods; apply @app.resource('/path') to generate endpoints
Use Pydantic models for request/response validation with automatic error handling
Files:
agave/fastapi/middlewares/loggin_route.py
🧬 Code graph analysis (1)
agave/fastapi/middlewares/loggin_route.py (2)
tests/utils.py (1)
status_code(32-33)agave/core/exc.py (1)
AgaveError(6-8)
⏰ 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). (6)
- GitHub Check: pytest (3.10)
- GitHub Check: pytest (3.13)
- GitHub Check: pytest (3.11)
- GitHub Check: pytest (3.12)
- GitHub Check: coverage
- GitHub Check: pytest (3.9)
🔇 Additional comments (2)
agave/fastapi/middlewares/loggin_route.py (2)
68-74: Excellent implementation addressing past feedback.The changes correctly unpack both status code and error detail, and log them in the response. This successfully addresses felipao-mx's suggestion to unify the functions and return a tuple.
27-27: Verify thatCuencaErrorfromcuenca_validations.errorshas astatus_codeattribute.The code at line 27 accesses
exc.status_codeonCuencaErrorinstances. While web searches confirmed that cuenca_validations.errors.CuencaError is the package's base exception class for validation-related errors, the search results did not explicitly confirm that this class exposes astatus_codeattribute. Please verify against the cuenca-validations package documentation or source code that this attribute exists before merging.
2 participants
Summary by CodeRabbit
New Features
Tests
Chores