From b056fe2ee1897b52347c93717e57fa6ccc33449a Mon Sep 17 00:00:00 2001
From: Teemu R <tpr@iki.fi>
Date: Tue, 20 Sep 2022 19:10:40 +0200
Subject: [PATCH] Skip write-only properties for miot status requests (#1525)

---
 miio/miot_device.py           | 29 +++++++++++++++++--
 miio/tests/test_miotdevice.py | 53 ++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/miio/miot_device.py b/miio/miot_device.py
index 43cafcc01..1e4cbee50 100644
--- a/miio/miot_device.py
+++ b/miio/miot_device.py
@@ -27,6 +27,27 @@ def _str2bool(x):
 MiotMapping = Dict[str, Dict[str, Any]]
 
 
+def _filter_request_fields(req):
+    """Return only the parts that belong to the request.."""
+    return {k: v for k, v in req.items() if k in ["did", "siid", "piid"]}
+
+
+def _is_readable_property(prop):
+    """Returns True if a property in the mapping can be read."""
+    # actions cannot be read
+    if "aiid" in prop:
+        _LOGGER.debug("Ignoring action %s for the request", prop)
+        return False
+
+    # if the mapping has access defined, check if the property is readable
+    access = getattr(prop, "access", None)
+    if access is not None and "read" not in access:
+        _LOGGER.debug("Ignoring %s as it has non-read access defined", prop)
+        return False
+
+    return True
+
+
 class MiotDevice(Device):
     """Main class representing a MIoT device.
 
@@ -64,10 +85,14 @@ def __init__(
 
     def get_properties_for_mapping(self, *, max_properties=15) -> list:
         """Retrieve raw properties based on mapping."""
+        mapping = self._get_mapping()
 
         # We send property key in "did" because it's sent back via response and we can identify the property.
-        mapping = self._get_mapping()
-        properties = [{"did": k, **v} for k, v in mapping.items() if "aiid" not in v]
+        properties = [
+            {"did": k, **_filter_request_fields(v)}
+            for k, v in mapping.items()
+            if _is_readable_property(v)
+        ]
 
         return self.get_properties(
             properties, property_getter="get_properties", max_properties=max_properties
diff --git a/miio/tests/test_miotdevice.py b/miio/tests/test_miotdevice.py
index 52787076a..03b8f5d61 100644
--- a/miio/tests/test_miotdevice.py
+++ b/miio/tests/test_miotdevice.py
@@ -1,7 +1,9 @@
+from unittest.mock import ANY
+
 import pytest
 
 from miio import Huizuo, MiotDevice
-from miio.miot_device import MiotValueType
+from miio.miot_device import MiotValueType, _filter_request_fields
 
 MIOT_DEVICES = MiotDevice.__subclasses__()
 # TODO: huizuo needs to be refactored to use _mappings,
@@ -15,6 +17,7 @@ def dev(module_mocker):
     device = MiotDevice(
         "127.0.0.1", "68ffffffffffffffffffffffffffffff", mapping=DUMMY_MAPPING
     )
+    device._model = "testmodel"
     module_mocker.patch.object(device, "send")
     return device
 
@@ -151,3 +154,51 @@ def test_supported_models(cls):
 
     # make sure that that _supported_models is not defined
     assert not cls._supported_models
+
+
+def test_call_action(dev):
+    dev._mappings["testmodel"] = {"test_action": {"siid": 1, "aiid": 1}}
+
+    dev.call_action("test_action")
+
+
+@pytest.mark.parametrize(
+    "props,included_in_request",
+    [
+        ({"access": ["read"]}, True),  # read only
+        ({"access": ["read", "write"]}, True),  # read-write
+        ({}, True),  # not defined
+        ({"access": ["write"]}, False),  # write-only
+        ({"aiid": "1"}, False),  # action
+    ],
+    ids=["read-only", "read-write", "access-not-defined", "write-only", "action"],
+)
+def test_get_properties_for_mapping_readables(mocker, dev, props, included_in_request):
+    base_props = {"readable_property": {"siid": 1, "piid": 1}}
+    base_request = [{"did": k, **v} for k, v in base_props.items()]
+    dev._mappings["testmodel"] = mapping = {
+        **base_props,
+        "property_under_test": {"siid": 1, "piid": 2, **props},
+    }
+    expected_request = [
+        {"did": k, **_filter_request_fields(v)} for k, v in mapping.items()
+    ]
+
+    req = mocker.patch.object(dev, "get_properties")
+    dev.get_properties_for_mapping()
+
+    try:
+
+        req.assert_called_with(
+            expected_request, property_getter=ANY, max_properties=ANY
+        )
+    except AssertionError:
+        if included_in_request:
+            raise AssertionError("Required property was not requested")
+        else:
+            try:
+                req.assert_called_with(
+                    base_request, property_getter=ANY, max_properties=ANY
+                )
+            except AssertionError as ex:
+                raise AssertionError("Tried to read unreadable property") from ex