prevent bindings from removing things #3205
Conversation
| if (!(ThingStatus.UNKNOWN.equals(newStatus) || ThingStatus.ONLINE.equals(newStatus) | ||
| || ThingStatus.OFFLINE.equals(newStatus) || ThingStatus.REMOVED.equals(newStatus))) { | ||
| throw new IllegalArgumentException(MessageFormat.format( | ||
| "Illegal status {0}. Bindings only may set {1}, {2}, {3} or {4}.", newStatus, |
There was a problem hiding this comment.
Why using indices at all if the order is iterative?
Doesn't it add workload to the next one that would like to add another one in front of the others?
| MessageFormat.format("Illegal status {0}. Bindings only may set {1}, {2}, {3} or {4}.", status, | ||
| ThingStatus.UNKNOWN, ThingStatus.ONLINE, ThingStatus.OFFLINE, ThingStatus.REMOVED)); | ||
| MessageFormat.format("Illegal status {0}. The thing was in state {1} and not in {2}", newStatus, | ||
| oldStatus, ThingStatus.REMOVING)); |
There was a problem hiding this comment.
Why using indices at all if the order is iterative?
Doesn't it add workload to the next one that would like to add another one in front of the others?
...and making the documentation more explicit about this. So far, bindings simply could set things to REMOVED at any time so they got dropped from the database. Now it is ensured they can only do so after the framework set it to REMOVING, i.e. they only are allowed to indicate that the handleRemove() is finished, but cannot trigger the process anymore. fixes eclipse-archived#3199 Signed-off-by: Simon Kaufmann <[email protected]>
|
Really good point, I didn't actually think about it when moving the code. |
|
Thanks |
...as MessageFormat needs the indices. relates to eclipse-archived#3205 Signed-off-by: Simon Kaufmann <[email protected]>
...as MessageFormat needs the indices. relates to #3205 Signed-off-by: Simon Kaufmann <[email protected]>
|
If I understand this, it's not possible for a bundle to request that a thing is removed? Is that correct? I would suggest that bridges at least should be able to request that a thing under its control is removed. If this isn't available, I think there will be quite some confusion about removing devices where a device is removed from a bridge. It will result in a two stage removal process where the device has to be removed from the bridge, and then the thing deleted. |
Yes, correct for the ThingHandler. This was not changed with this PR though, it was like that before. Generally it's a bad idea to mess with Things, even for a bridge. Bindings don't create Things magically (they put them into the inbox) and they should not delete them without the user's consent. Nevertheless, I agree that as of today there is no suitable way for a bridge to indicate that it knows a device is gone. How about adding another ThingStatusDetail, it can be set to OFFLINE/"DEVICE_IS_GONE" (for the lack of a better name right now...)? Of course, if a solution doesn't want to bother the users with Thing management, there could be a service cleaing up those Things in the same way as it would "auto-approve" Things from the Inbox. Does that make sense for you? Would you create an issue for it then? |
I don't disagree, but we now have a situation where a user has to "consent" twice and this is confusing... A concrete case -: In ZWave, to exclude a device, you put the bridge into exclude mode, then go to the device and click a button. The bridge then gets a message saying "Node 4 is removed". The user is now confused as to why Node 4 (ie the thing) is still there, and they now have to go and remove the thing as well. Currently, when the bridge gets this message, it marks the thing as removed. It's not normal for the binding to tell the network to remove a specific node (except in failure case) - just to put the zwave network into exclusion mode, and then to remove the node when you click the button. So, we couldn't have (for example) an implementation where the user simply deleted the thing, and it was excluded from the network as there's no guarantee that the thing (ie zwave device) that they deleted will be excluded. One other scenario - at the moment, there's no easy way to update a things "configuration" (not parameters, but channels etc) when the XML is updated for the thing-type. Currently I tell people to delete the thing and add it back again in order to update the definition - if we linked "delete the thing" directly to removing the device from the network we would have another problem...
I think your approach sounds ok - have a device_is_gone state and allow an auto approval to have it removed. I wonder if the implementation shouldn't check that this can only be done by the things bridge (if it has a bridge) - but that's a detail. I'll raise an issue - thanks. |
IIRC there is already an issue (or perhaps only a comment in an issue) to store only stuff that is changed and so new sections in the thing type are considered automatically. |
|
See #2555 |
...and making the documentation more explicit about this.
So far, bindings simply could set things to REMOVED at any time so they got
dropped from the database. Now it is ensured they can only do so after the
framework set it to REMOVING, i.e. they only are allowed to indicate that
the handleRemove() is finished, but cannot trigger the process anymore.
fixes #3199
Signed-off-by: Simon Kaufmann [email protected]