Fix bug in migrateThingType to not call managed thing provider directly #3162
Conversation
The `migrateThingType` call the managed thing provider directly instead of checking if it's actually a managed thing. This check is done in the `thingUpdated` method. Changed the code so it makes the same call as in `thingUpdated`. Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
|
Thank you a lot @Hilbrand . |
| @@ -472,6 +447,35 @@ protected synchronized void deactivate(ComponentContext componentContext) { | |||
| readyService.unregisterTracker(this); | |||
| } | |||
|
|
|||
| private void thingUpdated(final Thing thing) { | |||
| thingUpdatedLock.add(thing.getUID()); | |||
| AccessController.doPrivileged((PrivilegedAction<@Nullable Void>) () -> { | |||
There was a problem hiding this comment.
I think that the AccessController can be removed, it is deprecated for removal in Java 17 anyway and I know of no code that would depend on it in openHAB. But we should probably clean-up all code that uses the SecurityManager after we switched to Java 17 at the end of this year.
...s/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java
Outdated
Show resolved
Hide resolved
| if (provider instanceof ManagedProvider) { | ||
| @SuppressWarnings("unchecked") | ||
| ManagedProvider<Thing, ThingUID> managedProvider = (ManagedProvider<Thing, ThingUID>) provider; | ||
| managedProvider.update(thing); | ||
| } else { | ||
| logger.debug("Only updating thing {} in the registry because provider {} is not managed.", | ||
| thing.getUID().getAsString(), provider); | ||
| Thing oldThing = thingRegistry.get(thing.getUID()); | ||
| if (oldThing == null) { | ||
| throw new IllegalArgumentException( | ||
| MessageFormat.format("Cannot update thing {0} because it is not known to the registry", | ||
| thing.getUID().getAsString())); | ||
| } | ||
| thingRegistry.updated(provider, oldThing, thing); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
In fact this contradicts the description in the ThingHandlerCallback interface:
/**
* Informs about an update of the whole thing.
*
* @param thing thing that was updated (must not be null)
* @throws IllegalStateException if the {@link Thing} is read-only.
*/
void thingUpdated(Thing thing);
However, if we strictly follow the interface description this makes any dynamic properties or updates to file-based things impossible. IMO the interface desciption should be updated to reflect the current code.
I also wonder why we check if the thing exists. I would be very astonished if we got a provider from the ThingRegistry when the thing itself is not in the registry. I think this was introduced in #1134 to prevent the null-error for Registry.updated. It would probably be better to check if the thing is present in the registry before we try to get the provider and fail early if it is not in the registry (the registry fails silently if you try to update a thing that is not in the registry).
...s/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java
Show resolved
Hide resolved
|
Thank you for analyzing this and solving this. For the record, I believe this migrateThingType method was introduced for the ZWave binding. It is also used in the Sonos, Shelly and Miio bindings. |
|
It's also used in 3rd party add-ons. |
|
@J-N-K the itests do seem to fail due to this change. But debugging these tests is a really horrible, so it will take me some time. Do you know how to enable logging in the test so it shows? The only log I get is from the iteat themselves, but not from the core code. |
|
I usually set the log messages I'm interested in to level ERROR, then they show up. |
|
You can easily change the log level for all loggers here: openhab-core/itests/itest-include.bndrun Line 40 in d00d14d |
|
No chance to finish before the new milestone is generated later today? |
wborn
added
bug
|
The tests failed because the thing handler was unregistered before migration but the registration of the new handler was missing. @openhab/core-maintainers Can someone else review now that I contributed to this PR? Thanks. |
J-N-K
force-pushed
the
fix-migrateThingTypeBug
branch
from
24d3629 to
ebca363
Compare
|
^ |
…ly (openhab#3162) The `migrateThingType` call the managed thing provider directly instead of checking if it's actually a managed thing. This check is done in the `thingUpdated` method. Changed the code so it makes the same call as in `thingUpdated`. Also-by: Jan N. Klug <github@klug.nrw> Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl> GitOrigin-RevId: 207c4a0
The
migrateThingTypecall the managed thing provider directly instead of checking if it's actually a managed thing. This check is done in thethingUpdatedmethod. Changed the code so it makes the same call as inthingUpdated.This bug was trigged by a change in the systeminfo binding: openhab/openhab-addons#13708.
I've refactored the
thingUpdatedinto a private method that is also called frommigrateThingTypeasthingUpdatedperforms the sameupdatemethod as was done inmigrateThingTypewhen the is managed. I've kept the lock anddoPrivilegedin the private method. I'm not sure if that is needed, but it seems to make sense as it's a guard in theupdateThing, so why not inmigrateThingType.