Skip to content
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.

Sign up for GitHub

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

Fix bug in migrateThingType to not call managed thing provider directly #3162

Merged
merged 2 commits into from
Dec 10, 2022
Merged

Fix bug in migrateThingType to not call managed thing provider directly #3162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e281d2
Fix bug in migrateThingType to not call managed thing provider directly
Hilbrand Nov 15, 2022
ebca363
fixes
J-N-K Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
66 changes: 36 additions & 30 deletions ...rg.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Thing> 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<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;
Comment on lines -273 to -288
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

});
thingUpdatedLock.remove(thing.getUID());
ThingManagerImpl.this.thingUpdated(thing);
}

@Override
Expand Down Expand Up @@ -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>) () -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final Provider<Thing> 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<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);
thingRegistry.updated(provider, oldThing, thing);
}
return null;
});
thingUpdatedLock.remove(thing.getUID());
}

wborn marked this conversation as resolved.
Show resolved Hide resolved
@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() {
Expand Down Expand Up @@ -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,
Expand Down