OpenHAB
/
openhab-core
mirror of https://github.com/openhab/openhab-core.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

[config] Enable config validation for updates by handler (#2712) 

* Enable config validation for updates by handler

Signed-off-by: Jan N. Klug <github@klug.nrw>
pull/2720/head
J-N-K 2022-02-01 10:10:37 +01:00 committed by GitHub
parent 9520bfdfe7
commit 61f5e7f57d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 102 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/BaseThingHandler.java
View File

@ -25,6 +25,7 @@ import org.openhab.core.common.ThreadPoolManager;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.config.core.validation.ConfigValidationException;
import org.openhab.core.thing.Bridge;
import org.openhab.core.thing.Channel;
import org.openhab.core.thing.ChannelUID;
import org.openhab.core.thing.Thing;
import org.openhab.core.thing.ThingStatus;
@ -242,7 +243,7 @@ public abstract class BaseThingHandler implements ThingHandler {
* Updates the state of the thing. Will use the thing UID to infer the
* unique channel UID from the given ID.
*
* @param channel ID id of the channel, which was updated
* @param channelID id of the channel, which was updated
* @param state new state
*/
protected void updateState(String channelID, State state) {
@ -384,6 +385,12 @@ public abstract class BaseThingHandler implements ThingHandler {
* Informs the framework, that a thing was updated. This method must be called after the configuration or channels
* was changed.
*
* Any method overriding this method has to make sure that only things with valid configurations are passed to the
* callback. This can be achieved by calling
* {@link ThingHandlerCallback#validateConfigurationParameters(Thing, Map)}. It is also necessary to ensure that all
* channel configurations are valid by calling
* {@link ThingHandlerCallback#validateConfigurationParameters(Channel, Map)}.
*
* @param thing thing, that was updated and should be persisted
*/
@SuppressWarnings("PMD.CompareObjectsWithEquals")
@ -392,14 +399,25 @@ public abstract class BaseThingHandler implements ThingHandler {
throw new IllegalArgumentException(
"Changes must not be done on the current thing - create a copy, e.g. via editThing()");
}
ThingHandlerCallback callback = this.callback;
if (callback == null) {
logger.warn("Handler {} tried updating thing {} although the handler was already disposed.",
this.getClass().getSimpleName(), thing.getUID());
return;
}
try {
callback.validateConfigurationParameters(thing, thing.getConfiguration().getProperties());
thing.getChannels().forEach(channel -> callback.validateConfigurationParameters(channel,
channel.getConfiguration().getProperties()));
} catch (ConfigValidationException e) {
logger.warn(
"Attempt to update thing '{}' with a thing containing invalid configuration '{}', blocked. This is most likely a bug.",
thing.getUID(), thing.getConfiguration());
return;
}
synchronized (this) {
if (this.callback != null) {
this.thing = thing;
this.callback.thingUpdated(thing);
} else {
logger.warn("Handler {} tried updating thing {} although the handler was already disposed.",
this.getClass().getSimpleName(), thing.getUID());
}
this.thing = thing;
callback.thingUpdated(thing);
}
}
@ -417,20 +435,30 @@ public abstract class BaseThingHandler implements ThingHandler {
/**
* Updates the configuration of the thing and informs the framework about it.
*
* Any method overriding this method has to make sure that only valid configurations are passed to the callback.
* This can be achieved by calling {@link ThingHandlerCallback#validateConfigurationParameters(Thing, Map)}.
*
* @param configuration configuration, that was updated and should be persisted
*/
protected void updateConfiguration(Configuration configuration) {
Map<String, Object> old = this.thing.getConfiguration().getProperties();
ThingHandlerCallback callback = this.callback;
if (callback == null) {
logger.warn("Handler {} tried updating its configuration although the handler was already disposed.",
this.getClass().getSimpleName());
return;
}
try {
callback.validateConfigurationParameters(this.thing, configuration.getProperties());
} catch (ConfigValidationException e) {
logger.warn("Attempt to apply invalid configuration '{}' on thing '{}' blocked. This is most likely a bug.",
configuration, thing.getUID());
return;
}
try {
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());
}
callback.thingUpdated(thing);
}
} catch (RuntimeException e) {
logger.warn(
@ -443,7 +471,7 @@ public abstract class BaseThingHandler implements ThingHandler {
/**
* Returns a copy of the properties map, that can be modified. The method {@link
* BaseThingHandler#updateProperties(Map<String, String> properties)} must be called to persist the properties.
* BaseThingHandler#updateProperties(Map)} must be called to persist the properties.
*
* @return copy of the thing properties (not null)
*/

12
bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/binding/ThingHandlerCallback.java
View File

@ -83,13 +83,23 @@ public interface ThingHandlerCallback {
/**
* Validates the given configuration parameters against the configuration description.
*
* @param thing thing with the updated configuration (must no be null)
* @param thing thing with the updated configuration (must not be null)
* @param configurationParameters the configuration parameters to be validated
* @throws ConfigValidationException if one or more of the given configuration parameters do not match
* their declarations in the configuration description
*/
void validateConfigurationParameters(Thing thing, Map<String, Object> configurationParameters);
/**
* Validates the given configuration parameters against the configuration description.
*
* @param channel channel with the updated configuration (must not be null)
* @param configurationParameters the configuration parameters to be validated
* @throws ConfigValidationException if one or more of the given configuration parameters do not match
* their declarations in the configuration description
*/
void validateConfigurationParameters(Channel channel, Map<String, Object> configurationParameters);
/**
* Informs about an updated configuration of a thing.
*

9
bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/ThingManagerImpl.java
View File

@ -291,6 +291,14 @@ public class ThingManagerImpl
}
}
@Override
public void validateConfigurationParameters(Channel channel, Map<String, Object> configurationParameters) {
ChannelType channelType = channelTypeRegistry.getChannelType(channel.getChannelTypeUID());
if (channelType != null && channelType.getConfigDescriptionURI() != null) {
configDescriptionValidator.validate(configurationParameters, channelType.getConfigDescriptionURI());
}
}
@Override
public void configurationUpdated(Thing thing) {
if (!ThingHandlerHelper.isHandlerInitialized(thing)) {
@ -556,7 +564,6 @@ public class ThingManagerImpl
// called from the thing handler itself, therefore
// it exists, is initializing/initialized and
// must not be informed (in order to prevent infinite loops)
// we assume the handler knows what he's doing and don't check config validity
replaceThing(getThing(thingUID), thing);
} else {
Lock lock1 = getLockForThing(thing.getUID());

39
itests/org.openhab.core.thing.tests/src/main/java/org/openhab/core/thing/binding/BindingBaseClassesOSGiTest.java
View File

@ -155,6 +155,11 @@ public class BindingBaseClassesOSGiTest extends JavaOSGiTest {
public void initialize() {
updateStatus(ThingStatus.ONLINE);
}
@Override
public void updateConfiguration(Configuration configuration) {
super.updateConfiguration(configuration);
}
}
class SimpleBridgeHandler extends BaseBridgeHandler {
@ -606,6 +611,40 @@ public class BindingBaseClassesOSGiTest extends JavaOSGiTest {
() -> thingRegistry.updateConfiguration(thingUID, Map.of("parameter", configuration)));
}
@Test
public void assertIllegalConfigurationParametersPreventUpdate() {
SimpleThingHandlerFactory thingHandlerFactory = new SimpleThingHandlerFactory();
thingHandlerFactory.activate(componentContext);
registerService(thingHandlerFactory, ThingHandlerFactory.class.getName());
registerThingTypeProvider();
registerConfigDescriptionProvider(true);
ThingTypeUID thingTypeUID = new ThingTypeUID("bindingId:type");
ThingUID thingUID = new ThingUID("bindingId:type:thingId");
Thing thing = ThingBuilder.create(thingTypeUID, thingUID)
.withConfiguration(new Configuration(Map.of("parameter", "someValue"))).build();
managedThingProvider.add(thing);
SimpleThingHandler handler = (SimpleThingHandler) thing.getHandler();
assertNotNull(handler);
Object parameter = handler.getThing().getConfiguration().get("parameter");
assertNotNull(parameter);
assertEquals("someValue", parameter);
handler.updateConfiguration(new Configuration(Map.of("parameter", "otherValue")));
parameter = handler.getThing().getConfiguration().get("parameter");
assertNotNull(parameter);
assertEquals("otherValue", parameter);
handler.updateConfiguration(new Configuration(Map.of()));
parameter = handler.getThing().getConfiguration().get("parameter");
// configuration should not change
assertNotNull(parameter);
assertEquals("otherValue", parameter);
}
@Test
public void assertConfigurationIsRolledbackOnError() {
SimpleThingHandlerFactory thingHandlerFactory = new SimpleThingHandlerFactory();