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

enable config validation for thing creation and update #2682

Merged
merged 3 commits into from
Jan 20, 2022
Merged

enable config validation for thing creation and update #2682

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
631dc6c
enable config validation for thing creation and update
J-N-K Jan 13, 2022
429c0a4
fix tests
J-N-K Jan 15, 2022
431f00d
address review comment
J-N-K Jan 16, 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
66 changes: 34 additions & 32 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 @@ -21,7 +21,6 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
Expand All @@ -42,11 +41,10 @@
import org.openhab.core.common.registry.Identifiable;
import org.openhab.core.common.registry.ManagedProvider;
import org.openhab.core.common.registry.Provider;
import org.openhab.core.config.core.ConfigDescription;
import org.openhab.core.config.core.ConfigDescriptionParameter;
import org.openhab.core.config.core.ConfigDescriptionRegistry;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.config.core.validation.ConfigDescriptionValidator;
import org.openhab.core.config.core.validation.ConfigValidationException;
import org.openhab.core.events.EventPublisher;
import org.openhab.core.service.ReadyMarker;
import org.openhab.core.service.ReadyMarkerFilter;
Expand Down Expand Up @@ -558,6 +556,7 @@ public void thingUpdated(final Thing thing, ThingTrackerEvent thingTrackerEvent)
// 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
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that this assumption is wrong as e.g. the ZWave handler updates the configuration with values outside of the valid range. @cweitkamp shall we add a validation here, too? And what shall happen if the thing handler tries to update with an invalid configuration? Log a warning and deny replacing the thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree. Both methods in BaseThingHandler which allows to update the configuration of a thing (updateThing(Thing) and updateConfiguration(Configuration)) lead to an unchecked configuration. There is no validation done.

Both updateThing() and updateConfiguration() call ThingHandlerCallback.thingUpdated() -> ThingRegistry.updated() -> ThingTracker.thingUpdated() of ThingManagerImpl.

In theorie we have to to check for a changed configuration of the thing and do the same like the ThingResource: call ThingRegistry.updateConfiguration() instead of just replacing the thing here. imo the handler has to be reinitialized if configuration will change.

Or did I miss something.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this assumption is wrong

I think the problem with this assumption is that the binding doesn't have access to the config description, so it isn't (easily) possible for it to do this as far as I can see. The framework only passes the values to the binding - maybe it should also make the descriptions more readily available as well now that we're enforcing these checks.

replaceThing(getThing(thingUID), thing);
} else {
Lock lock1 = getLockForThing(thing.getUID());
Expand All @@ -571,7 +570,23 @@ public void thingUpdated(final Thing thing, ThingTrackerEvent thingTrackerEvent)
oldThing.setHandler(null);
}
thing.setHandler(thingHandler);
safeCaller.create(thingHandler, ThingHandler.class).build().thingUpdated(thing);

if (isInitializable(thing, getThingType(thing))) {
safeCaller.create(thingHandler, ThingHandler.class).build().thingUpdated(thing);
} else {
final ThingHandlerFactory thingHandlerFactory = findThingHandlerFactory(
thing.getThingTypeUID());
if (thingHandlerFactory != null) {
if (isBridge(thing)) {
unregisterAndDisposeChildHandlers((Bridge) thing, thingHandlerFactory);
}
disposeHandler(thing, thingHandler);
setThingStatus(thing,
buildStatusInfo(ThingStatus.UNINITIALIZED,
ThingStatusDetail.HANDLER_CONFIGURATION_PENDING,
"Missing or invalid configuration."));
Copy link
Contributor

Choose a reason for hiding this comment

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

As the ThingStatusInfo is translatable I am wondering if we should introduce a custom translation key for this description (e.g.@text/missing-or-invalid-configuration).

Copy link
Member Author

@J-N-K J-N-K Jan 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Where Do I have to place the correct value then? ThingManagerImpl.properties in the i18n-folder? Or ThingStatusInfo.properties?

Copy link
Contributor

@cweitkamp cweitkamp Jan 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should be placed into a new properties file with name SystemThingStatusInfos.properties.

}
}
} else {
logger.debug(
"Cannot notify handler about updated thing '{}', because handler is not initialized (thing must be in status UNKNOWN, ONLINE or OFFLINE).",
Expand Down Expand Up @@ -744,23 +759,25 @@ private void initializeHandler(Thing thing) {
setThingStatus(thing, buildStatusInfo(ThingStatus.INITIALIZING, ThingStatusDetail.NONE));
doInitializeHandler(handler);
} else {
logger.debug("Thing '{}' not initializable, check required configuration parameters.", thing.getUID());
setThingStatus(thing,
buildStatusInfo(ThingStatus.UNINITIALIZED, ThingStatusDetail.HANDLER_CONFIGURATION_PENDING));
logger.debug(
"Thing '{}' not initializable, check if required configuration parameters are present and set values are valid.",
thing.getUID());
setThingStatus(thing, buildStatusInfo(ThingStatus.UNINITIALIZED,
ThingStatusDetail.HANDLER_CONFIGURATION_PENDING, "Missing or invalid configuration."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the same custom key here.

}
} finally {
lock.unlock();
}
}

private boolean isInitializable(Thing thing, @Nullable ThingType thingType) {
if (!isComplete(thingType, thing.getUID(), tt -> tt.getConfigDescriptionURI(), thing.getConfiguration())) {
if (!isComplete(thingType, thing.getUID(), ThingType::getConfigDescriptionURI, thing.getConfiguration())) {
return false;
}

for (Channel channel : thing.getChannels()) {
ChannelType channelType = channelTypeRegistry.getChannelType(channel.getChannelTypeUID());
if (!isComplete(channelType, channel.getUID(), ct -> ct.getConfigDescriptionURI(),
if (!isComplete(channelType, channel.getUID(), ChannelType::getConfigDescriptionURI,
channel.getConfiguration())) {
return false;
}
Expand All @@ -786,35 +803,20 @@ private <T extends Identifiable<?>> boolean isComplete(@Nullable T prototype, UI
return true;
}

ConfigDescription description = resolve(configDescriptionURIFunction.apply(prototype), null);
if (description == null) {
logger.debug("Config description for '{}' is not resolvable, assuming '{}' is initializable",
URI configDescriptionURI = configDescriptionURIFunction.apply(prototype);
if (configDescriptionURI == null) {
logger.debug("Config description URI for '{}' not found, assuming '{}' is initializable",
prototype.getUID(), targetUID);
return true;
}

List<String> requiredParameters = getRequiredParameters(description);
Set<String> propertyKeys = configuration.getProperties().keySet();
if (logger.isDebugEnabled()) {
logger.debug("Configuration of '{}' needs {}, has {}.", targetUID, requiredParameters, propertyKeys);
try {
configDescriptionValidator.validate(configuration.getProperties(), configDescriptionURI);
} catch (ConfigValidationException e) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a trace log here to allow users to see validation messages in the log file? This might help to find the culprit for #2710.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #2711

}
return propertyKeys.containsAll(requiredParameters);
}

private @Nullable ConfigDescription resolve(@Nullable URI configDescriptionURI, @Nullable Locale locale) {
return configDescriptionURI != null
? configDescriptionRegistry.getConfigDescription(configDescriptionURI, locale)
: null;
}

private List<String> getRequiredParameters(ConfigDescription description) {
List<String> requiredParameters = new ArrayList<>();
for (ConfigDescriptionParameter param : description.getParameters()) {
if (param.isRequired()) {
requiredParameters.add(param.getName());
}
}
return requiredParameters;
return true;
}

private void doInitializeHandler(final ThingHandler thingHandler) {
Expand Down
3 changes: 2 additions & 1 deletion ....thing.tests/src/main/java/org/openhab/core/thing/binding/BindingBaseClassesOSGiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,8 @@ public void assertBaseThingHandlerNotifiesThingManagerAboutConfigurationUpdates(

// ThingHandler.initialize() has not been called; thing with status UNINITIALIZED.HANDLER_CONFIGURATION_PENDING
ThingStatusInfo uninitialized = ThingStatusInfoBuilder
.create(ThingStatus.UNINITIALIZED, ThingStatusDetail.HANDLER_CONFIGURATION_PENDING).build();
.create(ThingStatus.UNINITIALIZED, ThingStatusDetail.HANDLER_CONFIGURATION_PENDING)
.withDescription("Missing or invalid configuration.").build();
assertThat(thing.getStatusInfo(), is(uninitialized));

thingRegistry.updateConfiguration(thingUID, Map.of("parameter", "value"));
Expand Down
3 changes: 2 additions & 1 deletion ....core.thing.tests/src/main/java/org/openhab/core/thing/internal/ThingManagerOSGiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable {

// ThingHandler.initialize() not called, thing status is UNINITIALIZED.HANDLER_CONFIGURATION_PENDING
ThingStatusInfo uninitializedPending = ThingStatusInfoBuilder
.create(ThingStatus.UNINITIALIZED, ThingStatusDetail.HANDLER_CONFIGURATION_PENDING).build();
.create(ThingStatus.UNINITIALIZED, ThingStatusDetail.HANDLER_CONFIGURATION_PENDING)
.withDescription("Missing or invalid configuration.").build();
verify(thingHandler, never()).initialize();
assertThat(thing.getStatusInfo(), is(uninitializedPending));

Expand Down