New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mqtt.homeassistant] Fix binding crash when home assistant discovery topics update with content #13518
[mqtt.homeassistant] Fix binding crash when home assistant discovery topics update with content #13518
Conversation
Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
I believe I picked this pattern of recreating thing with list of channels from some other binding. Is it ok? Or are there side effects eg to links? I have not tested this with full setup, only unit tests |
Tested with mqtt.generic 3.4.0-SNAPSHOT https://github.com/ssalonen/openhab2-addons/releases/tag/mqtt-homeassistant-b2e6c55 succesfully, seems to work fine |
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
.../java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java
Outdated
Show resolved
Hide resolved
@antroids : would you like to review that change ? |
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Yes, looks interesting, let me check. |
.../java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Waiting for @antroids review before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good. Just two minor issues found.
It would be better to create two different PRs next time: one with Bug fix and one with refactoring.
...ssistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Vacuum.java
Outdated
Show resolved
Hide resolved
.../java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java
Outdated
Show resolved
Hide resolved
Co-Authored-by: @antroids github handle Signed-off-by: Sami Salonen <ssalonen@gmail.com>
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
…topics update with content (openhab#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-Authored-by: @antroids github handle Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
…topics update with content (openhab#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-Authored-by: @antroids github handle
…topics update with content (openhab#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-Authored-by: @antroids github handle
…topics update with content (openhab#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-Authored-by: @antroids github handle
…topics update with content (openhab#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes openhab#13517 Possibly resolves openhab#9711 and openhab#12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-Authored-by: @antroids github handle Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
Fixes #13517
Possibly resolves #9711 and #12295 as well.
The implementation does not handle updating discovery topics -- when the content is not exactly the same. Example of real life scenario documented in #13517 with OpenMQTTGateway. To be honest, I am not sure if there are other kind of situations when we might hit this error.
This fix ensures that conflicting channels are first removed, and only then we add new channels. This means that more recent discovery data always overrides older... at least when it comes to channels.
Signed-off-by: Sami Salonen ssalonen@gmail.com