[config] Enable config validation for updates by handler

[J-N-K]

See #2682 (comment)

It was assumed that thing handlers only update valid configuration. After merging #2682 it was discovered that this is not true. This PR enables the same validation for updates through the handler. If an invalid configuration or otherwise illegal (not-initializable) thing is detected, the changes are discarded and a warning logged.

Signed-off-by: Jan N. Klug [email protected]

[cweitkamp]

I have the slight feeling that this will it work because the BaseThingHandler:: update configuration () methods replaces the configuration properties before calling the updateThing() methods on the callback.

openhab-core/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/BaseThingHandler.java

Lines 425 to 434 in 16bc932

	this.thing.getConfiguration().setProperties(configuration.getProperties());
	synchronized (this) {
	if (this.callback != null) {
	this.callback.thingUpdated(thing);
	} else {
	logger.warn(
	"Handler {} tried updating its configuration although the handler was already disposed.",
	this.getClass().getSimpleName());
	}
	}

Meaning the wrong parameters will be applied even if you discard the changes later.

[J-N-K]

I reworked that. Now the BaseThingHandler validates the configuration before they are applied or updated (both in updateThing and updateConfiguration). There is a slight chance that an invalid configuration is applied anyway if the implementation of the ThingHandler overrides these methods and does not do a validation.

Do you think updateProperties should also validate its parameters?

[cweitkamp]

Do you think updateProperties should also validate its parameters?

No, I don't think so. Do you see a specific reason for doing it?

[J-N-K]

After checking the code: no. I was wondering if it is possible to change configuration with updateProperties, but it seems that the only place where properties and configuration are mixed is when building a discovery result. After the thing is added from the INBOX, they are separated.

[cweitkamp]

If you are afraid that we loose this safety check by overridden methods we can try to move it deeper into the framework. E.g. into the ThingHandlerCallback::updateThing() method.

[J-N-K]

But then we are back where we were before: the wrong config is applied before it is validated. I think that overriding updateThing or updateConfiguration is no very common and if it is sone should be secured by review.

[cweitkamp]

Let's add a note about that to the JavaDoc. This way people know what the method is doing and what to take care of if they override it.

[J-N-K]

Done

[cweitkamp]

LGTM. Thanks.