Skip to content

Implement minimums for WebSocket API #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 56 additions & 25 deletions homewizard_energy/v2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,13 @@
import asyncio
import logging
import ssl
from collections.abc import Callable, Coroutine
from collections.abc import Coroutine
from http import HTTPStatus
from typing import Any, TypeVar
from typing import Any, Callable, TypeVar

import aiohttp
import async_timeout
import backoff
from aiohttp.client import (
ClientError,
ClientResponseError,
ClientSession,
ClientTimeout,
TCPConnector,
)
from aiohttp.hdrs import METH_DELETE, METH_GET, METH_POST, METH_PUT

from homewizard_energy.errors import (
DisabledError,
Expand All @@ -30,6 +23,7 @@

from .cacert import CACERT
from .models import Device, Measurement, System, SystemUpdate
from .websocket import Websocket

_LOGGER = logging.getLogger(__name__)

Expand All @@ -54,9 +48,10 @@
class HomeWizardEnergyV2:
"""Communicate with a HomeWizard Energy device."""

_clientsession: ClientSession | None = None
_clientsession: aiohttp.ClientSession | None = None
_close_clientsession: bool = False
_request_timeout: int = 10
_websocket: Websocket | None = None

def __init__(
self,
Expand Down Expand Up @@ -89,6 +84,16 @@
"""
return self._host

@property
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

Check warning on line 95 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L94-L95

Added lines #L94 - L95 were not covered by tests
Comment on lines +88 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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


@authorized_method
async def device(self) -> Device:
"""Return the device object."""
Expand All @@ -115,7 +120,7 @@
if update is not None:
data = update.as_dict()
status, response = await self._request(
"/api/system", method=METH_PUT, data=data
"/api/system", method=aiohttp.hdrs.METH_PUT, data=data
)

else:
Expand All @@ -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)

Check warning on line 141 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L141

Added line #L141 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

return True

async def get_token(
Expand All @@ -142,7 +147,7 @@
) -> str:
"""Get authorization token from device."""
status, response = await self._request(
"/api/user", method=METH_POST, data={"name": f"local/{name}"}
"/api/user", method=aiohttp.hdrs.METH_POST, data={"name": f"local/{name}"}
)

if status == HTTPStatus.FORBIDDEN:
Expand All @@ -168,7 +173,7 @@
"""Delete authorization token from device."""
status, response = await self._request(
"/api/user",
method=METH_DELETE,
method=aiohttp.hdrs.METH_DELETE,
data={"name": name} if name is not None else None,
)

Expand All @@ -180,11 +185,34 @@
if name is None:
self._token = None

async def _get_clientsession(self) -> ClientSession:
@property
def token(self) -> str | None:
"""Return the token of the device.

Returns:
token: The used token

"""
return self._token

Check warning on line 196 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L196

Added line #L196 was not covered by tests

@property
def request_timeout(self) -> int:
"""Return the request timeout of the device.

Returns:
request_timeout: The used request timeout

"""
return self._request_timeout

Check warning on line 206 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L206

Added line #L206 was not covered by tests

async def get_clientsession(self) -> aiohttp.ClientSession:
"""
Get a clientsession that is tuned for communication with the HomeWizard Energy Device
"""

if self._clientsession is not None:
return self._clientsession

Check warning on line 214 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L214

Added line #L214 was not covered by tests
Comment on lines +213 to +214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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


def _build_ssl_context() -> ssl.SSLContext:
context = ssl.create_default_context(cadata=CACERT)
if self._identifier is not None:
Expand All @@ -199,26 +227,28 @@
loop = asyncio.get_running_loop()
context = await loop.run_in_executor(None, _build_ssl_context)

connector = TCPConnector(
connector = aiohttp.TCPConnector(

Check warning on line 230 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L230

Added line #L230 was not covered by tests
enable_cleanup_closed=True,
ssl=context,
limit_per_host=1,
)

Comment on lines +230 to 235
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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

return ClientSession(
connector=connector, timeout=ClientTimeout(total=self._request_timeout)
self._clientsession = aiohttp.ClientSession(

Check warning on line 236 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L236

Added line #L236 was not covered by tests
connector=connector,
timeout=aiohttp.ClientTimeout(total=self._request_timeout),
)

return self._clientsession

Check warning on line 241 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L241

Added line #L241 was not covered by tests

@backoff.on_exception(backoff.expo, RequestError, max_tries=5, logger=None)
async def _request(
self, path: str, method: str = METH_GET, data: object = None
self, path: str, method: str = aiohttp.hdrs.METH_GET, data: object = None
) -> Any:
"""Make a request to the API."""

if self._clientsession is None:
self._clientsession = await self._get_clientsession()
_clientsession = await self.get_clientsession()

Check warning on line 249 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L249

Added line #L249 was not covered by tests

if self._clientsession.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
Comment on lines +251 to 254
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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")

Expand All @@ -235,7 +265,7 @@

try:
async with async_timeout.timeout(self._request_timeout):
resp = await self._clientsession.request(
resp = await _clientsession.request(

Check warning on line 268 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L268

Added line #L268 was not covered by tests
method,
url,
json=data,
Expand All @@ -249,7 +279,7 @@
raise RequestError(
f"Timeout occurred while connecting to the HomeWizard Energy device at {self.host}"
) from exception
except (ClientError, ClientResponseError) as exception:
except (aiohttp.ClientError, aiohttp.ClientResponseError) as exception:

Check warning on line 282 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L282

Added line #L282 was not covered by tests
raise RequestError(
f"Error occurred while communicating with the HomeWizard Energy device at {self.host}"
) from exception
Expand All @@ -276,6 +306,7 @@
_LOGGER.debug("Closing clientsession")
if self._clientsession is not None:
await self._clientsession.close()
self._clientsession = None

Check warning on line 309 in homewizard_energy/v2/__init__.py

View check run for this annotation

Codecov / codecov/patch

homewizard_energy/v2/__init__.py#L309

Added line #L309 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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


async def __aenter__(self) -> HomeWizardEnergyV2:
"""Async enter.
Expand Down
12 changes: 11 additions & 1 deletion homewizard_energy/v2/const.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
"""Constants for HomeWizard Energy."""

SUPPORTED_API_VERSION = "v1"
from enum import StrEnum

SUPPORTED_API_VERSION = "2.0.0"

SUPPORTS_STATE = ["HWE-SKT"]
SUPPORTS_IDENTIFY = ["HWE-SKT", "HWE-P1", "HWE-WTR"]


class WebsocketTopic(StrEnum):
"""Websocket topics."""

DEVICE = "device"
MEASUREMENT = "measurement"
SYSTEM = "system"
Loading
Loading