Skip to content

Commit 63a53c7

Browse files
dcjclaude
andcommitted
fix: refuse two children of one parent sharing an id
The same defect as the ancestor guard, one step sideways, and equally silent. Two children of one parent with the same id derive the same base topic, so their $description publishes overwrite each other on the broker and whichever wrote last defines the device; the parent meanwhile names that child twice in its own `children` list, which is malformed. Observed before the guard: children_ids() : ['circuit-1', 'circuit-1'] a's nodes : ['meter'] b's nodes : ['switch'] ...both retained at : ebus/5/circuit-1/$description Costs no new state: the parent already tracks its children, so this is a scan of parent.children() on a construction that already walks the ancestor chain. delete() detaches a child, so recreating one after deleting it is not a false positive, and DeviceTreeBuilder.add() already handles re-fired lifecycles at its own level. Refs #67 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5a0a733 commit 63a53c7

3 files changed

Lines changed: 28 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan
1212

1313
- `build_from_declarations` and `DeviceTreeBuilder` now REUSE an observable property the model already holds instead of replacing it. `_materialize` guarded the model group with `has_group` and then, two lines later, added the property unconditionally, and `GroupedPropertyDict.add_property` is a wholesale `self._properties[property_id] = property`. So a producer handing over a model it had already populated got that property swapped for a fresh one, losing its value and, worse, every callback and `entity_setter` attached to it: `$description` kept advertising `settable: true` while the actuator behind it was gone, and an arriving `/set` did nothing. Nor was it self-healing, since the builder path seeds only a static `initial_value` and `Property.set_value` fires callbacks only on an actual change, so a value written once at group creation never republished. This is the exact case `DeviceTreeBuilder` documents as the reason it accepts a model rather than creating one, which made the gap a documented guarantee the code did not provide. The builder now records only properties it actually created, so removing a device deletes what it added and leaves what the producer owned. A spec whose python type disagrees with the property already in the model raises rather than binding a Homie twin to a mismatched observable. ([#66](https://github.com/electrification-bus/python-sdk/issues/66))
1414

15-
- `Device` refuses a child whose id collides with any ancestor's. `Device.__init__` appended to `parent._children` with no check, so a child carrying the root's id made the root name itself in its own `children` and put two devices on the same topics, with no exception and no warning. The obvious way to reach it was trying to express "capabilities on the root" as a `DeviceSpec` with `parent=None` and the root's own id, which is a real thing to want and which the builder does not yet support; failing loudly beats materializing a malformed tree. Ids still only have to be unique within a tree's ancestry, so the same id under a different root is unaffected. ([#67](https://github.com/electrification-bus/python-sdk/issues/67))
15+
- `Device` refuses a child whose id collides with any ancestor's. `Device.__init__` appended to `parent._children` with no check, so a child carrying the root's id made the root name itself in its own `children` and put two devices on the same topics, with no exception and no warning. The obvious way to reach it was trying to express "capabilities on the root" as a `DeviceSpec` with `parent=None` and the root's own id, which is a real thing to want and which the builder does not yet support; failing loudly beats materializing a malformed tree. Ids still only have to be unique within a tree's ancestry, so the same id under a different root is unaffected. The same defect one step sideways is refused too: two children of one parent sharing an id derived the same base topic, so their `$description` publishes overwrote each other on the broker while the parent named that child twice in its own `children` list. `delete()` detaches a child, so recreating one after deleting it is not affected. ([#67](https://github.com/electrification-bus/python-sdk/issues/67))
1616

1717
### Documentation
1818

src/ebus_sdk/homie.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,6 +1564,16 @@ def __init__(
15641564
"same topics and the ancestor would name itself in its own children"
15651565
)
15661566
ancestor = ancestor.parent()
1567+
# Same defect one step sideways: two children of one parent sharing an
1568+
# id derive the same base topic, so their $description publishes
1569+
# overwrite each other on the broker and the parent names the child
1570+
# twice in its own `children`. The parent already tracks its children,
1571+
# so this costs no new state.
1572+
if any(child.id() == id for child in parent.children()):
1573+
raise ValueError(
1574+
f"Device id={id}: parent id={parent.id()} already has a child with this id; "
1575+
"both would publish to the same topics and the parent would name it twice"
1576+
)
15671577

15681578
# The domain is a per-TREE property, like the connection and the QoS: one
15691579
# tree publishes under one prefix, and a child under a different domain

tests/test_homie_device.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3074,3 +3074,20 @@ def test_the_same_id_under_a_different_root_is_fine(self, mock_paho):
30743074
Device("circuit-1", parent=root_b)
30753075
assert root_a.children_ids() == ["circuit-1"]
30763076
assert root_b.children_ids() == ["circuit-1"]
3077+
3078+
def test_two_children_of_one_parent_cannot_share_an_id(self, mock_paho):
3079+
"""Same defect one step sideways: identical base topics, and a child named twice."""
3080+
root, _ = _make_device(mock_paho, device_id="enclosure-1")
3081+
Device("circuit-1", parent=root)
3082+
with pytest.raises(ValueError, match="already has a child with this id"):
3083+
Device("circuit-1", parent=root)
3084+
assert root.children_ids() == ["circuit-1"]
3085+
3086+
def test_recreating_a_deleted_child_is_allowed(self, mock_paho):
3087+
"""delete() detaches, so rebuild-after-delete is not a false positive."""
3088+
root, _ = _make_device(mock_paho, device_id="enclosure-1")
3089+
child = Device("circuit-1", parent=root)
3090+
child.delete()
3091+
assert root.children_ids() == []
3092+
Device("circuit-1", parent=root) # must not raise
3093+
assert root.children_ids() == ["circuit-1"]

0 commit comments

Comments
 (0)