From f8503e4df6fdab56ae4dbe6d8eceaca6bef281d7 Mon Sep 17 00:00:00 2001 From: "Steven B." <51370195+sdb9696@users.noreply.github.com> Date: Sun, 15 Dec 2024 16:03:12 +0000 Subject: [PATCH] Force single for some smartcam requests (#1374) `onboarding` requests do not return the method key and need to be sent as single requests. --- kasa/protocols/smartprotocol.py | 28 +++++++++- tests/fakeprotocol_smartcam.py | 17 ++++-- tests/protocols/test_smartprotocol.py | 80 +++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 8 deletions(-) diff --git a/kasa/protocols/smartprotocol.py b/kasa/protocols/smartprotocol.py index 0e092547..deb1a405 100644 --- a/kasa/protocols/smartprotocol.py +++ b/kasa/protocols/smartprotocol.py @@ -69,6 +69,13 @@ REDACTORS: dict[str, Callable[[Any], Any] | None] = { "map_data": lambda x: "#SCRUBBED_MAPDATA#" if x else "", } +# Queries that are known not to work properly when sent as a +# multiRequest. They will not return the `method` key. +FORCE_SINGLE_REQUEST = { + "getConnectStatus", + "scanApList", +} + class SmartProtocol(BaseProtocol): """Class for the new TPLink SMART protocol.""" @@ -89,6 +96,7 @@ class SmartProtocol(BaseProtocol): self._transport._config.batch_size or self.DEFAULT_MULTI_REQUEST_BATCH_SIZE ) self._redact_data = True + self._method_missing_logged = False def get_smart_request(self, method: str, params: dict | None = None) -> str: """Get a request message as a string.""" @@ -178,6 +186,7 @@ class SmartProtocol(BaseProtocol): multi_requests = [ {"method": method, "params": params} if params else {"method": method} for method, params in requests.items() + if method not in FORCE_SINGLE_REQUEST ] end = len(multi_requests) @@ -246,7 +255,20 @@ class SmartProtocol(BaseProtocol): responses = response_step["result"]["responses"] for response in responses: - method = response["method"] + # some smartcam devices calls do not populate the method key + # these should be defined in DO_NOT_SEND_AS_MULTI_REQUEST. + if not (method := response.get("method")): + if not self._method_missing_logged: + # Avoid spamming the logs + self._method_missing_logged = True + _LOGGER.error( + "No method key in response for %s, skipping: %s", + self._host, + response_step, + ) + # These will end up being queried individually + continue + self._handle_response_error_code( response, method, raise_on_error=raise_on_error ) @@ -255,7 +277,9 @@ class SmartProtocol(BaseProtocol): result, method, retry_count=retry_count ) multi_result[method] = result - # Multi requests don't continue after errors so requery any missing + + # Multi requests don't continue after errors so requery any missing. + # Will also query individually any DO_NOT_SEND_AS_MULTI_REQUEST. for method, params in requests.items(): if method not in multi_result: resp = await self._transport.send( diff --git a/tests/fakeprotocol_smartcam.py b/tests/fakeprotocol_smartcam.py index 68cebd1e..e8cc6f30 100644 --- a/tests/fakeprotocol_smartcam.py +++ b/tests/fakeprotocol_smartcam.py @@ -34,6 +34,7 @@ class FakeSmartCamTransport(BaseTransport): list_return_size=10, is_child=False, verbatim=False, + components_not_included=False, ): super().__init__( config=DeviceConfig( @@ -59,12 +60,16 @@ class FakeSmartCamTransport(BaseTransport): # self.child_protocols = self._get_child_protocols() self.list_return_size = list_return_size - self.components = { - comp["name"]: comp["version"] - for comp in self.info["getAppComponentList"]["app_component"][ - "app_component_list" - ] - } + # Setting this flag allows tests to create dummy transports without + # full fixture info for testing specific cases like list handling etc + self.components_not_included = (components_not_included,) + if not components_not_included: + self.components = { + comp["name"]: comp["version"] + for comp in self.info["getAppComponentList"]["app_component"][ + "app_component_list" + ] + } @property def default_port(self): diff --git a/tests/protocols/test_smartprotocol.py b/tests/protocols/test_smartprotocol.py index 988c95eb..7961df68 100644 --- a/tests/protocols/test_smartprotocol.py +++ b/tests/protocols/test_smartprotocol.py @@ -2,6 +2,7 @@ import logging import pytest import pytest_mock +from pytest_mock import MockerFixture from kasa.exceptions import ( SMART_RETRYABLE_ERRORS, @@ -14,6 +15,7 @@ from kasa.smart import SmartDevice from ..conftest import device_smart from ..fakeprotocol_smart import FakeSmartTransport +from ..fakeprotocol_smartcam import FakeSmartCamTransport DUMMY_QUERY = {"foobar": {"foo": "bar", "bar": "foo"}} DUMMY_MULTIPLE_QUERY = { @@ -448,3 +450,81 @@ async def test_smart_queries_redaction( await dev.update() assert device_id not in caplog.text assert "REDACTED_" + device_id[9::] in caplog.text + + +async def test_no_method_returned_multiple( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """Test protocol handles multiple requests that don't return the method.""" + req = { + "getDeviceInfo": {"device_info": {"name": ["basic_info", "info"]}}, + "getAppComponentList": {"app_component": {"name": "app_component_list"}}, + } + res = { + "result": { + "responses": [ + { + "method": "getDeviceInfo", + "result": { + "device_info": { + "basic_info": { + "device_model": "C210", + }, + } + }, + "error_code": 0, + }, + { + "result": {"app_component": {"app_component_list": []}}, + "error_code": 0, + }, + ] + }, + "error_code": 0, + } + + transport = FakeSmartCamTransport( + {}, + "dummy-name", + components_not_included=True, + ) + protocol = SmartProtocol(transport=transport) + mocker.patch.object(protocol._transport, "send", return_value=res) + await protocol.query(req) + assert "No method key in response" in caplog.text + caplog.clear() + await protocol.query(req) + assert "No method key in response" not in caplog.text + + +async def test_no_multiple_methods( + mocker: MockerFixture, caplog: pytest.LogCaptureFixture +): + """Test protocol sends NO_MULTI methods as single call.""" + req = { + "getDeviceInfo": {"device_info": {"name": ["basic_info", "info"]}}, + "getConnectStatus": {"onboarding": {"get_connect_status": {}}}, + } + info = { + "getDeviceInfo": { + "device_info": { + "basic_info": { + "avatar": "Home", + } + } + }, + "getConnectStatus": { + "onboarding": { + "get_connect_status": {"current_ssid": "", "err_code": 0, "status": 0} + } + }, + } + transport = FakeSmartCamTransport( + info, + "dummy-name", + components_not_included=True, + ) + protocol = SmartProtocol(transport=transport) + send_spy = mocker.spy(protocol._transport, "send") + await protocol.query(req) + assert send_spy.call_count == 2