From 81dbc6fa03bfaae80eef8b72d674bb50186b63de Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:24:22 +0000 Subject: [PATCH 1/5] Key devices on the OCF device ID instead of the serial number Two Samsung air purifiers of the same model report the identical, well formed serialNum `BS7SP9AW400114A` (issue #381). Since the entry's unique_id, the device registry identifiers and every entity unique_id were all minted from that string, the second unit was refused as already configured, and would have collided entity-for-entity even if it hadn't been. This is the third firmware family to ship an unusable serialNum, after `Nothing(SVC)` (#83) and the flash-unset sentinel (#189), and the first one no heuristic can catch: the value is well formed, it's just shared. `is_placeholder_serial` was a dead end. So identity moves onto /oic/d's `di`, falling back to /oic/p's `pi`, then the serial, then the host. `di` is what the protocol already uses to address the endpoint -- if it were wrong or shared, OCF discovery and the DTLS association wouldn't work at all -- and it's device-scoped, where `pi` is platform-scoped and would be shared by a board hosting several logical devices. Both units in #381 report a distinct `di`. A board that answers neither resource lands exactly where it did before, so no existing hardware regresses. The re-key can't happen in async_migrate_entry: the UUID is only readable from the device, and an entry can load entirely from its snapshot while the appliance is off (#295). So v3 -> v4 only records the legacy key, and the coordinator adopts the UUID on the first live poll, rewriting the entity registry, the device registry (including subdevice identifiers) and the entry's unique_id together. Rewriting rather than recreating is what lets a user keep entity_ids, names, areas, statistics and every automation that references them. Three rules keep that adoption from misfiring: - A poll that reads no UUID never demotes a UUID-keyed entry back onto its serial, so one failed reconnect doesn't re-key every entity. - A changed UUID is followed only when the serial still corroborates it (a factory reset may regenerate `di`) or when the entry was keyed on its IP, which was never an identity to defend. - When the identity is rejected as a different appliance, the serial isn't adopted either -- otherwise the intruder would gain exactly the corroboration needed to win the next poll. Also stop redacting `di`/`pi` from diagnostics. They're randomly assigned per-unit UUIDs, not account data, and blanking them is what made the first #381 diagnostics download unable to answer the only question it was requested to answer. The owner-set device name stays redacted. Fixes #381 --- README.md | 5 +- custom_components/localthings/__init__.py | 90 ++--- custom_components/localthings/config_flow.py | 31 +- custom_components/localthings/const.py | 10 + custom_components/localthings/coordinator.py | 186 +++++++-- custom_components/localthings/entity.py | 2 +- .../localthings/registry/identity.py | 85 +++++ .../localthings/registry/redact.py | 25 +- custom_components/localthings/rekey.py | 110 ++++++ custom_components/localthings/sensor.py | 2 +- tests/localthings/conftest.py | 11 +- tests/localthings/test_config_flow.py | 94 ++++- tests/localthings/test_coordinator.py | 24 +- tests/localthings/test_device_removal.py | 2 +- tests/localthings/test_diagnostics.py | 11 +- tests/localthings/test_migration.py | 359 ++++++++++++++++-- .../localthings/test_statistics_migration.py | 24 +- tests/test_air_purifier_airflow_fan.py | 2 +- tests/test_air_purifier_vtww_fan.py | 2 +- tests/test_airconditioner_artik051_krac.py | 2 +- .../test_airconditioner_tp1x_rac_01001_fan.py | 2 +- tests/test_climate_ac_modes.py | 2 +- tests/test_entity_naming.py | 2 +- tests/test_fridge_capabilities.py | 2 +- tests/test_identity.py | 118 +++++- tests/test_range_hood_fan.py | 2 +- tests/test_redact.py | 23 +- tests/test_select_options.py | 2 +- tests/test_sensor_enum_options.py | 2 +- tests/test_sensor_hysteresis.py | 2 +- tests/test_sensor_sticky.py | 2 +- tests/test_subdevice_discovery.py | 6 +- tests/test_water_heater_ehs.py | 2 +- 33 files changed, 1073 insertions(+), 171 deletions(-) create mode 100644 custom_components/localthings/rekey.py diff --git a/README.md b/README.md index 671effc..ea6ebd9 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ This repo doesn't include the needed CA bundle. For an example of how to obtain 5. The flow sends a DTLS `ClientHello` to every port in the `49152-49160` range at once and keeps the one that answers -- a real DTLS server identifies itself in about one round trip, and the probe stops there, so nothing is left behind on the appliance. Only that port is then given a real certificate handshake: it fetches the current UUID from Samsung's cloud gateway, mints a leaf cert signed by your CA, and reads the device's identity and `/device/0`. On success it creates the config entry, already knowing the appliance's serial, model, and type. 6. Every subsequent device only asks for the host IP. The stored CA credentials are reused, and so is the leaf cert itself -- every appliance accepts the same one -- so adding a second appliance doesn't depend on Samsung's cloud being reachable at all. If a device rejects the reused cert (the UUID behind it does rotate), the flow mints a fresh one and retries by itself. -Entities appear under one HA device per appliance, named for the appliance's type and model. Rename freely: the device is keyed on its serial, not its name. +Entities appear under one HA device per appliance, named for the appliance's type and model. Rename freely: the device is keyed on the appliance's own OCF device ID, not its name. (Some Samsung models ship the same serial number on every unit of a model, so the serial can't tell two of them apart -- the OCF device ID can.) --- @@ -237,7 +237,8 @@ docker-compose.yml / ha_config/ Local HA dev environment If your appliance's type isn't recognized, or it exposes resources this integration doesn't model yet, a Repairs issue appears under Settings > System > Repairs pointing you at Settings > Devices & Services > this device > the menu > Download diagnostics. That download is already redacted of account/network identifiers (Bixby login -email, access tokens, device IDs, MAC addresses, serial numbers) before it's generated, so it's safe to attach +email, access tokens, hashed device IDs, MAC addresses, serial numbers, and the owner-set device name) before it's +generated, so it's safe to attach directly to a new issue using the linked device-support template. This is the fastest way to help add or expand support for hardware the maintainers don't have. diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index 79bc444..3f82537 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -19,6 +19,7 @@ from homeassistant.helpers.typing import ConfigType from .const import CONF_DEVICE_TYPE, CONF_HOST, CONF_PORT, CONF_SERIAL, DOMAIN, PLATFORMS from .coordinator import LocalThingsCoordinator, snapshot_store from .registry.identity import resolve_serial +from .rekey import rekey_entry from .services import async_setup_services _LOGGER = logging.getLogger(__name__) @@ -82,67 +83,20 @@ def _repair_placeholder_keys(hass: HomeAssistant, entry: ConfigEntry, serial: st """Re-key registry entries this entry minted from the placeholder identity. Before the identity moved onto the config entry, the coordinator seeded - `device_serial` with the host and only replaced it after the first poll. + its device key with the host and only replaced it after the first poll. Anything that registered in between -- the connection-mode sensor especially, added unconditionally rather than from `bound` -- was written into the registry keyed on the IP permanently, orphaned the moment the serial-keyed identity appeared (issue #236). Deleting the orphans by hand didn't help: the next restart that lost the same race recreated them. - - Rewriting beats deleting where possible -- an entity keeps its - entity_id, name, area and automations. Only possible when the - serial-keyed key is still free; where both exist the placeholder-keyed - one is the dead duplicate (unavailable since the restart that created - it), so it goes. """ host = entry.data[CONF_HOST] if serial == host: # A board with no usable serial resolves to the host, so its keys # were never placeholders. return - - ent_reg = er.async_get(hass) - stale_prefix = f"{DOMAIN}_{host}_" - for entity in list(er.async_entries_for_config_entry(ent_reg, entry.entry_id)): - if not entity.unique_id.startswith(stale_prefix): - continue - new_unique_id = f"{DOMAIN}_{serial}_{entity.unique_id[len(stale_prefix) :]}" - if ent_reg.async_get_entity_id(entity.domain, DOMAIN, new_unique_id): - _LOGGER.debug("removing orphaned entity %s", entity.entity_id) - ent_reg.async_remove(entity.entity_id) - else: - _LOGGER.debug("re-keying entity %s to %s", entity.entity_id, new_unique_id) - ent_reg.async_update_entity(entity.entity_id, new_unique_id=new_unique_id) - - dev_reg = dr.async_get(hass) - for device in list(dr.async_entries_for_config_entry(dev_reg, entry.entry_id)): - # `host` for the master, `host_` for a subdevice (device_info_for). - stale = { - ident - for ident in device.identifiers - if ident[0] == DOMAIN and (ident[1] == host or ident[1].startswith(f"{host}_")) - } - if not stale: - continue - fresh = {(DOMAIN, f"{serial}{ident[1][len(host) :]}") for ident in stale} - existing = dev_reg.async_get_device(identifiers=fresh) - if existing is not None and existing.id != device.id: - # Removing a device takes its entities with it. Anything still - # attached here was re-keyed rather than removed above -- the - # surviving copy, not a duplicate -- so move it onto the device - # it now belongs to before the removal destroys it too. - for entity in er.async_entries_for_device( - ent_reg, device.id, include_disabled_entities=True - ): - ent_reg.async_update_entity(entity.entity_id, device_id=existing.id) - _LOGGER.debug("removing orphaned device %s", device.id) - dev_reg.async_remove_device(device.id) - else: - _LOGGER.debug("re-keying device %s to %s", device.id, fresh) - dev_reg.async_update_device( - device.id, new_identifiers=(device.identifiers - stale) | fresh - ) + rekey_entry(hass, entry, host, serial) # Registries whose Dust/FineDust/SuperFineDust sensors gained pm10/pm25/pm1 @@ -249,8 +203,22 @@ async def async_migrate_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: v2 -> v3 relabels the recorded statistics for the particulate sensors, which gained a device_class/unit in the same release (issue #325). + + v3 -> v4 moves the entry off the serialNum as its identity and onto the + OCF device UUID (issue #381). Deliberately almost a no-op here: the + UUID lives on the device, and this runs before any I/O -- and before + the device is even known to be reachable, since an entry can load + entirely from its snapshot while the appliance is off (issue #295). + So the migration only guarantees CONF_SERIAL is populated, which is + what the coordinator rewrites *from* once a live poll finally hands it + a UUID to rewrite *to*. + + The version is still bumped now rather than at that point, because the + bump's real job is the `entry.version > 4` downgrade guard below: an + entry re-keyed onto a UUID and then loaded by a release that reads + CONF_SERIAL as the key would silently orphan every entity it has. """ - if entry.version > 3: + if entry.version > 4: return False # downgrade: this release doesn't know the newer shape if entry.version == 1: @@ -268,6 +236,28 @@ async def async_migrate_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: hass.config_entries.async_update_entry(entry, version=3) _LOGGER.debug("migrated entry %s to version 3", entry.entry_id) + if entry.version == 3: + # `or host` mirrors what the coordinator has always fallen back to, + # so the recorded legacy key is the one this entry's registry + # entries were actually minted under even if CONF_SERIAL never got + # written (a v1 entry whose migration predates it). Both being + # absent shouldn't happen for an entry the config flow created, but + # an exception raised here fails the whole entry -- so it bumps the + # version and leaves the data alone rather than taking that risk + # for a value the coordinator re-derives on its next poll anyway. + legacy_key = entry.data.get(CONF_SERIAL) or entry.data.get(CONF_HOST) + hass.config_entries.async_update_entry( + entry, + data={**entry.data, CONF_SERIAL: legacy_key} if legacy_key else entry.data, + version=4, + ) + _LOGGER.debug( + "migrated entry %s to version 4 (legacy key=%s, awaiting a poll to adopt " + "the OCF device id)", + entry.entry_id, + legacy_key, + ) + return True diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 04a0122..304e83a 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -43,6 +43,7 @@ from .const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_FINISH_TIME_HYSTERESIS_MINUTES, CONF_HOST, @@ -662,7 +663,12 @@ def _read_device(sess, host: str, port: int) -> dict: from .registry.batch import parse_device0_batch from .registry.by_type import resolve as resolve_registry - from .registry.identity import read_identity, resolve_model, resolve_serial + from .registry.identity import ( + read_identity, + resolve_device_key, + resolve_model, + resolve_serial, + ) identity = read_identity(sess, None) @@ -680,12 +686,19 @@ def _read_device(sess, host: str, port: int) -> dict: info = resources.get("/information/vs/0", {}) registry = resolve_registry(resources, device_types=identity.device_types) + raw_serial = info.get("x.com.samsung.da.serialNum") return { "port": port, # Resolved through the same helpers _run_discovery uses, so the device # the coordinator registers up front is the one discovery would have # produced -- no rename, and no re-key, once the first poll lands. - "serial": resolve_serial(info.get("x.com.samsung.da.serialNum"), host), + # + # `device_key` is what the entry is actually keyed on; the serial is + # kept alongside it because the coordinator corroborates a later + # change of key against it (issue #381). read_identity has already + # fetched /oic/p and /oic/d above, so this costs no extra round trip. + "device_key": resolve_device_key(identity, raw_serial, host), + "serial": resolve_serial(raw_serial, host), "model": resolve_model(info.get("x.com.samsung.da.modelNum", ""), identity), "manufacturer": identity.manufacturer or "Samsung", "device_type_name": registry.name if registry is not None else None, @@ -798,7 +811,10 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # v3 relabels the particulate sensors' recorded statistics; a freshly # created entry has none to relabel, so it starts at the migrated # version rather than walking through v2 (see async_migrate_entry). - VERSION = 3 + # v4 keys the entry on the OCF device UUID (issue #381), which the probe + # below resolves up front -- so a new entry is already on the v4 shape + # and has nothing to re-key either. + VERSION = 4 def __init__(self) -> None: self._host: str = "" @@ -817,7 +833,7 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): """Persist everything the probe resolved, identity included. The identity fields aren't decoration: the coordinator seeds - `device_serial` and its DeviceInfo from them at construction time, + `device_key` and its DeviceInfo from them at construction time, so entity unique_ids are correct from the first entity that registers, even if the first poll is slow or fails (issue #236). """ @@ -832,6 +848,7 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): CONF_CA_KEY_PEM: self._ca_key_pem, CONF_LEAF_CERT_PEM: info["leaf_cert_pem"], CONF_LEAF_KEY_PEM: info["leaf_key_pem"], + CONF_DEVICE_KEY: info["device_key"], CONF_SERIAL: info["serial"], CONF_MODEL: info["model"], CONF_MANUFACTURER: info["manufacturer"], @@ -878,7 +895,11 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.exception("Unexpected error during device probe") errors["base"] = "unknown" else: - await self.async_set_unique_id(f"localthings_{info['serial']}") + # Keyed on the OCF device UUID rather than the serialNum + # (issue #381): two units of a model that ship the same + # well-formed serial are indistinguishable here otherwise, + # and the second one is turned away as already configured. + await self.async_set_unique_id(f"localthings_{info['device_key']}") self._abort_if_unique_id_configured() if info["device_type_recognized"]: return self._create_entry(info) diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index bd4eb9b..67d3c44 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -30,6 +30,16 @@ CONF_LEAF_KEY_PEM = "leaf_key_pem" # output, the host itself for a placeholder-serial board -- issues # #83/#189), so it matches what _run_discovery computes on the first poll. CONF_SERIAL = "serial" +# The identity this entry's devices and entities are actually keyed on -- +# registry.identity.resolve_device_key's output, normally the OCF device +# UUID (issue #381). Distinct from CONF_SERIAL, which stays as (a) the key +# a pre-v4 entry was minted with, so the one-time re-key knows what to +# rewrite from, and (b) the corroborating identity that tells a device +# whose `di` rotated across a factory reset apart from a different +# appliance that moved onto this address. Absent on an entry that has not +# polled since upgrading: the OCF resources are only readable from the +# device, so the coordinator adopts this on the first live poll. +CONF_DEVICE_KEY = "device_key" CONF_MODEL = "model" CONF_MANUFACTURER = "manufacturer" CONF_DEVICE_TYPE = "device_type" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index dc80977..6c85954 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -31,6 +31,7 @@ from .const import ( CONF_BYPASS_REMOTE_CONTROL, CONF_CLOUD_COURSES, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_HOST, CONF_LEAF_CERT_PEM, @@ -66,6 +67,7 @@ from .registry.entities import ClimateDesc from .registry.identity import ( DeviceIdentity, device_display_name, + ocf_device_key, read_identity, resolve_model, resolve_serial, @@ -78,6 +80,7 @@ from .registry.subdevices import ( enumerate_subdevices, normalize_seed_batch, ) +from .rekey import rekey_entry # Sentinel for apply_cloud_courses: "leave this field as it is", # distinct from None which means "clear it". @@ -205,7 +208,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): bound: list[BoundEntity] device_info: DeviceInfo - device_serial: str + device_key: str # Class-level so tests can shrink these via patch.object() without # touching the production defaults. @@ -310,15 +313,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._push_pending = False self._push_pending_lock = threading.Lock() # Identity is resolved once by the config flow's probe (issue #236). - # device_serial mints permanent registry keys, so it must be correct + # device_key mints permanent registry keys, so it must be correct # before the first entity registers -- a placeholder corrected once # the first poll lands orphans the first device/entity pair instead. - # The host fallback covers a pre-migration entry and matches what - # resolve_serial itself returns for a placeholder-serial board - # (issues #83/#189). - self.device_serial = entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] + # + # CONF_DEVICE_KEY is what a v4 entry stores (the OCF device UUID, + # issue #381); CONF_SERIAL is both the pre-v4 key and the fallback + # for a board that answers no OCF identity resource; and the host + # covers a pre-migration entry, matching what resolve_serial itself + # returns for a placeholder-serial board (issues #83/#189). The + # chain is ordered so an entry that has not polled since upgrading + # keeps loading under the key its registry entries already carry. + self.device_key = ( + entry.data.get(CONF_DEVICE_KEY) or entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] + ) self.device_info = DeviceInfo( - identifiers={(DOMAIN, self.device_serial)}, + identifiers={(DOMAIN, self.device_key)}, name=device_display_name( entry.data.get(CONF_DEVICE_TYPE), entry.data.get(CONF_MODEL) or "" ), @@ -768,8 +778,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): else "Secondary Subdevice" ) return DeviceInfo( - identifiers={(DOMAIN, f"{self.device_serial}_{subdevice.key}")}, - via_device=(DOMAIN, self.device_serial), + identifiers={(DOMAIN, f"{self.device_key}_{subdevice.key}")}, + via_device=(DOMAIN, self.device_key), name=f"{base_name} {label}", manufacturer=self.device_info.get("manufacturer") or "Samsung", model=model or None, @@ -1093,8 +1103,120 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._skipped_subdevice_resources = skipped return kept + def _resolve_identity(self, polled_serial: str, *, from_snapshot: bool) -> tuple[str, str]: + """The (key, serial) this entry should be registered under, re-keying + the registries if the key has moved. + + Returns both together because they have to agree: the serial is what + corroborates a later change of key, so writing this poll's serial + while *defending* the stored key against a different appliance would + quietly hand that appliance the corroboration it needs to win the + next poll. The serial is therefore only adopted alongside a key we + accepted -- never on a poll whose identity we just rejected. + + The stored key wins by default: re-keying an entry that already has + registry entries orphans them unless the rewrite goes with it + (issue #236), so nothing here changes a key without calling + `rekey_entry` in the same breath. + + Four things can happen: + + * A snapshot replay (issue #295) never re-keys. That path did not + reach the device, so its "reported" identity is only whatever the + snapshot happened to preserve. + * Nothing stored (a pre-v4 entry, or a legacy migration that could + not recover an identity) -- adopt what the device reports and + rewrite the registries off the legacy key onto it. This is the + one-time move onto the OCF device UUID (issue #381), deferred to + here because the UUID is only readable from the device itself. + * A key is stored and this poll produced no UUID -- keep it. The + device saying nothing is not the device saying something + different, and demoting a UUID-keyed entry back onto its serial + because one reconnect couldn't read /oic/d would re-key every + entity the user has for the duration of an outage. + * A key is stored and the UUID differs. Either this appliance's + `di` rotated (an OCF hard reset is allowed to regenerate it) or a + *different* appliance now answers on this address. The serialNum + tells them apart: a usable one that still matches what this entry + was registered with means same unit, new UUID, so follow it. + Anything else keeps the registered identity and warns -- re-adding + is the user's call, not something a poll gets to decide. + """ + host = self._entry.data[CONF_HOST] + stored_serial = self._entry.data.get(CONF_SERIAL) + if from_snapshot: + return self.device_key, stored_serial or polled_serial + + polled_ocf = ocf_device_key(self._identity) + stored_key = self._entry.data.get(CONF_DEVICE_KEY) + + if stored_key is None: + # `polled_serial` is already resolve_serial's output, so this is + # exactly resolve_device_key's chain: the UUID where the device + # reports one, today's serial/host answer where it doesn't. + polled_key = polled_ocf or polled_serial + legacy_key = stored_serial or host + if legacy_key != polled_key: + self._log.info( + "moving this device's registry entries from %r onto %r", + legacy_key, + polled_key, + ) + rekey_entry(self.hass, self._entry, legacy_key, polled_key) + return polled_key, polled_serial + + if polled_ocf == stored_key: + # Confirmed the registered appliance, so this poll's serial is + # authoritative -- firmware is allowed to correct it. + return stored_key, polled_serial + if polled_ocf is None: + # Nothing to compare against: this may or may not be the + # registered appliance, so neither half is updated. + return stored_key, stored_serial or polled_serial + polled_key = polled_ocf + + # Excluding the host answer is what keeps this from firing on two + # *different* placeholder-serial units: `polled_serial` is already + # resolved, so anything that fell back to the address is not an + # identity and can't corroborate anything. + same_unit = polled_serial == stored_serial and polled_serial != host + # An entry keyed on the host never made an identity claim to defend + # -- it is registered against "whatever answers at this address" + # (issues #83/#189). Any real UUID is a strict improvement on that, + # so it is adopted without needing the serial to corroborate it; + # requiring corroboration would strand exactly the placeholder-serial + # boards this whole change is meant to rescue, since their serial + # resolves to the host and can never match. + if not (stored_key == host or same_unit): + self._log.warning( + "device at %s identifies as %r but this entry is registered as %r " + "(serial %r, registered %r); keeping the registered identity", + host, + polled_key, + stored_key, + polled_serial, + stored_serial, + ) + return stored_key, stored_serial or polled_serial + + # Corroborated: same unit with a new UUID (a factory reset is + # allowed to regenerate `di`), or an address-keyed entry finally + # learning a real identity. Following it keeps the user's + # entity_ids, history and automations rather than stranding them on + # a key the device will never report again. + self._log.info( + "device %s (serial %r) changed key from %r to %r; following it", + host, + polled_serial, + stored_key, + polled_key, + ) + rekey_entry(self.hass, self._entry, stored_key, polled_key) + return polled_key, polled_serial + def _persist_identity( self, + device_key: str | None, serial: str, model: str, manufacturer: str, @@ -1108,9 +1230,16 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): means the next restart names the device fully instead of renaming it again once a poll lands. + `device_key` is what the registries are keyed on; `serial` is stored + alongside it rather than replaced by it, because it is what + `_resolve_key` corroborates a changed UUID against on a later poll. + None leaves whatever key is already stored untouched -- see the + caller for why a snapshot replay must not write one. + Runs on the event loop, which async_update_entry requires. """ identity = { + **({CONF_DEVICE_KEY: device_key} if device_key is not None else {}), CONF_SERIAL: serial, CONF_MODEL: model, CONF_MANUFACTURER: manufacturer, @@ -1195,6 +1324,11 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): model=ident.get("model") or "", name=ident.get("name") or "", serial=ident.get("serial"), + # Absent from a snapshot written before these fields + # existed; _resolve_key never re-keys from a replay, so + # a missing UUID here costs nothing beyond diagnostics. + device_id=ident.get("device_id"), + platform_id=ident.get("platform_id"), device_types=tuple(ident.get("device_types") or ()), raw=ident.get("raw") or {}, ) @@ -1346,26 +1480,11 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._unbound_hrefs = unbound self._refresh_learnable_hrefs() - # The entry's stored identity wins; this poll's answer is only - # adopted when nothing is stored (a legacy migration couldn't - # recover it), then written back. Re-keying an entry with existing - # registry entries orphans them (issue #236). polled_serial = resolve_serial( info.get("x.com.samsung.da.serialNum"), self._entry.data[CONF_HOST] ) - serial = self._entry.data.get(CONF_SERIAL) or polled_serial - if serial != polled_serial: - # Same IP, different appliance (or firmware that changed what it - # reports) -- keep the registered identity; re-adding is the - # user's call. - self._log.warning( - "device at %s reports serial %r but this entry is registered " - "as %r; keeping the registered identity", - self._entry.data[CONF_HOST], - polled_serial, - serial, - ) - self.device_serial = serial + key, serial = self._resolve_identity(polled_serial, from_snapshot=from_snapshot) + self.device_key = key ident = self._identity model = resolve_model(model_num, ident) @@ -1373,12 +1492,19 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): mfr = (ident.manufacturer if ident else "") or "Samsung" self.device_info = DeviceInfo( - identifiers={(DOMAIN, serial)}, + identifiers={(DOMAIN, key)}, name=name, manufacturer=mfr, model=model, ) - self._persist_identity(serial, model, mfr, device_type_name) + # A snapshot replay passes None: it never reached the device, so it + # has no standing to write a device key. Persisting `self.device_key` + # there would freeze a pre-v4 entry's legacy key into CONF_DEVICE_KEY + # as if a poll had confirmed it, and _resolve_key would then treat + # the real UUID -- when a live poll finally produced one -- as a + # changed identity to defend against rather than the one-time + # adoption it is. + self._persist_identity(None if from_snapshot else key, serial, model, mfr, device_type_name) if not from_snapshot: # A coverage gap is a claim about what the device reports, so only # a live poll gets to make it. Replaying a snapshot would restate @@ -1392,9 +1518,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._discovered = True self._log.info( - "discovered %d entities (serial=%s) hot=%s warm=%s subdevices=%s", + "discovered %d entities (key=%s) hot=%s warm=%s subdevices=%s", len(bound), - serial, + key, self._hot_hrefs, self._warm_hrefs, [su.key for su in self.subdevices], diff --git a/custom_components/localthings/entity.py b/custom_components/localthings/entity.py index a6fc65c..c8a97b5 100644 --- a/custom_components/localthings/entity.py +++ b/custom_components/localthings/entity.py @@ -88,7 +88,7 @@ class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]): super().__init__(coordinator) self._bound = bound self._state_key = _key(bound) - self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_{self._state_key}" + self._attr_unique_id = f"{DOMAIN}_{coordinator.device_key}_{self._state_key}" if bound.desc.translation_placeholders is not None: self._attr_translation_placeholders = dict(bound.desc.translation_placeholders) elif bound.desc.use_instance_name: diff --git a/custom_components/localthings/registry/identity.py b/custom_components/localthings/registry/identity.py index c53bcdb..c9b5dcc 100644 --- a/custom_components/localthings/registry/identity.py +++ b/custom_components/localthings/registry/identity.py @@ -13,6 +13,12 @@ class DeviceIdentity: model: str name: str serial: str | None + # /oic/d's `di` and /oic/p's `pi` -- OCF's own device and platform + # UUIDs. Promoted out of `raw` into named fields because + # resolve_device_key mints permanent registry keys from them; see its + # docstring for why `di` leads. + device_id: str | None = None + platform_id: str | None = None device_types: tuple[str, ...] = () raw: dict[str, dict | list] = field(default_factory=dict) @@ -57,6 +63,83 @@ def resolve_serial(raw_serial: str | None, host: str) -> str: return s +def is_usable_device_id(value: str | None) -> bool: + """True for an OCF `di`/`pi` that actually identifies one unit. + + Firmware that never had a UUID assigned reports OCF's nil UUID + (all-zero, dashes aside), which is identical on every unit of the + family -- the #189 failure mode transplanted onto a new field, and not + something `is_placeholder_serial`'s repeated-hex-digit rule catches, + because the dashes make more than one distinct character. + + Past that, the same known-junk rules that disqualify a serialNum + disqualify a UUID: a board firmware-flashed with 'Nothing(SVC)' in one + identity field is not a board to trust in another. Anything else is + accepted as-is. A *shared but well-formed* `di` would be undetectable + here, exactly as issue #381's shared-but-well-formed serialNum is -- + heuristics can't see that, which is the whole reason this moves onto a + field the protocol itself has to keep distinct. + """ + s = (value or "").strip() + if not s: + return False + if not set(s) - {"0", "-"}: + return False + return not is_placeholder_serial(s) + + +def ocf_device_key(identity: DeviceIdentity | None) -> str | None: + """The OCF-derived half of resolve_device_key's chain, or None when the + device reported no usable UUID. + + Split out because "the device has no UUID" and "the device has this + UUID" are different answers to a caller holding an existing key: the + coordinator must never demote an entry from a UUID back onto a + serialNum just because one poll couldn't read /oic/d (a reconnect, a + timeout), which is exactly what it would do if it only saw the + collapsed string resolve_device_key returns. + + Normalized because the result is compared against its stored form on + every poll; firmware that changes case between reads would otherwise + look like a different appliance. + """ + if identity is None: + return None + for candidate in (identity.device_id, identity.platform_id): + if candidate is not None and is_usable_device_id(candidate): + return candidate.strip().lower() + return None + + +def resolve_device_key(identity: DeviceIdentity | None, raw_serial: str | None, host: str) -> str: + """The identity to mint this device's permanent registry keys from. + + Tried in order: /oic/d's `di`, /oic/p's `pi`, the serialNum, the host. + + `di` leads because it is the identifier the protocol we are already + speaking uses to address this endpoint -- if it were wrong or shared, + OCF discovery and the DTLS association would not work in the first + place. serialNum, by contrast, is a vendor-populated string no part of + the stack depends on, which is why three separate firmware families + have shipped it unusable: 'Nothing(SVC)' (#83), a flash-unset sentinel + (#189), and -- unfixable by any heuristic -- a well-formed serial + duplicated across every unit of the model (#381). + + `pi` is only the fallback despite the OCF spec calling it immutable: + it identifies the *platform*, so a board hosting more than one logical + OCF device shares one `pi` across all of them, reintroducing the very + collision this exists to prevent. `di` is device-scoped, which is the + granularity of a config entry. + + The serial stays in the chain below both so a board that answers + neither OCF resource lands exactly where it did before this existed, + and the host stays last for the same reason -- it is an address rather + than an identity (a new DHCP lease silently makes it someone else's), + so it is strictly a last resort. + """ + return ocf_device_key(identity) or resolve_serial(raw_serial, host) + + def resolve_model(model_num: str, identity: DeviceIdentity | None) -> str: """The model string to name and register a device under. @@ -143,6 +226,8 @@ def read_identity(sess, serial: str | None) -> DeviceIdentity: model=p.get("mnmo") or "", name=d.get("n") or "", serial=serial, + device_id=d.get("di") if isinstance(d.get("di"), str) else None, + platform_id=p.get("pi") if isinstance(p.get("pi"), str) else None, device_types=_device_types(d), # Kept whole rather than field-by-field: outside the /device/0 dump # diagnostics already captures, and we don't yet know which fields diff --git a/custom_components/localthings/registry/redact.py b/custom_components/localthings/registry/redact.py index 1ed8e47..a4dda45 100644 --- a/custom_components/localthings/registry/redact.py +++ b/custom_components/localthings/registry/redact.py @@ -30,13 +30,24 @@ _SENSITIVE_SUBSTRINGS = ( "secret", ) -# Matched whole, not as substrings: OCF's /oic/d and /oic/p identify the -# unit with bare one/two-letter keys too short for the substring rules above -# ('di' is a substring of 'condition', 'display', ...). 'di'/'pi' are the -# device/platform UUIDs; 'n' is /oic/d's free-text device name, which may -# carry a person's name -- the device-type signal we actually want from -# that resource is `rt`, which is not redacted. -_SENSITIVE_EXACT = frozenset({"di", "pi", "n"}) +# Matched whole, not as substrings: these are bare one/two-letter keys too +# short for the substring rules above ('n' is a substring of very nearly +# everything). 'n' is /oic/d's free-text device name, which the owner sets +# from the SmartThings app and can carry a person's name -- the device-type +# signal we actually want from that resource is `rt`, which is not redacted. +# +# OCF's /oic/d `di` and /oic/p `pi` used to be redacted here too. They are +# not account data: they're randomly-assigned per-unit UUIDs, carrying no +# more about their owner than the appliance-internal subdeviceIdList +# diagnostics already reports for the same reason (see diagnostics.py). +# Redacting them cost more than it bought -- issue #381 was two units +# colliding on a duplicated serialNum, and the first diagnostics download +# asking whether their `di`/`pi` differed came back with both values +# blanked, so the question could only be answered by walking the reporter +# through a manual read_resource call. They are now also what +# resolve_device_key mints registry keys from, so a report that hides them +# hides the identity every entity in it is named after. +_SENSITIVE_EXACT = frozenset({"n"}) def _is_sensitive_key(key: str) -> bool: diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py new file mode 100644 index 0000000..db04631 --- /dev/null +++ b/custom_components/localthings/rekey.py @@ -0,0 +1,110 @@ +"""Move an entry's registry entries from one device key to another. + +The key a device's registry entries are minted from (see +registry.identity.resolve_device_key) appears in three permanent places: +the config entry's unique_id, the device registry's identifiers, and every +entity's unique_id. Changing it therefore can't be a matter of writing a +new value and restarting -- everything already in the registries would be +orphaned, and the user would find a duplicate device whose entities all +carry a `_2` suffix, with their history, area and automations attached to +the dead copy. + +So the key change is performed *on* the registries instead. Rewriting +beats deleting: an entity keeps its entity_id, and with it its name, area, +long-term statistics and every automation and dashboard that references +it. + +Lives in its own module rather than in __init__.py because both callers +need it and they sit on opposite sides of an import edge: the v1 -> v2 +migration in __init__.py, and the coordinator's first-poll adoption of the +OCF device UUID (issue #381), which can only happen once the device has +actually been reached. +""" + +from __future__ import annotations + +import logging + +from homeassistant.config_entries import ConfigEntry +from homeassistant.core import HomeAssistant, callback +from homeassistant.helpers import device_registry as dr +from homeassistant.helpers import entity_registry as er + +from .const import DOMAIN + +_LOGGER = logging.getLogger(__name__) + + +@callback +def rekey_entry(hass: HomeAssistant, entry: ConfigEntry, old_key: str, new_key: str) -> None: + """Rewrite everything this entry registered under `old_key` to `new_key`. + + Covers all three places the key is permanent: the entity registry + (unique_ids are f"{DOMAIN}_{key}_{state_key}"), the device registry + (identifiers are (DOMAIN, key) for the device itself and + (DOMAIN, f"{key}_{subdevice}") for each subdevice of a composite + appliance -- see coordinator.device_info_for), and the config entry's + own unique_id. + + Leaving the entry's unique_id behind would half-migrate it: the config + flow's duplicate check (_abort_if_unique_id_configured) would still be + comparing new devices against the key this entry no longer uses, so + re-adding this very appliance would be waved through as a second entry. + + Idempotent: a second call finds nothing left under `old_key` and does + nothing, which is what makes it safe to attempt on every poll rather + than having to track whether it has already run. + + Where both keys somehow already exist, the `old_key` copy is the dead + one -- unavailable since whichever restart created the split -- so it + is removed rather than rewritten over the live entry. + + Must run on the event loop; the registry helpers require it. + """ + if old_key == new_key: + return + + new_unique_id = f"{DOMAIN}_{new_key}" + if entry.unique_id != new_unique_id: + hass.config_entries.async_update_entry(entry, unique_id=new_unique_id) + + ent_reg = er.async_get(hass) + stale_prefix = f"{DOMAIN}_{old_key}_" + for entity in list(er.async_entries_for_config_entry(ent_reg, entry.entry_id)): + if not entity.unique_id.startswith(stale_prefix): + continue + new_unique_id = f"{DOMAIN}_{new_key}_{entity.unique_id[len(stale_prefix) :]}" + if ent_reg.async_get_entity_id(entity.domain, DOMAIN, new_unique_id): + _LOGGER.debug("removing orphaned entity %s", entity.entity_id) + ent_reg.async_remove(entity.entity_id) + else: + _LOGGER.debug("re-keying entity %s to %s", entity.entity_id, new_unique_id) + ent_reg.async_update_entity(entity.entity_id, new_unique_id=new_unique_id) + + dev_reg = dr.async_get(hass) + for device in list(dr.async_entries_for_config_entry(dev_reg, entry.entry_id)): + stale = { + ident + for ident in device.identifiers + if ident[0] == DOMAIN and (ident[1] == old_key or ident[1].startswith(f"{old_key}_")) + } + if not stale: + continue + fresh = {(DOMAIN, f"{new_key}{ident[1][len(old_key) :]}") for ident in stale} + existing = dev_reg.async_get_device(identifiers=fresh) + if existing is not None and existing.id != device.id: + # Removing a device takes its entities with it. Anything still + # attached here was re-keyed rather than removed above -- the + # surviving copy, not a duplicate -- so move it onto the device + # it now belongs to before the removal destroys it too. + for entity in er.async_entries_for_device( + ent_reg, device.id, include_disabled_entities=True + ): + ent_reg.async_update_entity(entity.entity_id, device_id=existing.id) + _LOGGER.debug("removing orphaned device %s", device.id) + dev_reg.async_remove_device(device.id) + else: + _LOGGER.debug("re-keying device %s to %s", device.id, fresh) + dev_reg.async_update_device( + device.id, new_identifiers=(device.identifiers - stale) | fresh + ) diff --git a/custom_components/localthings/sensor.py b/custom_components/localthings/sensor.py index a7086d5..283afef 100644 --- a/custom_components/localthings/sensor.py +++ b/custom_components/localthings/sensor.py @@ -187,7 +187,7 @@ class LocalThingsConnectionModeSensor(CoordinatorEntity[LocalThingsCoordinator], def __init__(self, coordinator: LocalThingsCoordinator) -> None: super().__init__(coordinator) - self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_connection_mode" + self._attr_unique_id = f"{DOMAIN}_{coordinator.device_key}_connection_mode" @property def device_info(self) -> DeviceInfo: diff --git a/tests/localthings/conftest.py b/tests/localthings/conftest.py index 8fe0f3f..6c682f8 100644 --- a/tests/localthings/conftest.py +++ b/tests/localthings/conftest.py @@ -14,6 +14,7 @@ from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_HOST, CONF_LEAF_CERT_PEM, @@ -82,6 +83,10 @@ MOCK_PORT = 49154 # what mock_coordinator_session polls -- so an entry built from ENTRY_DATA and # the device it "reaches" agree on who they are, the same as in production. MOCK_SERIAL = "TEST-SERIAL-0000" +# The OCF device UUID (/oic/d's `di`) the probe resolves the entry's key from +# (issue #381). Distinct from MOCK_SERIAL so a test that confuses the two +# fails rather than passing by coincidence. +MOCK_DEVICE_KEY = "7b1f0c9e-2a44-4d6b-9f10-4c8e2b5a0d31" MOCK_MODEL = "TEST-MODEL" MOCK_DEVICE_TYPE = "refrigerator" MOCK_CA_CERT_PEM = "-----BEGIN CERTIFICATE-----\nTEST-CA\n-----END CERTIFICATE-----" @@ -98,6 +103,7 @@ ENTRY_DATA = { CONF_LEAF_KEY_PEM: MOCK_LEAF_KEY_PEM, # Identity the config flow's probe resolved (issue #236) -- what the # coordinator keys its devices and entities on from construction. + CONF_DEVICE_KEY: MOCK_DEVICE_KEY, CONF_SERIAL: MOCK_SERIAL, CONF_MODEL: MOCK_MODEL, CONF_MANUFACTURER: "Samsung", @@ -131,6 +137,7 @@ def fridge_resources(): def _probe_result(*, recognized: bool) -> dict: return { "port": MOCK_PORT, + "device_key": MOCK_DEVICE_KEY, "serial": MOCK_SERIAL, "model": MOCK_MODEL, "manufacturer": "Samsung", @@ -244,8 +251,8 @@ def mock_entry(hass): entry = MockConfigEntry( domain=DOMAIN, data=ENTRY_DATA, - unique_id=f"localthings_{MOCK_SERIAL}", - version=2, + unique_id=f"localthings_{MOCK_DEVICE_KEY}", + version=4, ) entry.add_to_hass(hass) return entry diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index 0a89c76..afd6596 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -16,11 +16,13 @@ from custom_components.localthings.const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_HOST, CONF_LEAF_CERT_PEM, CONF_LEARN_MODES, CONF_LEARNED_MODES, CONF_PORT, + CONF_SERIAL, DOMAIN, ) @@ -28,11 +30,13 @@ from .conftest import ( ENTRY_DATA, MOCK_CA_CERT_PEM, MOCK_CA_KEY_PEM, + MOCK_DEVICE_KEY, MOCK_HOST, MOCK_LEAF_CERT_PEM, MOCK_MODEL, MOCK_PORT, MOCK_SERIAL, + _probe_result, ) @@ -1047,7 +1051,11 @@ async def test_unknown_type_step_description_makes_no_version_claim( async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None: - """Second add of same serial: flow aborts. + """Second add of the same *device key*: flow aborts. + + Keyed on the OCF device UUID rather than the serialNum (issue #381), so + this is now the check that two genuinely distinct units can no longer + trip -- see test_same_serial_on_two_units_is_not_a_duplicate. When a device already exists the form only asks for host (CA creds are reused), so we only submit CONF_HOST in the second configure call. @@ -1055,7 +1063,7 @@ async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None existing = MockConfigEntry( domain=DOMAIN, data=ENTRY_DATA, - unique_id=f"localthings_{MOCK_SERIAL}", + unique_id=f"localthings_{MOCK_DEVICE_KEY}", ) existing.add_to_hass(hass) @@ -1069,6 +1077,88 @@ async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None assert result["reason"] == "already_configured" +async def test_same_serial_on_two_units_is_not_a_duplicate(hass: HomeAssistant, mock_probe) -> None: + """Issue #381: two Samsung air purifiers of one model ship the identical, + well-formed serialNum, so keying the entry on it turned the second one + away as already configured. The OCF device UUID differs between them, + and it is what the entry is keyed on now, so both can be added. + + Deliberately holds the serial *constant* across the two probes and varies + only the device key -- the exact shape of the bug report. + """ + first = MockConfigEntry( + domain=DOMAIN, + data={**ENTRY_DATA, CONF_HOST: "192.168.0.3"}, + unique_id=f"localthings_{MOCK_DEVICE_KEY}", + ) + first.add_to_hass(hass) + + second_probe = { + **_probe_result(recognized=True), + "device_key": "3771f8bf-c184-3a2d-d885-e4c9818736d2", + "serial": MOCK_SERIAL, + } + with patch( + "custom_components.localthings.config_flow._probe_and_validate", + return_value=second_probe, + ): + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: "192.168.0.14"} + ) + + assert result["type"] == FlowResultType.CREATE_ENTRY + assert result["data"][CONF_DEVICE_KEY] == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + # Both entries exist, and the shared serial is still recorded on each -- + # it is what corroborates a later change of key. + assert len(hass.config_entries.async_entries(DOMAIN)) == 2 + assert result["data"][CONF_SERIAL] == first.data[CONF_SERIAL] + + +def test_probe_reads_the_device_key_from_oic_d_without_an_extra_round_trip(monkeypatch): + """`_read_device` already fetches /oic/p and /oic/d for the device-type + signal, so keying on the OCF UUID costs no additional GET -- it reads + the identity that call already returned.""" + from custom_components.localthings.config_flow import _read_device + + device0 = [ + {"rt": ["x.com.samsung.devcol"]}, + { + "href": "/information/vs/0", + "rep": { + "x.com.samsung.da.modelNum": "AVT-WW-TP1-23-AXX500|10251941", + "x.com.samsung.da.serialNum": "BS7SP9AW400114A", + }, + }, + ] + + class _Session: + def __init__(self): + self.paths = [] + + def get(self, path, timeout=10.0): + self.paths.append(tuple(path)) + table = { + ("oic", "p"): {"mnmn": "Samsung Electronics", "pi": "PLATFORM-UUID"}, + ("oic", "d"): {"di": "CCFD73B3-AEB4-792A-1100-68F06F5D603B"}, + ("device", "0"): device0, + } + body = table.get(tuple(path)) + if body is None: + return 0x84, b"" + import cbor2 + + return 0x45, cbor2.dumps(body) + + sess = _Session() + info = _read_device(sess, "192.168.0.3", MOCK_PORT) + + assert info["device_key"] == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + assert info["serial"] == "BS7SP9AW400114A" + # Exactly the three reads the probe already made before this change. + assert sess.paths == [("oic", "p"), ("oic", "d"), ("oic", "res"), ("device", "0")] + + def test_probe_marks_washer_as_recognized(monkeypatch): """A washer reports no oneUiVersion at all -- its consumer-model code must still resolve so setup doesn't warn about an unrecognized type.""" diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 2429e3c..ed1844e 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -33,7 +33,13 @@ from custom_components.localthings.registry.capabilities.common import ( remote_control_required_for_write, ) -from .conftest import ENTRY_DATA, MOCK_MODEL, MOCK_SERIAL, FakeObserveSession +from .conftest import ( + ENTRY_DATA, + MOCK_DEVICE_KEY, + MOCK_MODEL, + MOCK_SERIAL, + FakeObserveSession, +) from .conftest import _load_fridge_resources as _load_fridge @@ -150,7 +156,7 @@ def test_run_discovery_falls_back_to_host_for_placeholder_serial( ) -> None: """Issue #83: the ARTIK051_DONGLE_REF firmware family reports the literal string 'Nothing(SVC)' as serialNum on every unit. Left as-is, - two such units get the same device_serial (which feeds both the HA + two such units get the same device_key (which feeds both the HA device-registry identifier and every entity's unique_id), so the second one's entities silently collide and get dropped. It must be treated the same as an empty serial and fall back to the host.""" @@ -164,7 +170,7 @@ def test_run_discovery_falls_back_to_host_for_placeholder_serial( } coordinator = LocalThingsCoordinator(hass, legacy_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == legacy_entry.data[CONF_HOST] + assert coordinator.device_key == legacy_entry.data[CONF_HOST] def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( @@ -175,7 +181,7 @@ def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( character the same repeated hex digit. A washer and a dryer, two different physical units, both reported the literal serialNum 'FFFFFFFFFFFFFFF', so without this fallback they'd collide on - device_serial exactly like the #83 case above.""" + device_key exactly like the #83 case above.""" resources = { "/information/vs/0": { "x.com.samsung.da.modelNum": "DA_WM_A51_20_COMMON|20221341|30010102001211000103000000000000", # noqa: E501 @@ -186,7 +192,7 @@ def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( } coordinator = LocalThingsCoordinator(hass, legacy_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == legacy_entry.data[CONF_HOST] + assert coordinator.device_key == legacy_entry.data[CONF_HOST] # --------------------------------------------------------------------------- @@ -198,7 +204,7 @@ def test_identity_is_resolved_before_any_poll(hass: HomeAssistant, mock_entry) - """The coordinator mints registry keys from the entry's stored identity at construction time. - `device_serial` is what entity unique_ids and device identifiers are built + `device_key` is what entity unique_ids and device identifiers are built from, and those are permanent. Seeding it with the host meant anything that registered before the first poll returned -- the connection-mode sensor especially, added unconditionally rather than from `bound` -- was written @@ -207,8 +213,8 @@ def test_identity_is_resolved_before_any_poll(hass: HomeAssistant, mock_entry) - """ coordinator = LocalThingsCoordinator(hass, mock_entry) - assert coordinator.device_serial == MOCK_SERIAL - assert coordinator.device_info["identifiers"] == {(DOMAIN, MOCK_SERIAL)} + assert coordinator.device_key == MOCK_DEVICE_KEY + assert coordinator.device_info["identifiers"] == {(DOMAIN, MOCK_DEVICE_KEY)} assert coordinator.device_info["model"] == MOCK_MODEL assert coordinator.device_info["name"] == f"Samsung Refrigerator ({MOCK_MODEL})" assert mock_entry.data[CONF_HOST] not in str(coordinator.device_info["identifiers"]) @@ -239,7 +245,7 @@ def test_discovery_keeps_the_registered_identity(hass: HomeAssistant, mock_entry coordinator = LocalThingsCoordinator(hass, mock_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == MOCK_SERIAL + assert coordinator.device_key == MOCK_DEVICE_KEY def test_discovery_backfills_a_legacy_entry_identity(hass: HomeAssistant, legacy_entry) -> None: diff --git a/tests/localthings/test_device_removal.py b/tests/localthings/test_device_removal.py index 8affe32..27d6fdb 100644 --- a/tests/localthings/test_device_removal.py +++ b/tests/localthings/test_device_removal.py @@ -39,7 +39,7 @@ async def test_stale_device_can_be_removed( stale = _device( hass, mock_entry, - {(DOMAIN, f"{coordinator.device_serial}_1")}, + {(DOMAIN, f"{coordinator.device_key}_1")}, ) assert await async_remove_config_entry_device(hass, mock_entry, stale) is True diff --git a/tests/localthings/test_diagnostics.py b/tests/localthings/test_diagnostics.py index 677813b..e35a351 100644 --- a/tests/localthings/test_diagnostics.py +++ b/tests/localthings/test_diagnostics.py @@ -76,8 +76,11 @@ async def test_diagnostics_include_ocf_identity( # fields identify a device type, so nothing is dropped up front beyond # what redaction takes out. assert identity["resources"]["/oic/p"]["mnmn"] == "Samsung Electronics" - assert identity["resources"]["/oic/d"]["di"] == REDACTED - assert identity["resources"]["/oic/p"]["pi"] == REDACTED + # The OCF UUIDs are reported rather than redacted: they are what this + # entry is keyed on (issue #381), and blanking them is what made the + # first duplicate-serial report unanswerable. + assert identity["resources"]["/oic/d"]["di"] == "ab-cd-ef" + assert identity["resources"]["/oic/p"]["pi"] == "12-34-56" # The owner-settable device name is redacted; `rt` -- the reason this # block exists -- is not. assert identity["resources"]["/oic/d"]["n"] == REDACTED @@ -125,9 +128,9 @@ async def test_diagnostics_include_oic_res_links( links = diag["identity"]["resources"]["/oic/res"] assert len(links) == 2 - assert links[0]["di"] == REDACTED + assert links[0]["di"] == "aaaa-1111" assert links[0]["href"] == "/device/0" - assert links[1]["di"] == REDACTED + assert links[1]["di"] == "bbbb-2222" assert links[1]["href"] == "/device/1" assert links[1]["rt"] == ["x.com.samsung.devcol", "oic.wk.col"] diff --git a/tests/localthings/test_migration.py b/tests/localthings/test_migration.py index 49a9152..8c4788a 100644 --- a/tests/localthings/test_migration.py +++ b/tests/localthings/test_migration.py @@ -1,13 +1,17 @@ -"""Config-entry migration and the placeholder-identity repair (issue #236).""" +"""Config-entry migration, the placeholder-identity repair (issue #236), and +the move onto the OCF device UUID (issue #381).""" from __future__ import annotations +from unittest.mock import patch + from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( + CONF_DEVICE_KEY, CONF_HOST, CONF_SERIAL, DOMAIN, @@ -39,36 +43,37 @@ async def test_migration_recovers_serial_from_unique_id( await hass.async_block_till_done() # Straight through to the current version: v2 -> v3 is a statistics - # relabel that no-ops for a family without particulate sensors. - assert entry.version == 3 + # relabel that no-ops for a family without particulate sensors, and + # v3 -> v4 only records the legacy key for the coordinator to re-key + # from once a poll produces an OCF device id. + assert entry.version == 4 assert entry.data[CONF_SERIAL] == MOCK_SERIAL -async def test_migration_collapses_the_host_port_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_collapses_the_host_port_unique_id(hass: HomeAssistant) -> None: """A board with no usable serial (issues #83/#189) used to be keyed two different ways at once: `host:port` on the config entry, `host` in the device and entity registries. Migration collapses the entry onto the registry's form, so the two finally name the same thing.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_{MOCK_HOST}:{MOCK_PORT}") - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" -async def test_migration_resolves_a_placeholder_serial_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_resolves_a_placeholder_serial_unique_id(hass: HomeAssistant) -> None: """An entry created before the placeholder rules landed was keyed on the placeholder itself (issues #83/#189), while the coordinator has been resolving those boards to the host ever since. The unique_id records what the flow believed then, not what the registry holds -- taking it at face value would re-key working devices back onto a string every unit of the family reports, which is the collision those issues are about.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_Nothing(SVC)") dev_reg = dr.async_get(hass) device = dev_reg.async_get_or_create( @@ -76,8 +81,7 @@ async def test_migration_resolves_a_placeholder_serial_unique_id( identifiers={(DOMAIN, MOCK_HOST)}, ) - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" @@ -86,14 +90,13 @@ async def test_migration_resolves_a_placeholder_serial_unique_id( assert unchanged.identifiers == {(DOMAIN, MOCK_HOST)} -async def test_migration_resolves_an_all_hex_placeholder_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_resolves_an_all_hex_placeholder_unique_id(hass: HomeAssistant) -> None: """The issue #189 flash-unset sentinel, same reasoning.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_FFFFFFFFFFFFFFF") - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST @@ -231,13 +234,13 @@ async def test_migration_removes_an_orphan_that_is_already_duplicated( assert dev_reg.async_get(real_device.id) is not None -async def test_migration_leaves_a_host_identity_device_alone( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_leaves_a_host_identity_device_alone(hass: HomeAssistant) -> None: """A board whose serial resolves *to* the host was never keyed on a placeholder -- its host-keyed device is the real one, and re-keying or removing it would orphan a working device to fix a problem it doesn't have.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_{MOCK_HOST}") dev_reg = dr.async_get(hass) device = dev_reg.async_get_or_create( @@ -245,8 +248,7 @@ async def test_migration_leaves_a_host_identity_device_alone( identifiers={(DOMAIN, MOCK_HOST)}, ) - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST unchanged = dev_reg.async_get(device.id) @@ -259,7 +261,7 @@ async def test_migration_rejects_a_future_entry_version(hass: HomeAssistant) -> written by a newer release.""" from custom_components.localthings import async_migrate_entry - entry = MockConfigEntry(domain=DOMAIN, data=LEGACY_ENTRY_DATA, version=4) + entry = MockConfigEntry(domain=DOMAIN, data=LEGACY_ENTRY_DATA, version=5) entry.add_to_hass(hass) assert await async_migrate_entry(hass, entry) is False @@ -277,3 +279,312 @@ async def test_migration_without_a_unique_id_falls_back_to_host(hass: HomeAssist assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == entry.data[CONF_HOST] assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" + + +# --------------------------------------------------------------------------- +# v3 -> v4: onto the OCF device UUID (issue #381) +# --------------------------------------------------------------------------- + +# The two purifiers from issue #381: one serialNum, two device UUIDs. +SHARED_SERIAL = "BS7SP9AW400114A" +UUID_A = "ccfd73b3-aeb4-792a-1100-68f06f5d603b" +UUID_B = "3771f8bf-c184-3a2d-d885-e4c9818736d2" + + +def _identity_reporting(device_id: str | None): + """A patched _connect_session that hands the coordinator the identity a + real DTLS connect would have read off /oic/p and /oic/d.""" + from custom_components.localthings.registry.identity import DeviceIdentity + + def _connect(self): + self._identity = ( + None + if device_id is None + else DeviceIdentity( + manufacturer="Samsung Electronics", + model="AVT-WW-TP1-23-AXX500", + name="Samsung AirPurifier", + serial=None, + device_id=device_id, + ) + ) + + return patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session", + _connect, + ) + + +def _v3_entry(hass: HomeAssistant, key: str, *, serial: str | None = None) -> MockConfigEntry: + """An entry as it sits on disk before this release: keyed on CONF_SERIAL, + with no CONF_DEVICE_KEY.""" + data = {**LEGACY_ENTRY_DATA, CONF_SERIAL: serial if serial is not None else key} + entry = MockConfigEntry(domain=DOMAIN, data=data, unique_id=f"{DOMAIN}_{key}", version=3) + entry.add_to_hass(hass) + return entry + + +async def test_v3_entry_moves_onto_the_device_uuid_keeping_its_entity_ids( + hass: HomeAssistant, fridge_resources +) -> None: + """The whole migration promise for an existing user, asserted end to end. + + The re-key happens on the first live poll rather than in + async_migrate_entry, because the UUID is only readable from the device + and an entry can load entirely from its snapshot while the appliance is + off (issue #295). What the user must keep across it: the entity_id (and + with it the entity's name, area, long-term statistics, and every + automation and dashboard that references it), and the device row. + """ + entry = _v3_entry(hass, MOCK_SERIAL) + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_SERIAL)}, + ) + existing = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", + config_entry=entry, + device_id=device.id, + suggested_object_id="kitchen_purifier_connection", + ) + + with ( + _identity_reporting(UUID_A), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.version == 4 + assert entry.data[CONF_DEVICE_KEY] == UUID_A + # The serial is kept alongside the key, not replaced by it: it is what + # corroborates a later change of UUID. + assert entry.data[CONF_SERIAL] == MOCK_SERIAL + # All three permanent places moved together. + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + rekeyed_device = dev_reg.async_get(device.id) + assert rekeyed_device is not None + assert rekeyed_device.identifiers == {(DOMAIN, UUID_A)} + kept = ent_reg.async_get(existing.entity_id) + assert kept is not None + assert kept.entity_id == "sensor.kitchen_purifier_connection" + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + # And nothing is left behind on the old key. + assert dev_reg.async_get_device(identifiers={(DOMAIN, MOCK_SERIAL)}) is None + + +async def test_a_host_keyed_entry_adopts_a_real_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """A placeholder-serial board (issues #83/#189) was keyed on its IP, which + is an address rather than an identity -- a new DHCP lease silently makes + it someone else's. Such an entry never made an identity claim to defend, + so a real UUID is adopted without needing the serial to corroborate it; + requiring corroboration would strand exactly these boards, since their + serial resolves to the host and can never match.""" + entry = _v3_entry(hass, MOCK_HOST) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_HOST)}, + ) + + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + rekeyed = dev_reg.async_get(device.id) + assert rekeyed is not None + assert rekeyed.identifiers == {(DOMAIN, UUID_B)} + + +async def test_a_poll_that_reads_no_uuid_does_not_demote_a_keyed_entry( + hass: HomeAssistant, fridge_resources +) -> None: + """The device saying nothing is not the device saying something different. + + A reconnect that can't read /oic/d (a timeout, a firmware hiccup) must + leave the key alone -- demoting back onto the serial would re-key every + entity the user has for the duration of an outage, and re-key them all + back afterwards.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + + with ( + _identity_reporting(None), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + + +async def test_a_rotated_uuid_on_the_same_serial_is_followed( + hass: HomeAssistant, fridge_resources +) -> None: + """OCF permits a hard factory reset to regenerate `di`. The serialNum is + what tells that apart from a different appliance moving onto the address, + and following it keeps the user's history rather than stranding it on a + UUID the device will never report again.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, UUID_A)}, + ) + + # fridge_resources reports MOCK_SERIAL, matching what the entry stored. + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + rekeyed = dev_reg.async_get(device.id) + assert rekeyed is not None + assert rekeyed.identifiers == {(DOMAIN, UUID_B)} + + +async def test_a_different_appliance_on_the_same_address_keeps_the_registered_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """Neither the UUID nor the serial matches what this entry was registered + with, so this is a different appliance answering at this address -- not a + reset of the registered one. Re-keying here would hand one appliance's + entities, history and automations to another; re-adding is the user's + call.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + **LEGACY_ENTRY_DATA, + CONF_SERIAL: "SOME-OTHER-APPLIANCE", + CONF_DEVICE_KEY: UUID_A, + }, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, UUID_A)}, + ) + + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_A + unchanged = dev_reg.async_get(device.id) + assert unchanged is not None + assert unchanged.identifiers == {(DOMAIN, UUID_A)} + # The rejected appliance's serial is not written either. The serial is + # what corroborates a later change of key, so adopting it here would + # hand the intruder exactly the corroboration it needs to win the *next* + # poll -- defending the identity once and then surrendering it on the + # following cycle. + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + coordinator = hass.data[DOMAIN][entry.entry_id] + coordinator._run_discovery(fridge_resources) + + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + +async def test_rekey_moves_subdevice_identifiers_too(hass: HomeAssistant) -> None: + """A composite appliance (issue #177) registers one device per logical + subdevice, keyed f"{key}_{subdevice}" and linked via_device to the + master's bare key. Rewriting only the exact-match identifier would strand + every sibling, so the prefix form moves with it. + + Calls rekey_entry directly: what it leaves behind is the contract, and + going through a setup would let the platforms re-adding their entities + hide a row that had in fact been orphaned. + """ + from custom_components.localthings.rekey import rekey_entry + + entry = _v3_entry(hass, MOCK_SERIAL) + dev_reg = dr.async_get(hass) + master = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_SERIAL)}, + ) + sub = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, f"{MOCK_SERIAL}_subdevice_1")}, + ) + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + assert dev_reg.async_get(master.id).identifiers == {(DOMAIN, UUID_A)} + assert dev_reg.async_get(sub.id).identifiers == {(DOMAIN, f"{UUID_A}_subdevice_1")} + + +async def test_rekey_is_idempotent(hass: HomeAssistant) -> None: + """Safe to attempt on every poll rather than having to track whether it + has already run -- a second call finds nothing under the old key.""" + from custom_components.localthings.rekey import rekey_entry + + entry = _v3_entry(hass, MOCK_SERIAL) + ent_reg = er.async_get(hass) + existing = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", config_entry=entry + ) + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + kept = ent_reg.async_get(existing.entity_id) + assert kept is not None + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" diff --git a/tests/localthings/test_statistics_migration.py b/tests/localthings/test_statistics_migration.py index 27fd7cd..d0da6e7 100644 --- a/tests/localthings/test_statistics_migration.py +++ b/tests/localthings/test_statistics_migration.py @@ -72,7 +72,7 @@ async def test_relabels_every_particulate_sensor(hass: HomeAssistant) -> None: # µg/m³ has a converter, so the class must be named, not None -- # passing neither is deprecated and breaks in HA Core 2026.11. assert call.kwargs["new_unit_class"] == "concentration" - assert entry.version == 3 + assert entry.version == 4 async def test_leaves_other_sensors_on_the_same_device_alone(hass: HomeAssistant) -> None: @@ -102,7 +102,7 @@ async def test_skips_families_that_did_not_gain_the_unit(hass: HomeAssistant) -> assert await async_migrate_entry(hass, entry) is True assert relabel.call_args_list == [], device_type - assert entry.version == 3 + assert entry.version == 4 async def test_defers_rather_than_consuming_the_migration_without_the_recorder( @@ -127,7 +127,7 @@ async def test_defers_rather_than_consuming_the_migration_without_the_recorder( assert await async_migrate_entry(hass, entry) is True assert len(relabel.call_args_list) == 1 - assert entry.version == 3 + assert entry.version == 4 async def test_omits_unit_class_on_an_older_home_assistant(hass: HomeAssistant) -> None: @@ -151,7 +151,7 @@ async def test_omits_unit_class_on_an_older_home_assistant(hass: HomeAssistant) assert await async_migrate_entry(hass, entry) is True assert seen == [{"new_unit_of_measurement": CONCENTRATION_MICROGRAMS_PER_CUBIC_METER}] - assert entry.version == 3 + assert entry.version == 4 async def test_a_relabel_failure_never_fails_the_entry(hass: HomeAssistant) -> None: @@ -165,7 +165,7 @@ async def test_a_relabel_failure_never_fails_the_entry(hass: HomeAssistant) -> N with patch(RELABEL, autospec=True, side_effect=TypeError("older HA signature")): assert await async_migrate_entry(hass, entry) is True - assert entry.version == 3 + assert entry.version == 4 async def test_follows_a_renamed_entity_rather_than_rebuilding_its_id( @@ -214,8 +214,16 @@ async def test_matches_subdevice_prefixed_and_instanced_keys(hass: HomeAssistant async def test_a_fresh_entry_starts_at_the_migrated_version(hass: HomeAssistant) -> None: - """A newly created entry has no statistics to relabel, so the config flow - mints v3 directly rather than walking through the migration.""" + """A newly created entry has nothing either migration step needs to do -- + no statistics to relabel, and the probe already resolved its device key + (issue #381) -- so the config flow mints the current version directly + rather than walking through them. + + Pinned rather than compared to a constant on purpose: the two must be + bumped together, and a migration step added without moving the flow's + VERSION never runs at all, because Home Assistant only calls + async_migrate_entry for an entry *behind* the flow's version. + """ from custom_components.localthings.config_flow import LocalThingsConfigFlow - assert LocalThingsConfigFlow.VERSION == 3 + assert LocalThingsConfigFlow.VERSION == 4 diff --git a/tests/test_air_purifier_airflow_fan.py b/tests/test_air_purifier_airflow_fan.py index b884261..cfab918 100644 --- a/tests/test_air_purifier_airflow_fan.py +++ b/tests/test_air_purifier_airflow_fan.py @@ -12,7 +12,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-AIRFLOW-SERIAL" + device_key = "TEST-AIRFLOW-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_air_purifier_vtww_fan.py b/tests/test_air_purifier_vtww_fan.py index ac17ef3..71d2b44 100644 --- a/tests/test_air_purifier_vtww_fan.py +++ b/tests/test_air_purifier_vtww_fan.py @@ -21,7 +21,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-VTWW-SERIAL" + device_key = "TEST-VTWW-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_airconditioner_artik051_krac.py b/tests/test_airconditioner_artik051_krac.py index 4f887d2..f6487c6 100644 --- a/tests/test_airconditioner_artik051_krac.py +++ b/tests/test_airconditioner_artik051_krac.py @@ -44,7 +44,7 @@ MODEL = "ARTIK051_KRAC_18K|10193441|60010119001111010100000000000000" class _FakeCoordinator: - device_serial = "TEST-KRAC-SERIAL" + device_key = "TEST-KRAC-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_airconditioner_tp1x_rac_01001_fan.py b/tests/test_airconditioner_tp1x_rac_01001_fan.py index 9d53da6..6b44135 100644 --- a/tests/test_airconditioner_tp1x_rac_01001_fan.py +++ b/tests/test_airconditioner_tp1x_rac_01001_fan.py @@ -28,7 +28,7 @@ FIXTURE = "airconditioner_tp1x_rac_01001" class _FakeCoordinator: - device_serial = "TEST-RAC-01001-SERIAL" + device_key = "TEST-RAC-01001-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_climate_ac_modes.py b/tests/test_climate_ac_modes.py index 49c29ef..91fd39c 100644 --- a/tests/test_climate_ac_modes.py +++ b/tests/test_climate_ac_modes.py @@ -104,7 +104,7 @@ def test_fac_bora_wind_strength_codes_fit_the_standard_scale(): from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-FAC-BORA-SERIAL" + device_key = "TEST-FAC-BORA-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_entity_naming.py b/tests/test_entity_naming.py index 74b95ed..7c3ba97 100644 --- a/tests/test_entity_naming.py +++ b/tests/test_entity_naming.py @@ -10,7 +10,7 @@ from custom_components.localthings.registry.entities import BinarySensorDesc class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, last_resources=None): self.last_resources = last_resources or {} diff --git a/tests/test_fridge_capabilities.py b/tests/test_fridge_capabilities.py index 6ad4e4b..267c834 100644 --- a/tests/test_fridge_capabilities.py +++ b/tests/test_fridge_capabilities.py @@ -463,7 +463,7 @@ class TestKimchiZone: ) class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, resources, data): self.last_resources = resources diff --git a/tests/test_identity.py b/tests/test_identity.py index 654e0c7..36d17ea 100644 --- a/tests/test_identity.py +++ b/tests/test_identity.py @@ -1,6 +1,12 @@ import cbor2 -from custom_components.localthings.registry.identity import read_identity +from custom_components.localthings.registry.identity import ( + DeviceIdentity, + is_usable_device_id, + ocf_device_key, + read_identity, + resolve_device_key, +) class FakeSession: @@ -124,3 +130,113 @@ def test_read_identity_tolerates_malformed_oic_res(): _device_types' handling of a malformed /oic/d rt.""" ident = read_identity(FakeSession({("oic", "res"): {"not": "a list"}}), serial=None) assert ident.raw["/oic/res"] == [] + + +# --------------------------------------------------------------------------- +# The device key: which identity field registry keys are minted from (#381) +# --------------------------------------------------------------------------- + + +def _identity(**kwargs) -> DeviceIdentity: + base = {"manufacturer": "Samsung", "model": "M", "name": "N", "serial": None} + return DeviceIdentity(**{**base, **kwargs}) + + +def test_read_identity_captures_the_ocf_uuids_as_named_fields(): + """`di`/`pi` are what resolve_device_key mints keys from, so they are + lifted out of `raw` rather than dug back out of it at every call site.""" + sess = FakeSession( + { + ("oic", "p"): {"pi": "ccfd73b3-aeb4-792a-1100-68f06f5d603b"}, + ("oic", "d"): {"di": "3771f8bf-c184-3a2d-d885-e4c9818736d2"}, + } + ) + ident = read_identity(sess, serial=None) + assert ident.device_id == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + assert ident.platform_id == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + + +def test_read_identity_ignores_non_string_uuids(): + """Firmware answering with a number or a map must not put a non-string + into a field that goes on to be string-formatted into a unique_id.""" + ident = read_identity(FakeSession({("oic", "d"): {"di": 42}, ("oic", "p"): {"pi": {}}}), None) + assert ident.device_id is None + assert ident.platform_id is None + + +def test_two_units_sharing_a_serial_get_distinct_keys(): + """Issue #381 exactly: two Samsung air purifiers of the same model ship + the identical, well-formed serialNum 'BS7SP9AW400114A', so keying on it + collapsed them onto one identity and the second was refused as already + configured. Their `di` differs, which is what makes them separable.""" + shared_serial = "BS7SP9AW400114A" + first = resolve_device_key( + _identity(device_id="ccfd73b3-aeb4-792a-1100-68f06f5d603b"), shared_serial, "192.168.0.3" + ) + second = resolve_device_key( + _identity(device_id="3771f8bf-c184-3a2d-d885-e4c9818736d2"), shared_serial, "192.168.0.14" + ) + assert first != second + assert shared_serial not in (first, second) + + +def test_platform_id_is_the_fallback_when_oic_d_is_unreadable(): + ident = _identity(device_id=None, platform_id="ccfd73b3-aeb4-792a-1100-68f06f5d603b") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == ( + "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + ) + + +def test_device_id_wins_over_platform_id(): + """`pi` is platform-scoped, so a board hosting more than one logical OCF + device shares it -- the collision this exists to prevent.""" + ident = _identity(device_id="dddddddd-0000-1111-2222-333333333333", platform_id="shared-plat") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == ( + "dddddddd-0000-1111-2222-333333333333" + ) + + +def test_falls_back_to_the_serial_then_the_host(): + """A board that answers neither OCF resource lands exactly where it did + before any of this existed -- no regression for existing hardware.""" + assert resolve_device_key(None, "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + assert resolve_device_key(_identity(), "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + # ...and a placeholder serial still resolves to the host (#83/#189). + assert resolve_device_key(_identity(), "Nothing(SVC)", "10.0.0.1") == "10.0.0.1" + + +def test_the_key_is_case_normalized(): + """The stored key is compared against a freshly polled one on every + poll; firmware that changed case between reads would otherwise look + like a different appliance every time.""" + ident = _identity(device_id="CCFD73B3-AEB4-792A-1100-68F06F5D603B") + assert resolve_device_key(ident, None, "10.0.0.1") == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + + +def test_the_nil_uuid_is_not_an_identity(): + """OCF's unset UUID is identical on every unit that never had one + assigned -- the #189 failure mode on a new field. Its dashes stop + is_placeholder_serial's repeated-digit rule from seeing it, so it needs + its own check; falling through to the serial is the right answer.""" + ident = _identity(device_id="00000000-0000-0000-0000-000000000000") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + assert not is_usable_device_id("00000000-0000-0000-0000-000000000000") + + +def test_known_junk_disqualifies_a_uuid_the_same_way_it_does_a_serial(): + """A board firmware-flashed with 'Nothing(SVC)' in one identity field is + not a board to trust in another.""" + assert not is_usable_device_id("Nothing(SVC)") + assert not is_usable_device_id("FFFFFFFFFFFFFFF") + assert not is_usable_device_id("") + assert not is_usable_device_id(None) + assert is_usable_device_id("3771f8bf-c184-3a2d-d885-e4c9818736d2") + + +def test_ocf_device_key_reports_absence_rather_than_collapsing_to_the_serial(): + """The coordinator needs "the device said nothing" and "the device said + this" to be different answers, so it never demotes a UUID-keyed entry + onto a serial because one poll couldn't read /oic/d.""" + assert ocf_device_key(None) is None + assert ocf_device_key(_identity(serial="REAL-SERIAL")) is None + assert ocf_device_key(_identity(device_id="abc-123")) == "abc-123" diff --git a/tests/test_range_hood_fan.py b/tests/test_range_hood_fan.py index 4eafe92..d102408 100644 --- a/tests/test_range_hood_fan.py +++ b/tests/test_range_hood_fan.py @@ -11,7 +11,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-HOOD-SERIAL" + device_key = "TEST-HOOD-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_redact.py b/tests/test_redact.py index 733972d..0585e95 100644 --- a/tests/test_redact.py +++ b/tests/test_redact.py @@ -75,9 +75,18 @@ def test_redact_resources_does_not_mutate_input(): assert resources["/information/vs/0"]["x.com.samsung.da.serialNum"] == original_serial -def test_redacts_bare_ocf_identity_keys(): - """/oic/d and /oic/p identify the unit with two-letter keys ('di', 'pi') - that the substring rules can't see.""" +def test_keeps_ocf_identity_uuids_but_redacts_the_owner_set_name(): + """/oic/d's `di` and /oic/p's `pi` survive redaction. + + They are randomly-assigned per-unit UUIDs, not account data, and they + are what the entry's registry keys are minted from (issue #381) -- a + report that blanks them hides the identity every entity in it is named + after, and can't answer the one question a duplicate-serial report + exists to ask: whether two units differ here at all. + + `n` is the opposite case and stays redacted: free text the owner sets + from the SmartThings app, so it can carry a person's name. + """ redacted = redact_resources( { "/oic/d": { @@ -89,12 +98,10 @@ def test_redacts_bare_ocf_identity_keys(): } ) - assert redacted["/oic/d"]["di"] == REDACTED - assert redacted["/oic/p"]["pi"] == REDACTED - # 'n' is free text the owner sets from the SmartThings app, so it can - # carry a person's name -- redacted too. `rt`, the device-type signal - # we actually want out of /oic/d, is not. + assert redacted["/oic/d"]["di"] == "ab-cd-ef" + assert redacted["/oic/p"]["pi"] == "12-34-56" assert redacted["/oic/d"]["n"] == REDACTED + # `rt`, the device-type signal we actually want out of /oic/d, is kept. assert redacted["/oic/d"]["rt"] == ["oic.wk.d", "oic.d.refrigerator"] assert redacted["/oic/p"]["mnmo"] == "RF9000B" diff --git a/tests/test_select_options.py b/tests/test_select_options.py index 1e4ba98..93448d7 100644 --- a/tests/test_select_options.py +++ b/tests/test_select_options.py @@ -18,7 +18,7 @@ from custom_components.localthings.select import LocalThingsSelect class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, last_resources): self.last_resources = last_resources diff --git a/tests/test_sensor_enum_options.py b/tests/test_sensor_enum_options.py index 5c8c652..a98ab9f 100644 --- a/tests/test_sensor_enum_options.py +++ b/tests/test_sensor_enum_options.py @@ -46,7 +46,7 @@ class _FakeConfigEntry: class _FakeCoordinator: def __init__(self): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry() self.resources: dict[str, dict] = {} diff --git a/tests/test_sensor_hysteresis.py b/tests/test_sensor_hysteresis.py index e4b5794..eecfe60 100644 --- a/tests/test_sensor_hysteresis.py +++ b/tests/test_sensor_hysteresis.py @@ -23,7 +23,7 @@ class _FakeCoordinator: """Just enough surface for LocalThingsEntity/LocalThingsSensor.""" def __init__(self, threshold_minutes): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry( { CONF_FINISH_TIME_HYSTERESIS_MINUTES: threshold_minutes, diff --git a/tests/test_sensor_sticky.py b/tests/test_sensor_sticky.py index 9790edf..207af1d 100644 --- a/tests/test_sensor_sticky.py +++ b/tests/test_sensor_sticky.py @@ -50,7 +50,7 @@ class _FakeCoordinator: """ def __init__(self): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry() self.resources: dict[str, dict] = {} diff --git a/tests/test_subdevice_discovery.py b/tests/test_subdevice_discovery.py index a8dcb6f..01b320c 100644 --- a/tests/test_subdevice_discovery.py +++ b/tests/test_subdevice_discovery.py @@ -147,7 +147,7 @@ async def test_pattern_a_sub1_device_info_links_via_device_to_master(hass: HomeA sub1 = next(su for su in coordinator.subdevices if su.key == "1") info = coordinator.device_info_for(sub1) - master_serial = coordinator.device_serial + master_serial = coordinator.device_key assert info["identifiers"] == {(DOMAIN, f"{master_serial}_1")} assert info["via_device"] == (DOMAIN, master_serial) # The subdevice's own /information/vs/1 (real, ARTIK051_DONGLE_FAC_RAC_18K) @@ -245,7 +245,7 @@ async def test_fac_bora_2in1_subdevice_device_info(hass: HomeAssistant): subdevice = coordinator.subdevices[0] info = coordinator.device_info_for(subdevice) - master_serial = coordinator.device_serial + master_serial = coordinator.device_key assert info["identifiers"] == {(DOMAIN, f"{master_serial}_{_SUB_UUID}")} assert info["via_device"] == (DOMAIN, master_serial) # Confirmed live by the reporter (DESIGN-177.md section 1): the wall @@ -266,7 +266,7 @@ async def test_fac_bora_2in1_unique_ids_include_subdevice_prefix(hass: HomeAssis entity = LocalThingsEntity(coordinator, sub_climate) expected_slug = _SUB_UUID.replace("-", "") assert entity._attr_unique_id == ( - f"{DOMAIN}_{coordinator.device_serial}_subdevice_{expected_slug}_climate" + f"{DOMAIN}_{coordinator.device_key}_subdevice_{expected_slug}_climate" ) diff --git a/tests/test_water_heater_ehs.py b/tests/test_water_heater_ehs.py index d3dc935..5b0615b 100644 --- a/tests/test_water_heater_ehs.py +++ b/tests/test_water_heater_ehs.py @@ -20,7 +20,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-EHS-SERIAL" + device_key = "TEST-EHS-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} From 1e9fbd4ec5817957e7b6bd57a6a4f800f09e966f Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:24:22 +0000 Subject: [PATCH 2/5] Cover the identity migration against what existing users can lose The move onto the OCF device UUID rewrites the identity of registry rows that a user's automations, dashboards, history and areas all hang off, and it does so on the first live poll rather than inside async_migrate_entry. That makes it the riskiest migration in the codebase and the one least covered by its own tests, so it gets a dedicated suite organised around what must not break rather than around the functions involved. tests/localthings/test_identity_migration.py (19 tests) covers: the v3 upgrade end to end; the full v1 -> v4 walk an oldest install takes; a host-keyed placeholder-serial board adopting a real identity; issue #381's actual two-unit install migrating to separate identities; every customization carried on an entity row (rename, icon, area, hidden_by) and the device's own area; a composite appliance's subdevice identifiers and via_device links; scoping to one config entry's rows; the key-boundary match; upgrading while the appliance is off, and the deferred adoption completing when it returns; restarting after the upgrade being a no-op; and the three guards that defend an identity once it has moved. tests/test_rekey_statistics_end_to_end.py drives a real in-memory recorder to prove the claim the in-place rewrite exists to make: statistics are filed under entity_id, so preserving the row preserves the history -- including on the one path that deletes a row rather than rewriting it. It lives outside tests/localthings/ for the same reason the existing statistics end-to-end test does (that package's autouse fixture starts HA before recorder_mock can claim its database). Every guarantee was checked by mutation: ten separate breakages of the production code -- dropping the unique_id update, the subdevice prefix, the entry scoping, the key boundary, the entity rewrite, both snapshot guards, the no-demote rule, the rejected-serial rule, and recreating the row under a new entity_id -- each fail the suite. Two gaps this found and closed: - ConfigFlow.VERSION had to move to 4. Home Assistant only calls async_migrate_entry for entries behind the flow's version, so the v3 -> v4 step silently never ran. Pinned with the reasoning written down. - An offline load could re-key the registry from a stale snapshot, and freeze that answer into CONF_DEVICE_KEY so the real UUID could never be adopted afterwards. The guards existed; nothing proved they held. The identity-migration tests move out of test_migration.py, which stays about the migrations that finish inside async_migrate_entry. --- tests/localthings/test_identity_migration.py | 728 +++++++++++++++++++ tests/localthings/test_migration.py | 320 +------- tests/test_rekey_statistics_end_to_end.py | 171 +++++ 3 files changed, 905 insertions(+), 314 deletions(-) create mode 100644 tests/localthings/test_identity_migration.py create mode 100644 tests/test_rekey_statistics_end_to_end.py diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py new file mode 100644 index 0000000..6972e47 --- /dev/null +++ b/tests/localthings/test_identity_migration.py @@ -0,0 +1,728 @@ +"""Moving an existing install onto the OCF device UUID (issue #381). + +Every other migration this integration has done finishes inside +`async_migrate_entry`. This one can't: the UUID is only readable from the +appliance, and an entry can load entirely from its stored snapshot while +the appliance is off (issue #295). So the config-entry version bumps up +front and the coordinator adopts the UUID on the first *live* poll, +rewriting the entity registry, the device registry and the entry's own +unique_id together. + +That makes this the riskiest migration in the codebase: it rewrites the +identity of registry rows a user's automations, dashboards, history and +areas all hang off. The suite is therefore organised around what an +existing user must not lose, rather than around the functions involved: + +* the entity_id, and every customization carried on that registry row +* long-term statistics (keyed by entity_id -- see the end-to-end test in + tests/test_rekey_statistics_end_to_end.py, which drives a real recorder) +* the device row, its area, and a composite appliance's subdevice links +* other config entries' rows, which this must never touch +* stability: an upgrade that happens twice, or offline, must not churn +""" + +from __future__ import annotations + +from contextlib import contextmanager +from unittest.mock import patch + +from homeassistant.core import HomeAssistant +from homeassistant.helpers import device_registry as dr +from homeassistant.helpers import entity_registry as er +from pytest_homeassistant_custom_component.common import MockConfigEntry + +from custom_components.localthings.const import ( + CONF_DEVICE_KEY, + CONF_HOST, + CONF_SERIAL, + DOMAIN, +) +from custom_components.localthings.registry.identity import DeviceIdentity + +from .conftest import LEGACY_ENTRY_DATA, MOCK_HOST, MOCK_SERIAL + +_COORD = "custom_components.localthings.coordinator.LocalThingsCoordinator" + +# The two purifiers from issue #381: one serialNum, two device UUIDs. +SHARED_SERIAL = "BS7SP9AW400114A" +UUID_A = "ccfd73b3-aeb4-792a-1100-68f06f5d603b" +UUID_B = "3771f8bf-c184-3a2d-d885-e4c9818736d2" + + +@contextmanager +def _reachable(resources: dict, device_id: str | None): + """A device that answers a poll, reporting `device_id` as its /oic/d + `di` -- which `_connect_session` is what normally reads, so a test that + patches it out otherwise leaves `_identity` None (indistinguishable + from firmware that reports no UUID at all).""" + + def _connect(self) -> None: + self._identity = ( + None + if device_id is None + else DeviceIdentity( + manufacturer="Samsung Electronics", + model="AVT-WW-TP1-23-AXX500", + name="Samsung AirPurifier", + serial=None, + device_id=device_id, + ) + ) + + with ( + patch(f"{_COORD}._connect_session", _connect), + patch(f"{_COORD}._poll_once", return_value=resources), + patch(f"{_COORD}._close_session"), + ): + yield + + +@contextmanager +def _unreachable(): + with ( + patch(f"{_COORD}._connect_session"), + patch(f"{_COORD}._poll_once", side_effect=OSError("device offline")), + patch(f"{_COORD}._close_session"), + ): + yield + + +def _entry( + hass: HomeAssistant, + *, + version: int, + key: str, + serial: str | None = None, + device_key: str | None = None, + host: str = MOCK_HOST, +) -> MockConfigEntry: + """An entry as it sits on disk at `version`. Pre-v4 entries carry no + CONF_DEVICE_KEY at all -- that absence is what tells the coordinator it + is looking at an entry that has never adopted a UUID.""" + data = {**LEGACY_ENTRY_DATA, CONF_HOST: host} + if version >= 2: + data[CONF_SERIAL] = serial if serial is not None else key + if device_key is not None: + data[CONF_DEVICE_KEY] = device_key + entry = MockConfigEntry( + domain=DOMAIN, + data=data, + unique_id=f"{DOMAIN}_{key}", + version=version, + ) + entry.add_to_hass(hass) + return entry + + +def _seed_registry(hass: HomeAssistant, entry: MockConfigEntry, key: str, **entity_kwargs): + """A device and one entity keyed on `key`, as a running install has.""" + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, key)}, + name=f"Samsung Air Purifier ({key})", + ) + entity = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{key}_connection_mode", + config_entry=entry, + device_id=device.id, + **entity_kwargs, + ) + return device, entity + + +# --------------------------------------------------------------------------- +# The upgrade itself +# --------------------------------------------------------------------------- + + +async def test_v3_entry_moves_onto_the_device_uuid_keeping_its_entity_ids( + hass: HomeAssistant, fridge_resources +) -> None: + """The migration promise for an existing user, asserted end to end: all + three permanent places move together, and the entity_id doesn't.""" + entry = _entry(hass, version=3, key=MOCK_SERIAL) + device, existing = _seed_registry( + hass, entry, MOCK_SERIAL, suggested_object_id="kitchen_purifier_connection" + ) + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.version == 4 + assert entry.data[CONF_DEVICE_KEY] == UUID_A + # The serial is kept alongside the key, not replaced by it: it is what + # corroborates a later change of UUID. + assert entry.data[CONF_SERIAL] == MOCK_SERIAL + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + + rekeyed_device = dr.async_get(hass).async_get(device.id) + assert rekeyed_device is not None + assert rekeyed_device.identifiers == {(DOMAIN, UUID_A)} + kept = er.async_get(hass).async_get(existing.entity_id) + assert kept is not None + assert kept.entity_id == "sensor.kitchen_purifier_connection" + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + # Nothing left behind on the old key. + assert dr.async_get(hass).async_get_device(identifiers={(DOMAIN, MOCK_SERIAL)}) is None + + +async def test_the_oldest_install_walks_all_the_way_from_v1( + hass: HomeAssistant, fridge_resources +) -> None: + """A v1 entry -- no stored identity at all, from before issue #236 -- + walks v1 -> v2 -> v3 -> v4 and then adopts the UUID on its first poll. + + The oldest installs take the longest path, and each step rewrites what + the next one reads, so the chain is worth pinning as one journey rather + than trusting the individual steps to compose. + """ + entry = _entry(hass, version=1, key=MOCK_SERIAL) + device, existing = _seed_registry( + hass, entry, MOCK_SERIAL, suggested_object_id="old_install_connection" + ) + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.version == 4 + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + kept = er.async_get(hass).async_get(existing.entity_id) + assert kept is not None + assert kept.entity_id == "sensor.old_install_connection" + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} + + +async def test_a_host_keyed_entry_adopts_a_real_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """A placeholder-serial board (issues #83/#189) was keyed on its IP, + which is an address rather than an identity -- a new DHCP lease silently + makes it someone else's. Such an entry never made an identity claim to + defend, so a real UUID is adopted without needing the serial to + corroborate it; requiring corroboration would strand exactly these + boards, since their serial resolves to the host and can never match.""" + entry = _entry(hass, version=3, key=MOCK_HOST) + device, _ = _seed_registry(hass, entry, MOCK_HOST) + + with _reachable(fridge_resources, UUID_B): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_B)} + + +async def test_the_two_units_from_the_issue_migrate_to_separate_identities( + hass: HomeAssistant, fridge_resources +) -> None: + """Issue #381's actual install: two entries whose stored identity is the + same shared serial. Before this, they could not both exist. Migrating + them must give each its own key rather than collapsing them again -- + including their entity unique_ids, which is where issue #83's Bug 4 + silently dropped the second unit's entities even once its entry existed. + """ + first = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.3") + _, first_entity = _seed_registry(hass, first, SHARED_SERIAL, suggested_object_id="purifier_a") + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(first.entry_id) + await hass.async_block_till_done() + + # Added only now: setting up the first entry loads the integration, which + # brings up every entry already registered -- so a second one created up + # front would come up inside the first one's patched identity and adopt + # its UUID, which is the collision this test exists to disprove. + second = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.14") + _, second_entity = _seed_registry(hass, second, SHARED_SERIAL, suggested_object_id="purifier_b") + + with _reachable(fridge_resources, UUID_B): + await hass.config_entries.async_setup(second.entry_id) + await hass.async_block_till_done() + + assert first.data[CONF_DEVICE_KEY] == UUID_A + assert second.data[CONF_DEVICE_KEY] == UUID_B + assert first.unique_id != second.unique_id + + ent_reg = er.async_get(hass) + assert ent_reg.async_get(first_entity.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_A}_connection_mode" + ) + assert ent_reg.async_get(second_entity.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_B}_connection_mode" + ) + + +# --------------------------------------------------------------------------- +# What the user must not lose +# --------------------------------------------------------------------------- + + +async def test_every_user_customization_on_the_row_survives( + hass: HomeAssistant, fridge_resources +) -> None: + """Re-keying rewrites the registry row in place rather than replacing + it, which is the whole reason to do it this way -- so everything the + user attached to that row rides along. A rename, an area, an icon + override and a deliberate hide are each things they would have to redo + by hand if the row were recreated instead. + """ + entry = _entry(hass, version=3, key=MOCK_SERIAL) + ent_reg = er.async_get(hass) + device, existing = _seed_registry( + hass, entry, MOCK_SERIAL, suggested_object_id="kitchen_purifier_connection" + ) + dr.async_get(hass).async_update_device(device.id, area_id="kitchen") + ent_reg.async_update_entity( + existing.entity_id, + name="Purifier link", + icon="mdi:air-filter", + area_id="kitchen", + hidden_by=er.RegistryEntryHider.USER, + ) + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + kept = ent_reg.async_get(existing.entity_id) + assert kept is not None + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert kept.name == "Purifier link" + assert kept.icon == "mdi:air-filter" + assert kept.area_id == "kitchen" + assert kept.hidden_by is er.RegistryEntryHider.USER + assert dr.async_get(hass).async_get(device.id).area_id == "kitchen" + + +async def test_a_composite_appliance_keeps_its_subdevice_links( + hass: HomeAssistant, fridge_resources +) -> None: + """A composite appliance (issue #177) registers one device per logical + subdevice, keyed f"{key}_{subdevice}" and linked via_device to the + master's bare key. Rewriting only the exact-match identifier would + strand every sibling under a via_device pointing at a device that no + longer exists, collapsing the user's device tree.""" + entry = _entry(hass, version=3, key=MOCK_SERIAL) + dev_reg = dr.async_get(hass) + master, _ = _seed_registry(hass, entry, MOCK_SERIAL) + sub = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, f"{MOCK_SERIAL}_subdevice_1")}, + via_device=(DOMAIN, MOCK_SERIAL), + ) + assert sub.via_device_id == master.id + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert dev_reg.async_get(master.id).identifiers == {(DOMAIN, UUID_A)} + rekeyed_sub = dev_reg.async_get(sub.id) + assert rekeyed_sub.identifiers == {(DOMAIN, f"{UUID_A}_subdevice_1")} + # Still the same parent row, so the device tree the user sees is intact. + assert rekeyed_sub.via_device_id == master.id + + +async def test_a_re_key_never_touches_another_entrys_rows(hass: HomeAssistant) -> None: + """The rewrite is scoped to one config entry's own registry rows. + + Issue #381's install is the case that makes this sharp: *both* entries + are keyed on the same shared serial, so both have registry rows under + the identical old key, and they migrate one at a time. An unscoped + rewrite -- matching on the key prefix alone -- would sweep up the other + appliance's device and entities and hand them to the first one to + migrate, which is the worst outcome this change could have. + + The two entries hold *different* entities under that one shared prefix + (HA's registry won't let two rows share a unique_id, which is issue + #83's Bug 4 in the first place), so only the config-entry scoping can + tell them apart -- prefix matching alone cannot. + + Calls rekey_entry directly so the scoping is what's under test, rather + than the coordinator's decision about whether to call it at all. + """ + from custom_components.localthings.rekey import rekey_entry + + migrating = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.3") + bystander = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.14") + ent_reg = er.async_get(hass) + moved = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{SHARED_SERIAL}_connection_mode", config_entry=migrating + ) + stays = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{SHARED_SERIAL}_power", config_entry=bystander + ) + + rekey_entry(hass, migrating, SHARED_SERIAL, UUID_A) + + assert ent_reg.async_get(moved.entity_id).unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + # The other appliance is still on the shared serial, waiting its turn. + assert ent_reg.async_get(stays.entity_id).unique_id == f"{DOMAIN}_{SHARED_SERIAL}_power" + assert bystander.unique_id == f"{DOMAIN}_{SHARED_SERIAL}" + + +async def test_a_re_key_stops_at_the_key_boundary(hass: HomeAssistant) -> None: + """Matching is on the whole key or the key plus a separator, never a + bare prefix. Serial numbers of one model are routinely prefixes of each + other, so a naive startswith would drag a *different* appliance's rows + along -- and the identifiers it would rewrite them to are nonsense.""" + from custom_components.localthings.rekey import rekey_entry + + entry = _entry(hass, version=3, key="TEST-SERIAL") + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + target = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, identifiers={(DOMAIN, "TEST-SERIAL")} + ) + # Same config entry, so scoping can't save this one -- only the boundary. + neighbour = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, identifiers={(DOMAIN, "TEST-SERIAL-0000")} + ) + neighbour_entity = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_TEST-SERIAL-0000_connection_mode", config_entry=entry + ) + + rekey_entry(hass, entry, "TEST-SERIAL", UUID_A) + + assert dev_reg.async_get(target.id).identifiers == {(DOMAIN, UUID_A)} + assert dev_reg.async_get(neighbour.id).identifiers == {(DOMAIN, "TEST-SERIAL-0000")} + assert ent_reg.async_get(neighbour_entity.entity_id).unique_id == ( + f"{DOMAIN}_TEST-SERIAL-0000_connection_mode" + ) + + +# --------------------------------------------------------------------------- +# Stability: upgrading twice, or offline, must not churn +# --------------------------------------------------------------------------- + + +async def test_upgrading_while_the_appliance_is_off_changes_nothing( + hass: HomeAssistant, fridge_resources, hass_storage +) -> None: + """The case a large share of users will actually hit: HA restarts onto + the new release while the appliance is unplugged or asleep. + + The entry comes up from its snapshot (issue #295), which never reached + the device -- so it has no standing to claim an identity. It must load + under the key its registry rows already carry and write nothing, or the + real UUID would later look like a *changed* identity to defend against + rather than the one-time adoption it is. + """ + entry = _entry(hass, version=3, key=MOCK_SERIAL) + _, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + # Bank a snapshot from a run on the old release, then take it down. + with _reachable(fridge_resources, None): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + assert entry.data[CONF_DEVICE_KEY] == MOCK_SERIAL + + # Now the appliance is off. Simulate the pre-v4 shape the upgrade finds. + hass.config_entries.async_update_entry( + entry, data={k: v for k, v in entry.data.items() if k != CONF_DEVICE_KEY}, version=3 + ) + + with _unreachable(): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == MOCK_SERIAL + # No key claimed, and the registry is exactly as it was. + assert CONF_DEVICE_KEY not in entry.data + assert entry.unique_id == f"{DOMAIN}_{MOCK_SERIAL}" + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" + ) + + +async def test_an_offline_load_never_rewrites_the_registry( + hass: HomeAssistant, fridge_resources, hass_storage +) -> None: + """A snapshot replay must not re-key on the strength of what the + snapshot says, because that is last run's answer rather than the + device's. + + Constructed so the two disagree: the snapshot was banked against a + device reporting one serial, while the entry is registered under + another. A replay that trusted the snapshot would rewrite every + registry row to match it -- without the appliance having been reachable + at any point -- and would then freeze that answer into CONF_DEVICE_KEY, + so the real UUID could never be adopted afterwards. + """ + entry = _entry(hass, version=3, key=MOCK_SERIAL) + + with _reachable(fridge_resources, None): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + # The entry is registered under a different identity than the snapshot's, + # and back on the pre-v4 shape the upgrade finds. + hass.config_entries.async_update_entry( + entry, + data={ + **{k: v for k, v in entry.data.items() if k != CONF_DEVICE_KEY}, + CONF_SERIAL: "LEGACY-KEY", + }, + unique_id=f"{DOMAIN}_LEGACY-KEY", + version=3, + ) + device, existing = _seed_registry(hass, entry, "LEGACY-KEY") + + with _unreachable(): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == "LEGACY-KEY" + assert CONF_DEVICE_KEY not in entry.data + assert entry.unique_id == f"{DOMAIN}_LEGACY-KEY" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, "LEGACY-KEY")} + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_LEGACY-KEY_connection_mode" + ) + + # And the deferred adoption still works once the appliance answers -- + # the offline load left nothing frozen behind it. + with _reachable(fridge_resources, UUID_A): + coordinator._connect_session() + coordinator._run_discovery(fridge_resources) + + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_A}_connection_mode" + ) + + +async def test_the_appliance_coming_back_completes_the_upgrade( + hass: HomeAssistant, fridge_resources +) -> None: + """The other half of the offline case: the deferred adoption is not + abandoned, it just waits. Once the device answers, the same re-key runs + and the entry finishes its upgrade.""" + entry = _entry(hass, version=3, key=MOCK_SERIAL) + _, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + with _reachable(fridge_resources, None): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + assert entry.data[CONF_DEVICE_KEY] == MOCK_SERIAL + + # The device is reachable again, now reporting its UUID. + coordinator = hass.data[DOMAIN][entry.entry_id] + with _reachable(fridge_resources, UUID_A): + coordinator._connect_session() + coordinator._run_discovery(fridge_resources) + + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_A}_connection_mode" + ) + + +async def test_restarting_after_the_upgrade_is_a_no_op( + hass: HomeAssistant, fridge_resources +) -> None: + """Every restart re-runs discovery, so the adoption path runs again on + an entry that has already moved. It must recognise its own work and do + nothing -- a re-key that fired every boot would churn the registry + forever.""" + entry = _entry(hass, version=3, key=MOCK_SERIAL) + _, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + after_first = dict(entry.data) + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert dict(entry.data) == after_first + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + kept = er.async_get(hass).async_get(existing.entity_id) + assert kept is not None + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + # Exactly one device, not a duplicate alongside it. + assert len(dr.async_entries_for_config_entry(dr.async_get(hass), entry.entry_id)) == 1 + + +async def test_an_already_migrated_entry_is_left_alone( + hass: HomeAssistant, fridge_resources +) -> None: + """A v4 entry created by the current config flow has nothing to migrate + and nothing to re-key -- it was minted on its UUID.""" + entry = _entry(hass, version=4, key=UUID_A, serial=MOCK_SERIAL, device_key=UUID_A) + device, existing = _seed_registry(hass, entry, UUID_A) + + with _reachable(fridge_resources, UUID_A): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.version == 4 + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_A}_connection_mode" + ) + + +# --------------------------------------------------------------------------- +# Guarding the identity once it has moved +# --------------------------------------------------------------------------- + + +async def test_a_poll_that_reads_no_uuid_does_not_demote_a_keyed_entry( + hass: HomeAssistant, fridge_resources +) -> None: + """The device saying nothing is not the device saying something + different. A reconnect that can't read /oic/d (a timeout, a firmware + hiccup) must leave the key alone -- demoting back onto the serial would + re-key every entity the user has for the duration of an outage, and + re-key them all back afterwards.""" + entry = _entry(hass, version=4, key=UUID_A, serial=MOCK_SERIAL, device_key=UUID_A) + + with _reachable(fridge_resources, None): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + + +async def test_a_rotated_uuid_on_the_same_serial_is_followed( + hass: HomeAssistant, fridge_resources +) -> None: + """OCF permits a hard factory reset to regenerate `di`. The serialNum is + what tells that apart from a different appliance moving onto the + address, and following it keeps the user's history rather than stranding + it on a UUID the device will never report again.""" + entry = _entry(hass, version=4, key=UUID_A, serial=MOCK_SERIAL, device_key=UUID_A) + device, existing = _seed_registry(hass, entry, UUID_A) + + # fridge_resources reports MOCK_SERIAL, matching what the entry stored. + with _reachable(fridge_resources, UUID_B): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_B)} + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_B}_connection_mode" + ) + + +async def test_a_different_appliance_on_the_same_address_keeps_the_registered_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """Neither the UUID nor the serial matches what this entry was + registered with, so this is a different appliance answering at this + address -- not a reset of the registered one. Re-keying here would hand + one appliance's entities, history and automations to another; re-adding + is the user's call.""" + entry = _entry(hass, version=4, key=UUID_A, serial="SOME-OTHER-APPLIANCE", device_key=UUID_A) + device, _ = _seed_registry(hass, entry, UUID_A) + + with _reachable(fridge_resources, UUID_B): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} + # The rejected appliance's serial is not written either. The serial + # is what corroborates a later change of key, so adopting it here + # would hand the intruder exactly the corroboration it needs to win + # the *next* poll -- defending the identity once and then + # surrendering it on the following cycle. + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + coordinator = hass.data[DOMAIN][entry.entry_id] + coordinator._run_discovery(fridge_resources) + + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + +# --------------------------------------------------------------------------- +# rekey_entry's own contract +# --------------------------------------------------------------------------- + + +async def test_rekey_is_idempotent(hass: HomeAssistant) -> None: + """Safe to attempt on every poll rather than having to track whether it + has already run -- a second call finds nothing under the old key. + + Calls rekey_entry directly: what it leaves behind is the contract, and + going through a setup would let the platforms re-adding their entities + hide a row that had in fact been orphaned. + """ + from custom_components.localthings.rekey import rekey_entry + + entry = _entry(hass, version=3, key=MOCK_SERIAL) + _, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + kept = er.async_get(hass).async_get(existing.entity_id) + assert kept is not None + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + + +async def test_rekey_removes_a_stale_row_rather_than_colliding(hass: HomeAssistant) -> None: + """Where the destination key is already taken, the old-key row is the + dead one -- unavailable since whichever restart created the split -- so + it goes rather than being rewritten onto a key that exists. Same rule + the #236 repair has always applied, now on the identity move.""" + from custom_components.localthings.rekey import rekey_entry + + entry = _entry(hass, version=3, key=MOCK_SERIAL) + ent_reg = er.async_get(hass) + live = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{UUID_A}_connection_mode", config_entry=entry + ) + stale = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", config_entry=entry + ) + assert stale.entity_id != live.entity_id + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + assert ent_reg.async_get(stale.entity_id) is None + assert ent_reg.async_get(live.entity_id) is not None + + +async def test_rekey_to_the_same_key_does_nothing(hass: HomeAssistant) -> None: + """The no-op guard that lets callers pass whatever they resolved without + checking first.""" + from custom_components.localthings.rekey import rekey_entry + + entry = _entry(hass, version=4, key=UUID_A, device_key=UUID_A) + _, existing = _seed_registry(hass, entry, UUID_A) + + rekey_entry(hass, entry, UUID_A, UUID_A) + + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{UUID_A}_connection_mode" + ) + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" diff --git a/tests/localthings/test_migration.py b/tests/localthings/test_migration.py index 8c4788a..9086907 100644 --- a/tests/localthings/test_migration.py +++ b/tests/localthings/test_migration.py @@ -1,17 +1,18 @@ -"""Config-entry migration, the placeholder-identity repair (issue #236), and -the move onto the OCF device UUID (issue #381).""" +"""Config-entry migration and the placeholder-identity repair (issue #236). + +The move onto the OCF device UUID (issue #381) has its own suite in +test_identity_migration.py -- it is the one migration step that can't +finish inside async_migrate_entry, so it needs a device to talk to. +""" from __future__ import annotations -from unittest.mock import patch - from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( - CONF_DEVICE_KEY, CONF_HOST, CONF_SERIAL, DOMAIN, @@ -279,312 +280,3 @@ async def test_migration_without_a_unique_id_falls_back_to_host(hass: HomeAssist assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == entry.data[CONF_HOST] assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" - - -# --------------------------------------------------------------------------- -# v3 -> v4: onto the OCF device UUID (issue #381) -# --------------------------------------------------------------------------- - -# The two purifiers from issue #381: one serialNum, two device UUIDs. -SHARED_SERIAL = "BS7SP9AW400114A" -UUID_A = "ccfd73b3-aeb4-792a-1100-68f06f5d603b" -UUID_B = "3771f8bf-c184-3a2d-d885-e4c9818736d2" - - -def _identity_reporting(device_id: str | None): - """A patched _connect_session that hands the coordinator the identity a - real DTLS connect would have read off /oic/p and /oic/d.""" - from custom_components.localthings.registry.identity import DeviceIdentity - - def _connect(self): - self._identity = ( - None - if device_id is None - else DeviceIdentity( - manufacturer="Samsung Electronics", - model="AVT-WW-TP1-23-AXX500", - name="Samsung AirPurifier", - serial=None, - device_id=device_id, - ) - ) - - return patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session", - _connect, - ) - - -def _v3_entry(hass: HomeAssistant, key: str, *, serial: str | None = None) -> MockConfigEntry: - """An entry as it sits on disk before this release: keyed on CONF_SERIAL, - with no CONF_DEVICE_KEY.""" - data = {**LEGACY_ENTRY_DATA, CONF_SERIAL: serial if serial is not None else key} - entry = MockConfigEntry(domain=DOMAIN, data=data, unique_id=f"{DOMAIN}_{key}", version=3) - entry.add_to_hass(hass) - return entry - - -async def test_v3_entry_moves_onto_the_device_uuid_keeping_its_entity_ids( - hass: HomeAssistant, fridge_resources -) -> None: - """The whole migration promise for an existing user, asserted end to end. - - The re-key happens on the first live poll rather than in - async_migrate_entry, because the UUID is only readable from the device - and an entry can load entirely from its snapshot while the appliance is - off (issue #295). What the user must keep across it: the entity_id (and - with it the entity's name, area, long-term statistics, and every - automation and dashboard that references it), and the device row. - """ - entry = _v3_entry(hass, MOCK_SERIAL) - dev_reg = dr.async_get(hass) - ent_reg = er.async_get(hass) - - device = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, MOCK_SERIAL)}, - ) - existing = ent_reg.async_get_or_create( - "sensor", - DOMAIN, - f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", - config_entry=entry, - device_id=device.id, - suggested_object_id="kitchen_purifier_connection", - ) - - with ( - _identity_reporting(UUID_A), - patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", - return_value=fridge_resources, - ), - patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), - ): - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - assert entry.version == 4 - assert entry.data[CONF_DEVICE_KEY] == UUID_A - # The serial is kept alongside the key, not replaced by it: it is what - # corroborates a later change of UUID. - assert entry.data[CONF_SERIAL] == MOCK_SERIAL - # All three permanent places moved together. - assert entry.unique_id == f"{DOMAIN}_{UUID_A}" - rekeyed_device = dev_reg.async_get(device.id) - assert rekeyed_device is not None - assert rekeyed_device.identifiers == {(DOMAIN, UUID_A)} - kept = ent_reg.async_get(existing.entity_id) - assert kept is not None - assert kept.entity_id == "sensor.kitchen_purifier_connection" - assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" - # And nothing is left behind on the old key. - assert dev_reg.async_get_device(identifiers={(DOMAIN, MOCK_SERIAL)}) is None - - -async def test_a_host_keyed_entry_adopts_a_real_identity( - hass: HomeAssistant, fridge_resources -) -> None: - """A placeholder-serial board (issues #83/#189) was keyed on its IP, which - is an address rather than an identity -- a new DHCP lease silently makes - it someone else's. Such an entry never made an identity claim to defend, - so a real UUID is adopted without needing the serial to corroborate it; - requiring corroboration would strand exactly these boards, since their - serial resolves to the host and can never match.""" - entry = _v3_entry(hass, MOCK_HOST) - dev_reg = dr.async_get(hass) - device = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, MOCK_HOST)}, - ) - - with ( - _identity_reporting(UUID_B), - patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", - return_value=fridge_resources, - ), - patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), - ): - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - assert entry.data[CONF_DEVICE_KEY] == UUID_B - rekeyed = dev_reg.async_get(device.id) - assert rekeyed is not None - assert rekeyed.identifiers == {(DOMAIN, UUID_B)} - - -async def test_a_poll_that_reads_no_uuid_does_not_demote_a_keyed_entry( - hass: HomeAssistant, fridge_resources -) -> None: - """The device saying nothing is not the device saying something different. - - A reconnect that can't read /oic/d (a timeout, a firmware hiccup) must - leave the key alone -- demoting back onto the serial would re-key every - entity the user has for the duration of an outage, and re-key them all - back afterwards.""" - entry = MockConfigEntry( - domain=DOMAIN, - data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, - unique_id=f"{DOMAIN}_{UUID_A}", - version=4, - ) - entry.add_to_hass(hass) - - with ( - _identity_reporting(None), - patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", - return_value=fridge_resources, - ), - patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), - ): - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - coordinator = hass.data[DOMAIN][entry.entry_id] - assert coordinator.device_key == UUID_A - assert entry.data[CONF_DEVICE_KEY] == UUID_A - - -async def test_a_rotated_uuid_on_the_same_serial_is_followed( - hass: HomeAssistant, fridge_resources -) -> None: - """OCF permits a hard factory reset to regenerate `di`. The serialNum is - what tells that apart from a different appliance moving onto the address, - and following it keeps the user's history rather than stranding it on a - UUID the device will never report again.""" - entry = MockConfigEntry( - domain=DOMAIN, - data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, - unique_id=f"{DOMAIN}_{UUID_A}", - version=4, - ) - entry.add_to_hass(hass) - dev_reg = dr.async_get(hass) - device = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, UUID_A)}, - ) - - # fridge_resources reports MOCK_SERIAL, matching what the entry stored. - with ( - _identity_reporting(UUID_B), - patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", - return_value=fridge_resources, - ), - patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), - ): - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - assert entry.data[CONF_DEVICE_KEY] == UUID_B - rekeyed = dev_reg.async_get(device.id) - assert rekeyed is not None - assert rekeyed.identifiers == {(DOMAIN, UUID_B)} - - -async def test_a_different_appliance_on_the_same_address_keeps_the_registered_identity( - hass: HomeAssistant, fridge_resources -) -> None: - """Neither the UUID nor the serial matches what this entry was registered - with, so this is a different appliance answering at this address -- not a - reset of the registered one. Re-keying here would hand one appliance's - entities, history and automations to another; re-adding is the user's - call.""" - entry = MockConfigEntry( - domain=DOMAIN, - data={ - **LEGACY_ENTRY_DATA, - CONF_SERIAL: "SOME-OTHER-APPLIANCE", - CONF_DEVICE_KEY: UUID_A, - }, - unique_id=f"{DOMAIN}_{UUID_A}", - version=4, - ) - entry.add_to_hass(hass) - dev_reg = dr.async_get(hass) - device = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, UUID_A)}, - ) - - with ( - _identity_reporting(UUID_B), - patch( - "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", - return_value=fridge_resources, - ), - patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), - ): - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - assert entry.data[CONF_DEVICE_KEY] == UUID_A - unchanged = dev_reg.async_get(device.id) - assert unchanged is not None - assert unchanged.identifiers == {(DOMAIN, UUID_A)} - # The rejected appliance's serial is not written either. The serial is - # what corroborates a later change of key, so adopting it here would - # hand the intruder exactly the corroboration it needs to win the *next* - # poll -- defending the identity once and then surrendering it on the - # following cycle. - assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" - - coordinator = hass.data[DOMAIN][entry.entry_id] - coordinator._run_discovery(fridge_resources) - - assert coordinator.device_key == UUID_A - assert entry.data[CONF_DEVICE_KEY] == UUID_A - assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" - - -async def test_rekey_moves_subdevice_identifiers_too(hass: HomeAssistant) -> None: - """A composite appliance (issue #177) registers one device per logical - subdevice, keyed f"{key}_{subdevice}" and linked via_device to the - master's bare key. Rewriting only the exact-match identifier would strand - every sibling, so the prefix form moves with it. - - Calls rekey_entry directly: what it leaves behind is the contract, and - going through a setup would let the platforms re-adding their entities - hide a row that had in fact been orphaned. - """ - from custom_components.localthings.rekey import rekey_entry - - entry = _v3_entry(hass, MOCK_SERIAL) - dev_reg = dr.async_get(hass) - master = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, MOCK_SERIAL)}, - ) - sub = dev_reg.async_get_or_create( - config_entry_id=entry.entry_id, - identifiers={(DOMAIN, f"{MOCK_SERIAL}_subdevice_1")}, - ) - - rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) - - assert dev_reg.async_get(master.id).identifiers == {(DOMAIN, UUID_A)} - assert dev_reg.async_get(sub.id).identifiers == {(DOMAIN, f"{UUID_A}_subdevice_1")} - - -async def test_rekey_is_idempotent(hass: HomeAssistant) -> None: - """Safe to attempt on every poll rather than having to track whether it - has already run -- a second call finds nothing under the old key.""" - from custom_components.localthings.rekey import rekey_entry - - entry = _v3_entry(hass, MOCK_SERIAL) - ent_reg = er.async_get(hass) - existing = ent_reg.async_get_or_create( - "sensor", DOMAIN, f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", config_entry=entry - ) - - rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) - rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) - - kept = ent_reg.async_get(existing.entity_id) - assert kept is not None - assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" - assert entry.unique_id == f"{DOMAIN}_{UUID_A}" diff --git a/tests/test_rekey_statistics_end_to_end.py b/tests/test_rekey_statistics_end_to_end.py new file mode 100644 index 0000000..e61b33e --- /dev/null +++ b/tests/test_rekey_statistics_end_to_end.py @@ -0,0 +1,171 @@ +"""Long-term statistics survive the move onto the OCF device UUID (#381). + +The identity migration rewrites entity registry rows in place rather than +letting Home Assistant create replacements. The reason that matters most to +an existing user is history: statistics are keyed by `statistic_id`, which +for a sensor *is* its entity_id, so an entity that came back as +`sensor.foo_2` would leave years of recorded data stranded under a name +nothing writes to any more -- silently, with no error and no repair. + +tests/localthings/test_identity_migration.py proves the entity_id is +preserved. It cannot prove that preserving it preserves the history, +because no recorder is running there. This drives a real in-memory recorder +end to end: statistics recorded, entry re-keyed, values read back. + +Lives here rather than under tests/localthings/ for the same reason +test_statistics_migration_end_to_end.py does: that package's autouse +`enable_custom_integrations` fixture depends on `hass`, which starts Home +Assistant before `recorder_mock` can claim its database URL. Nothing here +loads the integration -- `rekey_entry` is called directly. +""" + +from __future__ import annotations + +from datetime import timedelta +from functools import partial +from typing import cast + +import pytest +from homeassistant.components.recorder.models import StatisticMeanType +from homeassistant.components.recorder.statistics import ( + async_import_statistics, + get_metadata, + statistics_during_period, +) +from homeassistant.components.recorder.util import get_instance +from homeassistant.core import HomeAssistant +from homeassistant.helpers import device_registry as dr +from homeassistant.helpers import entity_registry as er +from homeassistant.util import dt as dt_util +from pytest_homeassistant_custom_component.common import MockConfigEntry +from pytest_homeassistant_custom_component.components.recorder.common import ( + async_wait_recording_done, +) + +from custom_components.localthings.const import CONF_HOST, CONF_SERIAL, DOMAIN +from custom_components.localthings.rekey import rekey_entry + +OLD_KEY = "BS7SP9AW400114A" # the shared serial from issue #381 +NEW_KEY = "ccfd73b3-aeb4-792a-1100-68f06f5d603b" # that unit's own /oic/d `di` +RECORDED = [11.0, 9.0, 14.0] + + +async def _seed_statistics(hass: HomeAssistant, entity_id: str) -> None: + start = dt_util.utcnow().replace(minute=0, second=0, microsecond=0) - timedelta(hours=4) + async_import_statistics( + hass, + { + "mean_type": StatisticMeanType.ARITHMETIC, + "has_sum": False, + "name": None, + "source": "recorder", + "statistic_id": entity_id, + "unit_class": None, + "unit_of_measurement": None, + }, + [ + {"start": start + timedelta(hours=i), "mean": value, "min": value, "max": value} + for i, value in enumerate(RECORDED) + ], + ) + await async_wait_recording_done(hass) + + +async def _means(hass: HomeAssistant, entity_id: str) -> list[float]: + rows = await get_instance(hass).async_add_executor_job( + statistics_during_period, + hass, + dt_util.utcnow() - timedelta(days=1), + None, + {entity_id}, + "hour", + None, + {"mean"}, + ) + return [cast(float, row["mean"]) for row in rows.get(entity_id, [])] + + +@pytest.fixture +def purifier_entry(hass: HomeAssistant) -> MockConfigEntry: + entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_HOST: "192.168.0.3", CONF_SERIAL: OLD_KEY}, + unique_id=f"{DOMAIN}_{OLD_KEY}", + version=3, + ) + entry.add_to_hass(hass) + return entry + + +async def test_recorded_history_survives_the_re_key( + recorder_mock, hass: HomeAssistant, purifier_entry: MockConfigEntry +) -> None: + """The claim the whole in-place rewrite exists to make good on.""" + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=purifier_entry.entry_id, identifiers={(DOMAIN, OLD_KEY)} + ) + dust = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{OLD_KEY}_dust", + config_entry=purifier_entry, + device_id=device.id, + suggested_object_id="bedroom_purifier_dust", + ) + await _seed_statistics(hass, dust.entity_id) + assert await _means(hass, dust.entity_id) == RECORDED + + rekey_entry(hass, purifier_entry, OLD_KEY, NEW_KEY) + await async_wait_recording_done(hass) + + # The row moved to the new identity... + moved = ent_reg.async_get(dust.entity_id) + assert moved is not None + assert moved.unique_id == f"{DOMAIN}_{NEW_KEY}_dust" + # ...without moving the entity_id, which is what the statistics are + # filed under -- so the history is still there, unchanged and still + # attached to the entity the user sees. + assert moved.entity_id == "sensor.bedroom_purifier_dust" + assert await _means(hass, dust.entity_id) == RECORDED + + # The metadata row is still filed under this entity_id too, so the + # recorder has not quietly started a second series alongside it. + metadata = await get_instance(hass).async_add_executor_job( + partial(get_metadata, hass, statistic_ids={dust.entity_id}) + ) + assert set(metadata) == {dust.entity_id} + + +async def test_a_removed_duplicate_does_not_take_the_surviving_history_with_it( + recorder_mock, hass: HomeAssistant, purifier_entry: MockConfigEntry +) -> None: + """Where the destination key is already taken, the old-key row is deleted + rather than rewritten. The history belongs to whichever entity_id the + user has been looking at all along -- the surviving row -- so deleting + the dead duplicate must not disturb it. + + This is the one path in the re-key that destroys a registry row, so it + is the one worth proving keeps its hands off the recorder. + """ + ent_reg = er.async_get(hass) + live = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{NEW_KEY}_dust", + config_entry=purifier_entry, + suggested_object_id="bedroom_purifier_dust", + ) + stale = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{OLD_KEY}_dust", config_entry=purifier_entry + ) + assert stale.entity_id != live.entity_id + await _seed_statistics(hass, live.entity_id) + + rekey_entry(hass, purifier_entry, OLD_KEY, NEW_KEY) + await async_wait_recording_done(hass) + + assert ent_reg.async_get(stale.entity_id) is None + assert ent_reg.async_get(live.entity_id) is not None + assert await _means(hass, live.entity_id) == RECORDED From c7aa66ef6e0a2f01d59114fc97a5f264fbd36fef Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:24:22 +0000 Subject: [PATCH 3/5] Address code review: close the migration-window duplicate and unify adoption Two real findings from review of the identity change. A pre-v4 entry keeps its serial-keyed unique_id until its first live poll adopts the UUID, which can be a long while for an appliance that is off since the entry loads from its snapshot meanwhile. The config flow's UUID check couldn't see such an entry, so re-adding the same appliance in that window was waved through as a second entry -- and the two would collide the moment the older one re-keyed, with rekey_entry resolving the collision by deleting the duplicate rows, taking the original's entity_ids, history and automations with them. The flow now also aborts on the legacy key, matched together with the host so issue #381's two same-serial units at different addresses stay separable, and gated on CONF_DEVICE_KEY being absent so the check disappears once the entry has migrated. Separately, the "nothing stored yet" branch adopted the polled identity unconditionally, silently dropping the "same IP, different appliance" guard _run_discovery has had since issue #236 -- and dropping it for precisely the users who have been running longest. _resolve_identity now computes the key an entry currently carries once and applies one corroboration rule to it, so a pre-v4 entry defends itself exactly as a migrated one does. Adoption still needs no corroboration where there is no identity claim to defend: an entry keyed on its address (issues #83/#189) or with no stored serial at all. Two tests had to change with it, both because their setup was unfaithful rather than because the behaviour regressed: the two-unit test now has its devices report the shared serial they actually report, and the offline test now models a placeholder-serial board, which is where an entry's key and a snapshot's serial can genuinely disagree. Three new tests cover the changed behaviour, and the mutation sweep is extended to thirteen breakages -- including the two guards added here and the host match that keeps the duplicate check from undoing #381's fix. Also from review: rename three stale _resolve_key doc references to _resolve_identity, and stop rebinding new_unique_id in rekey_entry. --- custom_components/localthings/config_flow.py | 22 ++++ custom_components/localthings/coordinator.py | 100 ++++++++++--------- custom_components/localthings/rekey.py | 6 +- tests/localthings/test_config_flow.py | 66 ++++++++++++ tests/localthings/test_identity_migration.py | 82 +++++++++++---- 5 files changed, 206 insertions(+), 70 deletions(-) diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 304e83a..e561503 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -895,6 +895,28 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.exception("Unexpected error during device probe") errors["base"] = "unknown" else: + # An entry created before v4 still carries the serial-keyed + # unique_id until its first *live* poll adopts the UUID + # (coordinator._resolve_identity) -- which can be a long + # while for an appliance that is off, since an entry loads + # from its snapshot in the meantime (issue #295). The UUID + # check below can't see such an entry, so re-adding this + # very appliance during that window would be waved through + # as a second entry; the two would then collide the moment + # the older one re-keyed, and rekey_entry resolves a + # collision by *deleting* the duplicate rows -- taking the + # original entry's entity_ids, history and automations with + # them. Matched on the legacy key together with the host, so + # issue #381's two units (same serial, different addresses) + # stay separable. + legacy_unique_id = f"localthings_{info['serial']}" + if any( + other.unique_id == legacy_unique_id + and other.data.get(CONF_HOST) == self._host + and CONF_DEVICE_KEY not in other.data + for other in existing + ): + return self.async_abort(reason="already_configured") # Keyed on the OCF device UUID rather than the serialNum # (issue #381): two units of a model that ship the same # well-formed serial are indistinguishable here otherwise, diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 6c85954..4a7dd09 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -1124,23 +1124,27 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): * A snapshot replay (issue #295) never re-keys. That path did not reach the device, so its "reported" identity is only whatever the snapshot happened to preserve. - * Nothing stored (a pre-v4 entry, or a legacy migration that could - not recover an identity) -- adopt what the device reports and - rewrite the registries off the legacy key onto it. This is the - one-time move onto the OCF device UUID (issue #381), deferred to - here because the UUID is only readable from the device itself. + * The polled identity matches what this entry is already keyed on -- + the overwhelmingly common case, and a no-op beyond recording it. * A key is stored and this poll produced no UUID -- keep it. The device saying nothing is not the device saying something different, and demoting a UUID-keyed entry back onto its serial because one reconnect couldn't read /oic/d would re-key every entity the user has for the duration of an outage. - * A key is stored and the UUID differs. Either this appliance's - `di` rotated (an OCF hard reset is allowed to regenerate it) or a + * The identity differs. Either this is the registered appliance + under a new identity -- the one-time move onto the OCF device UUID + (issue #381), or a `di` regenerated by an OCF hard reset -- or a *different* appliance now answers on this address. The serialNum - tells them apart: a usable one that still matches what this entry - was registered with means same unit, new UUID, so follow it. - Anything else keeps the registered identity and warns -- re-adding - is the user's call, not something a poll gets to decide. + is what tells those apart. + + That last decision is deliberately the same whether or not this + entry has adopted a UUID yet. A pre-v4 entry is the population this + change exists to move, but it is also the population that has been + running longest, so it is the last one that should lose its + registry rows to an appliance that merely happens to share its + address -- a guard `_run_discovery` has had since issue #236 and + which an unconditional "adopt whatever the device says" would have + quietly dropped for exactly those users. """ host = self._entry.data[CONF_HOST] stored_serial = self._entry.data.get(CONF_SERIAL) @@ -1149,31 +1153,25 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): polled_ocf = ocf_device_key(self._identity) stored_key = self._entry.data.get(CONF_DEVICE_KEY) + # What this entry's registry rows actually carry today. A pre-v4 + # entry has no CONF_DEVICE_KEY, so it is whatever the coordinator + # has always fallen back to -- see this class's __init__. + current_key = stored_key if stored_key is not None else (stored_serial or host) + # `polled_serial` is already resolve_serial's output, so this is + # exactly resolve_device_key's chain: the UUID where the device + # reports one, today's serial/host answer where it doesn't. + polled_key = polled_ocf or polled_serial - if stored_key is None: - # `polled_serial` is already resolve_serial's output, so this is - # exactly resolve_device_key's chain: the UUID where the device - # reports one, today's serial/host answer where it doesn't. - polled_key = polled_ocf or polled_serial - legacy_key = stored_serial or host - if legacy_key != polled_key: - self._log.info( - "moving this device's registry entries from %r onto %r", - legacy_key, - polled_key, - ) - rekey_entry(self.hass, self._entry, legacy_key, polled_key) - return polled_key, polled_serial + if polled_key == current_key: + # Returned rather than short-circuited so a pre-v4 entry records + # the key it has always had, which is what stops the next poll + # from treating this same answer as a change. + return current_key, polled_serial - if polled_ocf == stored_key: - # Confirmed the registered appliance, so this poll's serial is - # authoritative -- firmware is allowed to correct it. - return stored_key, polled_serial - if polled_ocf is None: + if stored_key is not None and polled_ocf is None: # Nothing to compare against: this may or may not be the # registered appliance, so neither half is updated. return stored_key, stored_serial or polled_serial - polled_key = polled_ocf # Excluding the host answer is what keeps this from firing on two # *different* placeholder-serial units: `polled_serial` is already @@ -1182,25 +1180,27 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): same_unit = polled_serial == stored_serial and polled_serial != host # An entry keyed on the host never made an identity claim to defend # -- it is registered against "whatever answers at this address" - # (issues #83/#189). Any real UUID is a strict improvement on that, - # so it is adopted without needing the serial to corroborate it; - # requiring corroboration would strand exactly the placeholder-serial - # boards this whole change is meant to rescue, since their serial - # resolves to the host and can never match. - if not (stored_key == host or same_unit): + # (issues #83/#189). Any real identity is a strict improvement on + # that, so it is adopted without needing the serial to corroborate + # it; requiring corroboration would strand exactly the + # placeholder-serial boards this whole change is meant to rescue, + # since their serial resolves to the host and can never match. An + # entry with no stored serial at all has nothing to corroborate + # against either, and is treated the same way. + if not (current_key == host or same_unit or stored_serial is None): self._log.warning( "device at %s identifies as %r but this entry is registered as %r " "(serial %r, registered %r); keeping the registered identity", host, polled_key, - stored_key, + current_key, polled_serial, stored_serial, ) - return stored_key, stored_serial or polled_serial + return current_key, stored_serial or polled_serial - # Corroborated: same unit with a new UUID (a factory reset is - # allowed to regenerate `di`), or an address-keyed entry finally + # Corroborated: a pre-v4 entry moving onto its UUID, the same unit + # with a regenerated `di`, or an address-keyed entry finally # learning a real identity. Following it keeps the user's # entity_ids, history and automations rather than stranding them on # a key the device will never report again. @@ -1208,10 +1208,10 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): "device %s (serial %r) changed key from %r to %r; following it", host, polled_serial, - stored_key, + current_key, polled_key, ) - rekey_entry(self.hass, self._entry, stored_key, polled_key) + rekey_entry(self.hass, self._entry, current_key, polled_key) return polled_key, polled_serial def _persist_identity( @@ -1232,7 +1232,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): `device_key` is what the registries are keyed on; `serial` is stored alongside it rather than replaced by it, because it is what - `_resolve_key` corroborates a changed UUID against on a later poll. + `_resolve_identity` corroborates a changed UUID against on a later + poll. None leaves whatever key is already stored untouched -- see the caller for why a snapshot replay must not write one. @@ -1325,8 +1326,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): name=ident.get("name") or "", serial=ident.get("serial"), # Absent from a snapshot written before these fields - # existed; _resolve_key never re-keys from a replay, so - # a missing UUID here costs nothing beyond diagnostics. + # existed; _resolve_identity never re-keys from a + # replay, so a missing UUID here costs nothing beyond + # diagnostics. device_id=ident.get("device_id"), platform_id=ident.get("platform_id"), device_types=tuple(ident.get("device_types") or ()), @@ -1500,9 +1502,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # A snapshot replay passes None: it never reached the device, so it # has no standing to write a device key. Persisting `self.device_key` # there would freeze a pre-v4 entry's legacy key into CONF_DEVICE_KEY - # as if a poll had confirmed it, and _resolve_key would then treat - # the real UUID -- when a live poll finally produced one -- as a - # changed identity to defend against rather than the one-time + # as if a poll had confirmed it, and _resolve_identity would then + # treat the real UUID -- when a live poll finally produced one -- as + # a changed identity to defend against rather than the one-time # adoption it is. self._persist_identity(None if from_snapshot else key, serial, model, mfr, device_type_name) if not from_snapshot: diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py index db04631..415b878 100644 --- a/custom_components/localthings/rekey.py +++ b/custom_components/localthings/rekey.py @@ -64,9 +64,9 @@ def rekey_entry(hass: HomeAssistant, entry: ConfigEntry, old_key: str, new_key: if old_key == new_key: return - new_unique_id = f"{DOMAIN}_{new_key}" - if entry.unique_id != new_unique_id: - hass.config_entries.async_update_entry(entry, unique_id=new_unique_id) + new_entry_unique_id = f"{DOMAIN}_{new_key}" + if entry.unique_id != new_entry_unique_id: + hass.config_entries.async_update_entry(entry, unique_id=new_entry_unique_id) ent_reg = er.async_get(hass) stale_prefix = f"{DOMAIN}_{old_key}_" diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index afd6596..09ec45a 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -1115,6 +1115,72 @@ async def test_same_serial_on_two_units_is_not_a_duplicate(hass: HomeAssistant, assert result["data"][CONF_SERIAL] == first.data[CONF_SERIAL] +async def test_re_adding_during_the_migration_window_is_still_a_duplicate( + hass: HomeAssistant, mock_probe +) -> None: + """An entry created before v4 keeps its serial-keyed unique_id until its + first *live* poll adopts the UUID, which can be a long while for an + appliance that is off (it loads from its snapshot meanwhile, issue + #295). The UUID check can't see such an entry, so without a second + check on the legacy key, re-adding this very appliance in that window + would be waved through -- and the two entries would collide the moment + the older one re-keyed, with rekey_entry resolving the collision by + deleting the duplicate rows and taking the original's entity_ids, + history and automations with them. + """ + existing = MockConfigEntry( + domain=DOMAIN, + data={k: v for k, v in ENTRY_DATA.items() if k != CONF_DEVICE_KEY}, + unique_id=f"localthings_{MOCK_SERIAL}", + version=3, + ) + existing.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: ENTRY_DATA[CONF_HOST]} + ) + + assert result["type"] == FlowResultType.ABORT + assert result["reason"] == "already_configured" + + +async def test_the_migration_window_check_still_separates_two_same_serial_units( + hass: HomeAssistant, mock_probe +) -> None: + """The legacy-key check above matches on the host as well as the serial, + so it cannot undo the fix: issue #381's two units share a serial but sit + at different addresses, and the second must still be addable while the + first is mid-migration.""" + existing = MockConfigEntry( + domain=DOMAIN, + data={ + **{k: v for k, v in ENTRY_DATA.items() if k != CONF_DEVICE_KEY}, + CONF_HOST: "192.168.0.3", + }, + unique_id=f"localthings_{MOCK_SERIAL}", + version=3, + ) + existing.add_to_hass(hass) + + second_probe = { + **_probe_result(recognized=True), + "device_key": "3771f8bf-c184-3a2d-d885-e4c9818736d2", + "serial": MOCK_SERIAL, + } + with patch( + "custom_components.localthings.config_flow._probe_and_validate", + return_value=second_probe, + ): + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: "192.168.0.14"} + ) + + assert result["type"] == FlowResultType.CREATE_ENTRY + assert result["data"][CONF_DEVICE_KEY] == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + + def test_probe_reads_the_device_key_from_oic_d_without_an_extra_round_trip(monkeypatch): """`_read_device` already fetches /oic/p and /oic/d for the device-type signal, so keying on the OCF UUID costs no additional GET -- it reads diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py index 6972e47..0ff3971 100644 --- a/tests/localthings/test_identity_migration.py +++ b/tests/localthings/test_identity_migration.py @@ -114,6 +114,14 @@ def _entry( return entry +def _reporting_serial(resources: dict, serial: str) -> dict: + """`resources` with the serialNum the device reports swapped out, so a + test can pair a fixture with the identity its scenario implies.""" + info = dict(resources["/information/vs/0"]) + info["x.com.samsung.da.serialNum"] = serial + return {**resources, "/information/vs/0": info} + + def _seed_registry(hass: HomeAssistant, entry: MockConfigEntry, key: str, **entity_kwargs): """A device and one entity keyed on `key`, as a running install has.""" dev_reg = dr.async_get(hass) @@ -229,10 +237,14 @@ async def test_the_two_units_from_the_issue_migrate_to_separate_identities( including their entity unique_ids, which is where issue #83's Bug 4 silently dropped the second unit's entities even once its entry existed. """ + # Both units report the shared serial, as the real ones do -- so the + # entry's stored identity still matches what the device says, and only + # the UUID separates them. + resources = _reporting_serial(fridge_resources, SHARED_SERIAL) first = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.3") _, first_entity = _seed_registry(hass, first, SHARED_SERIAL, suggested_object_id="purifier_a") - with _reachable(fridge_resources, UUID_A): + with _reachable(resources, UUID_A): await hass.config_entries.async_setup(first.entry_id) await hass.async_block_till_done() @@ -243,7 +255,7 @@ async def test_the_two_units_from_the_issue_migrate_to_separate_identities( second = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.14") _, second_entity = _seed_registry(hass, second, SHARED_SERIAL, suggested_object_id="purifier_b") - with _reachable(fridge_resources, UUID_B): + with _reachable(resources, UUID_B): await hass.config_entries.async_setup(second.entry_id) await hass.async_block_till_done() @@ -453,14 +465,16 @@ async def test_an_offline_load_never_rewrites_the_registry( snapshot says, because that is last run's answer rather than the device's. - Constructed so the two disagree: the snapshot was banked against a - device reporting one serial, while the entry is registered under - another. A replay that trusted the snapshot would rewrite every - registry row to match it -- without the appliance having been reachable - at any point -- and would then freeze that answer into CONF_DEVICE_KEY, - so the real UUID could never be adopted afterwards. + Modelled on a placeholder-serial board (issues #83/#189), which is + where the two can genuinely disagree: the entry is keyed on its address + because the board reports no usable serial, while the snapshot banked + whatever serial the polled resources carried. A replay that trusted the + snapshot would rewrite every registry row onto that serial -- without + the appliance having been reachable at any point -- and would then + freeze the answer into CONF_DEVICE_KEY, so the real UUID could never be + adopted afterwards. """ - entry = _entry(hass, version=3, key=MOCK_SERIAL) + entry = _entry(hass, version=3, key=MOCK_HOST) with _reachable(fridge_resources, None): await hass.config_entries.async_setup(entry.entry_id) @@ -468,30 +482,29 @@ async def test_an_offline_load_never_rewrites_the_registry( await hass.config_entries.async_unload(entry.entry_id) await hass.async_block_till_done() - # The entry is registered under a different identity than the snapshot's, - # and back on the pre-v4 shape the upgrade finds. + # Back to the pre-v4 shape the upgrade finds, still keyed on the address. hass.config_entries.async_update_entry( entry, data={ **{k: v for k, v in entry.data.items() if k != CONF_DEVICE_KEY}, - CONF_SERIAL: "LEGACY-KEY", + CONF_SERIAL: MOCK_HOST, }, - unique_id=f"{DOMAIN}_LEGACY-KEY", + unique_id=f"{DOMAIN}_{MOCK_HOST}", version=3, ) - device, existing = _seed_registry(hass, entry, "LEGACY-KEY") + device, existing = _seed_registry(hass, entry, MOCK_HOST) with _unreachable(): await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() coordinator = hass.data[DOMAIN][entry.entry_id] - assert coordinator.device_key == "LEGACY-KEY" + assert coordinator.device_key == MOCK_HOST assert CONF_DEVICE_KEY not in entry.data - assert entry.unique_id == f"{DOMAIN}_LEGACY-KEY" - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, "LEGACY-KEY")} + assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_HOST)} assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_LEGACY-KEY_connection_mode" + f"{DOMAIN}_{MOCK_HOST}_connection_mode" ) # And the deferred adoption still works once the appliance answers -- @@ -662,6 +675,39 @@ async def test_a_different_appliance_on_the_same_address_keeps_the_registered_id assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" +async def test_a_pre_v4_entry_defends_itself_against_a_different_appliance( + hass: HomeAssistant, fridge_resources +) -> None: + """The "same IP, different appliance" guard applies to an entry that has + not migrated yet, exactly as it does to one that has. + + A pre-v4 entry is the population this change exists to move, but it is + also the population that has been running longest -- so it is the last + one that should hand its entity_ids, history and automations to an + appliance that merely happens to have taken over its address. Adoption + is the migration's job only when the identity is corroborated. + """ + entry = _entry(hass, version=3, key=MOCK_SERIAL) + device, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + # Neither the serial nor (therefore) the UUID belongs to the registered + # appliance. + resources = _reporting_serial(fridge_resources, "SOME-OTHER-APPLIANCE") + with _reachable(resources, UUID_B): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == MOCK_SERIAL + assert entry.data[CONF_DEVICE_KEY] == MOCK_SERIAL + assert entry.data[CONF_SERIAL] == MOCK_SERIAL + assert entry.unique_id == f"{DOMAIN}_{MOCK_SERIAL}" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_SERIAL)} + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" + ) + + # --------------------------------------------------------------------------- # rekey_entry's own contract # --------------------------------------------------------------------------- From 1b8e8368ca6bf135ac435b013555d5adbe533428 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:24:23 +0000 Subject: [PATCH 4/5] Type-check the test suite, not just the component CI runs `ty check custom_components tests`; the identity work was only checked against `custom_components`, so eighteen diagnostics in the new tests went unnoticed until the PR run. Two shapes, both in the new files. `dev_reg.async_get` and `ent_reg.async_get` return `... | None`, so chaining an attribute off them is both a type error and, on the failure it exists to catch, an AttributeError instead of the assertion that would say what went wrong -- replaced with `_device_identifiers`/`_entity_unique_id` helpers that assert the row survived and return the field. And `_identity`'s `**kwargs` dict-merge erased the field types it was constructing; spelling the three optional fields out is clearer anyway. No behaviour change; the full suite and the thirteen-mutation sweep still pass. --- tests/localthings/test_identity_migration.py | 85 +++++++++++--------- tests/test_identity.py | 18 ++++- 2 files changed, 61 insertions(+), 42 deletions(-) diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py index 0ff3971..5d3710c 100644 --- a/tests/localthings/test_identity_migration.py +++ b/tests/localthings/test_identity_migration.py @@ -122,6 +122,25 @@ def _reporting_serial(resources: dict, serial: str) -> dict: return {**resources, "/information/vs/0": info} +def _device_identifiers(hass: HomeAssistant, device_id: str) -> set[tuple[str, str]]: + """This device row's identifiers, asserting the row still exists. + + `dev_reg.async_get` returns `DeviceEntry | None`, so reading through it + directly would crash with an AttributeError on a row the re-key + wrongly removed instead of failing the assertion that says so. + """ + row = dr.async_get(hass).async_get(device_id) + assert row is not None + return row.identifiers + + +def _entity_unique_id(hass: HomeAssistant, entity_id: str) -> str: + """This entity row's unique_id, asserting the row still exists.""" + row = er.async_get(hass).async_get(entity_id) + assert row is not None + return row.unique_id + + def _seed_registry(hass: HomeAssistant, entry: MockConfigEntry, key: str, **entity_kwargs): """A device and one entity keyed on `key`, as a running install has.""" dev_reg = dr.async_get(hass) @@ -205,7 +224,7 @@ async def test_the_oldest_install_walks_all_the_way_from_v1( assert kept is not None assert kept.entity_id == "sensor.old_install_connection" assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} + assert _device_identifiers(hass, device.id) == {(DOMAIN, UUID_A)} async def test_a_host_keyed_entry_adopts_a_real_identity( @@ -225,7 +244,7 @@ async def test_a_host_keyed_entry_adopts_a_real_identity( await hass.async_block_till_done() assert entry.data[CONF_DEVICE_KEY] == UUID_B - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_B)} + assert _device_identifiers(hass, device.id) == {(DOMAIN, UUID_B)} async def test_the_two_units_from_the_issue_migrate_to_separate_identities( @@ -263,11 +282,8 @@ async def test_the_two_units_from_the_issue_migrate_to_separate_identities( assert second.data[CONF_DEVICE_KEY] == UUID_B assert first.unique_id != second.unique_id - ent_reg = er.async_get(hass) - assert ent_reg.async_get(first_entity.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_A}_connection_mode" - ) - assert ent_reg.async_get(second_entity.entity_id).unique_id == ( + assert _entity_unique_id(hass, first_entity.entity_id) == (f"{DOMAIN}_{UUID_A}_connection_mode") + assert _entity_unique_id(hass, second_entity.entity_id) == ( f"{DOMAIN}_{UUID_B}_connection_mode" ) @@ -311,7 +327,9 @@ async def test_every_user_customization_on_the_row_survives( assert kept.icon == "mdi:air-filter" assert kept.area_id == "kitchen" assert kept.hidden_by is er.RegistryEntryHider.USER - assert dr.async_get(hass).async_get(device.id).area_id == "kitchen" + rekeyed_device = dr.async_get(hass).async_get(device.id) + assert rekeyed_device is not None + assert rekeyed_device.area_id == "kitchen" async def test_a_composite_appliance_keeps_its_subdevice_links( @@ -336,8 +354,9 @@ async def test_a_composite_appliance_keeps_its_subdevice_links( await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() - assert dev_reg.async_get(master.id).identifiers == {(DOMAIN, UUID_A)} + assert _device_identifiers(hass, master.id) == {(DOMAIN, UUID_A)} rekeyed_sub = dev_reg.async_get(sub.id) + assert rekeyed_sub is not None assert rekeyed_sub.identifiers == {(DOMAIN, f"{UUID_A}_subdevice_1")} # Still the same parent row, so the device tree the user sees is intact. assert rekeyed_sub.via_device_id == master.id @@ -375,9 +394,9 @@ async def test_a_re_key_never_touches_another_entrys_rows(hass: HomeAssistant) - rekey_entry(hass, migrating, SHARED_SERIAL, UUID_A) - assert ent_reg.async_get(moved.entity_id).unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert _entity_unique_id(hass, moved.entity_id) == f"{DOMAIN}_{UUID_A}_connection_mode" # The other appliance is still on the shared serial, waiting its turn. - assert ent_reg.async_get(stays.entity_id).unique_id == f"{DOMAIN}_{SHARED_SERIAL}_power" + assert _entity_unique_id(hass, stays.entity_id) == f"{DOMAIN}_{SHARED_SERIAL}_power" assert bystander.unique_id == f"{DOMAIN}_{SHARED_SERIAL}" @@ -404,9 +423,9 @@ async def test_a_re_key_stops_at_the_key_boundary(hass: HomeAssistant) -> None: rekey_entry(hass, entry, "TEST-SERIAL", UUID_A) - assert dev_reg.async_get(target.id).identifiers == {(DOMAIN, UUID_A)} - assert dev_reg.async_get(neighbour.id).identifiers == {(DOMAIN, "TEST-SERIAL-0000")} - assert ent_reg.async_get(neighbour_entity.entity_id).unique_id == ( + assert _device_identifiers(hass, target.id) == {(DOMAIN, UUID_A)} + assert _device_identifiers(hass, neighbour.id) == {(DOMAIN, "TEST-SERIAL-0000")} + assert _entity_unique_id(hass, neighbour_entity.entity_id) == ( f"{DOMAIN}_TEST-SERIAL-0000_connection_mode" ) @@ -453,7 +472,7 @@ async def test_upgrading_while_the_appliance_is_off_changes_nothing( # No key claimed, and the registry is exactly as it was. assert CONF_DEVICE_KEY not in entry.data assert entry.unique_id == f"{DOMAIN}_{MOCK_SERIAL}" - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + assert _entity_unique_id(hass, existing.entity_id) == ( f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" ) @@ -502,10 +521,8 @@ async def test_an_offline_load_never_rewrites_the_registry( assert coordinator.device_key == MOCK_HOST assert CONF_DEVICE_KEY not in entry.data assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_HOST)} - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{MOCK_HOST}_connection_mode" - ) + assert _device_identifiers(hass, device.id) == {(DOMAIN, MOCK_HOST)} + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{MOCK_HOST}_connection_mode") # And the deferred adoption still works once the appliance answers -- # the offline load left nothing frozen behind it. @@ -515,9 +532,7 @@ async def test_an_offline_load_never_rewrites_the_registry( assert coordinator.device_key == UUID_A assert entry.data[CONF_DEVICE_KEY] == UUID_A - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_A}_connection_mode" - ) + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{UUID_A}_connection_mode") async def test_the_appliance_coming_back_completes_the_upgrade( @@ -543,9 +558,7 @@ async def test_the_appliance_coming_back_completes_the_upgrade( assert coordinator.device_key == UUID_A assert entry.data[CONF_DEVICE_KEY] == UUID_A assert entry.unique_id == f"{DOMAIN}_{UUID_A}" - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_A}_connection_mode" - ) + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{UUID_A}_connection_mode") async def test_restarting_after_the_upgrade_is_a_no_op( @@ -591,10 +604,8 @@ async def test_an_already_migrated_entry_is_left_alone( assert entry.version == 4 assert entry.data[CONF_DEVICE_KEY] == UUID_A - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_A}_connection_mode" - ) + assert _device_identifiers(hass, device.id) == {(DOMAIN, UUID_A)} + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{UUID_A}_connection_mode") # --------------------------------------------------------------------------- @@ -637,10 +648,8 @@ async def test_a_rotated_uuid_on_the_same_serial_is_followed( await hass.async_block_till_done() assert entry.data[CONF_DEVICE_KEY] == UUID_B - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_B)} - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_B}_connection_mode" - ) + assert _device_identifiers(hass, device.id) == {(DOMAIN, UUID_B)} + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{UUID_B}_connection_mode") async def test_a_different_appliance_on_the_same_address_keeps_the_registered_identity( @@ -659,7 +668,7 @@ async def test_a_different_appliance_on_the_same_address_keeps_the_registered_id await hass.async_block_till_done() assert entry.data[CONF_DEVICE_KEY] == UUID_A - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, UUID_A)} + assert _device_identifiers(hass, device.id) == {(DOMAIN, UUID_A)} # The rejected appliance's serial is not written either. The serial # is what corroborates a later change of key, so adopting it here # would hand the intruder exactly the corroboration it needs to win @@ -702,8 +711,8 @@ async def test_a_pre_v4_entry_defends_itself_against_a_different_appliance( assert entry.data[CONF_DEVICE_KEY] == MOCK_SERIAL assert entry.data[CONF_SERIAL] == MOCK_SERIAL assert entry.unique_id == f"{DOMAIN}_{MOCK_SERIAL}" - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_SERIAL)} - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + assert _device_identifiers(hass, device.id) == {(DOMAIN, MOCK_SERIAL)} + assert _entity_unique_id(hass, existing.entity_id) == ( f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" ) @@ -768,7 +777,5 @@ async def test_rekey_to_the_same_key_does_nothing(hass: HomeAssistant) -> None: rekey_entry(hass, entry, UUID_A, UUID_A) - assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_{UUID_A}_connection_mode" - ) + assert _entity_unique_id(hass, existing.entity_id) == (f"{DOMAIN}_{UUID_A}_connection_mode") assert entry.unique_id == f"{DOMAIN}_{UUID_A}" diff --git a/tests/test_identity.py b/tests/test_identity.py index 36d17ea..f88f837 100644 --- a/tests/test_identity.py +++ b/tests/test_identity.py @@ -137,9 +137,21 @@ def test_read_identity_tolerates_malformed_oic_res(): # --------------------------------------------------------------------------- -def _identity(**kwargs) -> DeviceIdentity: - base = {"manufacturer": "Samsung", "model": "M", "name": "N", "serial": None} - return DeviceIdentity(**{**base, **kwargs}) +def _identity( + *, + serial: str | None = None, + device_id: str | None = None, + platform_id: str | None = None, +) -> DeviceIdentity: + """A DeviceIdentity carrying only the fields the key chain reads.""" + return DeviceIdentity( + manufacturer="Samsung", + model="M", + name="N", + serial=serial, + device_id=device_id, + platform_id=platform_id, + ) def test_read_identity_captures_the_ocf_uuids_as_named_fields(): From ccb4a09c65d473178f4e2031d5978f7adece63d2 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:33:49 +0000 Subject: [PATCH 5/5] Trim the identity work's comments to the contributing guidelines CONTRIBUTING.md asks for one or two sentences over an essay, a pointer rather than a re-derivation, and module docstrings that orient rather than document the design. The identity change was written before that guidance was read, and its comments narrate the reasoning at roughly twice the length the conclusions need. Cut to the conclusion and the evidence that makes it credible, keeping every issue reference: _resolve_identity's docstring 41 lines -> 22 (now shorter than the two longest already in that file), rekey_entry 23 -> 14, its module docstring 21 -> 10, resolve_device_key 24 -> 16, is_usable_device_id 15 -> 8, and the same treatment for the inline blocks in _resolve_identity, the CONF_DEVICE_KEY note, the redaction rationale, and the migration suite's module docstring. Comments only; the suite and the thirteen-mutation sweep are unchanged. --- custom_components/localthings/const.py | 13 +- custom_components/localthings/coordinator.py | 115 ++++++------------ .../localthings/registry/identity.py | 64 ++++------ .../localthings/registry/redact.py | 16 +-- custom_components/localthings/rekey.py | 52 +++----- tests/localthings/test_identity_migration.py | 26 ++-- 6 files changed, 93 insertions(+), 193 deletions(-) diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index 67d3c44..d5a0607 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -30,15 +30,10 @@ CONF_LEAF_KEY_PEM = "leaf_key_pem" # output, the host itself for a placeholder-serial board -- issues # #83/#189), so it matches what _run_discovery computes on the first poll. CONF_SERIAL = "serial" -# The identity this entry's devices and entities are actually keyed on -- -# registry.identity.resolve_device_key's output, normally the OCF device -# UUID (issue #381). Distinct from CONF_SERIAL, which stays as (a) the key -# a pre-v4 entry was minted with, so the one-time re-key knows what to -# rewrite from, and (b) the corroborating identity that tells a device -# whose `di` rotated across a factory reset apart from a different -# appliance that moved onto this address. Absent on an entry that has not -# polled since upgrading: the OCF resources are only readable from the -# device, so the coordinator adopts this on the first live poll. +# What this entry's devices and entities are keyed on -- normally the OCF +# device UUID (issue #381). CONF_SERIAL stays alongside it as the pre-v4 +# key to re-key from, and as what corroborates a later change of UUID. +# Absent until the first live poll, since only the device can report it. CONF_DEVICE_KEY = "device_key" CONF_MODEL = "model" CONF_MANUFACTURER = "manufacturer" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 4a7dd09..a7da859 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -316,14 +316,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # device_key mints permanent registry keys, so it must be correct # before the first entity registers -- a placeholder corrected once # the first poll lands orphans the first device/entity pair instead. - # - # CONF_DEVICE_KEY is what a v4 entry stores (the OCF device UUID, - # issue #381); CONF_SERIAL is both the pre-v4 key and the fallback - # for a board that answers no OCF identity resource; and the host - # covers a pre-migration entry, matching what resolve_serial itself - # returns for a placeholder-serial board (issues #83/#189). The - # chain is ordered so an entry that has not polled since upgrading - # keeps loading under the key its registry entries already carry. + # Ordered so an entry that has not polled since upgrading still + # loads under the key its registry rows already carry: the v4 UUID + # (issue #381), else the pre-v4 serial, else the host (#83/#189). self.device_key = ( entry.data.get(CONF_DEVICE_KEY) or entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] ) @@ -1107,44 +1102,25 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): """The (key, serial) this entry should be registered under, re-keying the registries if the key has moved. - Returns both together because they have to agree: the serial is what - corroborates a later change of key, so writing this poll's serial - while *defending* the stored key against a different appliance would - quietly hand that appliance the corroboration it needs to win the - next poll. The serial is therefore only adopted alongside a key we - accepted -- never on a poll whose identity we just rejected. + Key and serial are returned together because they have to agree: the + serial corroborates a later change of key, so adopting it while + *defending* the stored key would hand a different appliance the + corroboration it needs to win the next poll. - The stored key wins by default: re-keying an entry that already has - registry entries orphans them unless the rewrite goes with it - (issue #236), so nothing here changes a key without calling - `rekey_entry` in the same breath. + Nothing changes a key without calling `rekey_entry` in the same + breath, or the existing registry rows are orphaned (issue #236). + Two things are therefore never treated as a change of identity: a + snapshot replay (issue #295), which never reached the device, and a + poll that read no UUID, since the device saying nothing is not the + device saying something different. - Four things can happen: - - * A snapshot replay (issue #295) never re-keys. That path did not - reach the device, so its "reported" identity is only whatever the - snapshot happened to preserve. - * The polled identity matches what this entry is already keyed on -- - the overwhelmingly common case, and a no-op beyond recording it. - * A key is stored and this poll produced no UUID -- keep it. The - device saying nothing is not the device saying something - different, and demoting a UUID-keyed entry back onto its serial - because one reconnect couldn't read /oic/d would re-key every - entity the user has for the duration of an outage. - * The identity differs. Either this is the registered appliance - under a new identity -- the one-time move onto the OCF device UUID - (issue #381), or a `di` regenerated by an OCF hard reset -- or a - *different* appliance now answers on this address. The serialNum - is what tells those apart. - - That last decision is deliberately the same whether or not this - entry has adopted a UUID yet. A pre-v4 entry is the population this - change exists to move, but it is also the population that has been - running longest, so it is the last one that should lose its - registry rows to an appliance that merely happens to share its - address -- a guard `_run_discovery` has had since issue #236 and - which an unconditional "adopt whatever the device says" would have - quietly dropped for exactly those users. + A genuine difference is either the registered appliance under a new + identity -- the move onto the OCF device UUID (issue #381), or a + `di` regenerated by a hard reset -- or a different appliance on this + address, and the serialNum is what tells them apart. That test is + deliberately the same before and after an entry has adopted a UUID: + a pre-v4 entry has been running longest, so it is the last one that + should lose its rows to whatever now answers at its address. """ host = self._entry.data[CONF_HOST] stored_serial = self._entry.data.get(CONF_SERIAL) @@ -1153,40 +1129,28 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): polled_ocf = ocf_device_key(self._identity) stored_key = self._entry.data.get(CONF_DEVICE_KEY) - # What this entry's registry rows actually carry today. A pre-v4 - # entry has no CONF_DEVICE_KEY, so it is whatever the coordinator - # has always fallen back to -- see this class's __init__. + # What this entry's rows carry today: a pre-v4 entry has no + # CONF_DEVICE_KEY, so it is whatever __init__ fell back to. current_key = stored_key if stored_key is not None else (stored_serial or host) - # `polled_serial` is already resolve_serial's output, so this is - # exactly resolve_device_key's chain: the UUID where the device - # reports one, today's serial/host answer where it doesn't. polled_key = polled_ocf or polled_serial if polled_key == current_key: # Returned rather than short-circuited so a pre-v4 entry records - # the key it has always had, which is what stops the next poll - # from treating this same answer as a change. + # the key it has always had, and the next poll sees no change. return current_key, polled_serial if stored_key is not None and polled_ocf is None: - # Nothing to compare against: this may or may not be the - # registered appliance, so neither half is updated. return stored_key, stored_serial or polled_serial - # Excluding the host answer is what keeps this from firing on two - # *different* placeholder-serial units: `polled_serial` is already - # resolved, so anything that fell back to the address is not an - # identity and can't corroborate anything. + # Excluding the host answer keeps this from firing on two *different* + # placeholder-serial units: an address is not an identity and can't + # corroborate anything. same_unit = polled_serial == stored_serial and polled_serial != host - # An entry keyed on the host never made an identity claim to defend - # -- it is registered against "whatever answers at this address" - # (issues #83/#189). Any real identity is a strict improvement on - # that, so it is adopted without needing the serial to corroborate - # it; requiring corroboration would strand exactly the - # placeholder-serial boards this whole change is meant to rescue, - # since their serial resolves to the host and can never match. An - # entry with no stored serial at all has nothing to corroborate - # against either, and is treated the same way. + # A host-keyed entry (issues #83/#189) is registered against whatever + # answers at this address, so it has no claim to defend and needs no + # corroboration -- demanding it would strand exactly the + # placeholder-serial boards this exists to rescue. Same for an entry + # with no stored serial to compare against. if not (current_key == host or same_unit or stored_serial is None): self._log.warning( "device at %s identifies as %r but this entry is registered as %r " @@ -1200,10 +1164,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): return current_key, stored_serial or polled_serial # Corroborated: a pre-v4 entry moving onto its UUID, the same unit - # with a regenerated `di`, or an address-keyed entry finally - # learning a real identity. Following it keeps the user's - # entity_ids, history and automations rather than stranding them on - # a key the device will never report again. + # with a regenerated `di`, or a host-keyed entry learning a real + # identity. Following it keeps the user's entity_ids and history. self._log.info( "device %s (serial %r) changed key from %r to %r; following it", host, @@ -1499,13 +1461,10 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): manufacturer=mfr, model=model, ) - # A snapshot replay passes None: it never reached the device, so it - # has no standing to write a device key. Persisting `self.device_key` - # there would freeze a pre-v4 entry's legacy key into CONF_DEVICE_KEY - # as if a poll had confirmed it, and _resolve_identity would then - # treat the real UUID -- when a live poll finally produced one -- as - # a changed identity to defend against rather than the one-time - # adoption it is. + # A snapshot replay passes None: it never reached the device, so + # writing a key here would freeze a pre-v4 entry's legacy key in as + # if a poll had confirmed it, and the real UUID would later look + # like an identity to defend against rather than one to adopt. self._persist_identity(None if from_snapshot else key, serial, model, mfr, device_type_name) if not from_snapshot: # A coverage gap is a claim about what the device reports, so only diff --git a/custom_components/localthings/registry/identity.py b/custom_components/localthings/registry/identity.py index c9b5dcc..94181a3 100644 --- a/custom_components/localthings/registry/identity.py +++ b/custom_components/localthings/registry/identity.py @@ -66,19 +66,12 @@ def resolve_serial(raw_serial: str | None, host: str) -> str: def is_usable_device_id(value: str | None) -> bool: """True for an OCF `di`/`pi` that actually identifies one unit. - Firmware that never had a UUID assigned reports OCF's nil UUID - (all-zero, dashes aside), which is identical on every unit of the - family -- the #189 failure mode transplanted onto a new field, and not - something `is_placeholder_serial`'s repeated-hex-digit rule catches, - because the dashes make more than one distinct character. - - Past that, the same known-junk rules that disqualify a serialNum - disqualify a UUID: a board firmware-flashed with 'Nothing(SVC)' in one - identity field is not a board to trust in another. Anything else is - accepted as-is. A *shared but well-formed* `di` would be undetectable - here, exactly as issue #381's shared-but-well-formed serialNum is -- - heuristics can't see that, which is the whole reason this moves onto a - field the protocol itself has to keep distinct. + Rejects OCF's nil UUID, which firmware that never had one assigned + reports on every unit of the family -- the #189 failure mode on a new + field, and one `is_placeholder_serial`'s repeated-digit rule misses + because the dashes make more than one distinct character. Past that the + same known-junk rules apply: a board flashed with 'Nothing(SVC)' in one + identity field is not one to trust in another. """ s = (value or "").strip() if not s: @@ -92,16 +85,11 @@ def ocf_device_key(identity: DeviceIdentity | None) -> str | None: """The OCF-derived half of resolve_device_key's chain, or None when the device reported no usable UUID. - Split out because "the device has no UUID" and "the device has this - UUID" are different answers to a caller holding an existing key: the - coordinator must never demote an entry from a UUID back onto a - serialNum just because one poll couldn't read /oic/d (a reconnect, a - timeout), which is exactly what it would do if it only saw the - collapsed string resolve_device_key returns. - - Normalized because the result is compared against its stored form on - every poll; firmware that changes case between reads would otherwise - look like a different appliance. + Split out because "no UUID" and "this UUID" are different answers to a + caller holding an existing key: the coordinator must never demote an + entry off its UUID just because one poll couldn't read /oic/d. + Normalized so firmware that changes case between reads doesn't look + like a different appliance. """ if identity is None: return None @@ -116,26 +104,18 @@ def resolve_device_key(identity: DeviceIdentity | None, raw_serial: str | None, Tried in order: /oic/d's `di`, /oic/p's `pi`, the serialNum, the host. - `di` leads because it is the identifier the protocol we are already - speaking uses to address this endpoint -- if it were wrong or shared, - OCF discovery and the DTLS association would not work in the first - place. serialNum, by contrast, is a vendor-populated string no part of - the stack depends on, which is why three separate firmware families - have shipped it unusable: 'Nothing(SVC)' (#83), a flash-unset sentinel - (#189), and -- unfixable by any heuristic -- a well-formed serial - duplicated across every unit of the model (#381). + `di` leads because it is what the protocol already uses to address this + endpoint: if it were wrong or shared, OCF discovery and the DTLS + association would not work at all. serialNum is a vendor-populated + string nothing depends on, which is why three firmware families have + shipped it unusable -- 'Nothing(SVC)' (#83), a flash-unset sentinel + (#189), and a well-formed serial duplicated across every unit (#381). - `pi` is only the fallback despite the OCF spec calling it immutable: - it identifies the *platform*, so a board hosting more than one logical - OCF device shares one `pi` across all of them, reintroducing the very - collision this exists to prevent. `di` is device-scoped, which is the - granularity of a config entry. - - The serial stays in the chain below both so a board that answers - neither OCF resource lands exactly where it did before this existed, - and the host stays last for the same reason -- it is an address rather - than an identity (a new DHCP lease silently makes it someone else's), - so it is strictly a last resort. + `pi` is only the fallback despite the spec calling it immutable: it is + *platform*-scoped, so a board hosting several logical OCF devices + shares one across all of them. `di` is device-scoped, the granularity + of a config entry. The serial and host stay below both so a board + answering neither resource lands where it always did. """ return ocf_device_key(identity) or resolve_serial(raw_serial, host) diff --git a/custom_components/localthings/registry/redact.py b/custom_components/localthings/registry/redact.py index a4dda45..c65b3de 100644 --- a/custom_components/localthings/registry/redact.py +++ b/custom_components/localthings/registry/redact.py @@ -36,17 +36,11 @@ _SENSITIVE_SUBSTRINGS = ( # from the SmartThings app and can carry a person's name -- the device-type # signal we actually want from that resource is `rt`, which is not redacted. # -# OCF's /oic/d `di` and /oic/p `pi` used to be redacted here too. They are -# not account data: they're randomly-assigned per-unit UUIDs, carrying no -# more about their owner than the appliance-internal subdeviceIdList -# diagnostics already reports for the same reason (see diagnostics.py). -# Redacting them cost more than it bought -- issue #381 was two units -# colliding on a duplicated serialNum, and the first diagnostics download -# asking whether their `di`/`pi` differed came back with both values -# blanked, so the question could only be answered by walking the reporter -# through a manual read_resource call. They are now also what -# resolve_device_key mints registry keys from, so a report that hides them -# hides the identity every entity in it is named after. +# /oic/d's `di` and /oic/p's `pi` are deliberately not redacted: they're +# randomly-assigned per-unit UUIDs rather than account data, and they are +# what registry keys are minted from (issue #381), so blanking them hides +# the identity every entity in a report is named after -- which is exactly +# what made #381's first diagnostics download unable to answer it. _SENSITIVE_EXACT = frozenset({"n"}) diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py index 415b878..7b7a51e 100644 --- a/custom_components/localthings/rekey.py +++ b/custom_components/localthings/rekey.py @@ -1,24 +1,13 @@ """Move an entry's registry entries from one device key to another. -The key a device's registry entries are minted from (see -registry.identity.resolve_device_key) appears in three permanent places: -the config entry's unique_id, the device registry's identifiers, and every -entity's unique_id. Changing it therefore can't be a matter of writing a -new value and restarting -- everything already in the registries would be -orphaned, and the user would find a duplicate device whose entities all -carry a `_2` suffix, with their history, area and automations attached to -the dead copy. +The key (registry.identity.resolve_device_key) is permanent in three +places, so changing it means rewriting the registries rather than storing +a new value -- anything left behind is orphaned. Rewriting rather than +recreating is what keeps an entity's entity_id, and with it its history, +area and automations. -So the key change is performed *on* the registries instead. Rewriting -beats deleting: an entity keeps its entity_id, and with it its name, area, -long-term statistics and every automation and dashboard that references -it. - -Lives in its own module rather than in __init__.py because both callers -need it and they sit on opposite sides of an import edge: the v1 -> v2 -migration in __init__.py, and the coordinator's first-poll adoption of the -OCF device UUID (issue #381), which can only happen once the device has -actually been reached. +Its own module because both callers need it: the v1 -> v2 migration in +__init__.py and the coordinator's first-poll adoption (issue #381). """ from __future__ import annotations @@ -39,25 +28,16 @@ _LOGGER = logging.getLogger(__name__) def rekey_entry(hass: HomeAssistant, entry: ConfigEntry, old_key: str, new_key: str) -> None: """Rewrite everything this entry registered under `old_key` to `new_key`. - Covers all three places the key is permanent: the entity registry - (unique_ids are f"{DOMAIN}_{key}_{state_key}"), the device registry - (identifiers are (DOMAIN, key) for the device itself and - (DOMAIN, f"{key}_{subdevice}") for each subdevice of a composite - appliance -- see coordinator.device_info_for), and the config entry's - own unique_id. + All three permanent places move together: entity unique_ids + (f"{DOMAIN}_{key}_{state_key}"), device identifiers ((DOMAIN, key), plus + (DOMAIN, f"{key}_{subdevice}") per subdevice -- see device_info_for), + and the entry's own unique_id. Leaving that last one behind would let + the config flow's duplicate check wave through a re-add of this very + appliance. - Leaving the entry's unique_id behind would half-migrate it: the config - flow's duplicate check (_abort_if_unique_id_configured) would still be - comparing new devices against the key this entry no longer uses, so - re-adding this very appliance would be waved through as a second entry. - - Idempotent: a second call finds nothing left under `old_key` and does - nothing, which is what makes it safe to attempt on every poll rather - than having to track whether it has already run. - - Where both keys somehow already exist, the `old_key` copy is the dead - one -- unavailable since whichever restart created the split -- so it - is removed rather than rewritten over the live entry. + Idempotent, so it is safe to attempt on every poll rather than tracking + whether it has run. Where both keys already exist the `old_key` copy is + the dead one, so it is removed rather than rewritten over the live entry. Must run on the event loop; the registry helpers require it. """ diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py index 5d3710c..6592b44 100644 --- a/tests/localthings/test_identity_migration.py +++ b/tests/localthings/test_identity_migration.py @@ -1,24 +1,16 @@ """Moving an existing install onto the OCF device UUID (issue #381). -Every other migration this integration has done finishes inside -`async_migrate_entry`. This one can't: the UUID is only readable from the -appliance, and an entry can load entirely from its stored snapshot while -the appliance is off (issue #295). So the config-entry version bumps up -front and the coordinator adopts the UUID on the first *live* poll, -rewriting the entity registry, the device registry and the entry's own +This migration can't finish inside `async_migrate_entry` -- the UUID is +only readable from the appliance, and an entry can load from its snapshot +while that appliance is off (issue #295) -- so the coordinator adopts it +on the first live poll, rewriting both registries and the entry's unique_id together. -That makes this the riskiest migration in the codebase: it rewrites the -identity of registry rows a user's automations, dashboards, history and -areas all hang off. The suite is therefore organised around what an -existing user must not lose, rather than around the functions involved: - -* the entity_id, and every customization carried on that registry row -* long-term statistics (keyed by entity_id -- see the end-to-end test in - tests/test_rekey_statistics_end_to_end.py, which drives a real recorder) -* the device row, its area, and a composite appliance's subdevice links -* other config entries' rows, which this must never touch -* stability: an upgrade that happens twice, or offline, must not churn +That makes it the riskiest migration here: it rewrites the identity of +rows a user's automations, history and areas hang off. These tests are +organised around what must not break rather than around the functions +involved. Statistics are covered separately, against a real recorder, in +tests/test_rekey_statistics_end_to_end.py. """ from __future__ import annotations