Allow updating things/items/sitemaps by recreating them (#157)

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.

---------

Signed-off-by: Jimmy Tanagra <[email protected]>

lib/openhab/core/emulate_hash.rb

@@ -103,7 +103,12 @@ def merge(*others, &block)
       def merge!(*others, &block)
         return self if others.empty?
 
-        replace(merge(*others, &block))
+        # don't call replace here so we don't touch other keys
+        others.shift.merge(*others, &block).each do |key, value|
+          value = yield key, self[key], value if key?(key) && block
+          store(key, value)
+        end
+        self
       end
       alias_method :update, :merge!
 

lib/openhab/core/items/group_item.rb

@@ -106,6 +106,9 @@ def inspect
           alias_method :to_s, :inspect
         end
 
+        # @!attribute [r] function
+        # @return [GroupFunction] Returns the function of this GroupItem
+
         # Override because we want to send them to the base item if possible
         %i[command update].each do |method|
           define_method(method) do |command|
@@ -152,6 +155,19 @@ def #{type}_item?                      # def date_time_item?
           RUBY
         end
 
+        #
+        # Compares all attributes of the item with another item.
+        #
+        # @param other [Item] The item to compare with
+        # @return [true,false] true if all attributes are equal, false otherwise
+        #
+        # @!visibility private
+        def config_eql?(other)
+          return false unless super
+
+          base_item&.type == other.base_item&.type && function&.inspect == other.function&.inspect
+        end
+
         private
 
         # Add base type and function details

lib/openhab/core/items/item.rb

@@ -264,12 +264,17 @@ def thing
         #
         # @return [Array<Thing>] An array of things or an empty array
         def things
-          registry = Things::Links::Provider.registry
-          channels = registry.get_bound_channels(name).to_a
-          channels.map(&:thing_uid).uniq.map { |tuid| EntityLookup.lookup_thing(tuid) }.compact
+          Things::Links::Provider.registry.get_bound_things(name).map { |thing| Things::Proxy.new(thing) }
         end
         alias_method :all_linked_things, :things
 
+        # Returns all of the item's links (channels and link configurations).
+        #
+        # @return [Array<ItemChannelLink>] An array of ItemChannelLink or an empty array
+        def links
+          Things::Links::Provider.registry.get_links(name)
+        end
+
         # @return [String]
         def inspect
           s = "#<OpenHAB::Core::Items::#{type}Item#{type_details} #{name} #{label.inspect} state=#{raw_state.inspect}"
@@ -286,6 +291,23 @@ def provider
           Provider.registry.provider_for(self)
         end
 
+        #
+        # Compares all attributes except metadata and channels/links of the item with another item.
+        #
+        # @param other [Item] The item to compare with
+        # @return [true,false] true if all attributes are equal, false otherwise
+        #
+        # @!visibility private
+        def config_eql?(other)
+          # GenericItem#equals checks whether other has the same name and class
+          return false unless equals(other)
+
+          %i[label category tags group_names].all? do |method|
+            # Don't use #send here. It is defined in GenericItem for sending commands
+            public_send(method) == other.public_send(method)
+          end
+        end
+
         def_type_predicate(:color)
         def_type_predicate(:contact)
         def_type_predicate(:date_time)

lib/openhab/core/items/number_item.rb

@@ -48,6 +48,11 @@ def format_type(command)
           super
         end
 
+        # @!visibility private
+        def config_eql?(other)
+          super && dimension == other.dimension
+        end
+
         protected
 
         # Adds the unit dimension

lib/openhab/core/items/registry.rb

@@ -45,13 +45,19 @@ def to_a
         # Enter the Item Builder DSL.
         #
         # @param (see Core::Provider.current)
+        # @param update [true, false]  Update existing items with the same name.
+        #   When false, an error will be raised if an item with the same name already exists.
         # @yield Block executed in the context of a {DSL::Items::Builder}
         # @return [Object] The return value of the block.
+        # @raise [ArgumentError] if an item with the same name already exists and `update` is false.
+        # @raise [FrozenError] if `update` is true but the existing item with the same name
+        #   wasn't created by the current provider.
         #
         # @see DSL::Items::Builder
         #
-        def build(preferred_provider = nil, &block)
-          DSL::Items::BaseBuilderDSL.new(preferred_provider).instance_eval_with_dummy_items(&block)
+        def build(preferred_provider = nil, update: true, &block)
+          DSL::Items::BaseBuilderDSL.new(preferred_provider, update: update)
+                                    .instance_eval_with_dummy_items(&block)
         end
 
         #

lib/openhab/core/registry.rb

@@ -20,12 +20,20 @@ class Registry
       def provider_for(key)
         elementReadLock.lock
         if key.is_a?(org.openhab.core.common.registry.Identifiable)
-          element = key
-        else
-          return nil unless (element = identifierToElement[key])
-        end
+          return unless (provider = elementToProvider[key])
+
+          # The HashMap lookup above uses java's hashCode() which has been overridden
+          # by GenericItem and ThingImpl to return object's uid, e.g. item's name, thing uid
+          # so it will return a provider even for an unmanaged object having the same uid
+          # as an existing managed object.
 
-        elementToProvider[element]
+          # So take an extra step to verify that the provider really holds the given instance.
+          # by using equal? to compare the object's identity.
+          # Only ManagedProviders have a #get method to look up the object by uid.
+          provider if !provider.is_a?(ManagedProvider) || provider.get(key.uid).equal?(key)
+        elsif (element = identifierToElement[key])
+          elementToProvider[element]
+        end
       ensure
         elementReadLock.unlock
       end

lib/openhab/core/sitemaps/provider.rb

@@ -57,6 +57,7 @@ def unregister
         #
         # Enter the Sitemap Builder DSL.
         #
+        # @param update [true, false]  When true, existing sitemaps with the same name will be updated.
         # @yield Block executed in the context of a {DSL::Sitemaps::Builder}
         # @return [void]
         #
@@ -82,8 +83,8 @@ def unregister
         #     end
         #   end
         #
-        def build(&block)
-          DSL::Sitemaps::Builder.new(self).instance_eval(&block)
+        def build(update: true, &block)
+          DSL::Sitemaps::Builder.new(self, update: update).instance_eval(&block)
         end
         # rubocop:enable Layout/LineLength
 
@@ -123,7 +124,7 @@ def notify_listeners_about_removed_element(element)
           @listeners.each { |l| l.model_changed(element.name, org.openhab.core.model.core.EventType::REMOVED) }
         end
 
-        def notify_listeners_about_updated_element(element)
+        def notify_listeners_about_updated_element(_old_element, element)
           @listeners.each { |l| l.model_changed(element.name, org.openhab.core.model.core.EventType::MODIFIED) }
         end
       end

lib/openhab/core/things/links/provider.rb

@@ -24,13 +24,11 @@ def registry
             end
 
             # @!visibility private
-            def link(item, channel, config = {})
+            def create_link(item, channel, config = {})
               config = Configuration.new(config.transform_keys(&:to_s))
               channel = ChannelUID.new(channel) if channel.is_a?(String)
               channel = channel.uid if channel.is_a?(Channel)
-              link = org.openhab.core.thing.link.ItemChannelLink.new(item.name, channel, config)
-
-              current.add(link)
+              org.openhab.core.thing.link.ItemChannelLink.new(item.name, channel, config)
             end
           end
 

lib/openhab/core/things/registry.rb

@@ -39,11 +39,17 @@ def to_a
         #
         # Enter the Thing Builder DSL.
         # @param (see Core::Provider.current)
+        # @param update [true, false]
+        #   When true, existing things with the same name will be redefined if they're different.
+        #   When false, an error will be raised if a thing with the same uid already exists.
         # @yield Block executed in the context of a {DSL::Things::Builder}.
         # @return [Object] The result of the block.
+        # @raise [ArgumentError] if a thing with the same uid already exists and `update` is false.
+        # @raise [FrozenError] if `update` is true but the existing thing with the same uid
+        #   wasn't created by the current provider.
         #
-        def build(preferred_provider = nil, &block)
-          DSL::Things::Builder.new(preferred_provider).instance_eval(&block)
+        def build(preferred_provider = nil, update: true, &block)
+          DSL::Things::Builder.new(preferred_provider, update: update).instance_eval(&block)
         end
 
         #

lib/openhab/core/things/thing.rb

@@ -197,6 +197,16 @@ def actions(scope = nil)
           $actions.get(scope || uid.binding_id, uid.to_s)
         end
 
+        #
+        # Compares all attributes of the thing with another thing.
+        #
+        # @param other [Thing] The thing to compare with
+        # @return [true,false] true if all attributes are equal, false otherwise
+        #
+        def config_eql?(other)
+          %i[uid label channels bridge_uid location configuration].all? { |method| send(method) == other.send(method) }
+        end
+
         #
         # Delegate missing methods to the thing's default actions scope.
         #

lib/openhab/dsl/items/builder.rb

@@ -118,23 +118,68 @@ class BaseBuilderDSL
         class ProviderWrapper
           attr_reader :provider
 
-          def initialize(provider)
+          def initialize(provider, update:)
             @provider = provider
+            @update = update
           end
 
           # @!visibility private
           def add(builder)
-            item = builder.build
-            provider.add(item)
+            if DSL.items.key?(builder.name)
+              raise ArgumentError, "Item #{builder.name} already exists" unless @update
+
+              # Use provider.get because openHAB's ManagedProvider does not support the #[] method.
+              unless (old_item = provider.get(builder.name))
+                raise FrozenError, "Item #{builder.name} is managed by #{DSL.items[builder.name].provider}"
+              end
+
+              item = builder.build
+              if item.config_eql?(old_item)
+                logger.debug { "Not replacing existing item #{item.uid} because it is identical" }
+                item = old_item
+              else
+                logger.debug { "Replacing existing item #{item.uid} because it is not identical" }
+                provider.update(item)
+              end
+              item.metadata.merge!(builder.metadata)
+              item.metadata
+                  .reject { |namespace, _| builder.metadata.key?(namespace) }
+                  .each do |namespace, metadata|
+                item.metadata.delete(namespace) if metadata.provider == Core::Items::Metadata::Provider.current
+              end
+            else
+              item = builder.build
+              item.metadata.merge!(builder.metadata)
+              provider.add(item)
+            end
+
+            item.update(builder.state) unless builder.state.nil?
+
             # make sure to add the item to the registry before linking it
-            builder.channels.each do |(channel, config)|
+            provider = Core::Things::Links::Provider.current
+            channel_uids = builder.channels.to_set do |(channel, config)|
+              # fill in partial channel names from group's thing id
               if !channel.include?(":") &&
                  (group = builder.groups.find { |g| g.is_a?(GroupItemBuilder) && g.thing })
                 thing = group.thing
                 channel = "#{thing}:#{channel}"
               end
-              Core::Things::Links::Provider.link(item, channel, config)
+
+              new_link = Core::Things::Links::Provider.create_link(item, channel, config)
+              if !(current_link = provider.get(new_link.uid))
+                provider.add(new_link)
+              elsif current_link.configuration != config
+                provider.update(new_link)
+              end
+
+              new_link.linked_uid
             end
+
+            # remove links not in the new item
+            provider.all.each do |link|
+              provider.remove(link.uid) if link.item_name == item.name && !channel_uids.include?(link.linked_uid)
+            end
+
             item
           end
         end
@@ -143,8 +188,8 @@ def add(builder)
         # @return [org.openhab.core.items.ItemProvider]
         attr_reader :provider
 
-        def initialize(provider)
-          @provider = ProviderWrapper.new(Core::Items::Provider.current(provider))
+        def initialize(provider, update:)
+          @provider = ProviderWrapper.new(Core::Items::Provider.current(provider), update: update)
         end
       end
 
@@ -549,12 +594,10 @@ def build
           tags.each do |tag|
             item.add_tag(tag)
           end
-          item.metadata.merge!(metadata)
-          item.metadata["autoupdate"] = autoupdate.to_s unless autoupdate.nil?
-          item.metadata["expire"] = expire if expire
-          item.metadata["stateDescription"] = { "pattern" => format } if format
-          item.metadata["unit"] = unit if unit
-          item.state = item.format_update(state) unless state.nil?
+          metadata["autoupdate"] = autoupdate.to_s unless autoupdate.nil?
+          metadata["expire"] = expire if expire
+          metadata["stateDescription"] = { "pattern" => format } if format
+          metadata["unit"] = unit if unit
           item
         end
 

lib/openhab/dsl/sitemaps/builder.rb

@@ -12,8 +12,9 @@ module Sitemaps
       # Base sitemap builder DSL
       class Builder
         # @!visibility private
-        def initialize(provider)
+        def initialize(provider, update:)
           @provider = provider
+          @update = update
         end
 
         # (see SitemapBuilder#initialize)
@@ -23,8 +24,12 @@ def initialize(provider)
         def sitemap(name, label = nil, icon: nil, &block)
           sitemap = SitemapBuilder.new(name, label, icon: icon)
           sitemap.instance_eval_with_dummy_items(&block) if block
-          @provider.add(sitemap.build)
-          sitemap
+          sitemap = sitemap.build
+          if @update && @provider.get(sitemap.uid)
+            @provider.update(sitemap)
+          else
+            @provider.add(sitemap)
+          end
         end
       end