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.

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

[RfC] Bind metadata to the item #3281

Open
spacemanspiff2007 opened this issue Dec 29, 2022 · 22 comments
Open

[RfC] Bind metadata to the item #3281

spacemanspiff2007 opened this issue Dec 29, 2022 · 22 comments

Comments

@spacemanspiff2007
Copy link
Contributor

With PR #3273 the metadata gets connected tighter to the item.
Item metadata that can live without the item is an inconsistency that can lead to hard to track errors.
I think binding the item metadata to the item can simplify things, both for the user and the openHAB code base.
My suggestion is to make item metadata a field on the item.

There were some small discussions in the past but some things have change and I think OH4 is a good change to clean up this inconsistency.
In the past item metadata was used as a cache to store additional information in rules. But now we have file local and global cache that can be used instead.

Also related:
#2877

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

I had a look at eclipse-archived/smarthome#4390 where the MetadataRegistry was introduced. It seems that besides textual configuration and REST API (which exist) additional MetadataProviders from add-ons were the reason to design it as independent registry. It seems that more than 4 1/2 years later no add-on has implemented a MetadataProvider (at least I did not find any implementation in openhab-addons).

@openhab/core-maintainers I would therefore propose to merge the Metadata into the Item as additional field. The GenericItemProvider can be adapted so no change is required for textual configuration. We can add a mergeMetadata command to #3268 so this can be done during the upgrade process for managed items.

@rkoshak
Copy link

rkoshak commented Dec 29, 2022

In the past item metadata was used as a cache to store additional information in rules. But now we have file local and global cache that can be used instead.

First let me say I'm not arguing against this. I think merging Item metadata with the Item is a good idea.

But metadata is used for a whole lot more than as a cache for rules, especially in OH 3. It's used to:

  • define the state description information for how to display the Item's state (more than just a label and an optional transformation)
  • define command options (I admit, I've no idea where/how this would be used)
  • synonyms used in conjunction with the semantic model by HABot's NLP processor (e.g. provide alternative names for locations and devices)
  • define the default widgets used to display the Item on MainUI
  • control the order the Items appear in list widgets
  • control the behavior of AutoUpdate
  • Expire
  • define/expose Items for third party integration (Alexa, Google, HomeKit)
  • eventually (hopefully) to define Number Item's units.

Those are just the "well known ones".

In addition, there are less well known Item metadata used by some of the rule templates on the marketplace for Item-by-Item configuration (e.g. provide a custom debounce time for each Item processed by the Debounce rule template. This is similar but not really the same thing as using global variables or the newly implemented caches. It is proper configuration metadata, not just a place to store a temporarily calculated value. It also needs to persist beyond an OH restart like configuration data should.

I just don't want to have this move forward without a full understanding of all the ways that metadata is used. It's much more than just a place for a few advanced rules writers to stuff values to share between rules.

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

@rkoshak I did a quick check who uses the metadata infrastructure in openhab-addons and found it's only HomeKit. And as far as I could see it would even simplify the code there if metadata was available from the item, not from a separate registry.

The usage would not be affected in any way, it would just not be in a separate registry with a key itemName:metadataName but within the item itself. You could then get the item from the ItemRegistry and do getMetadata(metadataName) instead of constructing the MetadataKey and get the same information from the MetadataRegistry.

What still needs to be considered: currently you can set metadata via REST API also for textual defined items (at least I believe the is possible). This would still be possible, but not persisted (i.e. it would not survive a restart). I didn't think of that before. While it's a bit inconsistent, it might break a lot of installations (depending on how common that is).

@rkoshak
Copy link

rkoshak commented Dec 29, 2022

@rkoshak I did a quick check who uses the metadata infrastructure in openhab-addons and found it's only HomeKit. And as far as I could see it would even simplify the code there if metadata was available from the item, not from a separate registry.

Alexa and Google Assistant code will be found in the Cloud Server, not in core or addons. But both do use metadata to identify those Items to expose and what those Items represent. Presumably the big impact to them will be in any changes to the REST API.

(at least I believe the is possible)

Yes, it takes the format of namespace="MyValue"[config1=val1, config2=val2] where the config array is optional and while value isn't optional, you can use an empty string. In the UI you'd create the namespace and it takes the form

value: MyValue
config:
    conf1: val1
    conf2: val2

in the code tab.

This would still be possible, but not persisted (i.e. it would not survive a restart). I didn't think of that before. While it's a bit inconsistent, it might break a lot of installations (depending on how common that is).

Given the list of ways that Item metadata is used today that would be absolutely devastating. You almost can't get by without using Item metadata in the UI.

It would render the bulk of the ways that most users use metadata (whether they know it or not) completely useless. It's no good to define a "default list item widget" to control how that Item appears if I have to redo that every time OH restarts. Same for autoupdate, expire, state description, and all the rest.

In all these cases, metadata is used as configuration, not a temporary variable. We should't have to redo it every time we reboot OH. That is untenable.

Everything in this list is Item metadata:

image

This probably isn't as big of a deal for text based Items though. In this case every time the .items file is unloaded all those Items get destroyed. Then when the .items file is reloaded the Items get created anew. When they are recreated, their metadata defined in the file will also get recreated. That mechanism will preserve the metadata between restarts. But the REST API/managed Items need metadata to be persisted across reboots too.

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

Maybe I was not clear enough:

  • No change for metadata configured in text files for items in text files, as you described this is re-created when the item file is reloaded.
  • No change for metadata for managed items, that is persisted in the items database.
  • Metadata set in UI for items in text files would not be persisted. I'm not sure how common that is.

The REST API needs no change, it's all below the /items endpoint anyway.

@rkoshak
Copy link

rkoshak commented Dec 29, 2022

Metadata set in UI for items in text files would not be persisted. I'm not sure how common that is.

Ah, I'm not sure how common that is either. I wonder if MainUI allows it. Clearly the REST API does. I know that unmanaged Items are read only in MainUI but I don't know if that extends to metadata.

I do know that users who use text based Items but create the Links in the UI is distressingly common so I would expect, if MainUI allows it, a number of users do it. Given how difficult it is to define/configure widgets outside of the UI, I bet it's pretty common actually (assuming it's allowed, I need to run a test).

Test run. MainUI does allow the user to set Item metadata and Links through the UI even for file based Items.

@ccutrer
Copy link
Contributor

ccutrer commented Dec 29, 2022

It seems that more than 4 1/2 years later no add-on has implemented a MetadataProvider (at least I did not find any implementation in openhab-addons).

Add me! See https://github.com/ccutrer/openhab-jrubyscripting/blob/main/lib/openhab/core/items/metadata/provider.rb - my JRuby helper library implements a metadata provider. But I did so specifically for this problem - users were using the metadata helper methods in the JRuby library, and unknowingly setting managed metadata that wasn't disappearing when the script was unloaded. So I implemented a provider that's basically volatile, and unloads itself (and all metadata it provides) when the script is unloaded. I did leave a mechanism to explicitly choose the ManagedMetadataProvider if they really meant to do so. I did the same for ItemProvider for similar reasons, and I'd have zero issues if those two were to be collapsed into a single provider.

But @rkoshak is right: I think it's fairly common to define items in files, but then do additional configuration (linking items to channels, configuring other things that are implicitly metadata) via MainUI. While myself, with a strong preference for file-based configuration, still use MainUI for auto-discovery of things, and then switch to file-based configuration for manually configured things, and anything to do with items. I think it's a tenuous argument to say "if you used file based configuration, you have to use it for all aspects, from items, through to metadata, channel linking, and things", especially given auto-discovery and things. If you say "okay, you can used managed things, but can no longer configure metadata on file-based items via UI", it seems like an arbitrary line. I'm just not seeing much benefit of collapsing metadata into items given the large effort that will be involved. Sure it would clean up some code like the HomeKit add-on, but that code is already written and well-tested. Even for use within rules, besides Rules DSL, I believe every language already abstract metadata access to act as if it's directly on the item anyway. So no gain other than a minor cleanup of existing code.

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

There is a bit more than clean-up in code. Without #3273 we have no control over when metadata appears. We might very well have the situation that metadata is needed (e.g. state descriptions) and we don't know if it is actually set but not loaded or if it is missing and we should use the default.

But I do see both your arguments. And IIRC the "managed metadata for unmanaged items" discussion did already take place in the past without a clear decision if that is working "by chance" or "as designed" - neglecting that it DID work and we might break a lot of installations without an easy migration path.

@spacemanspiff2007
Copy link
Contributor Author

I've had it the other way around:
I tried to create some items in the GUI and link some channel to it and added some metadata.
Then I removed it and created it as an item in textual configuration.
But suddenly my item was behaving strange, even though according the *.items file the item definition it was correct.
I would send commands and the wrong device would be turning on. I almost went insane because I quadruple checked the *.items file and it was correct.
Even for me, a more or less advanced user it took way to long to figure out what was going on.


So on the one hand we have the item definition from the file which should actually be stateless and the only truth for the item but on the other hand we allow a "history" to change this definition.

This will lead to issues when the user will want to use this definition, e.g.

  • copying the file from one installation to another
  • restoring the item without the jsondb
  • changing something in the item definition that was previously defined through the RestAPI
    e.g. link it to a different channel

It could be argued that the first two points are more or less exotic but the last one is the dangerous one because one could believe that the item definition from the file will overwrite the link/metadata when in reality it will not.

we might break a lot of installations without an easy migration path.

That sounds again like a migration script for textual users 🙄 😉 .

@ghys
Copy link
Member

ghys commented Dec 30, 2022

It seems that besides textual configuration and REST API (which exist) additional MetadataProviders from add-ons were the reason to design it as independent registry. It seems that more than 4 1/2 years later no add-on has implemented a MetadataProvider (at least I did not find any implementation in openhab-addons).

FYI - there is an important MetadataProvider which provides the read-only semantics namespace from the tags, and is used extensively in the UI:

https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java

@J-N-K
Copy link
Member

J-N-K commented Dec 30, 2022

Thanks, I only searched in openhab-addons, not in openhab-core 🤦. That could be refactored, but we now have at least two examples where a MetadataProvider is implemented, the cons start to outweigh the pros for this proposal.

@J-N-K
Copy link
Member

J-N-K commented Dec 30, 2022

Please also see #3282 (comment). If this is implemented (which I believe is a very elegant solution to the uom-peristence issue) it would require managed metadata for unmanaged items.

@spacemanspiff2007
Copy link
Contributor Author

Please also see #3282 (comment). If this is implemented (which I believe is a very elegant solution to the uom-peristence issue) it would require managed metadata for unmanaged items.

I think this is dangerous for the reasons mentioned above.
This can lead to the issue that with the same *.items file on one machine the value is persisted in e.g. kWh and on the other in Wh. Or I delete the item and add it again and it still persists in the old unit even though this was not explicitly set.

Item metadata should die with the item - it's event called "item metadata".
And if it dies with the item it makes only sense to bind it directly to the item.

@J-N-K
Copy link
Member

J-N-K commented Dec 30, 2022

If that is the case, then probably textual items will become a second class citizen in openHAB. I doubt that someone will retrofit everything to DSL and we cannot stand still because of that. Not that I care, you all know that I would prefer to ditch them.

@spacemanspiff2007
Copy link
Contributor Author

I doubt that someone will retrofit everything to DSL

What do you mean by that?

@J-N-K
Copy link
Member

J-N-K commented Dec 30, 2022

Add-on set/modified metadata will not work if we have a clear separation between textual items/textual metadata and managed items/managed metadata. It would require ugly workarounds to make that possible and I doubt someone will implement that. We already have the issue that we can't set complex metadata in files (#992).

@spacemanspiff2007
Copy link
Contributor Author

Just so I get this right and don't interpret something in it:
The reason why it's not a good idea is because we (currently) can't configure complex metadata in items files?

@J-N-K
Copy link
Member

J-N-K commented Dec 30, 2022

I don't interpret anything. Not being able to have metadata set by add-ons for textual configured items is a second point (besides complex metadata) where textual items can't use the same functionality like managed items if we don't allow managed metadata for textual items. The bigger the difference gets, the more second class citizens textual configuration becomes.

@spacemanspiff2007
Copy link
Contributor Author

I don't interpret anything.

I wanted a more detailed explanation so I don't interpret something in your answer.

Not being able to have metadata set by add-ons

Could you describe how this is currently used?

@rkoshak
Copy link

rkoshak commented Dec 31, 2022

Could you describe how this is currently used?

In one case, a binding can set the State Description on an Item at link time. State Description is metadata.

I'm not sure of other current cases but, if the idea of creating a bundle including a binding, rule templates, and MainUI widgets ever moves forward, I could see the binding wanting to set some metadata on the Item to help create/configure the templates and widgets.

@spacemanspiff2007
Copy link
Contributor Author

In one case, a binding can set the State Description on an Item at link time. State Description is metadata.

I have trouble visualizing how this works, can you make a short made up example?
I have an item that has the state set to %.1f °C and the binding changes it to %.1f K?

I'm not sure of other current cases but, if the idea of creating a bundle including a binding, rule templates, and MainUI widgets ever moves forward, I could see the binding wanting to set some metadata on the Item to help create/configure the templates and widgets.

But this can be achieved without changing the item definition - currently it's only used because metadata is already there.

I think changing the item - and changing the item metadata is changing the item - is a poor idea because behavior will depend on the bindings the item is linked to.
If one binding defines state description as A and one defines state description as B and I link the item to both functionality A will not work. Or if I try to fix A and link to A again B will not work.

@rkoshak
Copy link

rkoshak commented Jan 3, 2023

I have trouble visualizing how this works, can you make a short made up example?
I have an item that has the state set to %.1f °C and the binding changes it to %.1f K?

More like you have an Item without a state description pattern. The binding adds %.1f °C.

Note, I hate this but some binding authors demand it.

A will not work. Or if I try to fix A and link to A again B will not work.

It's only updated at link time and based on the last time I tested it, if the metadata already exists it is not changed. So in this case, when you link to B it sees the state description is already defined and nothing happens.

Assuming the user didn't set it themselves, first to link gets to set the state description and all other suggestions from bindings are ignored. (There was a bug on this where the user values were not overriding the binding provided metadata but that got fixed some time ago.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants