improved get icd11 diagnoses functions #2566
Conversation
care/facility/api/viewsets/icd.py
Outdated
| except ValueError as err: | ||
| raise ValidationError(detail="ID must be an integer.") from err | ||
|
|
||
| diagnosis, error = get_icd11_diagnosis_object_by_id(pk, as_dict=True) |
There was a problem hiding this comment.
@sainak should we switch to raising exceptions instead of returning tuple containing error?
There was a problem hiding this comment.
I was facing problem with raising exceptions directly from the function
- unable to provide a detailed error in api responses.
- For above issue we can solve it by raising APIExceptions from the function but since that is also used at other non views functions, we shouldn't use APIExceptions in the function
There was a problem hiding this comment.
you'll have to extend the apiexception to give the specific method, that will also allow in better error handelling
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2566 +/- ##
===========================================
+ Coverage 69.55% 69.63% +0.07%
===========================================
Files 212 212
Lines 11966 12006 +40
Branches 1208 1207 -1
===========================================
+ Hits 8323 8360 +37
- Misses 3274 3277 +3
Partials 369 369 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request significantly enhance error handling and validation across multiple components of the application. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ICDViewSet
participant Database
participant Redis
Client->>ICDViewSet: Request diagnosis with ID
ICDViewSet->>ICDViewSet: Validate ID
alt ID is valid
ICDViewSet->>Redis: Check diagnosis in Redis
alt Diagnosis found
Redis-->>ICDViewSet: Return diagnosis
else Diagnosis not found
ICDViewSet->>Database: Check diagnosis in DB
alt Diagnosis found
Database-->>ICDViewSet: Return diagnosis
else Diagnosis not found
ICDViewSet-->>Client: 404 Not Found
end
end
else ID is invalid
ICDViewSet-->>Client: 400 Bad Request
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
care/utils/exceptions.py (3)
3-4: I see we're being quite generous with vertical spacing here...A single empty line between classes would suffice, but I suppose two empty lines make the code feel more... spacious? 🤔
class CeleryTaskError(Exception): pass - - class ICD11DiagnosisNotFoundError(Exception):
5-11: The implementation looks fine, though it could be more... efficient.While the implementation is correct, we could make it more concise by removing the redundant message assignment since Exception already handles this.
class ICD11DiagnosisNotFoundError(Exception): """Custom exception for ICD11 diagnosis not found.""" def __init__(self, message): - self.message = message - super().__init__(self.message) + super().__init__(message)
12-18: Oh look, it's the same code again...I see we're following the DRY principle... just kidding! 😉 This implementation is identical to the previous exception class. While that's not necessarily wrong, we could consider creating a base exception class if we plan to add more similar exceptions in the future.
Here's a more elegant approach:
+class BaseICD11Error(Exception): + """Base exception for ICD11-related errors.""" + def __init__(self, message): + super().__init__(message) + +class ICD11DiagnosisNotFoundError(BaseICD11Error): + """Custom exception for ICD11 diagnosis not found.""" + +class ICD11RedisConnectionError(BaseICD11Error): + """Custom exception for Redis connection issues."""care/facility/tests/test_icd11_api.py (2)
61-68: Consider adding a test case for negative IDs... if you feel like it.While the current test cases cover invalid string and zero IDs quite nicely, it might be worth adding a test for negative IDs. You know, just to be thorough... unless that's too much work.
# Additional test case suggestion res = self.client.get("/api/v1/icd/-1/") self.assertEqual(res.status_code, status.HTTP_404_NOT_FOUND) self.assertIn("Diagnosis with the specified ID not found.", res.json()["detail"])
95-101: The unexpected error test could use a bit more... personality.While testing for generic exceptions is fine, it might be more helpful to test specific unexpected scenarios. You know, just in case someone wants to actually debug this someday.
Consider adding specific test cases for common unexpected errors:
@patch("care.facility.static_data.icd11.ICD11.get") def test_retrieve_specific_unexpected_errors(self, mock_redis_get): # Test memory error mock_redis_get.side_effect = MemoryError("Out of memory") response = self.client.get("/api/v1/icd/123/") self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR) # Test timeout error mock_redis_get.side_effect = TimeoutError("Operation timed out") response = self.client.get("/api/v1/icd/123/") self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)config/settings/base.py (1)
357-360: Consider a more granular approach to log suppression.While disabling logs during tests is reasonable, completely disabling CRITICAL logs might hide important issues. Perhaps we could:
- Only disable specific loggers
- Or maintain CRITICAL logs while disabling others
- Or make it configurable via an environment variable
After all, some of us might want to know when things go terribly wrong during tests... just saying.
Consider this more granular approach:
-# Disable logs when running tests -if "test" in sys.argv: - logging.disable(logging.CRITICAL) +# Configure logging for test environment +if "test" in sys.argv: + LOGGING["root"]["level"] = env("TEST_LOG_LEVEL", default="CRITICAL") + # Optionally disable specific loggers while keeping CRITICAL logs enabled + for logger in LOGGING.get("loggers", {}): + LOGGING["loggers"][logger]["level"] = env("TEST_LOG_LEVEL", default="CRITICAL")care/facility/api/viewsets/icd.py (1)
29-33: Refactor exception handling by creating a custom exceptionManually setting
status_codeon anAPIExceptionfeels a bit clunky. Defining a custom exception class that inherits fromAPIExceptionand sets the desiredstatus_codewould make the code cleaner and more maintainable.Here's how you might implement it:
from rest_framework.exceptions import APIException class ICD11RedisConnectionException(APIException): status_code = 400 default_detail = "Redis connection error."Then, update your exception handling:
- except ICD11RedisConnectionError as e: - error_message = e.message - exception = APIException(error_message) - exception.status_code = 400 - raise exception from e + except ICD11RedisConnectionError as e: + raise ICD11RedisConnectionException(detail=e.message) from ecare/facility/static_data/icd11.py (3)
64-67: Consider enhancing the docstring to provide more detailed information.Adding parameter and return value descriptions can improve code readability and maintenance.
You might update the docstring as follows:
def get_icd11_diagnosis_object_by_id( diagnosis_id: int, as_dict=False ) -> ICD11 | ICD11Object | None: + """ + Retrieves ICD11 diagnosis by ID with Redis lookup and database fallback. + + Parameters: + diagnosis_id (int): The ID of the diagnosis to retrieve. + as_dict (bool): Whether to return the diagnosis as a dictionary representation. + + Returns: + ICD11 | ICD11Object | None: The diagnosis object or its dictionary representation. + + Raises: + ICD11DiagnosisNotFoundError: If the diagnosis with the specified ID is not found. + ICD11RedisConnectionError: If there is a Redis connection issue. + Exception: If an unexpected error occurs. + """
86-88: Consider logging the exception before raisingICD11DiagnosisNotFoundError.Adding a log entry can assist in debugging when a diagnosis is not found.
You might update the code as follows:
except ICD11Diagnosis.DoesNotExist as e: + logger.warning( + "Diagnosis with ID %s not found in the database.", + diagnosis_id, + ) error_message = "Diagnosis with the specified ID not found." raise ICD11DiagnosisNotFoundError(error_message) from e
90-93: Consider logging theRedisErrorbefore raisingICD11RedisConnectionError.This can help in diagnosing Redis connection issues.
You might update the code as follows:
except RedisError as e: + logger.error( + "Redis connection issue encountered.", + exc_info=True, + ) error_message = "Redis connection issue encountered." raise ICD11RedisConnectionError(error_message) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
care/facility/api/viewsets/icd.py(1 hunks)care/facility/static_data/icd11.py(2 hunks)care/facility/tests/test_icd11_api.py(2 hunks)care/utils/exceptions.py(1 hunks)config/settings/base.py(2 hunks)
🔇 Additional comments (4)
care/utils/exceptions.py (1)
5-18: The new exception classes align well with the PR objectives.
The addition of these custom exceptions improves error handling specificity for ICD11-related operations, which was a key objective of this PR. The docstrings are clear and the inheritance hierarchy is appropriate.
Let's verify these exceptions are being used consistently:
✅ Verification successful
The exceptions are used appropriately, though they could use a bit more coverage
The exceptions are properly imported and handled in the ICD11-related operations, specifically in:
care/facility/static_data/icd11.py: Raises both exceptions with proper error propagationcare/facility/api/viewsets/icd.py: Catches and transforms them into appropriate API responses
The error handling follows a clean pattern where:
ICD11DiagnosisNotFoundErrormaps to HTTP 404 (NotFound)ICD11RedisConnectionErrormaps to a generic APIException
Though it would have been nice to see these exceptions used in more places, the current implementation is technically sound and follows good practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new exception classes
# Expected: These exceptions should be used in ICD11-related operations
# Search for exception usage
echo "Checking exception usage patterns..."
rg -A 2 "ICD11DiagnosisNotFoundError|ICD11RedisConnectionError"
# Check for any other ICD11-related error handling we might have missed
echo "Checking for other potential ICD11 error handling..."
rg -i "icd.*error|icd.*exception"
Length of output: 2814
care/facility/tests/test_icd11_api.py (2)
1-4: LGTM! The imports are properly organized.
The new Redis-related exception imports align perfectly with the PR's objective of improving error handling.
70-94: Excellent error scenario coverage!
The test cases for Redis/DB not found and connection errors are well-structured and comprehensive. The use of mocking is particularly elegant.
config/settings/base.py (1)
6-6: LGTM: Clean import addition.
The sys module import is properly placed with other standard library imports.
|
@sainak can you please review it |
|
Proposed Changes
get_icd11_diagnosis_object_by_idfunction for better errror handling.Associated Issue
Merge Checklist
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation
Chores