Refactor and implement feedback on v2 (#1973)

Implement feedback from #1931

Author: iMicknl

pyoverkiz/action_queue.py

@@ -8,9 +8,10 @@
 from dataclasses import dataclass
 from typing import TYPE_CHECKING, Any
 
+from pyoverkiz.models import Action
+
 if TYPE_CHECKING:
     from pyoverkiz.enums import CommandMode
-    from pyoverkiz.models import Action
 
 
 @dataclass(frozen=True, slots=True)
@@ -108,6 +109,15 @@ def __init__(
         self._flush_task: asyncio.Task[None] | None = None
         self._lock = asyncio.Lock()
 
+    @staticmethod
+    def _copy_action(action: Action) -> Action:
+        """Return an `Action` copy with an independent commands list.
+
+        The queue merges commands for duplicate devices, so caller-owned action
+        instances must be copied to avoid mutating user input while batching.
+        """
+        return Action(device_url=action.device_url, commands=list(action.commands))
+
     async def add(
         self,
         actions: list[Action],
@@ -142,8 +152,9 @@ async def add(
         for action in actions:
             existing = normalized_index.get(action.device_url)
             if existing is None:
-                normalized_actions.append(action)
-                normalized_index[action.device_url] = action
+                action_copy = self._copy_action(action)
+                normalized_actions.append(action_copy)
+                normalized_index[action.device_url] = action_copy
             else:
                 existing.commands.extend(action.commands)
 
@@ -155,17 +166,15 @@ async def add(
                 batches_to_execute.append(self._prepare_flush())
 
             # Add actions to pending queue
+            pending_index = {
+                pending_action.device_url: pending_action
+                for pending_action in self._pending_actions
+            }
             for action in normalized_actions:
-                pending = next(
-                    (
-                        pending_action
-                        for pending_action in self._pending_actions
-                        if pending_action.device_url == action.device_url
-                    ),
-                    None,
-                )
+                pending = pending_index.get(action.device_url)
                 if pending is None:
                     self._pending_actions.append(action)
+                    pending_index[action.device_url] = action
                 else:
                     pending.commands.extend(action.commands)
             self._pending_mode = mode

pyoverkiz/client.py

@@ -598,7 +598,16 @@ async def get_action_groups(self) -> list[ActionGroup]:
 
     @retry_on_auth_error
     async def get_places(self) -> Place:
-        """List the places."""
+        """Get the hierarchical structure of places (house, rooms, areas, zones).
+
+        The Place model represents a hierarchical organization where the root place is
+        typically the house/property, and `sub_places` contains nested child places
+        (floors, rooms, areas). This structure can be recursively navigated to build
+        a complete map of all locations in the setup. Each place has:
+        - `label`: Human-readable name for the place
+        - `type`: Numeric identifier for the place type
+        - `sub_places`: List of nested places within this location
+        """
         response = await self._get("setup/places")
         places = Place(**humps.decamelize(response))
         return places
@@ -739,6 +748,11 @@ async def _get(self, path: str) -> Any:
             ssl=self._ssl,
         ) as response:
             await self.check_response(response)
+
+            # 204 has no body.
+            if response.status == 204:
+                return None
+
             return await response.json()
 
     async def _post(
@@ -756,6 +770,9 @@ async def _post(
             ssl=self._ssl,
         ) as response:
             await self.check_response(response)
+            # 204 has no body.
+            if response.status == 204:
+                return None
             return await response.json()
 
     async def _delete(self, path: str) -> None:

pyoverkiz/models.py

@@ -2,6 +2,8 @@
 
 from __future__ import annotations
 
+import json
+import logging
 import re
 from collections.abc import Iterator
 from typing import Any, cast
@@ -31,7 +33,11 @@
 # pylint: disable=unused-argument, too-many-instance-attributes, too-many-locals
 
 # <protocol>://<gatewayId>/<deviceAddress>[#<subsystemId>]
-DEVICE_URL_RE = r"(?P<protocol>.+)://(?P<gatewayId>[^/]+)/(?P<deviceAddress>[^#]+)(#(?P<subsystemId>\d+))?"
+DEVICE_URL_RE = re.compile(
+    r"(?P<protocol>[^:]+)://(?P<gatewayId>[^/]+)/(?P<deviceAddress>[^#]+)(#(?P<subsystemId>\d+))?"
+)
+
+_LOGGER = logging.getLogger(__name__)
 
 
 @define(init=False, kw_only=True)
@@ -180,7 +186,7 @@ def is_sub_device(self) -> bool:
     @classmethod
     def from_device_url(cls, device_url: str) -> DeviceIdentifier:
         """Parse a device URL into its structured identifier components."""
-        match = re.search(DEVICE_URL_RE, device_url)
+        match = DEVICE_URL_RE.fullmatch(device_url)
         if not match:
             raise ValueError(f"Invalid device URL: {device_url}")
 
@@ -589,8 +595,24 @@ def __init__(
         # Overkiz (cloud) returns all state values as a string
         # We cast them here based on the data type provided by Overkiz
         # Overkiz (local) returns all state values in the right format
-        if isinstance(self.value, str) and self.type in DATA_TYPE_TO_PYTHON:
-            self.value = DATA_TYPE_TO_PYTHON[self.type](self.value)
+        if not isinstance(self.value, str) or self.type not in DATA_TYPE_TO_PYTHON:
+            return
+
+        caster = DATA_TYPE_TO_PYTHON[self.type]
+        if self.type in (DataType.JSON_ARRAY, DataType.JSON_OBJECT):
+            self.value = self._cast_json_value(self.value)
+            return
+
+        self.value = caster(self.value)
+
+    def _cast_json_value(self, raw_value: str) -> StateType:
+        """Cast JSON event state values; raise on decode errors."""
+        try:
+            return json.loads(raw_value)
+        except json.JSONDecodeError as err:
+            raise ValueError(
+                f"Invalid JSON for event state `{self.name}` ({self.type.name}): {err}"
+            ) from err
 
 
 @define(init=False)
@@ -1059,7 +1081,13 @@ def __init__(
 
 @define(init=False, kw_only=True)
 class Place:
-    """Representation of a place (house/room) in a setup."""
+    """Hierarchical representation of a location (house, room, area) in a setup.
+
+    Places form a tree structure where the root place is typically the entire house
+    or property, and `sub_places` contains nested child locations. This recursive
+    structure allows navigation from house -> floors/rooms -> individual areas.
+    Each place has associated metadata like creation time, label, and type identifier.
+    """
 
     creation_time: int
     last_update_time: int | None = None

pyoverkiz/serializers.py

@@ -16,24 +16,24 @@
 _ABBREV_MAP: dict[str, str] = {"deviceUrl": "deviceURL"}
 
 
-def _fix_abbreviations(obj: Any) -> Any:
-    if isinstance(obj, dict):
-        out = {}
-        for k, v in obj.items():
-            k = _ABBREV_MAP.get(k, k)
-            out[k] = _fix_abbreviations(v)
-        return out
-    if isinstance(obj, list):
-        return [_fix_abbreviations(i) for i in obj]
-    return obj
+def _camelize_key(key: str) -> str:
+    """Camelize a single key and apply abbreviation fixes in one step."""
+    camel = humps.camelize(key)
+    return _ABBREV_MAP.get(camel, camel)
 
 
 def prepare_payload(payload: Any) -> Any:
     """Convert snake_case payload to API-ready camelCase and apply fixes.
 
+    Performs camelization and abbreviation fixes in a single recursive pass
+    to avoid walking the structure twice.
+
     Example:
         payload = {"device_url": "x", "commands": [{"name": "close"}]}
         => {"deviceURL": "x", "commands": [{"name": "close"}]}
     """
-    camel = humps.camelize(payload)
-    return _fix_abbreviations(camel)
+    if isinstance(payload, dict):
+        return {_camelize_key(k): prepare_payload(v) for k, v in payload.items()}
+    if isinstance(payload, list):
+        return [prepare_payload(item) for item in payload]
+    return payload

tests/test_action_queue.py

@@ -214,6 +214,29 @@ async def test_action_queue_duplicate_device_merge_order(mock_executor):
     ]
 
 
+@pytest.mark.asyncio
+async def test_action_queue_duplicate_device_merge_does_not_mutate_inputs(
+    mock_executor,
+):
+    """Test that merge behavior does not mutate caller-owned Action commands."""
+    queue = ActionQueue(executor=mock_executor, delay=0.1)
+
+    action1 = Action(
+        device_url="io://1234-5678-9012/1",
+        commands=[Command(name=OverkizCommand.CLOSE)],
+    )
+    action2 = Action(
+        device_url="io://1234-5678-9012/1",
+        commands=[Command(name=OverkizCommand.OPEN)],
+    )
+
+    queued = await queue.add([action1, action2])
+    await queued
+
+    assert [command.name for command in action1.commands] == [OverkizCommand.CLOSE]
+    assert [command.name for command in action2.commands] == [OverkizCommand.OPEN]
+
+
 @pytest.mark.asyncio
 async def test_action_queue_manual_flush(mock_executor):
     """Test manual flush of the queue."""

tests/test_client.py

@@ -480,6 +480,40 @@ async def test_get_setup_options(
             for option in options:
                 assert isinstance(option, Option)
 
+    @pytest.mark.asyncio
+    async def test_get_returns_none_for_204_without_json_parse(
+        self, client: OverkizClient
+    ) -> None:
+        """Ensure `_get` skips JSON parsing for 204 responses and returns `None`."""
+        resp = MockResponse("", status=204)
+        resp.json = AsyncMock(return_value={})
+
+        with (
+            patch.object(client, "_refresh_token_if_expired", new=AsyncMock()),
+            patch.object(aiohttp.ClientSession, "get", return_value=resp),
+        ):
+            result = await client._get("setup/options")
+
+        assert result is None
+        assert not resp.json.called
+
+    @pytest.mark.asyncio
+    async def test_post_returns_none_for_204_without_json_parse(
+        self, client: OverkizClient
+    ) -> None:
+        """Ensure `_post` skips JSON parsing for 204 responses and returns `None`."""
+        resp = MockResponse("", status=204)
+        resp.json = AsyncMock(return_value={})
+
+        with (
+            patch.object(client, "_refresh_token_if_expired", new=AsyncMock()),
+            patch.object(aiohttp.ClientSession, "post", return_value=resp),
+        ):
+            result = await client._post("setup/devices/states/refresh")
+
+        assert result is None
+        assert not resp.json.called
+
     @pytest.mark.asyncio
     async def test_execute_action_group_omits_none_fields(self, client: OverkizClient):
         """Ensure `type` and `parameters` that are None are omitted from the request payload."""

tests/test_models.py

@@ -10,6 +10,7 @@
     CommandDefinitions,
     Definition,
     Device,
+    EventState,
     State,
     StateDefinition,
     States,
@@ -183,11 +184,18 @@ def test_base_url_parsing(
         assert device.identifier.subsystem_id == subsystem_id
         assert device.identifier.is_sub_device == is_sub_device
 
-    def test_invalid_device_url_raises(self):
+    @pytest.mark.parametrize(
+        "device_url",
+        [
+            "foo://whatever",
+            "io://1234-5678-9012/10077486#8 trailing",
+        ],
+    )
+    def test_invalid_device_url_raises(self, device_url: str):
         """Invalid device URLs should raise during identifier parsing."""
         test_device = {
             **RAW_DEVICES,
-            **{"deviceURL": "foo://whatever"},
+            **{"deviceURL": device_url},
         }
         hump_device = humps.decamelize(test_device)
 
@@ -578,6 +586,25 @@ def test_bad_list_value(self):
             assert state.value_as_list
 
 
+class TestEventState:
+    """Unit tests for EventState cloud payload casting behavior."""
+
+    def test_json_string_is_parsed(self):
+        """Valid JSON payload strings are cast to typed Python values."""
+        state = EventState(name="state", type=DataType.JSON_OBJECT, value='{"foo": 1}')
+
+        assert state.value == {"foo": 1}
+
+    def test_invalid_json_string_raises(self):
+        """Malformed JSON payload strings raise ValueError."""
+        with pytest.raises(ValueError, match="Invalid JSON for event state"):
+            EventState(
+                name="state",
+                type=DataType.JSON_ARRAY,
+                value="[not-valid-json",
+            )
+
+
 def test_command_to_payload_omits_none():
     """Command.to_payload omits None fields from the resulting payload."""
     from pyoverkiz.enums.command import OverkizCommand