diff --git a/pyoverkiz/action_queue.py b/pyoverkiz/action_queue.py index 54caf2cd..f02d2553 100644 --- a/pyoverkiz/action_queue.py +++ b/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 diff --git a/pyoverkiz/client.py b/pyoverkiz/client.py index e40704be..8d53566a 100644 --- a/pyoverkiz/client.py +++ b/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: diff --git a/pyoverkiz/models.py b/pyoverkiz/models.py index b6220b2e..b4e5f9ed 100644 --- a/pyoverkiz/models.py +++ b/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 # :///[#] -DEVICE_URL_RE = r"(?P.+)://(?P[^/]+)/(?P[^#]+)(#(?P\d+))?" +DEVICE_URL_RE = re.compile( + r"(?P[^:]+)://(?P[^/]+)/(?P[^#]+)(#(?P\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,28 @@ 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 and keep raw payloads on decode errors.""" + try: + return json.loads(raw_value) + except json.JSONDecodeError as err: + _LOGGER.warning( + "Failed to decode JSON event state; keeping raw value for `%s` (%s): %s", + self.name, + self.type.name, + err, + ) + return raw_value @define(init=False) @@ -1059,7 +1085,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 diff --git a/pyoverkiz/serializers.py b/pyoverkiz/serializers.py index ee4dc1aa..dc3995e2 100644 --- a/pyoverkiz/serializers.py +++ b/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 diff --git a/tests/test_action_queue.py b/tests/test_action_queue.py index 752e344d..94833aaf 100644 --- a/tests/test_action_queue.py +++ b/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.""" diff --git a/tests/test_client.py b/tests/test_client.py index 6c68f6a5..5a6b6641 100644 --- a/tests/test_client.py +++ b/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.""" diff --git a/tests/test_models.py b/tests/test_models.py index 8fc78a99..a789e368 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -2,6 +2,8 @@ from __future__ import annotations +import logging + import humps import pytest @@ -10,6 +12,7 @@ CommandDefinitions, Definition, Device, + EventState, State, StateDefinition, States, @@ -183,11 +186,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 +588,37 @@ 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_is_left_untouched(self): + """Malformed JSON payload strings remain raw to avoid hard failures.""" + state = EventState( + name="state", + type=DataType.JSON_ARRAY, + value="[not-valid-json", + ) + + assert state.value == "[not-valid-json" + + def test_invalid_json_string_logs_warning(self, caplog: pytest.LogCaptureFixture): + """Malformed JSON payload strings emit a warning for easier diagnostics.""" + with caplog.at_level(logging.WARNING): + EventState( + name="state", + type=DataType.JSON_OBJECT, + value="{not-valid-json", + ) + + assert "Failed to decode JSON event state" in caplog.text + + def test_command_to_payload_omits_none(): """Command.to_payload omits None fields from the resulting payload.""" from pyoverkiz.enums.command import OverkizCommand