Merge pull request #59 from jfmlima/fix/rpc-result-unwrap

Read the RPC result out of the response frame

Author: jfmlima

packages/core/src/core/gateways/device/shelly_device_gateway.py

@@ -21,6 +21,7 @@
 from ...services.auth_state_cache import AuthStateCache
 from ...utils.validation import normalize_mac
 from ..network.network import RpcNetworkGateway
+from ..network.rpc_envelope import RpcError, rpc_result
 from .device import DeviceGateway
 from .legacy_device_gateway import LegacyDeviceGateway
 from .legacy_route import LegacyRoute
@@ -265,7 +266,15 @@ async def execute_component_action(
                 message=(
                     f"{action_name.method} executed successfully on {component_key}"
                 ),
-                data=response,
+                data=rpc_result(response),
+            )
+
+        except RpcError as e:
+            # The device answered, and refused. That is a failed action, not a
+            # transport problem, and it carries the device's own reason.
+            return envelope.failed(
+                message=f"{action_name.method} failed on {component_key}",
+                error=str(e),
             )
 
         except Exception as e:

packages/core/src/core/gateways/network/rpc_envelope.py

@@ -0,0 +1,41 @@
+"""Reading a Shelly JSON-RPC response frame.
+
+A device answers every ``/rpc`` call with HTTP 200 and a JSON-RPC frame:
+``{"id": ..., "src": ..., "result": {...}}`` when the call worked, and the same
+frame carrying ``"error"`` instead when it did not. Neither the payload nor the
+failure is visible unless the frame is opened, so reading the frame as if it
+were the payload turns device rejections into successes and stores envelopes
+wherever the payload belongs.
+"""
+
+from typing import Any
+
+
+class RpcError(Exception):
+    """A device answered an RPC call with an error member."""
+
+    def __init__(self, code: Any, message: str) -> None:
+        self.code = code
+        self.message = message
+        super().__init__(f"{message} (code: {code})")
+
+
+def rpc_result(response: Any) -> Any:
+    """The payload carried by a JSON-RPC response frame.
+
+    A response that is not a frame is returned unchanged, so callers that
+    already hold a bare payload keep working.
+
+    Raises:
+        RpcError: If the device answered with an error member.
+    """
+    if not isinstance(response, dict):
+        return response
+
+    error = response.get("error")
+    if isinstance(error, dict):
+        raise RpcError(error.get("code"), str(error.get("message", "Unknown error")))
+
+    if "result" in response:
+        return response["result"]
+    return response

packages/core/tests/unit/gateways/device/test_shelly_device_gateway.py

@@ -289,6 +289,63 @@ async def test_it_gets_device_status_with_zigbee_failure(
         assert calls[2] == (("192.168.1.100", "Shelly.GetStatus"), {"timeout": 10.0})
         assert calls[3] == (("192.168.1.100", "Shelly.ListMethods"), {"timeout": 10.0})
 
+    async def test_it_returns_the_rpc_payload_not_the_response_frame(
+        self, gateway, mock_rpc_client
+    ):
+        # Frame captured verbatim from a Gen4 device: the config a caller wants
+        # sits under "result", alongside the request id and the device id.
+        mock_rpc_client.make_rpc_request = AsyncMock(
+            side_effect=[
+                ({"methods": ["Switch.GetConfig"]}, 0.1),
+                (
+                    {
+                        "id": "68ef3f1a-b13a-490b-9ab0-d4b8f21d5580",
+                        "src": "shelly1pmminig4-7c2c676d30d4",
+                        "result": {"id": 0, "name": None, "in_mode": "flip"},
+                    },
+                    0.1,
+                ),
+            ]
+        )
+
+        result = await gateway.execute_component_action(
+            "192.168.1.100", "switch:0", "GetConfig"
+        )
+
+        assert result.success is True
+        assert result.data == {"id": 0, "name": None, "in_mode": "flip"}
+
+    async def test_it_reports_a_device_rejection_as_a_failed_action(
+        self, gateway, mock_rpc_client
+    ):
+        # Devices answer a rejected call with HTTP 200 and an "error" member,
+        # so a frame that is never opened reads as a success.
+        mock_rpc_client.make_rpc_request = AsyncMock(
+            side_effect=[
+                ({"methods": ["Switch.GetConfig"]}, 0.1),
+                (
+                    {
+                        "id": "b036bb7f-91f8-4b4b-b81b-0376cafedee0",
+                        "src": "shelly1pmminig4-7c2c676d30d4",
+                        "error": {
+                            "code": -105,
+                            "message": "Argument 'id', value 99 not found!",
+                        },
+                    },
+                    0.1,
+                ),
+            ]
+        )
+
+        result = await gateway.execute_component_action(
+            "192.168.1.100", "switch:99", "GetConfig"
+        )
+
+        assert result.success is False
+        assert result.data is None
+        assert "value 99 not found" in result.error
+        assert "-105" in result.error
+
     async def test_it_handles_update_check_failure_gracefully(
         self, gateway, mock_rpc_client
     ):

packages/core/tests/unit/gateways/network/test_rpc_envelope.py

@@ -0,0 +1,44 @@
+import pytest
+from core.gateways.network.rpc_envelope import RpcError, rpc_result
+
+# Frames captured verbatim from a Shelly Gen4 (S4SW-001P8EU, fw 1.7.1) over
+# POST /rpc. Both arrive as HTTP 200; only the member distinguishes them.
+SUCCESS_FRAME = {
+    "id": "probe-1",
+    "src": "shelly1pmminig4-7c2c676d30d4",
+    "result": {"id": 0, "name": None, "in_mode": "flip", "auto_off_delay": 60.0},
+}
+ERROR_FRAME = {
+    "id": "probe-4",
+    "src": "shelly1pmminig4-7c2c676d30d4",
+    "error": {"code": -105, "message": "Argument 'id', value 99 not found!"},
+}
+
+
+class TestRpcResult:
+    def test_it_returns_the_payload_not_the_frame(self):
+        assert rpc_result(SUCCESS_FRAME) == {
+            "id": 0,
+            "name": None,
+            "in_mode": "flip",
+            "auto_off_delay": 60.0,
+        }
+
+    def test_it_raises_on_a_device_rejection(self):
+        with pytest.raises(RpcError) as excinfo:
+            rpc_result(ERROR_FRAME)
+
+        assert excinfo.value.code == -105
+        assert "value 99 not found" in str(excinfo.value)
+
+    def test_it_returns_an_empty_result_as_is(self):
+        # A method with no return value answers with an empty result member.
+        assert rpc_result({"id": "x", "src": "y", "result": {}}) == {}
+
+    def test_it_passes_through_a_response_that_is_not_a_frame(self):
+        assert rpc_result({"methods": ["Switch.Toggle"]}) == {
+            "methods": ["Switch.Toggle"]
+        }
+
+    def test_it_passes_through_a_non_mapping(self):
+        assert rpc_result(None) is None