From e038ae2affb188772b314c062a261db4dd3961d8 Mon Sep 17 00:00:00 2001
From: Jimmy Tanagra <jcode@tanagra.id.au>
Date: Sat, 23 Sep 2023 20:57:36 +1000
Subject: [PATCH] Support updating existing things/items/sitemaps

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
---
 lib/openhab/core/emulate_hash.rb          |   6 +-
 lib/openhab/core/items/group_item.rb      |  12 +
 lib/openhab/core/items/item.rb            |  27 ++-
 lib/openhab/core/items/metadata/hash.rb   |   2 +-
 lib/openhab/core/items/number_item.rb     |   6 +
 lib/openhab/core/items/registry.rb        |   6 +-
 lib/openhab/core/sitemaps/provider.rb     |   7 +-
 lib/openhab/core/things/registry.rb       |   6 +-
 lib/openhab/core/things/thing.rb          |  20 ++
 lib/openhab/dsl/items/builder.rb          |  73 ++++--
 lib/openhab/dsl/sitemaps/builder.rb       |  11 +-
 lib/openhab/dsl/things/builder.rb         |  32 ++-
 spec/openhab/dsl/items/builder_spec.rb    | 262 +++++++++++++++++++++-
 spec/openhab/dsl/sitemaps/builder_spec.rb |  12 +
 spec/openhab/dsl/things/builder_spec.rb   |  79 +++++++
 15 files changed, 523 insertions(+), 38 deletions(-)

diff --git a/lib/openhab/core/emulate_hash.rb b/lib/openhab/core/emulate_hash.rb
index 12aefad283..aaa13e65a2 100644
--- a/lib/openhab/core/emulate_hash.rb
+++ b/lib/openhab/core/emulate_hash.rb
@@ -103,7 +103,11 @@ 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
       end
       alias_method :update, :merge!
 
diff --git a/lib/openhab/core/items/group_item.rb b/lib/openhab/core/items/group_item.rb
index 8ff8b2a020..9eee353a9d 100644
--- a/lib/openhab/core/items/group_item.rb
+++ b/lib/openhab/core/items/group_item.rb
@@ -152,6 +152,18 @@ 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
+        #
+        def config_eql?(other)
+          return false unless super
+
+          base_item&.name == other.base_item&.name && function.to_s == other.function.to_s
+        end
+
         private
 
         # Add base type and function details
diff --git a/lib/openhab/core/items/item.rb b/lib/openhab/core/items/item.rb
index 672a901370..618c2ece4b 100644
--- a/lib/openhab/core/items/item.rb
+++ b/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,22 @@ 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
+        #
+        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)
diff --git a/lib/openhab/core/items/metadata/hash.rb b/lib/openhab/core/items/metadata/hash.rb
index b6d4a5c7e7..b3e2860d26 100644
--- a/lib/openhab/core/items/metadata/hash.rb
+++ b/lib/openhab/core/items/metadata/hash.rb
@@ -237,7 +237,7 @@ def provider
               return preferred_provider unless provider
 
               unless provider.is_a?(org.openhab.core.common.registry.ManagedProvider)
-                raise FrozenError, "Cannot modify metadata from provider #{provider.inspect} for #{uid}."
+                raise FrozenError, "Cannot modify metadata #{@metadata} from provider #{provider.inspect} for #{uid}."
               end
 
               if preferred_provider != provider
diff --git a/lib/openhab/core/items/number_item.rb b/lib/openhab/core/items/number_item.rb
index 72b320a737..8b5af73925 100644
--- a/lib/openhab/core/items/number_item.rb
+++ b/lib/openhab/core/items/number_item.rb
@@ -48,6 +48,12 @@ def format_type(command)
           super
         end
 
+        def config_eql?(other)
+          return false unless super
+
+          dimension == other.dimension
+        end
+
         protected
 
         # Adds the unit dimension
diff --git a/lib/openhab/core/items/registry.rb b/lib/openhab/core/items/registry.rb
index 6bb17d8880..eca0d027c6 100644
--- a/lib/openhab/core/items/registry.rb
+++ b/lib/openhab/core/items/registry.rb
@@ -45,13 +45,15 @@ def to_a
         # Enter the Item Builder DSL.
         #
         # @param (see Core::Provider.current)
+        # @param update [true, false]  Update existing items with the same name.
         # @yield Block executed in the context of a {DSL::Items::Builder}
         # @return [Object] The return value of the block.
         #
         # @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: false, &block)
+          DSL::Items::BaseBuilderDSL.new(preferred_provider, update: update)
+                                    .instance_eval_with_dummy_items(&block)
         end
 
         #
diff --git a/lib/openhab/core/sitemaps/provider.rb b/lib/openhab/core/sitemaps/provider.rb
index cbb4a67da0..33932fe16e 100644
--- a/lib/openhab/core/sitemaps/provider.rb
+++ b/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: false, &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
diff --git a/lib/openhab/core/things/registry.rb b/lib/openhab/core/things/registry.rb
index fa34a06897..8f24784aae 100644
--- a/lib/openhab/core/things/registry.rb
+++ b/lib/openhab/core/things/registry.rb
@@ -39,11 +39,13 @@ def to_a
         #
         # Enter the Thing Builder DSL.
         # @param (see Core::Provider.current)
+        # @param update [true, false]
+        #   When true, existing items with the same name will be redefined if they're different.
         # @yield Block executed in the context of a {DSL::Things::Builder}.
         # @return [Object] The result of the block.
         #
-        def build(preferred_provider = nil, &block)
-          DSL::Things::Builder.new(preferred_provider).instance_eval(&block)
+        def build(preferred_provider = nil, update: false, &block)
+          DSL::Things::Builder.new(preferred_provider, update: update).instance_eval(&block)
         end
 
         #
diff --git a/lib/openhab/core/things/thing.rb b/lib/openhab/core/things/thing.rb
index e1e1069d8c..89cc3e6da0 100644
--- a/lib/openhab/core/things/thing.rb
+++ b/lib/openhab/core/things/thing.rb
@@ -192,6 +192,26 @@ 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)
+          return false unless eql?(other)
+
+          %i[
+            uid
+            label
+            channels
+            bridgeuid
+            location
+          ].all? do |method|
+            public_send(method) == other.public_send(method)
+          end
+        end
+
         #
         # Delegate missing methods to the thing's default actions scope.
         #
diff --git a/lib/openhab/dsl/items/builder.rb b/lib/openhab/dsl/items/builder.rb
index 6fd5348c18..0781e1e813 100644
--- a/lib/openhab/dsl/items/builder.rb
+++ b/lib/openhab/dsl/items/builder.rb
@@ -118,23 +118,72 @@ 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)
-            # make sure to add the item to the registry before linking it
-            builder.channels.each do |(channel, config)|
+            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)
+            else
+              item = builder.build
+              item.metadata.merge!(builder.metadata)
+              provider.add(item)
+            end
+            # item.metadata.merge!(builder.metadata)
+
+            item.update(builder.state) unless builder.state.nil?
+
+            # fill in partial channel names from group's thing id
+            channels = builder.channels.map do |(channel, config)|
               if !channel.include?(":") &&
                  (group = builder.groups.find { |g| g.is_a?(GroupItemBuilder) && g.thing })
                 thing = group.thing
                 channel = "#{thing}:#{channel}"
               end
+              [channel, config]
+            end
+
+            # make sure to add the item to the registry before linking it
+            provider = Core::Things::Links::Provider.current
+            channels.each do |(channel, config)|
+              current_link = provider.all.find { |link| link.item_name == item.name && link.linked_uid == channel }
+              if current_link
+                next if current_link.configuration == config
+
+                provider.remove(current_link.uid)
+              end
+
               Core::Things::Links::Provider.link(item, channel, config)
             end
+
+            # remove links not in the new item
+            removed_links = provider.all.select do |link|
+              next unless link.item_name == item.name
+
+              channels.none? { |(channel, _config)| channel == link.linked_uid }
+            end
+            removed_links.each do |link|
+              provider.remove(link.uid)
+            end
+
             item
           end
         end
@@ -143,8 +192,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
 
@@ -537,12 +586,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
 
diff --git a/lib/openhab/dsl/sitemaps/builder.rb b/lib/openhab/dsl/sitemaps/builder.rb
index 51902f573f..385f1dfce6 100644
--- a/lib/openhab/dsl/sitemaps/builder.rb
+++ b/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
 
diff --git a/lib/openhab/dsl/things/builder.rb b/lib/openhab/dsl/things/builder.rb
index 5a6e80d557..74a8182fc8 100644
--- a/lib/openhab/dsl/things/builder.rb
+++ b/lib/openhab/dsl/things/builder.rb
@@ -40,8 +40,9 @@ class Builder
         # @return [org.openhab.core.thing.ManagedThingProvider]
         attr_reader :provider
 
-        def initialize(provider)
+        def initialize(provider, update: false)
           @provider = Core::Things::Provider.current(provider)
+          @update = update
         end
 
         # Create a new Bridge
@@ -61,10 +62,28 @@ def thing(*args, **kwargs, &block)
         def build(klass, *args, **kwargs, &block)
           builder = klass.new(*args, **kwargs)
           builder.instance_eval(&block) if block
-          thing = provider.add(builder.build)
-          thing = Core::Things::Proxy.new(thing)
+          thing = builder.build
+
+          if DSL.things.key?(thing.uid)
+            raise ArgumentError, "Thing #{thing.uid} already exists" unless @update
+
+            unless (old_thing = provider.get(thing.uid))
+              raise FrozenError,
+                    "Thing #{thing.uid} is managed by #{Core::Things::Provider.registry.provider_for(thing.uid)}"
+            end
+
+            if thing.config_eql?(old_thing)
+              logger.debug { "Not replacing existing thing #{thing.uid}" }
+              thing = old_thing
+            else
+              provider.update(thing)
+            end
+          else
+            provider.add(thing)
+          end
           thing.enable(enabled: builder.enabled) unless builder.enabled.nil?
-          thing
+
+          Core::Things::Proxy.new(thing)
         end
       end
 
@@ -196,6 +215,7 @@ def build
           builder = org.openhab.core.thing.binding.builder.ThingBuilder
                        .create(thing_type_uid, uid)
                        .with_label(label)
+                       .with_location(location)
                        .with_configuration(configuration)
                        .with_bridge(bridge_uid)
                        .with_channels(channels)
@@ -210,9 +230,7 @@ def build
             builder.with_properties(thing_type.properties)
           end
 
-          thing = builder.build
-          Core::Things.manager.set_enabled(uid, enabled) unless enabled.nil?
-          thing
+          builder.build
         end
 
         private
diff --git a/spec/openhab/dsl/items/builder_spec.rb b/spec/openhab/dsl/items/builder_spec.rb
index ee5ef5a8d7..3dfde95637 100644
--- a/spec/openhab/dsl/items/builder_spec.rb
+++ b/spec/openhab/dsl/items/builder_spec.rb
@@ -11,9 +11,265 @@
     end
   end
 
-  it "complains if you try to add an item that already exists" do
-    items.build { switch_item "Switch1" }
-    expect { items.build { switch_item "Switch1" } }.to raise_error(ArgumentError)
+  context "when items already exist and build is called" do
+    context "with default arguments" do
+      it "raises an error" do
+        items.build { switch_item "Switch1" }
+        expect { items.build { switch_item "Switch1" } }.to raise_error(ArgumentError)
+      end
+
+      it "keeps the original item unmodified" do
+        things.build do
+          thing "a:b:c" do
+            channel "old", "switch"
+            channel "new", "switch"
+          end
+        end
+
+        properties = {
+          tags: ["Light"],
+          unit: "W",
+          format: "%f",
+          ga: ["Light", {}],
+          groups: ["Group1"],
+          icon: "light",
+          autoupdate: "true",
+          expire: "1h",
+          channel: "a:b:c:old",
+          state: 1
+        }.freeze
+
+        items.build { number_item "Number1", "Old Label", **properties }
+
+        expect do
+          items.build do
+            number_item "Number1",
+                        "New Label",
+                        tags: "Measurement",
+                        unit: "°C",
+                        format: "%s",
+                        ga: "Fan",
+                        groups: "Group2",
+                        icon: "window",
+                        autoupdate: "false",
+                        expire: ["2h", { ignore_state_updates: true }],
+                        channel: "a:b:c:new",
+                        state: 2
+          end
+        end.to raise_error(ArgumentError)
+        expect(Number1.label).to eql "Old Label"
+        expect(Number1.tags.to_a).to eql properties[:tags]
+        expect(Number1.unit.to_s).to eql properties[:unit]
+        expect(Number1.state_description.pattern).to eql properties[:format]
+        expect(Number1.metadata[:ga]).to eq properties[:ga]
+        expect(Number1.group_names.to_a).to eql properties[:groups]
+        expect(Number1.category).to eql properties[:icon]
+        expect(Number1.metadata[:autoupdate].value).to eql properties[:autoupdate]
+        expect(Number1.metadata[:expire].value).to eq properties[:expire]
+        expect(Number1.metadata[:expire]).to be_empty
+        expect(Number1.links.first.linked_uid.to_s).to eql properties[:channel]
+        expect(Number1.links.size).to be 1
+        expect(Number1.state.to_i).to eql properties[:state]
+      end
+    end
+
+    context "with update: true argument" do
+      it "raises an error if the existing item is from a different provider" do
+        items_file = OpenHAB::Core.config_folder / "items" / "test.items"
+        name = "FileSwitch1" # use a unique name so we don't have to wait for its removal
+        File.write(items_file, "Switch #{name}")
+        wait { expect(items[name]).not_to be_nil }
+        expect { items.build(update: true) { number_item name } }.to raise_error(FrozenError)
+      ensure
+        File.delete(items_file)
+      end
+
+      context "with changes" do
+        before do
+          things.build do
+            thing "a:b:c" do
+              channel "old", "switch"
+              channel "new", "switch"
+            end
+          end
+        end
+
+        it "replaces the old item when the item type is different" do
+          items.build do
+            switch_item "ReplacedItem"
+          end
+          items.build(update: true) do
+            string_item "ReplacedItem"
+          end
+          expect(ReplacedItem).to be_a_string_item
+        end
+
+        it "replaces the old item when the label is different" do
+          items.build do
+            switch_item "ReplacedItem", "Old Label"
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem", "New Label"
+          end
+          expect(ReplacedItem.label).to eql "New Label"
+        end
+
+        it "replaces the old item when the dimension is different" do
+          items.build do
+            number_item "ReplacedItem", dimension: "Power"
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem", dimension: "Length"
+          end
+          expect(ReplacedItem.dimension.ruby_class).to be javax.measure.quantity.Length
+        end
+
+        it "replaces the old item when a unit is added" do
+          items.build do
+            number_item "ReplacedItem"
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem", unit: "W"
+          end
+          expect(ReplacedItem.unit.to_s).to eql "W"
+        end
+
+        it "replaces the old item when a unit is removed" do
+          items.build do
+            number_item "ReplacedItem", unit: "W"
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem"
+          end
+          expect(ReplacedItem.unit).to be_nil
+        end
+
+        it "replaces the old item when the groups changed" do
+          items.build do
+            number_item "ReplacedItem", group: "Group1"
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem", groups: %w[Group1 Group2]
+          end
+          expect(ReplacedItem.group_names).to match_array %w[Group1 Group2]
+        end
+
+        it "replaces the old item when the tags changed" do
+          items.build do
+            number_item "ReplacedItem", tags: %w[Tag1 Tag2 Light]
+          end
+          # The `Light` tag causes the `semantics` namespace to be added to the metadata
+          expect(ReplacedItem.metadata[:semantics]).not_to be_nil
+          items.build(update: true) do
+            number_item "ReplacedItem", tags: %w[Tag1 Tag2]
+          end
+          expect(ReplacedItem.tags).to match_array %w[Tag1 Tag2]
+          expect(ReplacedItem.metadata[:semantics]).to be_nil
+        end
+
+        it "keeps the old item but updates its links when only channels are different" do
+          old_item = items.build do
+            switch_item "ReplacedItem", channel: "a:b:c:old"
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem", channel: "a:b:c:new"
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+          expect(ReplacedItem.links.map(&:linked_uid)).to match_array(things["a:b:c"].channels["new"].uid)
+        end
+
+        it "keeps the old item but updates its links when the channel configs are different" do
+          old_item = items.build do
+            switch_item "ReplacedItem", channel: ["a:b:c:new", { foo: "bar" }]
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem", channel: ["a:b:c:new", { foo: "baz" }]
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+          link = ReplacedItem.links.first
+          expect(link.linked_uid).to eql things["a:b:c"].channels["new"].uid
+          expect(link.configuration).to eq({ "foo" => "baz" })
+        end
+
+        it "replaces the old item when the format is different" do
+          items.build do
+            switch_item "ReplacedItem", format: "OLD %s"
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem", format: "NEW %s"
+          end
+          expect(ReplacedItem.state_description.pattern).to eql "NEW %s"
+        end
+
+        it "keeps the old item and its metadata when only its metadata is missing in the new item" do
+          old_item = items.build do
+            switch_item "ReplacedItem", metadata: { foo: "baz" }
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem"
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+          expect(ReplacedItem.metadata[:foo].value).to eq "baz"
+        end
+
+        it "keeps the old item and merge the new metadata" do
+          old_item = items.build do
+            switch_item "ReplacedItem", metadata: { foo: "bar", moo: "cow" }
+          end
+          items.build(update: true) do
+            switch_item "ReplacedItem", metadata: { foo: "baz" }
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+          expect(ReplacedItem.metadata[:foo].value).to eq "baz"
+          expect(ReplacedItem.metadata[:moo].value).to eq "cow"
+        end
+
+        it "replaces the old item when icon is different" do
+          items.build do
+            number_item "ReplacedItem", icon: :light
+          end
+          expect(ReplacedItem.category).to eql "light"
+          items.build(update: true) do
+            number_item "ReplacedItem"
+          end
+          expect(ReplacedItem.category).to be_nil
+        end
+      end
+
+      context "with no changes" do
+        it "keeps the old item" do
+          properties = {
+            tags: "Light",
+            unit: "W",
+            format: "%f",
+            ga: "Light",
+            groups: "Group1",
+            icon: :light,
+            autoupdate: true,
+            expire: "1h",
+            channel: "a:b:c:old"
+          }
+          old_item = items.build do
+            number_item "ReplacedItem", "Same Label", **properties
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem", "Same Label", **properties
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+        end
+
+        it "updates to the new state" do
+          old_item = items.build do
+            number_item "ReplacedItem", state: 1
+          end
+          items.build(update: true) do
+            number_item "ReplacedItem", state: 2
+          end
+          expect(ReplacedItem.__getobj__).to be old_item.__getobj__
+          expect(ReplacedItem.state).to eq 2
+        end
+      end
+    end
   end
 
   it "can remove an item" do
diff --git a/spec/openhab/dsl/sitemaps/builder_spec.rb b/spec/openhab/dsl/sitemaps/builder_spec.rb
index 2e2c13666f..60711d482f 100644
--- a/spec/openhab/dsl/sitemaps/builder_spec.rb
+++ b/spec/openhab/dsl/sitemaps/builder_spec.rb
@@ -167,4 +167,16 @@
       end
     end
   end
+
+  context "when redefining a sitemap" do
+    it "complains if you try to create a sitemap with the same name" do
+      sitemaps.build { sitemap "default" }
+      expect { sitemaps.build { sitemap "default" } }.to raise_error(ArgumentError)
+    end
+
+    it "allows you to redefine a sitemap with the same name if update: true" do
+      sitemaps.build { sitemap "default" }
+      sitemaps.build(update: true) { sitemap "default" }
+    end
+  end
 end
diff --git a/spec/openhab/dsl/things/builder_spec.rb b/spec/openhab/dsl/things/builder_spec.rb
index ca0e8f6449..5107d7f90d 100644
--- a/spec/openhab/dsl/things/builder_spec.rb
+++ b/spec/openhab/dsl/things/builder_spec.rb
@@ -10,6 +10,85 @@
     expect(home = things["astro:sun:home"]).not_to be_nil
     expect(home.channels["rise#event"]).not_to be_nil
     expect(home.configuration.get("geolocation")).to eq "0,0"
+    expect(home).to be_enabled
+  end
+
+  context "when things already exist and build is called" do
+    context "with default arguments" do
+      it "complains if you try to create a thing with the same UID" do
+        uid = "a:b:c"
+        things.build { thing uid, "old label" }
+        expect { things.build { thing uid, "new label" } }.to raise_error(ArgumentError)
+      end
+
+      it "keeps the original thing unmodified" do
+        uid = "a:b:c"
+        old_channel = nil
+        properties = {
+          bridge: "a:b:bridge_a",
+          location: "location_a",
+          config: { identity: "config_a" },
+          enabled: true
+        }.freeze
+        old_thing = things.build do
+          thing uid, "old label", **properties do
+            old_channel = channel "x", "string"
+          end
+        end
+
+        expect(things[uid].enabled?).to eql properties[:enabled]
+
+        expect do
+          things.build do
+            thing uid,
+                  "new label",
+                  bridge: "a:b:bridge_b",
+                  location: "location_b",
+                  config: { identity: "config_b" },
+                  enabled: !properties[:enabled] do
+              channel y: "number"
+            end
+          end
+        end.to raise_error(ArgumentError)
+        thing = things[uid]
+        expect(thing.label).to eql "old label"
+        expect(thing.__getobj__).to be old_thing.__getobj__
+        expect(thing.bridge_uid).to eq properties[:bridge]
+        expect(thing.location).to eq properties[:location]
+        expect(thing.configuration).to eq properties[:config]
+        expect(thing.enabled?).to eql properties[:enabled]
+        expect(thing.channels).to match_array(old_channel)
+      end
+    end
+
+    context "with update: true argument" do
+      it "refuses to update an existing thing from a different provider" do
+        uid = "a:b:file-thing" # use a unique uid so we don't have to wait for its removal
+        things_file = OpenHAB::Core.config_folder / "things" / "test.things"
+        File.write(things_file, "Thing #{uid}")
+        wait { expect(things[uid]).not_to be_nil }
+        expect { things.build(update: true) { thing uid } }.to raise_error(FrozenError)
+      ensure
+        File.delete(things_file)
+      end
+
+      it "can create a new thing" do
+        things.build(update: true) do
+          thing "astro:sun:home", "Astro Sun Data", config: { "geolocation" => "0,0" }
+        end
+        expect(home = things["astro:sun:home"]).not_to be_nil
+        expect(home.channels["rise#event"]).not_to be_nil
+        expect(home.configuration.get("geolocation")).to eq "0,0"
+      end
+
+      it "replaces the existing things" do
+        uid = "replace:existing:thing"
+        things.build { thing uid, "Old Label" }
+        expect(things[uid].label).to eql "Old Label"
+        things.build(update: true) { thing uid, "New Label" }
+        expect(things[uid].label).to eql "New Label"
+      end
+    end
   end
 
   it "can create a thing with separate binding and type params" do