From 2c6815ea3fc130066ead62dd3d747773b31065ab Mon Sep 17 00:00:00 2001 From: Michael Chisholm Date: Fri, 28 Aug 2026 15:26:09 +1000 Subject: [PATCH] Fix swallowed exceptions in dlna_dmr action handlers (#180408) --- .../components/dlna_dmr/media_player.py | 37 +++++---- .../components/dlna_dmr/strings.json | 8 ++ .../components/dlna_dmr/test_media_player.py | 82 +++++++++++++++---- 3 files changed, 98 insertions(+), 29 deletions(-) diff --git a/homeassistant/components/dlna_dmr/media_player.py b/homeassistant/components/dlna_dmr/media_player.py index a51e4fe1c5dc..29dbfb035202 100644 --- a/homeassistant/components/dlna_dmr/media_player.py +++ b/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: diff --git a/homeassistant/components/dlna_dmr/strings.json b/homeassistant/components/dlna_dmr/strings.json index fe186bce1c1d..3821615bc032 100644 --- a/homeassistant/components/dlna_dmr/strings.json +++ b/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" diff --git a/tests/components/dlna_dmr/test_media_player.py b/tests/components/dlna_dmr/test_media_player.py index ac5334e8e5b6..cb2138e3da37 100644 --- a/tests/components/dlna_dmr/test_media_player.py +++ b/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,