[Bridge] Gracefully handle unknown message IDs (#8)

Author: robgee86

src/arduino/app_utils/bridge.py

@@ -516,10 +516,10 @@ def _handle_msg(self, msg: list):
         try:
             if msg_type == 0:  # Request: [0, msgid, method, params]
                 if len(msg) != 4:
-                    raise ValueError("Invalid RPC request: expected length 4")
+                    raise ValueError(f"Invalid RPC request: expected length 4, got {len(msg)}")
                 _, msgid, method, params = msg
                 if not isinstance(params, (list, tuple)):
-                    raise ValueError("Invalid RPC request params: expected array/tuple")
+                    raise ValueError("Invalid RPC request params: expected array or tuple")
 
                 method_name = self._decode_method(method)
 
@@ -538,7 +538,7 @@ def _handle_msg(self, msg: list):
 
             elif msg_type == 1:  # Response: [1, msgid, error, result]
                 if len(msg) != 4:
-                    raise ValueError("Invalid RPC response: expected length 4")
+                    raise ValueError(f"Invalid RPC response: expected length 4, got {len(msg)}")
                 _, msgid, error, result = msg
                 if error and (not isinstance(error, list) or len(error) < 2):
                     raise ValueError("Invalid error format in RPC response")
@@ -559,11 +559,11 @@ def _handle_msg(self, msg: list):
                         else:
                             on_result([GENERIC_ERR, "Unknown error occurred."])
                 else:
-                    on_error([GENERIC_ERR, f"Response for unknown msgid {msgid} received."])
+                    logger.warning(f"Response for unknown msgid {msgid} received.")
 
             elif msg_type == 2:  # Notification: [2, method, params]
                 if len(msg) != 3:
-                    raise ValueError("Invalid RPC notification: expected length 3")
+                    raise ValueError(f"Invalid RPC notification: expected length 3, got {len(msg)}")
                 _, method, params = msg
                 if not isinstance(params, (list, tuple)):
                     raise ValueError("Invalid RPC notification params: expected array or tuple")

tests/arduino/app_utils/bridge/test_unit_common.py

@@ -15,7 +15,9 @@ def setUp(self):
 
         # Patch dependencies
         # Mock the logger used by ClientServer
-        patch("arduino.app_utils.bridge.logger", MagicMock()).start()
+        self.mock_logger = MagicMock()
+        self.logger_patcher = patch("arduino.app_utils.bridge.logger", self.mock_logger)
+        self.logger_patcher.start()
 
         # Mock the socket instance that will be created
         self.mock_socket_instance = MagicMock()
@@ -32,5 +34,6 @@ def setUp(self):
 
     def tearDown(self):
         """This method is called after each test and cleans up the patched dependencies."""
+        self.logger_patcher.stop()
         self.socket_patcher.stop()
         self.threading_patcher.stop()

tests/arduino/app_utils/bridge/test_unit_handle_msg.py

@@ -9,6 +9,58 @@
 
 
 class TestHandleMsg(UnitTest):
+    def test_empty_msg(self):
+        """Test handling of an empty message."""
+        client = ClientServer()
+        client._handle_msg([])
+        self.mock_logger.warning.assert_called_once_with("Invalid RPC message received (must be a non-empty list).")
+        self.mock_logger.error.assert_not_called()
+
+    def test_unknown_msg_type(self):
+        """Test handling of an unknown message type."""
+        client = ClientServer()
+        client._handle_msg([99, 1, None, "result"])  # Msg type 99 does not exist
+        self.mock_logger.warning.assert_called_once_with("Invalid RPC message type received: 99")
+        self.mock_logger.error.assert_not_called()
+
+    def test_unknown_msg_id(self):
+        """Test handling of an unknown message id."""
+        client = ClientServer()
+        client._handle_msg([1, 9999, None, "result"])  # Msg id 9999 does not exist
+        self.mock_logger.warning.assert_called_once_with("Response for unknown msgid 9999 received.")
+        self.mock_logger.error.assert_not_called()
+
+    def test_malformed_messages(self):
+        """Test handling of malformed messages."""
+        client = ClientServer()
+
+        client._handle_msg([0, 1, "method", [0, 1], "extra field"])  # Malformed payload
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid RPC request: expected length 4, got 5")
+        self.mock_logger.reset_mock()
+        client._handle_msg([0, 1, "method", 1])  # Malformed params
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid RPC request params: expected array or tuple")
+        self.mock_logger.reset_mock()
+
+        client._handle_msg([1, 1, None, "result", "extra field"])  # Malformed payload
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid RPC response: expected length 4, got 5")
+        self.mock_logger.reset_mock()
+        client._handle_msg([1, 1, 42, "result"])  # Malformed error
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid error format in RPC response")
+        self.mock_logger.reset_mock()
+
+        client._handle_msg([2, 1, [0, 1], "extra field"])  # Malformed payload
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid RPC notification: expected length 3, got 4")
+        self.mock_logger.reset_mock()
+        client._handle_msg([2, 1, 42])  # Malformed params
+        self.mock_logger.warning.assert_not_called()
+        self.mock_logger.error.assert_called_once_with("Message validation error: Invalid RPC notification params: expected array or tuple")
+        self.mock_logger.reset_mock()
+
     def test_handle_msg_request(self):
         """Test handling of an incoming request message."""
         client = ClientServer()
@@ -27,6 +79,22 @@ def test_handle_msg_request(self):
         handler_mock.assert_called_once_with(*params)
         client._send_response.assert_called_once_with(msgid, None, "handled")
 
+    def test_handle_msg_request_handler_fail(self):
+        """Test handling of a request for a method that fails running its handler."""
+        client = ClientServer()
+        client._send_response = MagicMock()
+
+        request_msg = [0, 111, "failing_method", []]
+        client.handlers["failing_method"] = MagicMock(side_effect=ValueError("Handler failed"))
+
+        client._handle_msg(request_msg)
+
+        client._send_response.assert_called_once()
+        args, _ = client._send_response.call_args
+        self.assertEqual(args[0], 111)  # msgid
+        self.assertIsInstance(args[1], ValueError)  # error
+        self.assertIsNone(args[2])  # result
+
     def test_handle_msg_request_method_not_found(self):
         """Test handling of a request for a method that is not found."""
         client = ClientServer()
@@ -79,7 +147,7 @@ def test_handle_msg_response(self):
         on_error_mock.assert_not_called()
         self.assertNotIn(msgid, client.callbacks)  # Callback should be removed
 
-    def test_handle_msg_response_function_not_found(self):
+    def test_handle_msg_generic_error_response(self):
         """Test handling of an incoming error response message."""
         client = ClientServer()
 
@@ -100,7 +168,7 @@ def test_handle_msg_response_function_not_found(self):
         on_error_mock.assert_called_once_with(result_error)
         self.assertNotIn(msgid, client.callbacks)  # Callback should be removed
 
-    def test_handle_msg_response_already_provided_error(self):
+    def test_handle_msg_method_exists_error_response(self):
         """Test handling of an incoming error response message that signals a method is already provided."""
         client = ClientServer()