Discovery of new Things causes configuration parameters of existing Things to be updated

[jlaur]

I noticed this behavior while working on openhab/openhab-addons#19516, but it turned out to have nothing to do with my changes, or the Miele binding at all. I was able to reproduce the issue with the Hue binding as well.

First, create a new discovered Thing. It needs to have its ThingUID generated in a consistent way, so that another discovery will create a result with the same ThingUID:

Now, let's change one parameter to something else:

And finally, let's trigger discovery:

And rather unexpected, our existing Thing is now "repaired", i.e. its configuration parameters are changed as a result of the discovery:

Expected Behavior

Discovery should not affect existing Things.

Current Behavior

Existing Thing is manipulated as a result of the discovery. handleConfigurationUpdate is called by the framework, and the existing thing handler derived from BaseThingHandler goes through dispose and initialize if that method is not implemented.

Possible Solution

For Miele and Hue the issue was reproduced by triggering mDNS discovery. However, currently I don't suspect MDNSDiscoveryParticipant in particular, but I'm rather afraid it's a general issue. I need to confirm that.

Steps to Reproduce (for Bugs)

See description.

I have not been able to reproduce this in 5.0.2, so I think it's caused by a recent regression in 5.1.

Context

Your Environment

• Version used: 5.1.0-SNAPSHOT - Build 4854

[jlaur]

@Nadahar - FYI. 🙂

[jlaur]

I was able to reproduce this with build 4847 as well, built October 7th. Since #5032 was previously mentioned, this is no longer relevant to look into, since it was merged after that.

[Nadahar]

I'm trying to reproduce it, but can't. I'm wondering what the representationProperty is in your case. For me, it's the MAC address, and that might be what prevents the "new discovery" from being applied? The ThingID should be the same, as it's generated using the MAC address.

[Nadahar]

It might be worth giving some focus to this code:

openhab-core/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/internal/PersistentInbox.java

Lines 271 to 307 in e72baac

	ThingUID thingUID = discoveryResult.getThingUID();
	Thing thing = thingRegistry.get(thingUID);
	if (thing == null) {
	DiscoveryResult inboxResult = get(thingUID);
	if (inboxResult == null) {
	discoveryResultStorage.put(discoveryResult.getThingUID().toString(), discoveryResult);
	notifyListeners(discoveryResult, EventType.ADDED);
	logger.info("Added new thing '{}' to inbox.", thingUID);
	discoveryResultWrapper.future.complete(true);
	} else {
	if (inboxResult instanceof DiscoveryResultImpl resultImpl) {
	resultImpl.synchronize(discoveryResult);
	discoveryResultStorage.put(discoveryResult.getThingUID().toString(), resultImpl);
	notifyListeners(resultImpl, EventType.UPDATED);
	logger.debug("Updated discovery result for '{}'.", thingUID);
	discoveryResultWrapper.future.complete(true);
	} else {
	logger.warn("Cannot synchronize result with implementation class '{}'.",
	inboxResult.getClass().getName());
	}
	}
	} else if (managedThingProvider.get(thingUID) != null) {
	// only try to update properties if thing is managed
	logger.debug(
	"Discovery result with thing '{}' not added as inbox entry. It is already present as thing in the ThingRegistry.",
	thingUID);
	boolean updated = synchronizeConfiguration(discoveryResult.getThingTypeUID(),
	discoveryResult.getProperties(), thing.getConfiguration());
	if (updated) {
	logger.debug("The configuration for thing '{}' is updated...", thingUID);
	managedThingProvider.update(thing);
	}
	}

To me, it looks like this is the intended behavior for managed Things. Maybe some recent change just made it start working?

[mherwege]

It had been my understanding that this is indeed intended behavior, has been there as long as I know, and I actually rely on it. I have devices that may change IP, and because of discovery, the thing will get updated and keep functioning without any need to do anything. If you use file based config with the IP as a config, you are on your own of course. I used to have an internet box which did not allow me to assign fixed IP addresses and the device in question did also not have an option to configure a fixed IP (nikohomecontol binding).

[Nadahar]

I can reproduce it now. I was mistaken about the ThingUID, it wasn't identical in my test. Once I made a Thing from a discovery result, and then try to change the IP address of that once, the next discovery will "correct it".

So, I'm not quite sure what has changed. Has this been "out of order" for a while?

When it comes to the functionality, I'd say it's a bit surprising. I see that it has use cases, but it can certainly cause trouble at times as well.... maybe this should be a setting somewhere?

[Nadahar]

There's another "catch" here - this "automatic update" will only apply to properties that have been resolved by discovery, in my test case, that is just IP address and MAC address, everything will be left as is, but: properties that don't exist in the Thing configuration, but does exist in the discovery result, will also be ignored:

openhab-core/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/internal/PersistentInbox.java

Lines 324 to 327 in e72baac

	// Check if the key is present in the configuration.
	if (!config.containsKey(propertyKey)) {
	continue;
	}

This isn't exactly "intuitive behavior", so I think the details of what is done here should be thoroughly documented somewhere. This has implications for what properties binding authors should "set" during discovery, and they need to know how it behaves to be able to make the right decision.

I see the use case of updating the IP address of dynamically assigned IPs (although I always handle that by reserving fixed IPs by MAC address in the DHCP server configuration, so that they are always assigned the same IP), but are there other situations where this is actually desirable? Maybe it would be better if it was an "autoupdate configuration" option that was off by default?

[jlaur]

Thanks to you both for your feedback, indeed it seems intended. I may have made some mistake when I tried to reproduce on 5.0.2, I will try to run those tests again.

I'm quite surprised by this functionality, but I guess that's another story than this bug report, because now it seems not to be a bug.

I spent a few hours struggling with this in the Miele multicast context. The multicast parameter is made optional, so it's possible to clear it to not receive multicast events. But that's overwritten by discovery.

Also, the concept relies on the ThingUID not to have been changed to something else, and on managed configuration as well. I wonder if documented somewhere.

[Nadahar]

I doubt that these rules are documented, I certainly can't remember reading it. It's quite intricate. What you could do to avoid it is to not set the interface address during discovery, but if that means to disable multicast, it would be off by default.

I would rather make it a separate configuration option, "disable multicast" for the Thing, and then let the binding resolve the correct interface if it's not configured. Most people have no idea what multicast is/does, so it's better if they don't have to configure it IMO.

[mherwege]

It is documented here: https://www.openhab.org/docs/developer/bindings/#re-discovered-results-and-things

The representation property is a concept that got introduced later than rhis. You could argue it should work on representation property, and not UID.