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

Allow bulk deletion of metadata #2984

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 29, 2022

This adds similar functionality that is available for links with #2970. Also support for cleaning the metadata registry is added. Regarding exposing the compression functionality the general question in #2970 should be answered.

Signed-off-by: Jan N. Klug github@klug.nrw

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K requested a review from a team as a code owner May 29, 2022 10:28
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label May 29, 2022
@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented May 30, 2022

Thanks for creating this.
I'd like to question again if it should be possible for item metadata to exist without an item.
The item metadata is always clearly associated with an item so imho it makes sense to delete the metadata when the item is deleted.
What do you think about attaching metadata directly to the item?

@J-N-K
Copy link
Member Author

J-N-K commented May 30, 2022

As already pointed out by @rkoshak adding managed metadata to unmanaged items is not uncommon. Also #992 seems to indicate that there are situations where metadata currently cannot be added via textual configuration.

Removing managed metadata on unmanaged items therefore would require a migration path to not break existing installations and I do not see how that could be achieved: We could probably merge managed metadata to managed items but not add them to unmanaged items in the file (which by the way is an argument for dropping files, because the prevent such refactoring).

@spacemanspiff2007
Copy link
Contributor

Also #992 seems to indicate that there are situations where metadata currently cannot be added via textual configuration.

Good catch! I was not aware of such an issue and that's definitely something that should be addressed before.

Removing managed metadata on unmanaged items therefore would require a migration path to not break existing installations and I do not see how that could be achieved: We could probably merge managed metadata to managed items but not add them to unmanaged items in the file

You are absolutely right - that's not something that's not done quickly.
A migration path is definitely possible, the question is if it's worth the effort on your side (or the maintainer implementing it).
There are three possible combinations

  1. Managed item and managed metadata
  2. Unmanaged item and unmanaged metadata
  3. Unmanaged item and managed metadata

Combination 1 and 2 are easy since they translate 1:1 to item bound metadata.
For 3 there are two possible migration paths:
a) migrate to managed item
b) migrate to unmanaged item

Both require an action by the user. The first step would be to make the user aware of the issue which means issuing a warning.
Then after one or more versions these could turn into errors.


which by the way is an argument for dropping files

It is not - if the users preference is to administrate openhab through files he has to do the migration there.
And it can be made easy with an appropriate warning message, e.g.

Mixture of metadata from GUI and from files for "itemName". Add "medata=data [config]" to your *.items file or migrate the item to GUI based configuration

Additionally I'll gladly provide a python script that does everything for the user.
But please let's not have this discussion again - this has been done in the forums extensively.
Whilst administration through GUI is easier for newcomers and offers some convenience administration through files is way quicker and much more comfortable for advanced users. Especially if you have to administrate multiple openhab instances and want to share items and logic between the instances.
It's good that openhab caters to both groups and it should stay that way.

@rkoshak
Copy link

rkoshak commented May 30, 2022

Mixture of metadata from GUI and from files for "itemName".

All I want to add is managed Item metadata can be created in a number of ways and not just through the GUI:

  • MainUI
  • REST API
  • Bindings
  • Rules

It's those last two that I'd have the most concern over. I suppose that bindings can just reinsert their metadata when the .items file is reloaded and the links recreated but we are still stuck with rules. Every time you reload the .items file you'd wipe out any metadata that a rule has added to that Item. So what you are proposing is either rules should never set Item metadata (which would be a huge breaking change for a lot of advanced users) or at a minimum Item metadata cannot be set from rules if and only if your Item is defined in a .item file. To me that's a very unsatisfactory limitation.

And anecdotally, when I had my Items in .items files, I depending on the fact that the managed metadata was not deleted every time the Item was reloaded.

@spacemanspiff2007
Copy link
Contributor

All I want to add is managed Item metadata can be created in a number of ways and not just through the GUI:

I'm aware of that - it was just an example how the message could help the user.

Every time you reload the .items file you'd wipe out any metadata that a rule has added to that Item

Just for clarification:
Isn't that the current behavior? My understanding was that if the *.items file has metadata defined it will overwrite the existing managed metadata for that item.

I suppose that bindings can just reinsert their metadata

What do you mean by "bindings can just reinsert their metadata"?

So what you are proposing is either rules should never set Item metadata (which would be a huge breaking change for a lot of advanced users)

That's not what I am proposing or implying in any way.

Item metadata cannot be set from rules if and only if your Item is defined in a .item file.

That would be the consequence, correct. Or to rephrase it:
To set item metadata from gui, rules or rest api the item has to be a managed item.

I'm aware that usage of metadata peaked at the end of OH2.5 to allow for more complex rules or to save states between rule executions.
While it is totally fine to use metadata to e.g. setup rules (the intended use case for metadata: transport additional information about that item) there is hardly a need to save additional states or values in metadata. Everything can either be translated to a global (complex) variable in e.g. a script or plain and simple just another item.
I would go as far and even label that as misuse.
Nonetheless if you still want to do it would still be possible - just make your unmanaged item a managed one.

@rkoshak
Copy link

rkoshak commented May 31, 2022

Isn't that the current behavior? My understanding was that if the *.items file has metadata defined it will overwrite the existing managed metadata for that item.

Correct, but what if the metadata is not and never was in the .items file? What if it was created and set by a rule?

What do you mean by "bindings can just reinsert their metadata"?

This is a feature that I hate because it causes unexpected behaviors and sometimes causes problems. Binding authors can inject their own Item metadata (e.g. State Description) to the Item when the Item is Linked. So when the Item is recreated, the binding will reinsert that metadata when the Link is recreated (I'm assuming, I don't really know the underlying mechanism for this).

That's not what I am proposing or implying in any way.

It's a side effect of automatically removing the metadata when an Item is deleted though. If the metadata is deleted when the Item is deleted, and the Item is deleted when the .items file is unloaded, any metadata set by a rule will be deleted on every load of the .items file. To avoid this metadata disappearing unexpectedly one would have to avoid using rules to set Item metadata when using .items files.

That would be the consequence, correct. Or to rephrase it:
To set item metadata from gui, rules or rest api the item has to be a managed item.

That seems like a pretty severe limitation to fix a problem that most people don't even have and which Jan has proposed a mechanism above to deal with in those rare circumstances that the problem does occur.

I'm aware that usage of metadata peaked at the end of OH2.5 to allow for more complex rules or to save states between rule executions.

I wouldn't say it peaked. Item metadata is used more now than ever before and more keeps being added. But metadata in these cases are not used to save state, that's what Items are for. It's used to save configuration data. For a rule, it might save scene configuration data, override flags, create/change "well known" Item metadata on the fly (e.g. set a different default list Item widget based on some state or context), etc. In many of these cases it's not really feasible to do this using just Items because the data is hierarchical in nature most of the time.

Global variables won't survive a restart of OH and until the registry gets merged isn't available at all in UI rules.

And now you are proposing to make it so that in some cases (i.e. when using .items files) this configuration data gets wiped out every time a .items file is reloaded.

@spacemanspiff2007
Copy link
Contributor

If the metadata is deleted when the Item is deleted, and the Item is deleted when the .items file is unloaded, any metadata set by a rule will be deleted on every load of the .items file. To avoid this metadata disappearing unexpectedly one would have to avoid using rules to set Item metadata when using .items files.

But since the metadata has to be in the .items file it also gets recreated every time the file gets loaded.
So this is a non-issue.

For a rule, it might save scene configuration data, override flags, create/change "well known" Item metadata on the fly (e.g. set a different default list Item widget based on some state or context), etc. In many of these cases it's not really feasible to do this using just Items because the data is hierarchical in nature most of the time.

But that's another issue: openHAB doesn't allow nested data structures as items and because metadata does it gets used to store this data instead.
You are also describing a mixture of run time data and configuration data which should always keep separated.

And now you are proposing to make it so that in some cases (i.e. when using .items files) this configuration data gets wiped out every time a .items file is reloaded.

No - you misunderstand. I want to make sure that the configuration is consistent and stateless. If I write in the file config a I want openHAB to internally have config a if the file is loaded.
What you are proposing (and how it currently is) is that even though I write config a openhab might be actually have config b after the file is loaded.


It all boils down to the following question:

Is metadata item data e.g. does it describe the item?
If yes then the data can not live without the item because you can not describe something that doesn't exist.
If no then it can live without the item but then it should not be allowed to use it to modify item behavior (e.g. expire).

@rkoshak
Copy link

rkoshak commented Jun 3, 2022

But since the metadata has to be in the .items file it also gets recreated every time the file gets loaded.
So this is a non-issue.

That's my point. The metadata doesn't have to be in the .items file. It is possible and reasonable to have Item metadata that is only ever created and used by rules. In that case the metadata would never have been in the .items file. But it would get deleted on every reload of the .items file with the behavior you've requested.

You are also describing a mixture of run time data and configuration data which should always keep separated.

I don't think I am by why? Why create this seemingly arbitrary distinction? The difference between configuration data and runtime data is fuzzy at best so what's the hard and fast rule here. If the metadata can/normally would be changed at runtime then it's not metadata and you (now) can't use metadata for that? Why does something suddenly become "state" and not configuration just because it's altered because of some rule instead of being hard coded by the admin?

Why break a bunch of advanced user's existing rules setups based on this distinction? I'm pretty sure this would break several of the already published rule templates on the marketplace like the Scene Controller Suite as well.

No - you misunderstand. I want to make sure that the configuration is consistent and stateless. If I write in the file config a I want openHAB to internally have config a if the file is loaded.
What you are proposing (and how it currently is) is that even though I write config a openhab might be actually have config b after the file is loaded.

I don't misunderstand, I don't agree. I don't agree that Item metadata must always be defined in the .items file. I don't agree the metadata must always be static and never changed except through a .items file. And even if I did, I don't agree that we should break a bunch of people's existing rules over this, especially since an alternative solution has been proposed and is being worked on that doesn't impose this limitation and solves the problem nicely.

If yes then the data can not live without the item because you can not describe something that doesn't exist.

It boils down to this "rule". If it were not for the fact that when using OH .items files each and every time that file is unloaded the Items get deleted I wouldn't have a problem with this statement. But it does. To most end users, modifying a .items file to change something about an Item doesn't conceptually cause that Item to be deleted and recreated, it modifies that existing Item. Heck, by default it even looks (to the end user) like the Item kept its state thanks to restoreOnStartup. But in the case of metadata, sorry, your metadata is now gone and at best replaced by the default in the .items file (if there is one) for every Item in that file, not just the one you changed.

If you want to be "pure" in your use of metadata that's fine and good and more power to you. Text config file users need to have some self imposed discipline anyway. I'd even encourage evangelizing this to anyone who will hear. But I don't think we should impose that purity on everyone, especially since it breaks a number of people's setups which have worked since early OH 2 timeframes.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Jun 4, 2022

This is one of those issues that should have been a short meeting. 😉


But in the case of metadata, sorry, your metadata is now gone and at best replaced by the default in the .items file (if there is one) for every Item in that file, not just the one you changed.

But why keep only metadata? Why not keep the item state, or the item tags or the item groups?
Everything can be set from rules or through the rest api, too.


It boils down to this "rule".

So we agree that

  • Item data can not exist without an item
  • Metadata is item data

That's good so we have at least a common understanding how things should be.
It's obvious that it currently is not that way but I think this is the very core goal we should try to achieve.

But it would get deleted on every reload of the .items file with the behavior you've requested.

It boils down to this "rule". If it were not for the fact that when using OH .items files each and every time that file is unloaded the Items get deleted I wouldn't have a problem with this statement. But it does.

That is the current implementation but that's something that can easily be changed. There is no need and logical reason to delete all items and recreate them every time the file gets reloaded. It's probably just a leftover from the early openhab days which worked well enough.

And even if I did, I don't agree that we should break a bunch of people's existing rules over this, especially since an alternative solution has been proposed and is being worked on that doesn't impose this limitation and solves the problem nicely.

So what is the rule here? If it would break something existing it can't be done?
Also I always emphasized that it should be a soft migration path (e.g. warnings first) so things would not break at all in the first place.

rules over this

But the issue is much deeper than "this":
Currently we issue an ItemCreatedEvent that doesn't contain the metadata, even though metadata might already exist. If Metadata gets added/changes there is no ItemChangedEvent, even though the item obviously behaves different (e.g. I change the expire timeout). There are so many corner cases that add quirky and hard to track behavior that it's definitely worth breaking things to reduce complexity there.

@rkoshak
Copy link

rkoshak commented Jun 6, 2022

That's good so we have at least a common understanding how things should be.

Ultimately, even though I can program I pretend I don't know how when it comes to openHAB. I don't care how things should be from the developer's perspective.

I care what the impact to the end user is. In this case, deleting the metadata on every reload of a .items file is a huge negative impact on the end user. Not only is it unexpected (which is always bad) it eliminates the possibility of certain use cases which have long been possible.

You figure out how to cause the metadata to be deleted with the Item without those impacts you'll have no complaints from me. Otherwise, it's going to be a net negative impact on the end users.

So what is the rule here? If it would break something existing it can't be done?

"If yes then the data can not live without the item because you can not describe something that doesn't exist."

If it will break something existing then at least it needs to wait until the next major version of OH. And by that logic, why not just get rid of text based Items files? That would fix the problem for sure. Obviously that's not going to happen, nor should it. But the same logic applies.

Even with a migration path, you are eliminating a way to use Item metadata from users who are already using metadata in this way for no clear end user benefit. It comes across as hostile to the end users. From their perspective "It's worked fine for half a decade now without problem and now you are breaking it and not providing me anything equivalent or better in exchange."

It's important to be clear here. When using managed Items the metadata does get deleted with the Item. Everything is all nice and clean and behaves as expected and we can still set Item metadata from rules. The only time a problem occurs is when using .items files and add/modify Item metadata in other ways.

Honestly, if you limit those warnings and deprecations and eliminating the ability to add metadata only for Items defined in .items files I don't have much of a problem. I'm not one of those users. I don't support those users on the forum any more. It has no personal impact on me. Someone will have to deal with those users though as they will not be happy. But if we eliminate the possibility to add/modify/delete metadata from rules for managed Items too, then that's where I have a real problem. It eliminates a use case that is actively used to solve a problem those users don't even have.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm

@kaikreuzer kaikreuzer merged commit b6acaf7 into openhab:main Jul 18, 2022
@kaikreuzer kaikreuzer added this to the 3.4 milestone Jul 18, 2022
@J-N-K J-N-K deleted the feature-restdeletemetadata branch July 18, 2022 16:47
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: b6acaf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core karaf REST/SSE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants