Refactor and implement feedback on v2

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

@@ -739,6 +739,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 +761,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

@@ -31,7 +31,9 @@
 # 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+))?"
+)
 
 
 @define(init=False, kw_only=True)
@@ -180,7 +182,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}")
 

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

@@ -183,11 +183,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)