enable config validation for thing creation and update #2682
Conversation
Signed-off-by: Jan N. Klug <github@klug.nrw>
J-N-K
force-pushed
the
feature-thing-config-validation
branch
from
2f9ebbe
to
429c0a4
Compare
| setThingStatus(thing, | ||
| buildStatusInfo(ThingStatus.UNINITIALIZED, | ||
| ThingStatusDetail.HANDLER_CONFIGURATION_PENDING, | ||
| "Missing or invalid configuration.")); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Where Do I have to place the correct value then? ThingManagerImpl.properties in the i18n-folder? Or ThingStatusInfo.properties?
There was a problem hiding this comment.
I think it should be placed into a new properties file with name SystemThingStatusInfos.properties.
| "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.")); |
There was a problem hiding this comment.
Use the same custom key here.
Signed-off-by: Jan N. Klug <github@klug.nrw>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/my-z-wave-is-borked/132448/15 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/my-z-wave-is-borked/132448/40 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/z-wave-handler-configuration-pending-with-oh-3-3-snapshot/132549/2 |
| try { | ||
| configDescriptionValidator.validate(configuration.getProperties(), configDescriptionURI); | ||
| } catch (ConfigValidationException e) { | ||
| return false; |
There was a problem hiding this comment.
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.
|
Yes, I‘ll prepare that immediately. It seems that at least some of the issues arise from signed/unsigned byte/int values. I‘ll check if that is a problem in the code or in the affected binding. |
|
I am online and have this issue, so let me know how I can help |
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-3-3-milestone-discussion/132715/40 |
Fixes #2674
Depends on #2683
Signed-off-by: Jan N. Klug github@klug.nrw