fix: reuse an existing model property, and refuse a child that shadows an ancestor - #69
Merged
Merged
Conversation
…s an ancestor Two defects reported against 0.21.0's DeviceTreeBuilder, both silent. lines later, added the PROPERTY unconditionally. GroupedPropertyDict .add_property is a wholesale `self._properties[id] = property`, so a producer handing over a model it had already populated got that property swapped for a fresh one. The value was the least of it: _change_callbacks, _set_callbacks and _entity_setter are instance state on the replaced object, so a producer that wired inbound control lost the actuator while $description kept advertising settable: true, and an arriving /set did nothing. Nor did it self-heal. 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 its reason for accepting a model rather than creating one, which made it a documented guarantee the code did not provide. An existing property is now reused, and the builder records only properties it actually created, so remove() deletes what it added and leaves what the producer owned. A spec whose python type disagrees with the property already there raises rather than binding a Homie twin to a mismatched observable. no check, so a child carrying an ancestor's id made that ancestor 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 expressing "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 need to be unique within a tree's ancestry. Closes #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
dcj
force-pushed
the
fix/reuse-existing-model-properties
branch
from
August 21, 2026 01:10
acbe551 to
63a53c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #66, and fixes the silent half of #67.
#66: the builder replaced properties in a model it does not own
_materializeguarded the model group withhas_group, then two lines later added the property unconditionally, andGroupedPropertyDict.add_propertyis a wholesaleself._properties[id] = property.The value loss was the least of it.
_change_callbacks,_set_callbacksand_entity_setterare instance state on the replaced object, so a producer that wired inbound control lost the actuator while$descriptionkept advertisingsettable: true. An arriving/setdid nothing.It did not self-heal either: the builder path seeds only a static
initial_value, andProperty.set_valuefires callbacks only on a real change, so a value written once at group creation never republished.This is the exact case
DeviceTreeBuilderdocuments as its reason for accepting a model rather than creating one, so it was a documented guarantee the code did not provide.Fix: reuse an existing property; record only properties the builder created, so
remove()deletes what it added and leaves what the producer owned; raise when a spec's python type disagrees with the property already there, rather than binding a Homie twin to a mismatched observable. That last one answers the open question in #66.#67 (silent half): a child could shadow an ancestor
Device.__init__appended toparent._childrenwith no check, so a child carrying the root's id made the root name itself in its ownchildrenand put two devices on the same topics. No exception, no warning.The obvious way to reach it was expressing "capabilities on the root" as a
DeviceSpecwithparent=Noneand the root's own id, which is a real thing to want and which the builder does not support yet. Failing loudly beats materializing a malformed tree. Ids still only need to be unique within a tree's ancestry, so the same id under a different root is unaffected.#67's actual feature request, materializing capabilities onto the root, is not addressed here.
Verification
669 tests pass (658 before, 11 new), including the reproductions from both issues verbatim. ruff and markdownlint clean.
🤖 Generated with Claude Code