diff --git a/homeassistant/helpers/device_registry.py b/homeassistant/helpers/device_registry.py index 935612f6711a26..b4d25e79ae3415 100644 --- a/homeassistant/helpers/device_registry.py +++ b/homeassistant/helpers/device_registry.py @@ -2207,31 +2207,28 @@ def async_get_or_create( # noqa: C901 else: connections = _normalize_connections(connections) - device: DeviceEntry | None = self.devices.get_entry( + # A child is referenced via parent_device_id, not adopted by a device info + if ( + matched_child_device := self.child_devices.get_entry( + identifiers=identifiers, config_entry_id=config_entry_id + ) + ) is not None: + raise DeviceInfoError( + config_entry.domain, + device_info, + f"identifiers {sorted(identifiers)} overlap with those of child device " + f"{matched_child_device.id} with identifiers " + f"{sorted(matched_child_device.identifiers)}", + ) + + device = self.devices.get_entry( connections=connections, identifiers=identifiers, config_entry_id=config_entry_id, ) - matched_child_device: ChildDeviceEntry | None = None - - # We do not allow registering a device without parent_device_id if the - # identifiers match an existing child. - # - # Reject registering a device whose identifiers belong to an existing child - # device: a child is referenced via parent_device_id, not adopted by a plain - # device info. The matched child is rejected below. - # - # Only look for a child when no connections were passed. A child device has no - # connections, so a device info that carries connections describes a different - # (main) device; an identifier overlap with a child is handle by the collision - # reconciliation below, not a child reference to reject. - if device is None and not connections: - matched_child_device = self.child_devices.get_entry( - identifiers=identifiers, config_entry_id=config_entry_id - ) self._async_reconcile_collisions( - device if device is not None else matched_child_device, + device, config_entry, device_info, identifiers, @@ -2241,17 +2238,6 @@ def async_get_or_create( # noqa: C901 # Collision reconciliation can update the matched device (e.g. detach # its via link) device = self.devices[device.id] - elif matched_child_device is not None: - # A device info whose identifiers belong to a child device is ambiguous: a - # child device is referenced explicitly via parent_device_id, not attached - # to by a device info. Raise rather than silently return or adopt the child. - raise DeviceInfoError( - config_entry.domain, - device_info, - f"identifiers {sorted(identifiers)} belong to child device " - f"{matched_child_device.id}; declare it with parent_device_id " - "to reference it", - ) # Resolved after collision reconciliation so a removed stale duplicate can't be # linked @@ -2567,6 +2553,23 @@ def async_get_or_create_child( identifiers=identifiers, config_entry_id=config_entry_id ) + # Identifiers are unique per config entry, so raise if identifiers are + # owned by another child device. + for identifier in sorted(identifiers): + if ( + other_child := self.child_devices.get_entry( + identifiers={identifier}, config_entry_id=config_entry_id + ) + ) is not None and ( + child_device is None or other_child.id != child_device.id + ): + raise DeviceInfoError( + domain, + device_info, + f"identifier {identifier} is already registered for child " + f"device {other_child.id} of the same config entry", + ) + if child_device is not None and child_device.parent_device_id != parent.id: raise DeviceInfoError( domain, @@ -2591,16 +2594,13 @@ def async_get_or_create_child( ) self._async_reconcile_collisions( - child_device if child_device is not None else matched_device, + matched_device, config_entry, device_info, identifiers, set(), ) - if child_device is not None: - # Collision reconciliation can update the matched child device - child_device = self.child_devices[child_device.id] - elif matched_device is not None: + if child_device is None and matched_device is not None: # The identifiers are registered by a full device of the config entry: # the integration split the device into child devices, so convert it, # preserving its id. @@ -2698,6 +2698,8 @@ def _async_validate_device_to_child_conversion( "devices itself, and a child device can't be the parent of another " "child device", ) + # The caller guarantees device and parent share the config entry; only + # subentry agreement is left to check if device.config_subentry_id != parent.config_subentry_id: raise DeviceInfoError( config_entry.domain, @@ -2728,7 +2730,7 @@ def _async_convert_device_to_child( The caller must have validated the conversion with _async_validate_device_to_child_conversion. """ - self.hass.verify_event_loop_thread("device_registry.async_get_or_create") + self.hass.verify_event_loop_thread("device_registry.async_get_or_create_child") # The update event reports the old values of every conceptually changed # field: the fields a child device does not have change to None / empty. @@ -3312,7 +3314,6 @@ def _async_update_child_device( self, child_device_id: str, *, - allow_collisions: bool = False, area_id: str | UndefinedType | None = UNDEFINED, disabled_by: DeviceEntryDisabler | UndefinedType | None = UNDEFINED, is_new: bool = False, @@ -3343,7 +3344,6 @@ def _async_update_child_device( child_device_id, old.config_entry_id, merge_identifiers, - allow_collisions, ) old_identifiers = old.identifiers if not merge_identifiers.issubset(old_identifiers): @@ -3357,7 +3357,6 @@ def _async_update_child_device( child_device_id, old.config_entry_id, new_identifiers, - allow_collisions, ) ) old_values["identifiers"] = old.identifiers @@ -3667,7 +3666,7 @@ def async_update_child_device( @callback def _async_reconcile_collisions( self, - matched_device: AnyDeviceEntry | None, + matched_device: DeviceEntry | None, config_entry: ConfigEntry, device_info: DeviceInfo, identifiers: set[tuple[str, str]], @@ -3677,15 +3676,12 @@ def _async_reconcile_collisions( Shared keys are stripped from stale duplicates (devices not registered this setup session); a duplicate left without any keys is removed. A collision - with a device registered this setup session raises. Devices and child - devices share the per-config-entry key namespace, so both are considered. + with a device registered this setup session raises. """ matched_device_id: str | None = None if matched_device is not None: matched_device_id = matched_device.id - if isinstance(matched_device, ChildDeviceEntry): - identifiers = matched_device.identifiers | identifiers - elif not matched_device.has_composite_identifiers: + if not matched_device.has_composite_identifiers: identifiers = matched_device.identifiers | identifiers connections = matched_device.connections | connections colliding = self.devices.get_colliding_device_ids( @@ -3694,14 +3690,6 @@ def _async_reconcile_collisions( config_entry_id=config_entry.entry_id, exclude_device_id=matched_device_id, ) - child_colliding: dict[str, set[tuple[str, str]]] = {} - for identifier in identifiers: - if ( - child_holder := self.child_devices.get_entry( - identifiers={identifier}, config_entry_id=config_entry.entry_id - ) - ) is not None and child_holder.id != matched_device_id: - child_colliding.setdefault(child_holder.id, set()).add(identifier) live_device_ids = self._live_device_ids.get(config_entry.entry_id, ()) for holder_id, (shared_identifiers, shared_connections) in colliding.items(): if holder_id not in live_device_ids: @@ -3713,15 +3701,6 @@ def _async_reconcile_collisions( f"{sorted(shared_identifiers | shared_connections)} are already " f"registered for device {holder_id} of the same config entry", ) - for holder_id, child_shared_identifiers in child_colliding.items(): - if holder_id not in live_device_ids: - continue - raise DeviceInfoError( - config_entry.domain, - device_info, - f"identifiers {sorted(child_shared_identifiers)} are already " - f"registered for child device {holder_id} of the same config entry", - ) for holder_id, (shared_identifiers, shared_connections) in colliding.items(): holder = self.devices[holder_id] remaining_identifiers = holder.identifiers - shared_identifiers @@ -3746,33 +3725,6 @@ def _async_reconcile_collisions( if shared_connections: strip_values["new_connections"] = remaining_connections self._async_update_device(holder_id, allow_collisions=True, **strip_values) - for holder_id, child_shared_identifiers in child_colliding.items(): - # A colliding device removed above may have cascade-removed this child - if holder_id not in self.child_devices: - continue - child_holder = self.child_devices[holder_id] - child_remaining_identifiers = ( - child_holder.identifiers - child_shared_identifiers - ) - if not child_remaining_identifiers: - _LOGGER.debug( - "Removing child device %s, its identifiers are all registered " - "by another device of the same config entry", - holder_id, - ) - self.async_remove_device(holder_id) - continue - _LOGGER.debug( - "Stripping %s from child device %s, registered by another device " - "of the same config entry", - sorted(child_shared_identifiers), - holder_id, - ) - self._async_update_child_device( - holder_id, - allow_collisions=True, - new_identifiers=child_remaining_identifiers, - ) @callback def _async_purge_colliding_deleted_devices( @@ -3878,16 +3830,12 @@ def _validate_child_identifiers( child_device_id: str, config_entry_id: str, identifiers: set[tuple[str, str]], - allow_collisions: bool, ) -> set[tuple[str, str]]: """Validate child device identifiers, raise on collision. Identifiers are unique per config entry, in a namespace shared between devices and child devices. """ - if allow_collisions: - return identifiers - for identifier in identifiers: if ( existing_child_device := self.child_devices.get_entry( diff --git a/tests/helpers/test_device_registry.py b/tests/helpers/test_device_registry.py index d1f1315a5506d9..17c5354fea116f 100644 --- a/tests/helpers/test_device_registry.py +++ b/tests/helpers/test_device_registry.py @@ -10501,7 +10501,7 @@ async def test_link_device_info_matching_child_raises( device_registry, mock_config_entry.entry_id ) - with pytest.raises(dr.DeviceInfoError, match="belong to child device"): + with pytest.raises(dr.DeviceInfoError, match="overlap with those of child device"): device_registry.async_get_or_create( config_entry_id=mock_config_entry.entry_id, identifiers=identifiers, @@ -10610,7 +10610,7 @@ async def test_primary_device_info_matching_child_raises( device_registry, mock_config_entry.entry_id ) - with pytest.raises(dr.DeviceInfoError, match="belong to child device"): + with pytest.raises(dr.DeviceInfoError, match="overlap with those of child device"): device_registry.async_get_or_create( config_entry_id=mock_config_entry.entry_id, identifiers={("test", "strip_outlet_1")}, @@ -11168,31 +11168,36 @@ async def test_async_cleanup_removes_child_device_with_stale_config_entry( @pytest.mark.usefixtures("hass") -async def test_child_device_stale_identifier_reconciliation( +async def test_device_info_with_connections_matching_child_raises( device_registry: dr.DeviceRegistry, mock_config_entry: MockConfigEntry, ) -> None: - """Test a stale child device's colliding identifiers are reconciled.""" + """Test a device info with connections claiming a child's identifier raises. + + Child device identifier collisions are always rejected, even for a stale child + and even when the device info carries connections. + """ _, child_device = _create_parent_and_child( device_registry, mock_config_entry.entry_id ) - # Make all devices of the config entry stale (a new setup session starts) + # A new setup session: the child device is stale, but is still not adopted device_registry.async_config_entry_unloaded(mock_config_entry.entry_id) - # A device registering the stale child device's only identifier replaces it, - # restoring the identity (id) from the deleted child device - device = device_registry.async_get_or_create( - config_entry_id=mock_config_entry.entry_id, - connections={(dr.CONNECTION_NETWORK_MAC, "12:34:56:ab:cd:ef")}, - identifiers={("test", "strip_outlet_1")}, - name="Not an outlet", - ) - assert isinstance(device, dr.DeviceEntry) - assert device.id == child_device.id + with pytest.raises(dr.DeviceInfoError, match="overlap with those of child device"): + device_registry.async_get_or_create( + config_entry_id=mock_config_entry.entry_id, + connections={(dr.CONNECTION_NETWORK_MAC, "12:34:56:ab:cd:ef")}, + identifiers={("test", "strip_outlet_1")}, + name="Not an outlet", + ) + + # The rejection leaves the child device untouched and creates no main device assert ( - device_registry.async_get(child_device.id, include_main_devices=False) is None + device_registry.async_get(child_device.id, include_main_devices=False) + is child_device ) - assert not device_registry.child_devices + assert len(device_registry.child_devices) == 1 + assert len(device_registry.devices) == 1 @pytest.mark.usefixtures("hass") @@ -11213,16 +11218,15 @@ async def test_live_child_device_identifier_collision_raises( update_events = async_capture_events(hass, dr.EVENT_DEVICE_REGISTRY_UPDATED) # A device matched by its own identifier that also claims a live child's identifier - # collides with the child and is rejected by reconciliation. (Claiming only the - # child's identifier instead routes to conversion, covered separately.) - with pytest.raises(dr.DeviceInfoError, match="already registered for child"): + # collides with the child and is rejected + with pytest.raises(dr.DeviceInfoError, match="overlap with those of child device"): device_registry.async_get_or_create( config_entry_id=mock_config_entry.entry_id, identifiers={("test", "hub"), ("test", "strip_outlet_1")}, name="Hub", ) - # The raise precedes every strip/remove in reconciliation, so nothing changed + # The raise precedes reconciliation, so nothing changed unchanged_child = device_registry.async_get(child_device.id) assert isinstance(unchanged_child, dr.ChildDeviceEntry) assert unchanged_child is child_device @@ -11870,36 +11874,19 @@ async def test_child_device_identifier_collision_with_other_child( ) -@pytest.mark.parametrize( - ("child_identifiers", "expected_remaining"), - [ - pytest.param({("test", "strip_outlet_1")}, None, id="all_taken_removes_child"), - pytest.param( - {("test", "strip_outlet_1"), ("test", "strip_outlet_1_alias")}, - {("test", "strip_outlet_1_alias")}, - id="partial_strip", - ), - ], -) @pytest.mark.usefixtures("hass") -async def test_stale_child_device_collision_stripped_or_removed( +async def test_stale_child_device_identifier_collision_raises( device_registry: dr.DeviceRegistry, mock_config_entry: MockConfigEntry, - child_identifiers: set[tuple[str, str]], - expected_remaining: set[tuple[str, str]] | None, ) -> None: - """Test a stale child device colliding with a registration is stripped or removed. + """Test a device claiming a stale child's identifier raises. - Registering a device matched by its own identifier that also claims a stale child's - identifier strips that identifier from the child, removing the child if it has no - other identifier. + Child device identifier collisions are rejected regardless of whether the child + was registered this setup session; stale children are never stripped or removed. """ _, child_device = _create_parent_and_child( device_registry, mock_config_entry.entry_id ) - device_registry.async_update_child_device( - child_device.id, new_identifiers=child_identifiers - ) hub = device_registry.async_get_or_create( config_entry_id=mock_config_entry.entry_id, identifiers={("test", "hub")}, @@ -11908,18 +11895,62 @@ async def test_stale_child_device_collision_stripped_or_removed( # A new setup session: every device of the config entry is now stale device_registry.async_config_entry_unloaded(mock_config_entry.entry_id) - registered = device_registry.async_get_or_create( + with pytest.raises(dr.DeviceInfoError, match="overlap with those of child device"): + device_registry.async_get_or_create( + config_entry_id=mock_config_entry.entry_id, + identifiers={("test", "hub"), ("test", "strip_outlet_1")}, + name="Hub", + ) + + # The rejection leaves the child device and the hub untouched + assert ( + device_registry.async_get(child_device.id, include_main_devices=False) + is child_device + ) + assert child_device.identifiers == {("test", "strip_outlet_1")} + assert device_registry.async_get(hub.id) is hub + assert hub.identifiers == {("test", "hub")} + + +@pytest.mark.usefixtures("hass") +async def test_get_or_create_child_identifier_owned_by_other_child_raises( + device_registry: dr.DeviceRegistry, + mock_config_entry: MockConfigEntry, +) -> None: + """Test a child registration claiming another child's identifier raises. + + A registration spanning the identifiers of two children is rejected instead of + merging them, even when the children are stale. + """ + parent, child_device = _create_parent_and_child( + device_registry, mock_config_entry.entry_id + ) + other_child = device_registry.async_get_or_create_child( config_entry_id=mock_config_entry.entry_id, - identifiers={("test", "hub"), ("test", "strip_outlet_1")}, - name="Hub", + identifiers={("test", "strip_outlet_2")}, + parent_device_id=parent.id, + name="Outlet 2", ) - # The registration is matched to the hub by its own identifier and adopts the - # stripped identifier - assert registered.id == hub.id - assert ("test", "strip_outlet_1") in registered.identifiers + # A new setup session: both children are stale + device_registry.async_config_entry_unloaded(mock_config_entry.entry_id) + + with pytest.raises(dr.DeviceInfoError, match="already registered for child"): + device_registry.async_get_or_create_child( + config_entry_id=mock_config_entry.entry_id, + identifiers={("test", "strip_outlet_1"), ("test", "strip_outlet_2")}, + parent_device_id=parent.id, + name="Merged outlet", + ) - result = device_registry.async_get(child_device.id, include_main_devices=False) - assert (result.identifiers if result is not None else None) == expected_remaining + # The rejection leaves both children untouched + assert ( + device_registry.async_get(child_device.id, include_main_devices=False) + is child_device + ) + assert ( + device_registry.async_get(other_child.id, include_main_devices=False) + is other_child + ) @pytest.mark.usefixtures("hass")