[discovery] Added validation for relation between ThingUID and BridgeUID #1481
Conversation
| @@ -36,7 +36,7 @@ | |||
| private final ThingUID thingUID; | |||
|
|
|||
| private @Nullable ThingUID bridgeUID; | |||
| private final Map<String, Object> properties = new HashMap<>(); | |||
| private @Nullable Map<String, Object> properties; | |||
There was a problem hiding this comment.
I would be careful here since it changes behavior of the builder. I know that 3.0.x addons must be recompiled to new packages, however such small changes remain undocumented and can blow up only when running a discovery with (most often) real things.
There was a problem hiding this comment.
You are right. I reverted it for the moment. Even if my change streamlined this builder with other OHC builders.
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
cweitkamp
force-pushed
the
feature-discovery-thinguid-validation
branch
from
0ed1f2a
to
7864ba3
Compare
cweitkamp
added
rebuild
| } | ||
|
|
||
| @Test | ||
| @Ignore |
There was a problem hiding this comment.
To streamline our builders (following eclipse-archived/smarthome#5929) I first changed the behavior of the withProperties(Map<String, Object>) method to replace existing properties map by the one set. This has been reverted after #1481 (comment) and thus the test is failing now. Will come up with a follow-up PR if the change should be applied at all to separate it from this feature.
There was a problem hiding this comment.
Have a look here for the changes I am talking about.
|
Thanks for merging. The unit test were not yet migrated to JUnit 5. I fixed them in #1619. |
|
Thanks for #1619 and sorry that I didn't notice that. |
Fixes exceptions thrown in OH3 when building discovery results for things with bridges where the thing UID is missing the bridge UID. Related to openhab/openhab-core#1481 Signed-off-by: Wouter Born <github@maindrain.net>
Fixes exceptions thrown in OH3 when building discovery results for things with bridges where the thing UID is missing the bridge UID. Related to openhab/openhab-core#1481 Signed-off-by: Wouter Born <github@maindrain.net>
Fixes exceptions thrown in OH3 when building discovery results for things with bridges where the thing UID is missing the bridge UID. Related to openhab/openhab-core#1481 Signed-off-by: Wouter Born <github@maindrain.net>
Fixes exceptions thrown in OH3 when building discovery results for things with bridges where the thing UID is missing the bridge UID. Related to openhab/openhab-core#1481 Signed-off-by: Wouter Born <github@maindrain.net>
Fixes exceptions thrown in OH3 when building discovery results for things with bridges where the thing UID is missing the bridge UID. Related to openhab/openhab-core#1481 Signed-off-by: Wouter Born <github@maindrain.net>
|
FYI I spotted this one while migrating my own bindings. Now it did hit bacnet integration. This validation rule is completely unclear to me. It was not enforced in 2.5 hence it impacts migration and makes maintenance of 2.5 and 3.0 versions troublesome. I believe this check is unnecessary since each and every binding declares bridge-thing matches. The
This change does introduce assumption that ThingUID include bridge identifier. I believe that this change might be also problematic when binding use multiple nesting levels. For example when there is more than just one bridge above the thing. Please also update developer/migration docs accordingly so other people do not fall into same trap all over again. |
|
It does feel that this is a bad idea. If we think back a few years, we had these links (ie the thing type, bridge were encoded in the thing uid). This was removed as it caused problems when the thing type or bridge changed. A UID should be universally unique - it should NEVER change through the life of a system (in theory at least). Now, by imposing a hard link to the bridge UID, it is no longer possible to move a thing between bridges (if I understand this correctly) without changing the thing UID (which IMHO is a no-no and will have side effects through persistence etc). |
|
I understand your concerns. But as stated in #1800 (comment) I do not seen a problem here. This validation is restricted for building discovery results and will not prevent switching bridges during runtime. AFAIK the UI uses a similar approach to create random ThingUIDs for Things related to Bridges. And it is the recommended syntax to be used for textual configuration. Both of you are long time contributors and probably remember a lot more historical changes than I do so if it really is a problem or a step backwards we can revert it before final OH 3 release. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/satel-binding-support-announcements-and-feature-requests/56135/252 |
…UID (openhab#1481) * Added validation for relation between ThingUID and BridgeUID Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de> GitOrigin-RevId: 7eb6d39
ThingUIDandBridgeUIDSee discussion in openhab/openhab-addons#7496 (comment)
Signed-off-by: Christoph Weitkamp github@christophweitkamp.de