Raise HomeAssistantError from Openhome action handlers (#181645)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

homeassistant/components/openhome/media_player.py

@@ -9,6 +9,8 @@ from openhomedevice.exceptions import OpenhomeError
 
 from homeassistant.components import media_source
 from homeassistant.components.media_player import (
+    SERVICE_PLAY_MEDIA,
+    SERVICE_SELECT_SOURCE,
     BrowseMedia,
     MediaPlayerEntity,
     MediaPlayerEntityFeature,
@@ -16,12 +18,27 @@ from homeassistant.components.media_player import (
     MediaType,
     async_process_play_media_url,
 )
+from homeassistant.const import (
+    SERVICE_MEDIA_NEXT_TRACK,
+    SERVICE_MEDIA_PAUSE,
+    SERVICE_MEDIA_PLAY,
+    SERVICE_MEDIA_PREVIOUS_TRACK,
+    SERVICE_MEDIA_STOP,
+    SERVICE_TURN_OFF,
+    SERVICE_TURN_ON,
+    SERVICE_VOLUME_DOWN,
+    SERVICE_VOLUME_MUTE,
+    SERVICE_VOLUME_SET,
+    SERVICE_VOLUME_UP,
+)
 from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers.device_registry import DeviceInfo
 from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback
 
 from . import OpenhomeConfigEntry
 from .const import DOMAIN
+from .services import SERVICE_INVOKE_PIN
 
 SUPPORT_OPENHOME = (
     MediaPlayerEntityFeature.SELECT_SOURCE
@@ -50,14 +67,16 @@ async def async_setup_entry(
 
 type _FuncType[_T, **_P, _R] = Callable[Concatenate[_T, _P], Awaitable[_R]]
 type _ReturnFuncType[_T, **_P, _R] = Callable[
-    Concatenate[_T, _P], Coroutine[Any, Any, _R | None]
+    Concatenate[_T, _P], Coroutine[Any, Any, _R]
 ]
 
 
-def catch_request_errors[_OpenhomeDeviceT: OpenhomeDevice, **_P, _R]() -> Callable[
+def catch_request_errors[_OpenhomeDeviceT: OpenhomeDevice, **_P, _R](
+    action: str,
+) -> Callable[
     [_FuncType[_OpenhomeDeviceT, _P, _R]], _ReturnFuncType[_OpenhomeDeviceT, _P, _R]
 ]:
-    """Catch OpenhomeError errors."""
+    """Return decorator that catches errors and raises HomeAssistantError."""
 
     def call_wrapper(
         func: _FuncType[_OpenhomeDeviceT, _P, _R],
@@ -67,13 +86,15 @@ def catch_request_errors[_OpenhomeDeviceT: OpenhomeDevice, **_P, _R]() -> Callab
         @functools.wraps(func)
         async def wrapper(
             self: _OpenhomeDeviceT, *args: _P.args, **kwargs: _P.kwargs
-        ) -> _R | None:
+        ) -> _R:
             """Catch OpenhomeError errors."""
             try:
                 return await func(self, *args, **kwargs)
             except OpenhomeError as err:
-                _LOGGER.error("Error during call %s: %s", func.__name__, err)
-            return None
+                raise HomeAssistantError(
+                    translation_domain=DOMAIN,
+                    translation_key=action,
+                ) from err
 
         return wrapper
 
@@ -173,22 +194,19 @@ class OpenhomeDevice(MediaPlayerEntity):
                 _LOGGER.warning("Error updating %s: %s", self.entity_id, err)
             self._attr_available = False
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_TURN_ON)
     @override
     async def async_turn_on(self) -> None:
         """Bring device out of standby."""
         await self._device.set_standby(False)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_TURN_OFF)
     @override
     async def async_turn_off(self) -> None:
         """Put device in standby."""
         await self._device.set_standby(True)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_PLAY_MEDIA)
     @override
     async def async_play_media(
         self, media_type: MediaType | str, media_id: str, **kwargs: Any
@@ -214,82 +232,71 @@ class OpenhomeDevice(MediaPlayerEntity):
         track_details = {"title": "Home Assistant", "uri": media_id}
         await self._device.play_media(track_details)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_MEDIA_PAUSE)
     @override
     async def async_media_pause(self) -> None:
         """Send pause command."""
         await self._device.pause()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_MEDIA_STOP)
     @override
     async def async_media_stop(self) -> None:
         """Send stop command."""
         await self._device.stop()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_MEDIA_PLAY)
     @override
     async def async_media_play(self) -> None:
         """Send play command."""
         await self._device.play()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_MEDIA_NEXT_TRACK)
     @override
     async def async_media_next_track(self) -> None:
         """Send next track command."""
         await self._device.skip(1)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_MEDIA_PREVIOUS_TRACK)
     @override
     async def async_media_previous_track(self) -> None:
         """Send previous track command."""
         await self._device.skip(-1)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_SELECT_SOURCE)
     @override
     async def async_select_source(self, source: str) -> None:
         """Select input source."""
         await self._device.set_source(self._source_index[source])
 
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_INVOKE_PIN)
     async def async_invoke_pin(self, pin):
         """Invoke pin."""
-        try:
-            if self._device.pins_enabled:
-                await self._device.invoke_pin(pin)
-            else:
-                _LOGGER.error("Pins service not supported")
-        except OpenhomeError as err:
-            _LOGGER.error("Error invoking pin %s: %s", pin, err)
+        if not self._device.pins_enabled:
+            raise HomeAssistantError(
+                translation_domain=DOMAIN,
+                translation_key="pins_not_supported",
+            )
+        await self._device.invoke_pin(pin)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_VOLUME_UP)
     @override
     async def async_volume_up(self) -> None:
         """Volume up media player."""
         await self._device.increase_volume()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_VOLUME_DOWN)
     @override
     async def async_volume_down(self) -> None:
         """Volume down media player."""
         await self._device.decrease_volume()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_VOLUME_SET)
     @override
     async def async_set_volume_level(self, volume: float) -> None:
         """Set volume level, range 0..1."""
         await self._device.set_volume(int(volume * 100))
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
-    @catch_request_errors()
+    @catch_request_errors(SERVICE_VOLUME_MUTE)
     @override
     async def async_mute_volume(self, mute: bool) -> None:
         """Mute (true) or unmute (false) media player."""

homeassistant/components/openhome/strings.json

@@ -1,4 +1,51 @@
 {
+  "exceptions": {
+    "invoke_pin": {
+      "message": "Failed to invoke the pin"
+    },
+    "media_next_track": {
+      "message": "Failed to move to the next track"
+    },
+    "media_pause": {
+      "message": "Failed to pause"
+    },
+    "media_play": {
+      "message": "Failed to play"
+    },
+    "media_previous_track": {
+      "message": "Failed to move to the previous track"
+    },
+    "media_stop": {
+      "message": "Failed to stop"
+    },
+    "pins_not_supported": {
+      "message": "This device does not support pins"
+    },
+    "play_media": {
+      "message": "Failed to play media"
+    },
+    "select_source": {
+      "message": "Failed to select the source"
+    },
+    "turn_off": {
+      "message": "Failed to turn off"
+    },
+    "turn_on": {
+      "message": "Failed to turn on"
+    },
+    "volume_down": {
+      "message": "Failed to turn down the volume"
+    },
+    "volume_mute": {
+      "message": "Failed to set the mute state"
+    },
+    "volume_set": {
+      "message": "Failed to set the volume"
+    },
+    "volume_up": {
+      "message": "Failed to turn up the volume"
+    }
+  },
   "services": {
     "invoke_pin": {
       "description": "Starts playing content pinned on the specified device.",

tests/components/openhome/test_media_player.py

@@ -0,0 +1,286 @@
+"""Tests for the Openhome media player platform."""
+
+from collections.abc import Callable, Generator
+from datetime import timedelta
+from typing import Any
+from unittest.mock import AsyncMock, MagicMock, patch
+
+from openhomedevice.device import Device
+from openhomedevice.exceptions import OpenhomeConnectionError
+import pytest
+
+from homeassistant.components.media_player import (
+    ATTR_INPUT_SOURCE,
+    ATTR_MEDIA_CONTENT_ID,
+    ATTR_MEDIA_CONTENT_TYPE,
+    ATTR_MEDIA_VOLUME_LEVEL,
+    ATTR_MEDIA_VOLUME_MUTED,
+    DOMAIN as MEDIA_PLAYER_DOMAIN,
+    SERVICE_PLAY_MEDIA,
+    SERVICE_SELECT_SOURCE,
+    MediaType,
+)
+from homeassistant.components.openhome.const import DOMAIN
+from homeassistant.components.openhome.services import (
+    ATTR_PIN_INDEX,
+    SERVICE_INVOKE_PIN,
+)
+from homeassistant.const import (
+    ATTR_ENTITY_ID,
+    CONF_HOST,
+    SERVICE_MEDIA_NEXT_TRACK,
+    SERVICE_MEDIA_PAUSE,
+    SERVICE_MEDIA_PLAY,
+    SERVICE_MEDIA_PREVIOUS_TRACK,
+    SERVICE_MEDIA_STOP,
+    SERVICE_TURN_OFF,
+    SERVICE_TURN_ON,
+    SERVICE_VOLUME_DOWN,
+    SERVICE_VOLUME_MUTE,
+    SERVICE_VOLUME_SET,
+    SERVICE_VOLUME_UP,
+    Platform,
+)
+from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
+from homeassistant.util import dt as dt_util
+
+from tests.common import MockConfigEntry, async_fire_time_changed
+
+ENTITY_ID = "media_player.friendly_name"
+
+SOURCES = [
+    {"index": 0, "name": "Playlist", "type": "Playlist"},
+    {"index": 1, "name": "Radio", "type": "Radio"},
+]
+
+# The device coroutines the actions drive, referenced from the library so a
+# rename upstream fails the test rather than silently skipping an action.
+ACTION_METHODS = (
+    Device.set_standby,
+    Device.play,
+    Device.pause,
+    Device.stop,
+    Device.skip,
+    Device.increase_volume,
+    Device.decrease_volume,
+    Device.set_volume,
+    Device.set_mute,
+    Device.set_source,
+    Device.play_media,
+    Device.invoke_pin,
+)
+
+# Each action, the coroutine it drives, and a source type that exposes the
+# supported feature it is gated behind.
+ACTIONS = [
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_TURN_ON,
+        {},
+        Device.set_standby,
+        "Playlist",
+        id="turn_on",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_TURN_OFF,
+        {},
+        Device.set_standby,
+        "Playlist",
+        id="turn_off",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_MEDIA_PLAY,
+        {},
+        Device.play,
+        "Playlist",
+        id="media_play",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_MEDIA_PAUSE,
+        {},
+        Device.pause,
+        "Playlist",
+        id="media_pause",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_MEDIA_STOP,
+        {},
+        Device.stop,
+        "Radio",
+        id="media_stop",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_MEDIA_NEXT_TRACK,
+        {},
+        Device.skip,
+        "Playlist",
+        id="media_next_track",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_MEDIA_PREVIOUS_TRACK,
+        {},
+        Device.skip,
+        "Playlist",
+        id="media_previous_track",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_VOLUME_UP,
+        {},
+        Device.increase_volume,
+        "Playlist",
+        id="volume_up",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_VOLUME_DOWN,
+        {},
+        Device.decrease_volume,
+        "Playlist",
+        id="volume_down",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_VOLUME_SET,
+        {ATTR_MEDIA_VOLUME_LEVEL: 0.5},
+        Device.set_volume,
+        "Playlist",
+        id="volume_set",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_VOLUME_MUTE,
+        {ATTR_MEDIA_VOLUME_MUTED: True},
+        Device.set_mute,
+        "Playlist",
+        id="volume_mute",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_SELECT_SOURCE,
+        {ATTR_INPUT_SOURCE: "Playlist"},
+        Device.set_source,
+        "Playlist",
+        id="select_source",
+    ),
+    pytest.param(
+        MEDIA_PLAYER_DOMAIN,
+        SERVICE_PLAY_MEDIA,
+        {
+            ATTR_MEDIA_CONTENT_TYPE: MediaType.MUSIC,
+            ATTR_MEDIA_CONTENT_ID: "http://localhost/track.flac",
+        },
+        Device.play_media,
+        "Playlist",
+        id="play_media",
+    ),
+    pytest.param(
+        DOMAIN,
+        SERVICE_INVOKE_PIN,
+        {ATTR_PIN_INDEX: 1},
+        Device.invoke_pin,
+        "Playlist",
+        id="invoke_pin",
+    ),
+]
+
+
+@pytest.fixture
+def mock_device() -> Generator[MagicMock]:
+    """Return a mocked Openhome device that polls successfully."""
+    with patch("homeassistant.components.openhome.Device", MagicMock()) as mock_class:
+        device = mock_class.return_value
+        device.init = AsyncMock()
+        device.uuid = MagicMock(return_value="uuid")
+        device.manufacturer = MagicMock(return_value="manufacturer")
+        device.model_name = MagicMock(return_value="model_name")
+        device.friendly_name = MagicMock(return_value="friendly_name")
+        device.volume_enabled = True
+        device.pins_enabled = True
+        device.room = AsyncMock(return_value="room")
+        device.track_info = AsyncMock(return_value={})
+        device.volume = AsyncMock(return_value=50)
+        device.is_muted = AsyncMock(return_value=False)
+        device.sources = AsyncMock(return_value=SOURCES)
+        device.is_in_standby = AsyncMock(return_value=False)
+        device.transport_state = AsyncMock(return_value="Playing")
+        for method in ACTION_METHODS:
+            setattr(device, method.__name__, AsyncMock())
+        yield device
+
+
+async def setup_platform(
+    hass: HomeAssistant, mock_device: MagicMock, source_type: str
+) -> None:
+    """Load the media player platform and poll once to populate features."""
+    mock_device.source = AsyncMock(
+        return_value={"index": 0, "name": source_type, "type": source_type}
+    )
+    entry = MockConfigEntry(
+        domain=DOMAIN, data={CONF_HOST: "http://localhost"}, unique_id="uuid"
+    )
+    entry.add_to_hass(hass)
+
+    with patch("homeassistant.components.openhome.PLATFORMS", [Platform.MEDIA_PLAYER]):
+        await hass.config_entries.async_setup(entry.entry_id)
+        await hass.async_block_till_done()
+
+    # Supported features are only set once the device has been polled.
+    async_fire_time_changed(hass, dt_util.utcnow() + timedelta(seconds=30))
+    await hass.async_block_till_done()
+
+
+@pytest.mark.parametrize(
+    ("domain", "service", "data", "method", "source_type"), ACTIONS
+)
+async def test_action_error_is_raised(
+    hass: HomeAssistant,
+    mock_device: MagicMock,
+    domain: str,
+    service: str,
+    data: dict[str, Any],
+    method: Callable[..., Any],
+    source_type: str,
+) -> None:
+    """Test every action raises when the device rejects the request."""
+    await setup_platform(hass, mock_device, source_type)
+
+    mocked = getattr(mock_device, method.__name__)
+    mocked.side_effect = OpenhomeConnectionError("no route to host")
+
+    with pytest.raises(HomeAssistantError) as err:
+        await hass.services.async_call(
+            domain, service, {ATTR_ENTITY_ID: ENTITY_ID, **data}, blocking=True
+        )
+
+    # The message is keyed on the service name, so each action reports its own.
+    assert err.value.translation_domain == DOMAIN
+    assert err.value.translation_key == service
+    mocked.assert_awaited()
+
+
+async def test_invoke_pin_without_pin_support(
+    hass: HomeAssistant, mock_device: MagicMock
+) -> None:
+    """Test invoking a pin on a device without pin support raises."""
+    mock_device.pins_enabled = False
+    await setup_platform(hass, mock_device, "Playlist")
+
+    with pytest.raises(HomeAssistantError) as err:
+        await hass.services.async_call(
+            DOMAIN,
+            SERVICE_INVOKE_PIN,
+            {ATTR_ENTITY_ID: ENTITY_ID, ATTR_PIN_INDEX: 1},
+            blocking=True,
+        )
+
+    assert err.value.translation_domain == DOMAIN
+    assert err.value.translation_key == "pins_not_supported"
+    mock_device.invoke_pin.assert_not_awaited()