Fix swallowed exceptions in dlna_dmr action handlers (#180408)

homeassistant/components/dlna_dmr/media_player.py

@@ -9,7 +9,11 @@ from typing import Any, Concatenate, override
 
 from async_upnp_client.client import UpnpService, UpnpStateVariable
 from async_upnp_client.const import NotificationSubType
-from async_upnp_client.exceptions import UpnpError, UpnpResponseError
+from async_upnp_client.exceptions import (
+    UpnpConnectionError,
+    UpnpError,
+    UpnpResponseError,
+)
 from async_upnp_client.profiles.dlna import DmrDevice, PlayMode, TransportState
 from async_upnp_client.utils import async_get_local_ip
 from didl_lite import didl_lite
@@ -29,6 +33,7 @@ from homeassistant.components.media_player import (
 )
 from homeassistant.const import CONF_DEVICE_ID, CONF_MAC, CONF_TYPE, CONF_URL
 from homeassistant.core import CoreState, HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import device_registry as dr, entity_registry as er
 from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback
 from homeassistant.helpers.service_info.ssdp import SsdpServiceInfo
@@ -79,10 +84,24 @@ def catch_request_errors[_DlnaDmrEntityT: DlnaDmrEntity, **_P, _R](
             return None
         try:
             return await func(self, *args, **kwargs)
+        except UpnpConnectionError as err:
+            # Inform user explicitly of connection issues (like device turned off)
+            self.check_available = True
+            raise HomeAssistantError(
+                translation_domain=DOMAIN,
+                translation_key="request_connection_error",
+                translation_placeholders={"action": func.__name__},
+            ) from err
         except UpnpError as err:
             self.check_available = True
-            _LOGGER.error("Error during call %s: %r", func.__name__, err)
-        return None
+            raise HomeAssistantError(
+                translation_domain=DOMAIN,
+                translation_key="request_error",
+                translation_placeholders={
+                    "action": func.__name__,
+                    "upnperror": str(err),
+                },
+            ) from err
 
     return wrapper
 
@@ -612,7 +631,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
             return None
         return self._device.volume_level
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_set_volume_level(self, volume: float) -> None:
@@ -628,7 +646,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
             return None
         return self._device.is_volume_muted
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_mute_volume(self, mute: bool) -> None:
@@ -637,7 +654,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         desired_mute = bool(mute)
         await self._device.async_mute_volume(desired_mute)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_pause(self) -> None:
@@ -645,7 +661,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         assert self._device is not None
         await self._device.async_pause()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_play(self) -> None:
@@ -653,7 +668,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         assert self._device is not None
         await self._device.async_play()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_stop(self) -> None:
@@ -661,7 +675,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         assert self._device is not None
         await self._device.async_stop()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_seek(self, position: float) -> None:
@@ -670,7 +683,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         time = timedelta(seconds=position)
         await self._device.async_seek_rel_time(time)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_play_media(
@@ -740,7 +752,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         await self._device.async_wait_for_can_play()
         await self.async_media_play()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_previous_track(self) -> None:
@@ -748,7 +759,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
         assert self._device is not None
         await self._device.async_previous()
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_media_next_track(self) -> None:
@@ -771,7 +781,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
 
         return play_mode in (PlayMode.SHUFFLE, PlayMode.RANDOM)
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_set_shuffle(self, shuffle: bool) -> None:
@@ -813,7 +822,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
 
         return RepeatMode.OFF
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_set_repeat(self, repeat: RepeatMode) -> None:
@@ -842,7 +850,6 @@ class DlnaDmrEntity(MediaPlayerEntity):
             return None
         return self._device.preset_names
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     @catch_request_errors
     @override
     async def async_select_sound_mode(self, sound_mode: str) -> None:

homeassistant/components/dlna_dmr/strings.json

@@ -36,6 +36,14 @@
       }
     }
   },
+  "exceptions": {
+    "request_connection_error": {
+      "message": "Could not connect to device when calling service for action {action}"
+    },
+    "request_error": {
+      "message": "Error when calling device service for action {action}: {upnperror}"
+    }
+  },
   "options": {
     "error": {
       "invalid_url": "Invalid URL"

tests/components/dlna_dmr/test_media_player.py

@@ -9,6 +9,7 @@ from unittest.mock import ANY, DEFAULT, Mock, patch
 
 from async_upnp_client.client import UpnpService, UpnpStateVariable
 from async_upnp_client.exceptions import (
+    UpnpActionResponseError,
     UpnpConnectionError,
     UpnpError,
     UpnpResponseError,
@@ -43,6 +44,7 @@ from homeassistant.const import (
     CONF_URL,
 )
 from homeassistant.core import CoreState, HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import device_registry as dr, entity_registry as er
 from homeassistant.helpers.entity_component import async_update_entity
 from homeassistant.helpers.service_info.ssdp import SsdpServiceInfo
@@ -1963,7 +1965,7 @@ async def test_become_unavailable(
     mock_entity_id: str,
     dmr_device_mock: Mock,
 ) -> None:
-    """Test a device becoming unavailable."""
+    """Test a device becoming unavailable due to a connection error."""
     # Check async_update currently works
     await async_update_entity(hass, mock_entity_id)
     dmr_device_mock.async_update.assert_called_with(do_ping=False)
@@ -1973,13 +1975,17 @@ async def test_become_unavailable(
     dmr_device_mock.async_update.reset_mock()
 
     # Interface service calls should flag that the device is unavailable, but
-    # not disconnect it immediately
-    await hass.services.async_call(
-        mp.DOMAIN,
-        ha_const.SERVICE_VOLUME_SET,
-        {ATTR_ENTITY_ID: mock_entity_id, mp.ATTR_MEDIA_VOLUME_LEVEL: 0.80},
-        blocking=True,
-    )
+    # not disconnect it immediately.
+    # An exception should also be raised to inform the user or automation.
+    with pytest.raises(
+        HomeAssistantError, match="Could not connect .* async_set_volume_level"
+    ):
+        await hass.services.async_call(
+            mp.DOMAIN,
+            ha_const.SERVICE_VOLUME_SET,
+            {ATTR_ENTITY_ID: mock_entity_id, mp.ATTR_MEDIA_VOLUME_LEVEL: 0.80},
+            blocking=True,
+        )
 
     mock_state = hass.states.get(mock_entity_id)
     assert mock_state is not None
@@ -1997,12 +2003,16 @@ async def test_become_unavailable(
     dmr_device_mock.async_update.reset_mock()
     dmr_device_mock.async_update.side_effect = UpnpConnectionError
 
-    await hass.services.async_call(
-        mp.DOMAIN,
-        ha_const.SERVICE_VOLUME_SET,
-        {ATTR_ENTITY_ID: mock_entity_id, mp.ATTR_MEDIA_VOLUME_LEVEL: 0.80},
-        blocking=True,
-    )
+    with pytest.raises(
+        HomeAssistantError, match="Could not connect .* async_set_volume_level"
+    ):
+        await hass.services.async_call(
+            mp.DOMAIN,
+            ha_const.SERVICE_VOLUME_SET,
+            {ATTR_ENTITY_ID: mock_entity_id, mp.ATTR_MEDIA_VOLUME_LEVEL: 0.80},
+            blocking=True,
+        )
+
     await async_update_entity(hass, mock_entity_id)
     dmr_device_mock.async_update.assert_called_with(do_ping=True)
     mock_state = hass.states.get(mock_entity_id)
@@ -2010,6 +2020,50 @@ async def test_become_unavailable(
     assert mock_state.state == ha_const.STATE_UNAVAILABLE
 
 
+async def test_generic_error(
+    hass: HomeAssistant,
+    mock_entity_id: str,
+    dmr_device_mock: Mock,
+) -> None:
+    """Test when a UpnpError occurs during an action, it is bubbled up to the user.
+
+    The behaviour should be the same as for a UpnpConnectionError.
+    """
+    # Generate a service-specific UpnpError on the next service call. This particular
+    # error simulates trying to play a file that the device does not support.
+    dmr_device_mock.async_play.side_effect = UpnpActionResponseError(
+        status=500, error_code=714, error_desc="Illegal MIME-type"
+    )
+
+    # Interface service calls should flag that the device may be unavailable,
+    # but not disconnect it immediately.
+    # An exception should also be raised to inform the user or automation.
+    with pytest.raises(
+        HomeAssistantError,
+        match="Error when calling device service for action async_media_play: .*Illegal MIME-type",
+    ):
+        await hass.services.async_call(
+            mp.DOMAIN,
+            ha_const.SERVICE_MEDIA_PLAY,
+            {ATTR_ENTITY_ID: mock_entity_id},
+            blocking=True,
+        )
+
+    mock_state = hass.states.get(mock_entity_id)
+    assert mock_state is not None
+    assert mock_state.state == MediaPlayerState.IDLE
+
+    # With a working connection, device should remain available on the next,
+    # update, and any thereafter.
+    for do_ping in (True, False):
+        dmr_device_mock.async_update.reset_mock()
+        await async_update_entity(hass, mock_entity_id)
+        dmr_device_mock.async_update.assert_called_with(do_ping=do_ping)
+        mock_state = hass.states.get(mock_entity_id)
+        assert mock_state is not None
+        assert mock_state.state == MediaPlayerState.IDLE
+
+
 async def test_poll_availability(
     hass: HomeAssistant,
     domain_data_mock: Mock,