From 207c4a04da39e045203f9ba06d52491289a61106 Mon Sep 17 00:00:00 2001 From: Hilbrand Bouwkamp Date: Sat, 10 Dec 2022 10:12:04 +0100 Subject: [PATCH] Fix bug in migrateThingType to not call managed thing provider directly (#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 Signed-off-by: Hilbrand Bouwkamp --- .../thing/binding/ThingHandlerCallback.java | 2 +- .../core/thing/internal/ThingManagerImpl.java | 66 ++++++++++--------- 2 files changed, 37 insertions(+), 31 deletions(-) diff --git a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java index bbad69fb362..2bbd491035b 100644 --- a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java +++ b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java @@ -77,7 +77,7 @@ public interface ThingHandlerCallback { * 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. + * @throws IllegalStateException if the {@link Thing} is can't be found */ void thingUpdated(Thing thing); diff --git a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java index 70cce8f606a..9745cfc16b1 100644 --- a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java +++ b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java @@ -262,32 +262,7 @@ private void handleBridgeChildStatusUpdate(Thing thing, ThingStatusInfo oldStatu @Override public void thingUpdated(final Thing thing) { - thingUpdatedLock.add(thing.getUID()); - AccessController.doPrivileged((PrivilegedAction<@Nullable Void>) () -> { - Provider provider = thingRegistry.getProvider(thing); - if (provider == null) { - throw new IllegalArgumentException(MessageFormat.format( - "Provider for thing {0} cannot be determined because it is not known to the registry", - thing.getUID().getAsString())); - } - if (provider instanceof ManagedProvider) { - @SuppressWarnings("unchecked") - ManagedProvider managedProvider = (ManagedProvider) 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; - }); - thingUpdatedLock.remove(thing.getUID()); + ThingManagerImpl.this.thingUpdated(thing); } @Override @@ -472,13 +447,40 @@ protected synchronized void deactivate(ComponentContext componentContext) { readyService.unregisterTracker(this); } + private void thingUpdated(final Thing thing) { + thingUpdatedLock.add(thing.getUID()); + final 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())); + } + AccessController.doPrivileged((PrivilegedAction<@Nullable Void>) () -> { + final Provider provider = thingRegistry.getProvider(thing); + if (provider == null) { + throw new IllegalArgumentException(MessageFormat.format( + "Provider for thing {0} cannot be determined because it is not known to the registry", + thing.getUID().getAsString())); + } + if (provider instanceof ManagedProvider) { + final ManagedProvider managedProvider = (ManagedProvider) provider; + managedProvider.update(thing); + } else { + logger.debug("Only updating thing {} in the registry because provider {} is not managed.", + thing.getUID().getAsString(), provider); + thingRegistry.updated(provider, oldThing, thing); + } + return null; + }); + thingUpdatedLock.remove(thing.getUID()); + } + @Override public void migrateThingType(final Thing thing, final ThingTypeUID thingTypeUID, final @Nullable Configuration configuration) { final ThingType thingType = thingTypeRegistry.getThingType(thingTypeUID); if (thingType == null) { throw new IllegalStateException( - MessageFormat.format("No thing type {0} registered, cannot change thing type for thing {1}", + MessageFormat.format("No thing type {0} registered, cannot change thing type for thing {1}", thingTypeUID.getAsString(), thing.getUID().getAsString())); } scheduler.schedule(new Runnable() { @@ -519,11 +521,15 @@ public void run() { thing.setProperty(entry.getKey(), entry.getValue()); } - // Change the ThingType + // set the new ThingTypeUID ((ThingImpl) thing).setThingTypeUID(thingTypeUID); - // Register the new Handler - ThingManager.updateThing() is going to take care of that - thingRegistry.update(thing); + // update the thing and register a new handler + thingUpdated(thing); + final ThingHandlerFactory newThingHandlerFactory = findThingHandlerFactory(thing.getThingTypeUID()); + if (newThingHandlerFactory != null) { + registerAndInitializeHandler(thing, newThingHandlerFactory); + } ThingHandler handler = thing.getHandler(); logger.debug("Changed ThingType of Thing {} to {}. New ThingHandler is {}.", thingUID,