openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
906 stars 420 forks source link

[REST] Thing configuration does not include defaults and actions #3071

Open spacemanspiff2007 opened 1 year ago

spacemanspiff2007 commented 1 year ago

A request to /things/{thingUID} does not include the configuration defaults if they are not set at least once.

Example Z-Wave device: The default for binding_cmdrepollperiod is set to 1500.

Initially the thing configuration does not include binding_cmdrepollperiod:

  "configuration": {
   ...
    "config_66_2": 3600,
    "config_154_4_7F000000": 127,
   ...
  }

When changing it to 1502 is correctly included

  "configuration": {
   ...
    "config_66_2": 3600,
    "binding_cmdrepollperiod": 1502,
    "config_154_4_7F000000": 127,
   ...
  }

After changing it back to 1500 it still is sent correctly:

  "configuration": {
   ...
    "config_66_2": 3600,
    "binding_cmdrepollperiod": 1500,
    "config_154_4_7F000000": 127,
   ...
  }

All configuration values should be sent with the thing configuration. When checking with /config-descriptions/{uri} there are even more parts that are not sent with the config. Such as

mhilbush commented 1 year ago

I'm curious why this would be considered an issue. If a config parameter is not present in the jsondb, the zwave binding uses the default value. I understand why it might be desirable to see all these values returned in the REST response. But functionally for the binding it's not necessary.

I went through this behavior recently with @cdjackson. The only time I observed an issue was when the binding was (erroneously) using a default value that was different than the value that was shown in the UI.

spacemanspiff2007 commented 1 year ago

I'm curious why this would be considered an issue.

I implemented some functionality which allows to change the thing configuration and for that I have to get the current configuration values. It broke some time ago but only now I got around to search for the error.

If a config parameter is not present in the jsondb, the zwave binding uses the default value. I understand why it might be desirable to see all these values returned in the REST response. But functionally for the binding it's not necessary.

This issue is raised purely from a RestAPI perspective. The thing response should contain the default values for the configuration, otherwise it's not the full configuration.

J-N-K commented 1 year ago

The issue here is that there is a policy for configuration parameters that states "if there is a default, then the parameter is not mandatory". Because of that the empty (i.e. not configured) parameters are not added to the configuration when the thing is initialized.

If https://github.com/openhab/openhab-core/pull/3024 is merged, a possible solution would be to add the missing parameters with their respective default values when normalizing the configuration before initializing the thing. This would also greatly simplify the thing initialization itself, because default values would already be set. The REST API would then automatically provide the configuration with either the configured values or the defaults.

spacemanspiff2007 commented 1 year ago

Because of that the empty (i.e. not configured) parameters are not added to the configuration when the thing is initialized.

Couldn't the RestAPI just merge the currently set configuration with the defaults?

add the missing parameters with their respective default values when normalizing the configuration before initializing the thing.

A value that is not set which means the default is used and a value that is explicitly set to the default value are two different things. But I guess in case of e.g. a migration to another default it would be sufficient just to migrate from the default value. Probably even the more correct one if I understand the current behavior correctly.

Sounds good!

J-N-K commented 1 year ago

For the thing it makes no difference if a default is applied or if the default value is explicitly set, in both cases the handler receives a configuration with the default value.

The only issue is that if the default value is stored as set value ("explicitly set") an updated default value will not automatically be applied. It is debatable what is the correct behavior here. One could also say that a thing should not change its behavior (which probably is the case if a default is changed) only because the add-on is upgraded, so keeping the old default for already existing things would be correct.