From bc3a42c65876548c585290f820901832766cbb37 Mon Sep 17 00:00:00 2001 From: Christopher Fenner <9592452+CFenner@users.noreply.github.com> Date: Thu, 19 Sep 2024 11:03:54 +0200 Subject: [PATCH] Fix serial handling in ViCare integration (#125495) * hand down device serial into common entity * fix platforms * Revert "fix platforms" This reverts commit 067af2b567538989f97c5a764be64f8744663daf. * handle event loop issue * hand in serial * Revert "Revert "fix platforms"" This reverts commit 9bbb55ee6da96ea31b98896e82c4b45ab001707b. * fix get serial call * handle other exceptions * also check device model for migration * merge entity and device migration * add test fixture without serial * adjust test cases * add dummy fixture * remove commented code * modify migration * use continue * break comment --- homeassistant/components/vicare/__init__.py | 108 +++++++++--------- .../components/vicare/binary_sensor.py | 15 ++- homeassistant/components/vicare/button.py | 6 +- homeassistant/components/vicare/climate.py | 8 +- homeassistant/components/vicare/entity.py | 6 +- homeassistant/components/vicare/fan.py | 8 +- homeassistant/components/vicare/number.py | 9 +- homeassistant/components/vicare/sensor.py | 15 ++- homeassistant/components/vicare/utils.py | 24 +++- .../components/vicare/water_heater.py | 6 +- .../fixtures/dummy-device-no-serial.json | 3 + tests/components/vicare/test_init.py | 84 ++++++++------ 12 files changed, 183 insertions(+), 109 deletions(-) create mode 100644 tests/components/vicare/fixtures/dummy-device-no-serial.json diff --git a/homeassistant/components/vicare/__init__.py b/homeassistant/components/vicare/__init__.py index ead210e2816..d6b9e4b923a 100644 --- a/homeassistant/components/vicare/__init__.py +++ b/homeassistant/components/vicare/__init__.py @@ -18,7 +18,7 @@ from PyViCare.PyViCareUtils import ( from homeassistant.components.climate import DOMAIN as DOMAIN_CLIMATE from homeassistant.config_entries import ConfigEntry from homeassistant.const import CONF_CLIENT_ID, CONF_PASSWORD, CONF_USERNAME -from homeassistant.core import HomeAssistant, callback +from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryAuthFailed from homeassistant.helpers import device_registry as dr, entity_registry as er from homeassistant.helpers.storage import STORAGE_DIR @@ -31,7 +31,7 @@ from .const import ( UNSUPPORTED_DEVICES, ) from .types import ViCareDevice -from .utils import get_device +from .utils import get_device, get_device_serial _LOGGER = logging.getLogger(__name__) _TOKEN_FILENAME = "vicare_token.save" @@ -51,9 +51,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: for device in hass.data[DOMAIN][entry.entry_id][DEVICE_LIST]: # Migration can be removed in 2025.4.0 - await async_migrate_devices(hass, entry, device) - # Migration can be removed in 2025.4.0 - await async_migrate_entities(hass, entry, device) + await async_migrate_devices_and_entities(hass, entry, device) await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) @@ -117,70 +115,72 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: return unload_ok -async def async_migrate_devices( +async def async_migrate_devices_and_entities( hass: HomeAssistant, entry: ConfigEntry, device: ViCareDevice ) -> None: """Migrate old entry.""" - registry = dr.async_get(hass) + device_registry = dr.async_get(hass) + entity_registry = er.async_get(hass) gateway_serial: str = device.config.getConfig().serial - device_serial: str = device.api.getSerial() + device_id = device.config.getId() + device_serial: str | None = await hass.async_add_executor_job( + get_device_serial, device.api + ) + device_model = device.config.getModel() old_identifier = gateway_serial - new_identifier = f"{gateway_serial}_{device_serial}" + new_identifier = ( + f"{gateway_serial}_{device_serial if device_serial is not None else device_id}" + ) # Migrate devices - for device_entry in dr.async_entries_for_config_entry(registry, entry.entry_id): - if device_entry.identifiers == {(DOMAIN, old_identifier)}: - _LOGGER.debug("Migrating device %s", device_entry.name) - registry.async_update_device( + for device_entry in dr.async_entries_for_config_entry( + device_registry, entry.entry_id + ): + if ( + device_entry.identifiers == {(DOMAIN, old_identifier)} + and device_entry.model == device_model + ): + _LOGGER.debug( + "Migrating device %s to new identifier %s", + device_entry.name, + new_identifier, + ) + device_registry.async_update_device( device_entry.id, serial_number=device_serial, new_identifiers={(DOMAIN, new_identifier)}, ) + # Migrate entities + for entity_entry in er.async_entries_for_device( + entity_registry, device_entry.id, True + ): + if entity_entry.unique_id.startswith(new_identifier): + # already correct, nothing to do + continue + unique_id_parts = entity_entry.unique_id.split("-") + # replace old prefix `` + # with `_` + unique_id_parts[0] = new_identifier + # convert climate entity unique id + # from `-` + # to `-heating-` + if entity_entry.domain == DOMAIN_CLIMATE: + unique_id_parts[len(unique_id_parts) - 1] = ( + f"{entity_entry.translation_key}-{unique_id_parts[len(unique_id_parts)-1]}" + ) + entity_new_unique_id = "-".join(unique_id_parts) -async def async_migrate_entities( - hass: HomeAssistant, entry: ConfigEntry, device: ViCareDevice -) -> None: - """Migrate old entry.""" - gateway_serial: str = device.config.getConfig().serial - device_serial: str = device.api.getSerial() - new_identifier = f"{gateway_serial}_{device_serial}" - - @callback - def _update_unique_id( - entity_entry: er.RegistryEntry, - ) -> dict[str, str] | None: - """Update unique ID of entity entry.""" - if not entity_entry.unique_id.startswith(gateway_serial): - # belongs to other device/gateway - return None - if entity_entry.unique_id.startswith(f"{gateway_serial}_"): - # Already correct, nothing to do - return None - - unique_id_parts = entity_entry.unique_id.split("-") - unique_id_parts[0] = new_identifier - - # convert climate entity unique id from `-` to `-heating-` - if entity_entry.domain == DOMAIN_CLIMATE: - unique_id_parts[len(unique_id_parts) - 1] = ( - f"{entity_entry.translation_key}-{unique_id_parts[len(unique_id_parts)-1]}" - ) - - entity_new_unique_id = "-".join(unique_id_parts) - - _LOGGER.debug( - "Migrating entity %s from %s to new id %s", - entity_entry.entity_id, - entity_entry.unique_id, - entity_new_unique_id, - ) - return {"new_unique_id": entity_new_unique_id} - - # Migrate entities - await er.async_migrate_entries(hass, entry.entry_id, _update_unique_id) + _LOGGER.debug( + "Migrating entity %s to new unique id %s", + entity_entry.name, + entity_new_unique_id, + ) + entity_registry.async_update_entity( + entity_id=entity_entry.entity_id, new_unique_id=entity_new_unique_id + ) def get_supported_devices( diff --git a/homeassistant/components/vicare/binary_sensor.py b/homeassistant/components/vicare/binary_sensor.py index 7fe248fa266..55f0ab96ed0 100644 --- a/homeassistant/components/vicare/binary_sensor.py +++ b/homeassistant/components/vicare/binary_sensor.py @@ -31,7 +31,13 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity from .types import ViCareDevice, ViCareRequiredKeysMixin -from .utils import get_burners, get_circuits, get_compressors, is_supported +from .utils import ( + get_burners, + get_circuits, + get_compressors, + get_device_serial, + is_supported, +) _LOGGER = logging.getLogger(__name__) @@ -116,6 +122,7 @@ def _build_entities( entities.extend( ViCareBinarySensor( description, + get_device_serial(device.api), device.config, device.api, ) @@ -131,6 +138,7 @@ def _build_entities( entities.extend( ViCareBinarySensor( description, + get_device_serial(device.api), device.config, device.api, component, @@ -166,12 +174,15 @@ class ViCareBinarySensor(ViCareEntity, BinarySensorEntity): def __init__( self, description: ViCareBinarySensorEntityDescription, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, component: PyViCareHeatingDeviceComponent | None = None, ) -> None: """Initialize the sensor.""" - super().__init__(description.key, device_config, device, component) + super().__init__( + description.key, device_serial, device_config, device, component + ) self.entity_description = description @property diff --git a/homeassistant/components/vicare/button.py b/homeassistant/components/vicare/button.py index 51a763c1fcc..49d142c1edb 100644 --- a/homeassistant/components/vicare/button.py +++ b/homeassistant/components/vicare/button.py @@ -24,7 +24,7 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity from .types import ViCareDevice, ViCareRequiredKeysMixinWithSet -from .utils import is_supported +from .utils import get_device_serial, is_supported _LOGGER = logging.getLogger(__name__) @@ -55,6 +55,7 @@ def _build_entities( return [ ViCareButton( description, + get_device_serial(device.api), device.config, device.api, ) @@ -88,11 +89,12 @@ class ViCareButton(ViCareEntity, ButtonEntity): def __init__( self, description: ViCareButtonEntityDescription, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, ) -> None: """Initialize the button.""" - super().__init__(description.key, device_config, device) + super().__init__(description.key, device_serial, device_config, device) self.entity_description = description def press(self) -> None: diff --git a/homeassistant/components/vicare/climate.py b/homeassistant/components/vicare/climate.py index 410395760ea..b742ad257fa 100644 --- a/homeassistant/components/vicare/climate.py +++ b/homeassistant/components/vicare/climate.py @@ -40,7 +40,7 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity from .types import HeatingProgram, ViCareDevice -from .utils import get_burners, get_circuits, get_compressors +from .utils import get_burners, get_circuits, get_compressors, get_device_serial _LOGGER = logging.getLogger(__name__) @@ -87,6 +87,7 @@ def _build_entities( """Create ViCare climate entities for a device.""" return [ ViCareClimate( + get_device_serial(device.api), device.config, device.api, circuit, @@ -143,12 +144,15 @@ class ViCareClimate(ViCareEntity, ClimateEntity): def __init__( self, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, circuit: PyViCareHeatingCircuit, ) -> None: """Initialize the climate device.""" - super().__init__(self._attr_translation_key, device_config, device, circuit) + super().__init__( + self._attr_translation_key, device_serial, device_config, device, circuit + ) self._device = device self._attributes: dict[str, Any] = {} self._attributes["vicare_programs"] = self._api.getPrograms() diff --git a/homeassistant/components/vicare/entity.py b/homeassistant/components/vicare/entity.py index f48243e83e1..dfb8c48dfc3 100644 --- a/homeassistant/components/vicare/entity.py +++ b/homeassistant/components/vicare/entity.py @@ -20,14 +20,16 @@ class ViCareEntity(Entity): def __init__( self, unique_id_suffix: str, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, component: PyViCareHeatingDeviceComponent | None = None, ) -> None: """Initialize the entity.""" gateway_serial = device_config.getConfig().serial - device_serial = device.getSerial() - identifier = f"{gateway_serial}_{device_serial}" + device_id = device_config.getId() + + identifier = f"{gateway_serial}_{device_serial if device_serial is not None else device_id}" self._api: PyViCareDevice | PyViCareHeatingDeviceComponent = ( component if component else device diff --git a/homeassistant/components/vicare/fan.py b/homeassistant/components/vicare/fan.py index d7dbd037b56..b787de20773 100644 --- a/homeassistant/components/vicare/fan.py +++ b/homeassistant/components/vicare/fan.py @@ -29,6 +29,7 @@ from homeassistant.util.percentage import ( from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity +from .utils import get_device_serial _LOGGER = logging.getLogger(__name__) @@ -100,7 +101,7 @@ async def async_setup_entry( async_add_entities( [ - ViCareFan(device.config, device.api) + ViCareFan(get_device_serial(device.api), device.config, device.api) for device in device_list if isinstance(device.api, PyViCareVentilationDevice) ] @@ -125,11 +126,14 @@ class ViCareFan(ViCareEntity, FanEntity): def __init__( self, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, ) -> None: """Initialize the fan entity.""" - super().__init__(self._attr_translation_key, device_config, device) + super().__init__( + self._attr_translation_key, device_serial, device_config, device + ) def update(self) -> None: """Update state of fan.""" diff --git a/homeassistant/components/vicare/number.py b/homeassistant/components/vicare/number.py index a7f679f7224..529caca6a87 100644 --- a/homeassistant/components/vicare/number.py +++ b/homeassistant/components/vicare/number.py @@ -33,7 +33,7 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity from .types import HeatingProgram, ViCareDevice, ViCareRequiredKeysMixin -from .utils import get_circuits, is_supported +from .utils import get_circuits, get_device_serial, is_supported _LOGGER = logging.getLogger(__name__) @@ -279,6 +279,7 @@ def _build_entities( entities.extend( ViCareNumber( description, + get_device_serial(device.api), device.config, device.api, ) @@ -289,6 +290,7 @@ def _build_entities( entities.extend( ViCareNumber( description, + get_device_serial(device.api), device.config, device.api, circuit, @@ -324,12 +326,15 @@ class ViCareNumber(ViCareEntity, NumberEntity): def __init__( self, description: ViCareNumberEntityDescription, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, component: PyViCareHeatingDeviceComponent | None = None, ) -> None: """Initialize the number.""" - super().__init__(description.key, device_config, device, component) + super().__init__( + description.key, device_serial, device_config, device, component + ) self.entity_description = description @property diff --git a/homeassistant/components/vicare/sensor.py b/homeassistant/components/vicare/sensor.py index bdcb6dfa3aa..79a93ffa345 100644 --- a/homeassistant/components/vicare/sensor.py +++ b/homeassistant/components/vicare/sensor.py @@ -51,7 +51,13 @@ from .const import ( ) from .entity import ViCareEntity from .types import ViCareDevice, ViCareRequiredKeysMixin -from .utils import get_burners, get_circuits, get_compressors, is_supported +from .utils import ( + get_burners, + get_circuits, + get_compressors, + get_device_serial, + is_supported, +) _LOGGER = logging.getLogger(__name__) @@ -868,6 +874,7 @@ def _build_entities( entities.extend( ViCareSensor( description, + get_device_serial(device.api), device.config, device.api, ) @@ -883,6 +890,7 @@ def _build_entities( entities.extend( ViCareSensor( description, + get_device_serial(device.api), device.config, device.api, component, @@ -920,12 +928,15 @@ class ViCareSensor(ViCareEntity, SensorEntity): def __init__( self, description: ViCareSensorEntityDescription, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, component: PyViCareHeatingDeviceComponent | None = None, ) -> None: """Initialize the sensor.""" - super().__init__(description.key, device_config, device, component) + super().__init__( + description.key, device_serial, device_config, device, component + ) self.entity_description = description @property diff --git a/homeassistant/components/vicare/utils.py b/homeassistant/components/vicare/utils.py index 2ba5ddbfb0a..5156ea4a41e 100644 --- a/homeassistant/components/vicare/utils.py +++ b/homeassistant/components/vicare/utils.py @@ -7,7 +7,12 @@ from PyViCare.PyViCareDeviceConfig import PyViCareDeviceConfig from PyViCare.PyViCareHeatingDevice import ( HeatingDeviceWithComponent as PyViCareHeatingDeviceComponent, ) -from PyViCare.PyViCareUtils import PyViCareNotSupportedFeatureError +from PyViCare.PyViCareUtils import ( + PyViCareInvalidDataError, + PyViCareNotSupportedFeatureError, + PyViCareRateLimitError, +) +import requests from homeassistant.config_entries import ConfigEntry @@ -27,6 +32,23 @@ def get_device( )() +def get_device_serial(device: PyViCareDevice) -> str | None: + """Get device serial for device if supported.""" + try: + return device.getSerial() + except PyViCareNotSupportedFeatureError: + _LOGGER.debug("Device does not offer a 'device.serial' data point") + except PyViCareRateLimitError as limit_exception: + _LOGGER.debug("Vicare API rate limit exceeded: %s", limit_exception) + except PyViCareInvalidDataError as invalid_data_exception: + _LOGGER.debug("Invalid data from Vicare server: %s", invalid_data_exception) + except requests.exceptions.ConnectionError: + _LOGGER.debug("Unable to retrieve data from ViCare server") + except ValueError: + _LOGGER.debug("Unable to decode data from ViCare server") + return None + + def is_supported( name: str, entity_description: ViCareRequiredKeysMixin, diff --git a/homeassistant/components/vicare/water_heater.py b/homeassistant/components/vicare/water_heater.py index 621d2f2a09b..5e241c9a3be 100644 --- a/homeassistant/components/vicare/water_heater.py +++ b/homeassistant/components/vicare/water_heater.py @@ -28,7 +28,7 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DEVICE_LIST, DOMAIN from .entity import ViCareEntity from .types import ViCareDevice -from .utils import get_circuits +from .utils import get_circuits, get_device_serial _LOGGER = logging.getLogger(__name__) @@ -69,6 +69,7 @@ def _build_entities( return [ ViCareWater( + get_device_serial(device.api), device.config, device.api, circuit, @@ -108,12 +109,13 @@ class ViCareWater(ViCareEntity, WaterHeaterEntity): def __init__( self, + device_serial: str | None, device_config: PyViCareDeviceConfig, device: PyViCareDevice, circuit: PyViCareHeatingCircuit, ) -> None: """Initialize the DHW water_heater device.""" - super().__init__(circuit.id, device_config, device) + super().__init__(circuit.id, device_serial, device_config, device) self._circuit = circuit self._attributes: dict[str, Any] = {} diff --git a/tests/components/vicare/fixtures/dummy-device-no-serial.json b/tests/components/vicare/fixtures/dummy-device-no-serial.json new file mode 100644 index 00000000000..268c73f0e37 --- /dev/null +++ b/tests/components/vicare/fixtures/dummy-device-no-serial.json @@ -0,0 +1,3 @@ +{ + "data": [] +} diff --git a/tests/components/vicare/test_init.py b/tests/components/vicare/test_init.py index fea7b5985f1..62bec7f50c5 100644 --- a/tests/components/vicare/test_init.py +++ b/tests/components/vicare/test_init.py @@ -14,74 +14,78 @@ from tests.common import MockConfigEntry # Device migration test can be removed in 2025.4.0 -async def test_device_migration( +async def test_device_and_entity_migration( hass: HomeAssistant, device_registry: dr.DeviceRegistry, + entity_registry: er.EntityRegistry, mock_config_entry: MockConfigEntry, ) -> None: """Test that the device registry is updated correctly.""" - fixtures: list[Fixture] = [Fixture({"type:boiler"}, "vicare/Vitodens300W.json")] + fixtures: list[Fixture] = [ + Fixture({"type:boiler"}, "vicare/Vitodens300W.json"), + Fixture({"type:boiler"}, "vicare/dummy-device-no-serial.json"), + ] with ( patch(f"{MODULE}.vicare_login", return_value=MockPyViCare(fixtures)), patch(f"{MODULE}.PLATFORMS", [Platform.CLIMATE]), ): mock_config_entry.add_to_hass(hass) - device_registry.async_get_or_create( + # device with serial data point + device0 = device_registry.async_get_or_create( config_entry_id=mock_config_entry.entry_id, identifiers={ (DOMAIN, "gateway0"), }, + model="model0", ) - - await hass.config_entries.async_setup(mock_config_entry.entry_id) - - await hass.async_block_till_done() - - assert device_registry.async_get_device(identifiers={(DOMAIN, "gateway0")}) is None - - assert ( - device_registry.async_get_device( - identifiers={(DOMAIN, "gateway0_deviceSerialVitodens300W")} - ) - is not None - ) - - -# Entity migration test can be removed in 2025.4.0 -async def test_climate_entity_migration( - hass: HomeAssistant, - entity_registry: er.EntityRegistry, - mock_config_entry: MockConfigEntry, -) -> None: - """Test that the climate entity unique_id gets migrated correctly.""" - fixtures: list[Fixture] = [Fixture({"type:boiler"}, "vicare/Vitodens300W.json")] - with ( - patch(f"{MODULE}.vicare_login", return_value=MockPyViCare(fixtures)), - patch(f"{MODULE}.PLATFORMS", [Platform.CLIMATE]), - ): - mock_config_entry.add_to_hass(hass) - - entry1 = entity_registry.async_get_or_create( + entry0 = entity_registry.async_get_or_create( domain=Platform.CLIMATE, platform=DOMAIN, config_entry=mock_config_entry, unique_id="gateway0-0", translation_key="heating", + device_id=device0.id, ) - entry2 = entity_registry.async_get_or_create( + entry1 = entity_registry.async_get_or_create( domain=Platform.CLIMATE, platform=DOMAIN, config_entry=mock_config_entry, unique_id="gateway0_deviceSerialVitodens300W-heating-1", translation_key="heating", + device_id=device0.id, ) - entry3 = entity_registry.async_get_or_create( + # device without serial data point + device1 = device_registry.async_get_or_create( + config_entry_id=mock_config_entry.entry_id, + identifiers={ + (DOMAIN, "gateway1"), + }, + model="model1", + ) + entry2 = entity_registry.async_get_or_create( domain=Platform.CLIMATE, platform=DOMAIN, config_entry=mock_config_entry, unique_id="gateway1-0", translation_key="heating", + device_id=device1.id, + ) + # device is not provided by api + device2 = device_registry.async_get_or_create( + config_entry_id=mock_config_entry.entry_id, + identifiers={ + (DOMAIN, "gateway2"), + }, + model="model2", + ) + entry3 = entity_registry.async_get_or_create( + domain=Platform.CLIMATE, + platform=DOMAIN, + config_entry=mock_config_entry, + unique_id="gateway2-0", + translation_key="heating", + device_id=device2.id, ) await hass.config_entries.async_setup(mock_config_entry.entry_id) @@ -89,11 +93,15 @@ async def test_climate_entity_migration( await hass.async_block_till_done() assert ( - entity_registry.async_get(entry1.entity_id).unique_id + entity_registry.async_get(entry0.entity_id).unique_id == "gateway0_deviceSerialVitodens300W-heating-0" ) assert ( - entity_registry.async_get(entry2.entity_id).unique_id + entity_registry.async_get(entry1.entity_id).unique_id == "gateway0_deviceSerialVitodens300W-heating-1" ) - assert entity_registry.async_get(entry3.entity_id).unique_id == "gateway1-0" + assert ( + entity_registry.async_get(entry2.entity_id).unique_id + == "gateway1_deviceId1-heating-0" + ) + assert entity_registry.async_get(entry3.entity_id).unique_id == "gateway2-0"