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 updating things/items/sitemaps by recreating them #157

Merged
merged 2 commits into from
Oct 7, 2023

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Sep 21, 2023

Also includes some minor bug fixes

  • notify_listeners_about_updated_element(_old_element, element) needed two parameters
  • do an identity lookup for elementToProvider. Note, I tried .to_h.compare_by_identity, but it didn't do as expected.
  • It was possible to "add" new items/things/sitemaps that already existed when they were created by other providers.

Previously a newly created item will replace the existing one accidentally when calling NewItem.label = "label" during the building process. #label= calls provider&.update and because provider look up is based on item's uid (item name) it found a provider, and proceeded to update/replace the existing item with the new item. This happens even though the newly created item hadn't been officially added to the provider, and in fact the subsequent call to provider.add will fail because it already had an element with the same uid, but the new element is now the active one, despite the failure to add.

Random notes as I come to think of them about the implementation within this PR:

  • When updating an item, the old metadata doesn't get deleted. It is now deleted.

@jimtng jimtng added the enhancement New feature or request label Sep 21, 2023
@jimtng jimtng marked this pull request as draft September 22, 2023 12:58
@jimtng jimtng force-pushed the provider-identity branch 14 times, most recently from ce06fb8 to 92f226d Compare September 25, 2023 12:00
@jimtng jimtng force-pushed the provider-identity branch 7 times, most recently from f7cdbd9 to eacb972 Compare September 26, 2023 12:27
@jimtng jimtng marked this pull request as ready for review September 26, 2023 12:32
@jimtng jimtng requested a review from ccutrer September 26, 2023 12:32
@jimtng jimtng force-pushed the provider-identity branch 2 times, most recently from 7bb71b4 to 8ddc165 Compare September 26, 2023 12:48
@jimtng
Copy link
Contributor Author

jimtng commented Sep 26, 2023

The way it handles metadata now is probably not right. It doesn't wipe out the metadata when updating an item.

  • Say you originally created an item with ga: Light, alexa: Light.
  • Then you recreated it with just alexa: Light.
    You would expect that it will no longer have ga: Light, wouldn't you? Right now the ga: Light stays there.

@jimtng
Copy link
Contributor Author

jimtng commented Sep 26, 2023

The way it handles metadata now is probably not right. It doesn't wipe out the metadata when updating an item.

  • Say you originally created an item with ga: Light, alexa: Light.
  • Then you recreated it with just alexa: Light.
    You would expect that it will no longer have ga: Light, wouldn't you? Right now the ga: Light stays there.

Updated to remove managed metadata that isn't specified in the new config

Copy link
Contributor

@ccutrer ccutrer left a comment

Choose a reason for hiding this comment

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

thoughts on instead of a update: true kwarg, to build, have a separate method rebuild that implies update: true?

alternately, would it make sense to have update default to true? given how it's currently implemented, it will already make sure you're not trying to replace an item from another provider, which is my biggest worry about not checking if an item already exists.

lib/openhab/core/items/number_item.rb Outdated Show resolved Hide resolved
lib/openhab/core/registry.rb Outdated Show resolved Hide resolved
lib/openhab/dsl/items/builder.rb Outdated Show resolved Hide resolved
lib/openhab/dsl/items/builder.rb Outdated Show resolved Hide resolved
lib/openhab/dsl/items/builder.rb Outdated Show resolved Hide resolved
lib/openhab/dsl/items/builder.rb Outdated Show resolved Hide resolved
@jimtng jimtng force-pushed the provider-identity branch 2 times, most recently from b625512 to 27e82de Compare September 30, 2023 00:38
@jimtng jimtng requested a review from ccutrer September 30, 2023 00:46
@jimtng
Copy link
Contributor Author

jimtng commented Sep 30, 2023

Please don't merge it yet - still need to change the default to true, or refactor into separate methods

@jimtng
Copy link
Contributor Author

jimtng commented Sep 30, 2023

thoughts on instead of a update: true kwarg, to build, have a separate method rebuild that implies update: true?

I think this makes it a bit ambiguous because you can use rebuild to create brand new elements, not just to 'rebuild', because normally you probably start out without any existing items anyway, so do you first call build, then if that failed, rebuild?

alternately, would it make sense to have update default to true? given how it's currently implemented, it will already make sure you're not trying to replace an item from another provider, which is my biggest worry about not checking if an item already exists.

I'd go with this option, which is to set the default to true. I am wondering though, would it even be necessary to offer the option of update: false at all? Should we simply just let it update whenever possible and only fail on FrozenError?

@jimtng jimtng force-pushed the provider-identity branch 6 times, most recently from a6d024d to 519da81 Compare October 1, 2023 01:21
@jimtng jimtng force-pushed the provider-identity branch from 519da81 to 969e51b Compare October 1, 2023 02:30
@ccutrer ccutrer merged commit 80df4e9 into openhab:main Oct 7, 2023
19 checks passed
@jimtng jimtng deleted the provider-identity branch October 8, 2023 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants