Implement minimums for WebSocket API #426
Conversation
|
@coderabbitai full review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #426 +/- ##
==========================================
- Coverage 77.37% 67.73% -9.64%
==========================================
Files 11 12 +1
Lines 579 716 +137
Branches 45 66 +21
==========================================
+ Hits 448 485 +37
- Misses 131 231 +100 ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request introduces significant updates across three files in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Websocket
participant Server
Client->>Websocket: connect()
Websocket->>Server: Establish WebSocket connection
Server-->>Websocket: Connection established
Websocket-->>Client: Connection successful
Client->>Websocket: subscribe("DEVICE")
Websocket->>Server: Subscribe to DEVICE topic
Server-->>Websocket: Subscription confirmed
Websocket->>Server: Listen for messages
Server-->>Websocket: Send message
Websocket-->>Client: Processed message
🪧 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: 6
🧹 Outside diff range and nitpick comments (8)
homewizard_energy/v2/websocket.py (5)
136-136: ReplaceUsing
_LOGGERto log debug information. This allows users of the library to configure logging behavior as needed.Apply this diff to improve logging:
- print(self._ws_subscriptions) + _LOGGER.debug("Current subscriptions: %s", self._ws_subscriptions)
109-109: Simplify logging of exceptions.When using
_LOGGER.exception, the exception information is automatically included in the log output. Including%sand the exception variable is redundant. You can simplify the log statement.Apply this diff to simplify the logging:
- _LOGGER.exception("Websocket disconnect error: %s", e) + _LOGGER.exception("Websocket disconnect error")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 107-109: homewizard_energy/v2/websocket.py#L107-L109
Added lines #L107 - L109 were not covered by tests
148-151: Avoid reassigning themsgvariable after parsing JSON.Reassigning
msgtomsg.json()can be confusing and may lead to errors if not managed carefully. It's clearer to assign the parsed JSON to a new variable, such asmsg_json.Apply this diff to improve code clarity:
try: - msg = msg.json() + msg_json = msg.json() except ValueError as ex: - raise ValueError(f"Invalid JSON received: {msg.data}") from ex + raise ValueError(f"Invalid JSON received: {msg.data}") from ex - if "type" not in msg: - raise ValueError(f"Missing 'type' in message: {msg}") + if "type" not in msg_json: + raise ValueError(f"Missing 'type' in message: {msg_json}") - msg_type = msg.get("type") - msg_data = msg.get("data") + msg_type = msg_json.get("type") + msg_data = msg_json.get("data")
98-109: Handle exceptions more granularly within the WebSocket loop.Currently, the entire WebSocket loop is wrapped in a try-except block catching
aiohttp.ClientError. This might mask other exceptions that could occur within the loop. Consider refining exception handling to catch specific exceptions where they might occur and allow unexpected exceptions to propagate, or handle them appropriately.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-99: homewizard_energy/v2/websocket.py#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: homewizard_energy/v2/websocket.py#L102
Added line #L102 was not covered by tests
[warning] 105-105: homewizard_energy/v2/websocket.py#L105
Added line #L105 was not covered by tests
[warning] 107-109: homewizard_energy/v2/websocket.py#L107-L109
Added lines #L107 - L109 were not covered by tests
178-180: Adjust error handling for unknown message types.Raising a
ValueErrorwhen an unknown message type is received may not be the best approach, as it will terminate the WebSocket loop. Instead, consider logging a warning and continuing to listen for messages.Apply this diff to handle unknown message types gracefully:
if parsed_data is None: - raise ValueError(f"Unknown message type: {msg_type}") + _LOGGER.warning("Unknown message type received: %s", msg_type) + return True # Continue processing messageshomewizard_energy/v2/__init__.py (3)
188-197: Add unit tests for thetokenpropertyThe
tokenproperty provides access to the_tokenattribute. Adding unit tests for this property will help ensure it returns the correct values under various scenarios.Would you like assistance in writing unit tests for the
tokenproperty?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 196-196: homewizard_energy/v2/init.py#L196
Added line #L196 was not covered by tests
198-207: Add unit tests for therequest_timeoutpropertyThe
request_timeoutproperty exposes the_request_timeoutattribute. To prevent potential issues, please consider adding unit tests to verify its behavior.Would you like assistance in creating unit tests for the
request_timeoutproperty?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 206-206: homewizard_energy/v2/init.py#L206
Added line #L206 was not covered by tests
282-282: Simplify exception handling by removing redundant exception
aiohttp.ClientResponseErroris a subclass ofaiohttp.ClientError, making it redundant to include both in the exception tuple. Simplify the exception handling by only catchingaiohttp.ClientError.Apply this diff to simplify the code:
- except (aiohttp.ClientError, aiohttp.ClientResponseError) as exception: + except aiohttp.ClientError as exception:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 282-282: homewizard_energy/v2/init.py#L282
Added line #L282 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
homewizard_energy/v2/__init__.py(13 hunks)homewizard_energy/v2/const.py(1 hunks)homewizard_energy/v2/websocket.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
homewizard_energy/v2/__init__.py
[warning] 94-95: homewizard_energy/v2/init.py#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 141-141: homewizard_energy/v2/init.py#L141
Added line #L141 was not covered by tests
[warning] 196-196: homewizard_energy/v2/init.py#L196
Added line #L196 was not covered by tests
[warning] 206-206: homewizard_energy/v2/init.py#L206
Added line #L206 was not covered by tests
[warning] 214-214: homewizard_energy/v2/init.py#L214
Added line #L214 was not covered by tests
[warning] 230-230: homewizard_energy/v2/init.py#L230
Added line #L230 was not covered by tests
[warning] 236-236: homewizard_energy/v2/init.py#L236
Added line #L236 was not covered by tests
[warning] 241-241: homewizard_energy/v2/init.py#L241
Added line #L241 was not covered by tests
[warning] 249-249: homewizard_energy/v2/init.py#L249
Added line #L249 was not covered by tests
[warning] 268-268: homewizard_energy/v2/init.py#L268
Added line #L268 was not covered by tests
[warning] 282-282: homewizard_energy/v2/init.py#L282
Added line #L282 was not covered by tests
[warning] 309-309: homewizard_energy/v2/init.py#L309
Added line #L309 was not covered by tests
homewizard_energy/v2/websocket.py
[warning] 32-32: homewizard_energy/v2/websocket.py#L32
Added line #L32 was not covered by tests
[warning] 38-43: homewizard_energy/v2/websocket.py#L38-L43
Added lines #L38 - L43 were not covered by tests
[warning] 45-47: homewizard_energy/v2/websocket.py#L45-L47
Added lines #L45 - L47 were not covered by tests
[warning] 49-50: homewizard_energy/v2/websocket.py#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-55: homewizard_energy/v2/websocket.py#L53-L55
Added lines #L53 - L55 were not covered by tests
[warning] 57-57: homewizard_energy/v2/websocket.py#L57
Added line #L57 was not covered by tests
[warning] 59-62: homewizard_energy/v2/websocket.py#L59-L62
Added lines #L59 - L62 were not covered by tests
[warning] 67-68: homewizard_energy/v2/websocket.py#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 79-80: homewizard_energy/v2/websocket.py#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 82-83: homewizard_energy/v2/websocket.py#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-86: homewizard_energy/v2/websocket.py#L86
Added line #L86 was not covered by tests
[warning] 90-90: homewizard_energy/v2/websocket.py#L90
Added line #L90 was not covered by tests
[warning] 93-93: homewizard_energy/v2/websocket.py#L93
Added line #L93 was not covered by tests
[warning] 95-95: homewizard_energy/v2/websocket.py#L95
Added line #L95 was not covered by tests
[warning] 98-99: homewizard_energy/v2/websocket.py#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: homewizard_energy/v2/websocket.py#L102
Added line #L102 was not covered by tests
[warning] 105-105: homewizard_energy/v2/websocket.py#L105
Added line #L105 was not covered by tests
[warning] 107-109: homewizard_energy/v2/websocket.py#L107-L109
Added lines #L107 - L109 were not covered by tests
🔇 Additional comments (4)
homewizard_energy/v2/const.py (2)
11-16: Clean implementation of WebSocket topics
The WebsocketTopic enum is well-structured and uses appropriate Python typing with StrEnum. The topics are logically separated and follow a consistent naming convention.
Let's verify if these topics align with any API documentation:
#!/bin/bash
# Description: Look for WebSocket topic documentation or examples
# Search for WebSocket documentation files
fd -e md -e rst | xargs rg -l "websocket.*topic|topic.*websocket"
# Search for any existing WebSocket-related code that might need updates
rg -l "websocket" --type py5-5: Verify API version compatibility
The API version has been bumped from "v1" to "2.0.0", which indicates a major version change. This could potentially break existing clients.
Let's verify the API version usage and documentation:
✅ Verification successful
API version change is properly structured and safe
The version change from "v1" to "2.0.0" is well-implemented with proper version isolation:
- Both v1 and v2 APIs coexist in separate directories with their own constants and models
- v1 API continues to validate against "v1" version
- v2 API introduces the new "2.0.0" version
- Version validation is properly implemented with clear error handling
- Test fixtures and snapshots maintain v1 compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for API version references and verify documentation updates
# Check for any hardcoded references to the old API version
rg -l "v1" --type py
# Check if README or documentation files exist and contain version information
fd -e md -e rst | xargs rg -l "api.*version|version.*api"
Length of output: 256
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# Check for v1/v2 directory structure and files
fd -t d "v1|v2"
# Search for version-related imports and usage
rg -l "from.*v1|import.*v1" --type py
# Look for version strings in Python files
rg "\"v1\"|'v1'|\"1\.0|'1\.0|\"2\.0|'2\.0" --type py
# Check for API version references in all text files
rg "api.*version|version.*api" -g "!*.pyc" -g "!*.py[cod]" -g "!*.so" -g "!*.o"
# Look for migration guides or changelogs
fd -e md -e rst | xargs rg -l "changelog|migration|breaking.*changes|upgrade.*guide"
Length of output: 6555
Script:
#!/bin/bash
# Let's check the implementation details of version handling
# Check version validation logic
rg -A 5 "SUPPORTED_API_VERSION" --type py
# Check for version-related error handling
rg "UnsupportedApiVersion|api.*version.*error|version.*not.*support" --type py
# Look for version compatibility checks
ast-grep --pattern 'if $_.api_version != $_: $$$'
Length of output: 1908
homewizard_energy/v2/websocket.py (1)
160-177: Ensure compatibility with supported Python versions.
The use of structural pattern matching (match statement) and union types with | (e.g., Device | Measurement | System) requires Python 3.10 or newer. If the library needs to support earlier Python versions, these features will cause syntax errors.
Are we targeting Python 3.10 and above? If so, this is fine. If support for earlier versions is needed, consider refactoring:
- Replace
matchstatements withif-elifchains. - Use
Unionfrom thetypingmodule instead of the|operator.
homewizard_energy/v2/__init__.py (1)
230-230: 🛠️ Refactor suggestion
Avoid deprecated parameter enable_cleanup_closed in aiohttp.TCPConnector
The enable_cleanup_closed parameter in aiohttp.TCPConnector is deprecated and will be removed in future versions of aiohttp. To maintain future compatibility, please remove this parameter.
Apply this diff to resolve the issue:
- enable_cleanup_closed=True,
ssl=context,
limit_per_host=1,Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 230-230: homewizard_energy/v2/init.py#L230
Added line #L230 was not covered by tests
| """ | ||
|
|
||
| def _unsub_ws_callback() -> None: | ||
| self._ws_subscriptions.remove({topic, ws_callback}) |
There was a problem hiding this comment.
Fix incorrect removal of subscription from the list.
In the _unsub_ws_callback function, you're using a set {topic, ws_callback} instead of a tuple (topic, ws_callback). Since _ws_subscriptions is a list of tuples, attempting to remove a set will result in a ValueError. Replace the set with a tuple to correctly remove the subscription.
Apply this diff to fix the issue:
def _unsub_ws_callback() -> None:
- self._ws_subscriptions.remove({topic, ws_callback})
+ self._ws_subscriptions.remove((topic, ws_callback))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._ws_subscriptions.remove({topic, ws_callback}) | |
| self._ws_subscriptions.remove((topic, ws_callback)) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-80: homewizard_energy/v2/websocket.py#L79-L80
Added lines #L79 - L80 were not covered by tests
| _connect_lock: asyncio.Lock = asyncio.Lock() | ||
| _ws_connection: aiohttp.ClientWebSocketResponse | None = None | ||
| _ws_subscriptions: list[tuple[str, OnMessageCallbackType]] = [] | ||
|
|
||
| _ws_authenticated: bool = False | ||
|
|
There was a problem hiding this comment.
Use instance variables instead of class variables for connection state.
The variables _connect_lock, _ws_connection, _ws_subscriptions, and _ws_authenticated are declared as class variables. This means they are shared across all instances of the Websocket class, which could lead to unintended behavior when multiple instances are created. It's better to define these as instance variables within the __init__ method to ensure that each instance maintains its own state.
Apply this diff to fix the issue:
-class Websocket:
- """Websocket client for HomeWizard Energy API."""
-
- _connect_lock: asyncio.Lock = asyncio.Lock()
- _ws_connection: aiohttp.ClientWebSocketResponse | None = None
- _ws_subscriptions: list[tuple[str, OnMessageCallbackType]] = []
- _ws_authenticated: bool = False
-
- def __init__(self, parent: "HomeWizardEnergyV2"):
+class Websocket:
+ """Websocket client for HomeWizard Energy API."""
+
+ def __init__(self, parent: "HomeWizardEnergyV2"):
+ self._connect_lock: asyncio.Lock = asyncio.Lock()
+ self._ws_connection: aiohttp.ClientWebSocketResponse | None = None
+ self._ws_subscriptions: list[tuple[str, OnMessageCallbackType]] = []
+ self._ws_authenticated: bool = False
self._parent = parent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _connect_lock: asyncio.Lock = asyncio.Lock() | |
| _ws_connection: aiohttp.ClientWebSocketResponse | None = None | |
| _ws_subscriptions: list[tuple[str, OnMessageCallbackType]] = [] | |
| _ws_authenticated: bool = False | |
| class Websocket: | |
| """Websocket client for HomeWizard Energy API.""" | |
| def __init__(self, parent: "HomeWizardEnergyV2"): | |
| self._connect_lock: asyncio.Lock = asyncio.Lock() | |
| self._ws_connection: aiohttp.ClientWebSocketResponse | None = None | |
| self._ws_subscriptions: list[tuple[str, OnMessageCallbackType]] = [] | |
| self._ws_authenticated: bool = False | |
| self._parent = parent |
| def websocket(self) -> Websocket: | ||
| """Return the websocket object. | ||
|
|
||
| Create a new websocket object if it does not exist. | ||
| """ | ||
| if self._websocket is None: | ||
| self._websocket = Websocket(self) | ||
| return self._websocket |
There was a problem hiding this comment.
Add unit tests for the websocket property initialization
The new websocket property introduces lazy initialization of the _websocket instance. Currently, there are no unit tests covering this property, which could lead to undetected bugs or regressions in the future. Please consider adding unit tests to ensure proper functionality.
Would you like assistance in creating unit tests for this property?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 94-95: homewizard_energy/v2/init.py#L94-L95
Added lines #L94 - L95 were not covered by tests
| @@ -133,7 +138,7 @@ | |||
| self, | |||
| ) -> bool: | |||
| """Send identify request.""" | |||
| await self._request("/api/system/identify", method=METH_PUT) | |||
| await self._request("/api/system/identify", method=aiohttp.hdrs.METH_PUT) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for the identify method
The identify method sends a request to "/api/system/identify" using the PUT method. To ensure it behaves as expected and handles possible exceptions, please consider adding unit tests for this method.
Would you like assistance in creating unit tests for the identify method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-141: homewizard_energy/v2/init.py#L141
Added line #L141 was not covered by tests
| if self._clientsession is not None: | ||
| return self._clientsession |
There was a problem hiding this comment.
Ensure thread safety in lazy initialization
The lazy initialization of _clientsession in get_clientsession and _websocket in the websocket property could lead to race conditions if accessed concurrently by multiple coroutines. To prevent multiple initializations and ensure thread safety, consider implementing an asyncio.Lock to synchronize access.
Apply this diff to address the issue:
+ import asyncio
+ _clientsession_lock = asyncio.Lock()
+ _websocket_lock = asyncio.Lock()
async def get_clientsession(self) -> aiohttp.ClientSession:
"""
Get a clientsession that is tuned for communication with the HomeWizard Energy Device
"""
+ async with self._clientsession_lock:
if self._clientsession is not None:
return self._clientsession
# proceed to initialize the clientsession
@property
def websocket(self) -> Websocket:
"""Return the websocket object.
Create a new websocket object if it does not exist.
"""
+ async def init_websocket():
+ async with self._websocket_lock:
+ if self._websocket is None:
+ self._websocket = Websocket(self)
+ return self._websocket
+ loop = asyncio.get_event_loop()
+ return loop.run_until_complete(init_websocket())Note: Since properties cannot be asynchronous, the workaround involves running an asynchronous method in the event loop. Alternatively, document that the class is not thread-safe and should be used accordingly.
Also applies to: 93-95
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 214-214: homewizard_energy/v2/init.py#L214
Added line #L214 was not covered by tests
| @@ -276,6 +306,7 @@ | |||
| _LOGGER.debug("Closing clientsession") | |||
| if self._clientsession is not None: | |||
| await self._clientsession.close() | |||
| self._clientsession = None | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for the close method
The close method is responsible for properly closing the _clientsession and setting it to None. To ensure resources are managed correctly and prevent potential memory leaks, consider adding unit tests for this method.
Would you like assistance in creating unit tests for the close method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 309-309: homewizard_energy/v2/init.py#L309
Added line #L309 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
homewizard_energy/v2/const.py (1)
11-16: Enhance WebsocketTopic documentationThe enum structure using StrEnum is appropriate for string-based topics. However, the documentation could be more detailed.
Consider enhancing the docstring to describe each topic's purpose:
class WebsocketTopic(StrEnum): - """Websocket topics.""" + """Websocket subscription topics for the HomeWizard Energy API. + + Topics: + DEVICE: Updates related to device state and configuration + MEASUREMENT: Real-time energy measurement updates + SYSTEM: System-level notifications and updates + """homewizard_energy/v2/websocket.py (2)
149-149: Avoid overwriting themsgvariableOverwriting the
msgvariable, which initially holds anaiohttp.WSMessageobject, can be confusing and may lead to errors. Assign the parsed JSON to a new variable instead.Apply this diff:
- msg = msg.json() + msg_json = msg.json()Update subsequent references to use
msg_json.
34-63: Increase test coverage for theconnectmethodThe
connectmethod is currently not covered by tests, as indicated by the code coverage report. Consider adding unit tests to cover various scenarios, including successful connections, timeouts, and error handling.Would you like assistance in generating unit tests for the
connectmethod?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-43: homewizard_energy/v2/websocket.py#L38-L43
Added lines #L38 - L43 were not covered by tests
[warning] 45-47: homewizard_energy/v2/websocket.py#L45-L47
Added lines #L45 - L47 were not covered by tests
[warning] 49-50: homewizard_energy/v2/websocket.py#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-55: homewizard_energy/v2/websocket.py#L53-L55
Added lines #L53 - L55 were not covered by tests
[warning] 57-57: homewizard_energy/v2/websocket.py#L57
Added line #L57 was not covered by tests
[warning] 59-62: homewizard_energy/v2/websocket.py#L59-L62
Added lines #L59 - L62 were not covered by testshomewizard_energy/v2/__init__.py (1)
282-282: Simplify exception handling by removing redundant exceptionThe
aiohttp.ClientResponseErroris a subclass ofaiohttp.ClientError, so including both in the exception handler is redundant. You can simplify the code by only catchingaiohttp.ClientError.Apply this diff to simplify the exception handling:
- except (aiohttp.ClientError, aiohttp.ClientResponseError) as exception: + except aiohttp.ClientError as exception:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 282-282: homewizard_energy/v2/init.py#L282
Added line #L282 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
homewizard_energy/v2/__init__.py(13 hunks)homewizard_energy/v2/const.py(1 hunks)homewizard_energy/v2/websocket.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
homewizard_energy/v2/__init__.py
[warning] 94-95: homewizard_energy/v2/init.py#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 141-141: homewizard_energy/v2/init.py#L141
Added line #L141 was not covered by tests
[warning] 196-196: homewizard_energy/v2/init.py#L196
Added line #L196 was not covered by tests
[warning] 206-206: homewizard_energy/v2/init.py#L206
Added line #L206 was not covered by tests
[warning] 214-214: homewizard_energy/v2/init.py#L214
Added line #L214 was not covered by tests
[warning] 230-230: homewizard_energy/v2/init.py#L230
Added line #L230 was not covered by tests
[warning] 236-236: homewizard_energy/v2/init.py#L236
Added line #L236 was not covered by tests
[warning] 241-241: homewizard_energy/v2/init.py#L241
Added line #L241 was not covered by tests
[warning] 249-249: homewizard_energy/v2/init.py#L249
Added line #L249 was not covered by tests
[warning] 268-268: homewizard_energy/v2/init.py#L268
Added line #L268 was not covered by tests
[warning] 282-282: homewizard_energy/v2/init.py#L282
Added line #L282 was not covered by tests
[warning] 309-309: homewizard_energy/v2/init.py#L309
Added line #L309 was not covered by tests
homewizard_energy/v2/websocket.py
[warning] 32-32: homewizard_energy/v2/websocket.py#L32
Added line #L32 was not covered by tests
[warning] 38-43: homewizard_energy/v2/websocket.py#L38-L43
Added lines #L38 - L43 were not covered by tests
[warning] 45-47: homewizard_energy/v2/websocket.py#L45-L47
Added lines #L45 - L47 were not covered by tests
[warning] 49-50: homewizard_energy/v2/websocket.py#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 53-55: homewizard_energy/v2/websocket.py#L53-L55
Added lines #L53 - L55 were not covered by tests
[warning] 57-57: homewizard_energy/v2/websocket.py#L57
Added line #L57 was not covered by tests
[warning] 59-62: homewizard_energy/v2/websocket.py#L59-L62
Added lines #L59 - L62 were not covered by tests
[warning] 67-68: homewizard_energy/v2/websocket.py#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 79-80: homewizard_energy/v2/websocket.py#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 82-83: homewizard_energy/v2/websocket.py#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-86: homewizard_energy/v2/websocket.py#L86
Added line #L86 was not covered by tests
[warning] 90-90: homewizard_energy/v2/websocket.py#L90
Added line #L90 was not covered by tests
[warning] 93-93: homewizard_energy/v2/websocket.py#L93
Added line #L93 was not covered by tests
[warning] 95-95: homewizard_energy/v2/websocket.py#L95
Added line #L95 was not covered by tests
[warning] 98-99: homewizard_energy/v2/websocket.py#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 102-102: homewizard_energy/v2/websocket.py#L102
Added line #L102 was not covered by tests
[warning] 105-105: homewizard_energy/v2/websocket.py#L105
Added line #L105 was not covered by tests
[warning] 107-109: homewizard_energy/v2/websocket.py#L107-L109
Added lines #L107 - L109 were not covered by tests
🔇 Additional comments (5)
homewizard_energy/v2/const.py (1)
5-5: Verify API version compatibility
The change from "v1" to "2.0.0" represents a major version bump which could indicate breaking changes.
Let's verify the API version compatibility:
homewizard_energy/v2/websocket.py (2)
25-30: Use instance variables instead of class variables for connection state
As previously mentioned, declaring _connect_lock, _ws_connection, _ws_subscriptions, and _ws_authenticated as class variables could lead to unintended behavior when multiple instances are created. They should be instance variables within the __init__ method to ensure each Websocket instance maintains its own state.
80-80: Fix incorrect removal of subscription from the list
As previously noted, you're using a set {topic, ws_callback} instead of a tuple (topic, ws_callback) in _unsub_ws_callback. Since _ws_subscriptions is a list of tuples, attempting to remove a set will result in a ValueError.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-80: homewizard_energy/v2/websocket.py#L79-L80
Added lines #L79 - L80 were not covered by tests
homewizard_energy/v2/__init__.py (2)
Line range hint 123-176: Good use of aiohttp.hdrs for HTTP method constants
Replacing hardcoded HTTP method strings with aiohttp.hdrs.METH_* constants improves code readability and maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-141: homewizard_energy/v2/init.py#L141
Added line #L141 was not covered by tests
Line range hint 219-226: Security concern: Skipping hostname verification
Disabling hostname verification by setting context.check_hostname = False can expose the connection to man-in-the-middle attacks. It's important to ensure secure connections by verifying the hostname or providing a valid identifier.
Consider reviewing the necessity of disabling hostname verification. If an identifier is not available, ensure that there's an alternative means of verifying the server's identity.
Run the following script to check where _identifier is used and ensure it's properly set:
✅ Verification successful
Security approach is appropriate for this use case
The implementation actually maintains a good security posture:
- When no identifier is provided, it still keeps
ssl.CERT_REQUIREDmode active, ensuring the certificate chain is validated - The hostname verification is only disabled when no identifier is provided, which is a valid approach for IoT devices on local networks where the hostname might not match the certificate
- The code properly logs a warning to inform users when hostname validation is skipped
This is a reasonable security trade-off for IoT devices that may need to communicate over local IP addresses while still maintaining certificate validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for instantiations of HomeWizardEnergyV2 without an identifier.
# Find all files where HomeWizardEnergyV2 is instantiated without the identifier parameter.
rg 'HomeWizardEnergyV2\(\s*host\s*=\s*[^\),]+(\)|,\s*\))'
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of HomeWizardEnergyV2
# First, find all instantiations of HomeWizardEnergyV2
rg "HomeWizardEnergyV2\(" -A 3
# Also search for the class definition to understand the parameters
ast-grep --pattern 'class HomeWizardEnergyV2'
# Let's also check where the hostname validation logic is used
rg "hostname_checks_common_name" -B 3 -A 3
Length of output: 753
Script:
#!/bin/bash
# Let's check the class definition and constructor to understand how _identifier is used
ast-grep --pattern 'class HomeWizardEnergyV2:
$$$
def __init__($$$) {
$$$
}
$$$'
# Also search for any tests or documentation about the identifier parameter
rg -l "_identifier" --type python
# Check for any documentation about the security implications
fd "README|SECURITY" --extension md --exec cat {}
Length of output: 2398
Script:
#!/bin/bash
# Let's try a different pattern to find the class definition and constructor
rg "class HomeWizardEnergyV2" -A 20
# Also check for any test files that might show the intended usage
fd "test.*\.py$" --exec rg -l "HomeWizardEnergyV2"
# Look for any documentation strings or comments about SSL/security
rg -B 2 -A 2 "ssl\." --type-add 'python:*.py'
Length of output: 2308
| await asyncio.wait_for(self._connect_lock.acquire(), timeout=0.1) | ||
| except asyncio.TimeoutError: | ||
| _LOGGER.debug("Failed to get connection lock") |
There was a problem hiding this comment.
Fix lock release error when lock not acquired
If acquiring the lock times out and raises a TimeoutError at line 42, the lock is not acquired, but self._connect_lock.release() is called at line 57 regardless. This will raise a RuntimeError since you cannot release an unlocked lock. Ensure that the lock is only released if it has been successfully acquired.
Apply this diff to fix the issue:
try:
await asyncio.wait_for(self._connect_lock.acquire(), timeout=0.1)
except asyncio.TimeoutError:
_LOGGER.debug("Failed to get connection lock")
+ return False
finally:
+ if self._connect_lock.locked():
self._connect_lock.release()Also applies to: 57-57
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-43: homewizard_energy/v2/websocket.py#L38-L43
Added lines #L38 - L43 were not covered by tests
| for topic, callback in self._ws_subscriptions: | ||
| if topic == msg_type: | ||
| try: | ||
| await callback(topic, parsed_data) |
There was a problem hiding this comment.
Ensure ws_callback is an async function or adjust invocation
At line 184, you're using await callback(topic, parsed_data), assuming that callback is an async function. However, OnMessageCallbackType is defined as Callable[[str, Device | Measurement | System], None], which could be a regular function. Awaiting a non-async function will raise a TypeError.
Consider redefining OnMessageCallbackType to enforce that callbacks are coroutines:
-from typing import Callable
+from typing import Callable, Awaitable
-OnMessageCallbackType = Callable[[str, Device | Measurement | System], None]
+OnMessageCallbackType = Callable[[str, Device | Measurement | System], Awaitable[None]]Alternatively, modify the code to handle both async and sync callbacks:
if asyncio.iscoroutinefunction(callback):
await callback(topic, parsed_data)
else:
callback(topic, parsed_data)| self._ws_authenticated = True | ||
|
|
||
| # Send subscription requests | ||
| print(self._ws_subscriptions) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace print statement with _LOGGER
Using print for output is not recommended in production code. Replace the print statement at line 136 with a logging call using _LOGGER for consistent logging.
Apply this diff:
- print(self._ws_subscriptions)
+ _LOGGER.debug("Current subscriptions: %s", self._ws_subscriptions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(self._ws_subscriptions) | |
| _LOGGER.debug("Current subscriptions: %s", self._ws_subscriptions) |
| if _clientsession.closed: | ||
| # Avoid runtime errors when connection is closed. | ||
| # This solves an issue when updates were scheduled and clientsession was closed. | ||
| return None |
There was a problem hiding this comment.
Ensure consistent return type from _request method
The _request method returns None when the client session is closed, but typically returns a tuple (status, response). This inconsistency could lead to unexpected errors in methods that rely on the return value. Consider raising a specific exception to maintain a consistent return type.
Apply this diff to raise an exception when the client session is closed:
if _clientsession.closed:
# Avoid runtime errors when connection is closed.
# This solves an issue when updates were scheduled and clientsession was closed.
- return None
+ raise RequestError("Client session is closed")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _clientsession.closed: | |
| # Avoid runtime errors when connection is closed. | |
| # This solves an issue when updates were scheduled and clientsession was closed. | |
| return None | |
| if _clientsession.closed: | |
| # Avoid runtime errors when connection is closed. | |
| # This solves an issue when updates were scheduled and clientsession was closed. | |
| raise RequestError("Client session is closed") |
| connector = aiohttp.TCPConnector( | ||
| enable_cleanup_closed=True, | ||
| ssl=context, | ||
| limit_per_host=1, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated enable_cleanup_closed parameter in aiohttp.TCPConnector
The enable_cleanup_closed parameter in aiohttp.TCPConnector is deprecated and will be removed in future versions of aiohttp. To ensure future compatibility, it's recommended to remove this parameter or find an alternative approach.
Apply this diff to remove the deprecated parameter:
connector = aiohttp.TCPConnector(
- enable_cleanup_closed=True,
ssl=context,
limit_per_host=1,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connector = aiohttp.TCPConnector( | |
| enable_cleanup_closed=True, | |
| ssl=context, | |
| limit_per_host=1, | |
| ) | |
| connector = aiohttp.TCPConnector( | |
| ssl=context, | |
| limit_per_host=1, | |
| ) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 230-230: homewizard_energy/v2/init.py#L230
Added line #L230 was not covered by tests
Summary by CodeRabbit
New Features
Bug Fixes
Documentation