From c510b642601bf7c72c92609ec368386dceaa43f8 Mon Sep 17 00:00:00 2001 From: SchiZzA Date: Sun, 26 Jul 2026 23:14:14 +0200 Subject: [PATCH] fix(review): fix(review): pick station device by entity count, drop ghost devices --- custom_components/sws12500/predecessor.py | 42 ++++++++- tests/test_predecessor_adoption.py | 106 ++++++++++++++++++++++ 2 files changed, 143 insertions(+), 5 deletions(-) diff --git a/custom_components/sws12500/predecessor.py b/custom_components/sws12500/predecessor.py index 37a666e..3cedbf5 100644 --- a/custom_components/sws12500/predecessor.py +++ b/custom_components/sws12500/predecessor.py @@ -182,17 +182,42 @@ def _adopt_devices(hass: HomeAssistant, entry: ConfigEntry, old_entry: ConfigEnt A predecessor entry can own more than one device (v2.0.0pre1 created a second one per Ecowitt channel), and only one device may hold a given identifier: passing the same - set twice raises `DeviceIdentifierCollisionError` and would fail setup for good. The - extra devices therefore keep their own identifiers and are only repointed, which - leaves them attached to this entry and reapable once they hold nothing. + set twice raises `DeviceIdentifierCollisionError` and would fail setup for good. So + exactly one of them can be the device our platforms resolve, and which one must not + be left to the order the registry happens to yield. Once the domain really changes, + nothing holds the target identifiers yet, so a per-channel row that happened to come + first would become the station device and every adopted entity would be moved onto + it, stranding the real one with the user's name, area and labels on it. The device + carrying the most entities wins instead, ties broken on `device.id` so the outcome + does not depend on storage order at all. The others keep their own identifiers and + are only repointed. + + A device carrying no entities is left alone entirely. Attaching this entry to it + would keep it alive forever: `device_registry.async_cleanup` subtracts + `references_config_entries` from the orphan set, so a device linked to any live + config entry is never reaped, and an empty device page would sit under this + integration for good. Left untouched, `async_remove(old_entry)` takes its last config + entry away and deletes it. Nothing rides on it - the remove branch of + `entity_registry.async_device_modified` only deletes entities whose `config_entry_id` + is one of the removed device's own, and by this point the adopted ones point at this + entry. """ device_registry = dr.async_get(hass) + registry = er.async_get(hass) identifiers = set(build_device_info(entry)["identifiers"]) - for device in dr.async_entries_for_config_entry(device_registry, old_entry.entry_id): + devices = dr.async_entries_for_config_entry(device_registry, old_entry.entry_id) + held = { + device.id: len(er.async_entries_for_device(registry, device.id, include_disabled_entities=True)) + for device in devices + } + populated = [device for device in devices if held[device.id]] + canonical = min(populated, key=lambda device: (-held[device.id], device.id), default=None) + + for device in populated: holder = device_registry.async_get_device(identifiers=identifiers) - if holder is not None and holder.id != device.id: + if device is not canonical or (holder is not None and holder.id != device.id): device_registry.async_update_device(device.id, add_config_entry_id=entry.entry_id) continue @@ -280,6 +305,13 @@ async def async_adopt_predecessor( "Restart Home Assistant and try again", old_entry.entry_id, ) + # Counted as left behind rather than passed over in silence. This is the + # run where *nothing* moved, so it is also where deleting the old entry + # by hand would cost the most - exactly what the repair notice is for. + registry = er.async_get(hass) + result.live.extend( + old.entity_id for old in er.async_entries_for_config_entry(registry, old_entry.entry_id) + ) continue # Scoped to this predecessor entry, then folded in: whether an entry may be diff --git a/tests/test_predecessor_adoption.py b/tests/test_predecessor_adoption.py index a5a9e6b..387de1c 100644 --- a/tests/test_predecessor_adoption.py +++ b/tests/test_predecessor_adoption.py @@ -365,6 +365,80 @@ async def test_a_second_device_does_not_collide(hass: HomeAssistant, entries) -> assert new.entry_id in kept.config_entries +async def test_the_station_device_wins_regardless_of_registry_order(hass: HomeAssistant, entries) -> None: + """Which device becomes ours must not be decided by storage order. + + Registered here in the order that punishes "first one claims the identifiers": the + single-entity channel row comes first. After the rename nothing holds the target + identifiers yet, so nothing else would stop it - and the station device, carrying the + user's own name, area and labels, would be stranded on the old-domain identifier + while every adopted entity is moved onto the channel row. + """ + old, new = entries + device_registry = dr.async_get(hass) + channel = device_registry.async_get_or_create( + config_entry_id=old.entry_id, + identifiers={(OLD_DOMAIN, "ecowitt_ch1")}, + name="Channel 1", + ) + station = device_registry.async_get_or_create( + config_entry_id=old.entry_id, + identifiers={(OLD_DOMAIN, "station")}, + name="Weather Station", + ) + ordered = [d.id for d in dr.async_entries_for_config_entry(device_registry, old.entry_id)] + assert ordered[0] == channel.id, "fixture no longer puts the smaller device first" + + _seed(hass, old, device_id=station.id) + registry = er.async_get(hass) + registry.async_get_or_create("sensor", OLD_DOMAIN, "ch1_temp", config_entry=old, device_id=channel.id) + + await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN) + + holder = device_registry.async_get_device(identifiers=set(build_device_info(new)["identifiers"])) + assert holder is not None + assert holder.id == station.id, "the device holding most of the entities has to be the one we adopt" + stayed = device_registry.async_get(channel.id) + assert stayed is not None + assert stayed.identifiers == {(OLD_DOMAIN, "ecowitt_ch1")} + + +async def test_an_empty_device_is_not_carried_over(hass: HomeAssistant, entries) -> None: + """An emptied device must not be kept alive by us, or it never goes away. + + `device_registry.async_cleanup` subtracts every device referenced by a live config + entry from the orphan set, so attaching this entry to a device that holds nothing + pins it in place permanently - an empty device page under the new integration that + no cleanup pass will ever reap. Leaving it alone lets `async_remove(old_entry)` take + its last config entry and delete it. + """ + old, new = entries + device_registry = dr.async_get(hass) + station = device_registry.async_get_or_create( + config_entry_id=old.entry_id, + identifiers={(OLD_DOMAIN,)}, # type: ignore[arg-type] + name="Weather Station", + ) + ghost = device_registry.async_get_or_create( + config_entry_id=old.entry_id, + identifiers={(OLD_DOMAIN, "ecowitt_ch1")}, + name="Channel 1", + ) + created = _seed(hass, old, device_id=station.id) + + result = await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN) + + assert result.complete + assert device_registry.async_get(ghost.id) is None, "the empty device outlived the migration" + survived = device_registry.async_get(station.id) + assert survived is not None + registry = er.async_get(hass) + for entity_id in created.values(): + entry = registry.async_get(entity_id) + assert entry is not None + assert entry.device_id == station.id + + # --------------------------------------------------------------------------- # More than one predecessor entry # --------------------------------------------------------------------------- @@ -419,6 +493,38 @@ async def test_a_stalled_migration_raises_a_repair_issue(hass: HomeAssistant, en assert issue.translation_placeholders == {"entities": blocked} +async def test_a_predecessor_that_will_not_unload_raises_a_repair_issue( + hass: HomeAssistant, entries, monkeypatch +) -> None: + """The run where nothing at all moved is the one that most needs the warning. + + Passed over in silence, an unloadable predecessor contributes nothing to the result, + so `attempted` stays False and the notice is deleted instead of raised - leaving the + user with a leftover entry, no explanation, and the one irreversible action a click + away. + """ + from homeassistant.config_entries import ConfigEntryState + + old, new = entries + created = _seed(hass, old) + old.mock_state(hass, ConfigEntryState.LOADED) + + async def _refuse(*_a: Any, **_kw: Any) -> bool: + return False + + monkeypatch.setattr(hass.config_entries, "async_unload", _refuse) + + result = await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN) + update_predecessor_adoption_issue(hass, new, result) + + assert not result.adopted + assert sorted(result.live) == sorted(created.values()) + assert not result.complete + issue = ir.async_get(hass).async_get_issue(DOMAIN, f"predecessor_adoption_{new.entry_id}") + assert issue is not None + assert issue.translation_placeholders == {"entities": ", ".join(sorted(created.values()))} + + async def test_a_finished_migration_raises_no_issue(hass: HomeAssistant, entries) -> None: """Nothing was left behind, so there is nothing to warn about.""" old, new = entries