fix asset class initialization #2611
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce several enhancements to the asset management API, specifically in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Qodana for Python1076 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at qodana-support@jetbrains.com
|
![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
🧹 Outside diff range and nitpick comments (5)
care/facility/tasks/asset_monitor.py (2)
Line range hint
73-82: That TODO comment is giving me anxietyThere's a TODO comment indicating temporary code for asset migration. It would be wonderful if we could get a timeline for when this block will be removed, or perhaps create a tracking issue for it?
Would you like me to create a GitHub issue to track the removal of this legacy code block? Just say the word! 😊
Line range hint
67-106: This error handling structure is... interestingThe nested try-except blocks with different error handling strategies for ONVIF vs other asset types make the code a bit hard to follow. Consider extracting the ONVIF-specific logic into a separate method for better maintainability.
Here's a suggestion to improve the structure:
+ def _handle_onvif_status(asset_class, asset): + try: + # Legacy middleware check + asset_config = asset.meta["camera_access_key"].split(":") + assets_config = [{ + "hostname": asset.meta.get("local_ip_address"), + "port": 80, + "username": asset_config[0], + "password": asset_config[1], + }] + return asset_class.api_post( + asset_class.get_url("cameras/status"), + data=assets_config + ) + except Exception: + return asset_class.api_get( + asset_class.get_url("cameras/status") + ) try: # Creating an instance of the asset class asset_class: BaseAssetIntegration = AssetClasses[ asset.asset_class ].value({ **asset.meta, "id": str(asset.external_id), "middleware_hostname": resolved_middleware, }) # Fetching the status of the device if asset.asset_class == "ONVIF": - try: - # TODO: Remove this block after all assets are migrated to the new middleware - asset_config = asset.meta["camera_access_key"].split(":") - assets_config = [{ - "hostname": asset.meta.get("local_ip_address"), - "port": 80, - "username": asset_config[0], - "password": asset_config[1], - }] - result = asset_class.api_post( - asset_class.get_url("cameras/status"), - data=assets_config - ) - except Exception: - result = asset_class.api_get( - asset_class.get_url("cameras/status") - ) + result = _handle_onvif_status(asset_class, asset) else: result = asset_class.api_get(asset_class.get_url("devices/status"))care/facility/tests/test_asset_api.py (1)
43-54: The test looks good, but could be more comprehensive... just saying.While the test correctly verifies ONVIF asset initialization, it might be nice to consider adding:
- Tests for other asset classes (HL7MONITOR, VENTILATOR)
- Invalid initialization cases
- A docstring explaining the test's purpose
Here's a suggested enhancement:
def test_asset_class_initialization(self): + """ + Verify that assets are correctly initialized with their respective classes + and properly handle string conversion of external_id. + """ + # Test ONVIF asset initialization asset = self.create_asset( self.asset_location, asset_class=AssetClasses.ONVIF.name ) asset_class = AssetClasses[asset.asset_class].value( { **asset.meta, "id": str(asset.external_id), "middleware_hostname": "middleware.local", } ) self.assertIsInstance(asset_class, BaseAssetIntegration) + + # Test HL7MONITOR asset initialization + hl7_asset = self.create_asset( + self.asset_location, asset_class=AssetClasses.HL7MONITOR.name + ) + hl7_class = AssetClasses[hl7_asset.asset_class].value( + { + **hl7_asset.meta, + "id": str(hl7_asset.external_id), + "middleware_hostname": "middleware.local", + } + ) + self.assertIsInstance(hl7_class, BaseAssetIntegration) + + # Test invalid initialization + with self.assertRaises(ValueError): + AssetClasses[asset.asset_class].value({})care/facility/api/viewsets/asset.py (2)
Line range hint
398-416: Avoid Catching the BroadExceptionIn the
operate_assetsmethod, the finalexcept Exception as e:block is catching all exceptions, which can mask unexpected errors and make debugging more challenging.Consider catching specific exceptions or re-raising the exception after logging. This makes error handling more precise and avoids swallowing unforeseen issues. Here's a suggested change:
except APIException as e: return Response( { "detail": f"Communication with the middleware failed.\nReceived status code: {e.status_code}" }, status=status.HTTP_502_BAD_GATEWAY, ) - except Exception as e: + except SomeSpecificException as e: logger.info("Failed to operate asset: %s", e) return Response( {"message": "Internal Server Error"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) + except Exception as e: + logger.exception("An unexpected error occurred") + raise eReplace
SomeSpecificExceptionwith the specific exceptions you anticipate. Re-raising the general exception ensures that truly unexpected errors are not silently handled.
Line range hint
398-416: Standardize Error Responses for ConsistencyThe error responses in the
operate_assetsmethod have varying structures and keys ("detail"vs."message"). Consistent error response formats enhance the API's usability and make client-side error handling more straightforward.Consider unifying the error response format. For example, always use the
"detail"key for error messages:except ValidationError as e: return Response({"detail": e.detail}, status=status.HTTP_400_BAD_REQUEST) except KeyError as e: - return Response( - {"message": {key: "is required" for key in e.args}}, + return Response( + {"detail": {key: "is required" for key in e.args}}, status=status.HTTP_400_BAD_REQUEST, ) except APIException as e: return Response( { "detail": f"Communication with the middleware failed.\nReceived status code: {e.status_code}" }, status=status.HTTP_502_BAD_GATEWAY, )This adjustment promotes consistency across your API responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
care/facility/api/viewsets/asset.py(1 hunks)care/facility/tasks/asset_monitor.py(1 hunks)care/facility/tests/test_asset_api.py(2 hunks)
🔇 Additional comments (4)
care/facility/tasks/asset_monitor.py (2)
68-68: The string conversion looks fine, I suppose...
Converting external_id to string is the right approach here, as it ensures consistent type handling when passed to the validator. This aligns nicely with the changes in the asset API.
Line range hint 1-150: Verify the impact of string conversion
While the string conversion is correct, let's verify that all code paths handling the external_id are prepared for string values.
✅ Verification successful
String conversion of external_id is handled consistently
After reviewing the codebase, I can confirm that external_id is consistently handled as a string across all code paths. The field is defined as a UUIDField in models and is properly converted to string in all relevant places:
- Models define it as
UUIDField(default=uuid4, unique=True, db_index=True) - API serializers use
UUIDField(source="external_id") - All comparisons and assignments properly handle string conversion
- File paths and cache keys correctly use string conversion with
str()
I suppose we could have saved time by trusting the original implementation, but better safe than sorry, right?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of external_id in the codebase to verify type handling
rg -A 2 "external_id" --type python
# Search for any type hints or assertions related to external_id
ast-grep --pattern 'external_id: $_'
Length of output: 105
Script:
#!/bin/bash
# Let's try searching without file type restriction since it wasn't recognized
rg -A 2 "external_id"
# Also search for model definition to understand the field type
ast-grep --pattern 'class Asset$_:
$$$
'
# And search for any direct assignments or comparisons
rg "external_id =" -A 2
rg "external_id ==" -A 2
Length of output: 106877
care/facility/tests/test_asset_api.py (1)
9-9: LGTM!
The import statement is properly placed with other related imports.
care/facility/api/viewsets/asset.py (1)
404-404: Finally Converting id to String Properly
It's good to see that asset.external_id is now explicitly converted to a string using str(asset.external_id). This ensures consistency when passing the id to the asset class and prevents potential type errors.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2611 +/- ##
===========================================
+ Coverage 69.53% 69.55% +0.02%
===========================================
Files 212 212
Lines 11966 11966
Branches 1208 1208
===========================================
+ Hits 8320 8323 +3
+ Misses 3277 3274 -3
Partials 369 369 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Proposed Changes
Merge Checklist
/docsOnly 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
New Features
middleware_hostnameto ensure proper formatting.Bug Fixes
Tests