From 519da81c9fc820847d9e561cd90134ae08efa28c Mon Sep 17 00:00:00 2001 From: Jimmy Tanagra Date: Tue, 26 Sep 2023 19:24:40 +1000 Subject: [PATCH] Support updating existing things/items/sitemaps Signed-off-by: Jimmy Tanagra --- lib/openhab/core/emulate_hash.rb | 7 +- lib/openhab/core/items/group_item.rb | 16 ++ lib/openhab/core/items/item.rb | 28 ++- lib/openhab/core/items/number_item.rb | 5 + lib/openhab/core/items/registry.rb | 10 +- lib/openhab/core/registry.rb | 4 +- lib/openhab/core/sitemaps/provider.rb | 7 +- lib/openhab/core/things/links/provider.rb | 6 +- lib/openhab/core/things/registry.rb | 10 +- lib/openhab/core/things/thing.rb | 10 + lib/openhab/dsl/items/builder.rb | 69 ++++-- lib/openhab/dsl/sitemaps/builder.rb | 11 +- lib/openhab/dsl/things/builder.rb | 32 ++- spec/openhab/dsl/items/builder_spec.rb | 271 +++++++++++++++++++++- spec/openhab/dsl/sitemaps/builder_spec.rb | 12 + spec/openhab/dsl/things/builder_spec.rb | 132 +++++++++++ 16 files changed, 586 insertions(+), 44 deletions(-) diff --git a/lib/openhab/core/emulate_hash.rb b/lib/openhab/core/emulate_hash.rb index 12aefad283..63dcc48b44 100644 --- a/lib/openhab/core/emulate_hash.rb +++ b/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! diff --git a/lib/openhab/core/items/group_item.rb b/lib/openhab/core/items/group_item.rb index 8ff8b2a020..af9f01c66f 100644 --- a/lib/openhab/core/items/group_item.rb +++ b/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 diff --git a/lib/openhab/core/items/item.rb b/lib/openhab/core/items/item.rb index 14ece064bd..ce3efcac14 100644 --- a/lib/openhab/core/items/item.rb +++ b/lib/openhab/core/items/item.rb @@ -264,12 +264,17 @@ def thing # # @return [Array] 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] An array of ItemChannelLink or an empty array + def links + Things::Links::Provider.registry.get_links(name) + end + # @return [String] def inspect s = "# 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..f277953574 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 #{thing.provider}" + 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 12742d062f..83f075b909 100644 --- a/spec/openhab/dsl/items/builder_spec.rb +++ b/spec/openhab/dsl/items/builder_spec.rb @@ -11,9 +11,274 @@ 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 update: false" do + it "raises an error" do + items.build { switch_item "Switch1" } + expect { items.build(update: false) { switch_item "Switch1" } }.to raise_error(ArgumentError) + end + + it "the failure in creating the new item doesn't cause any changes to the original item" 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(update: false) 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 the default argument (update: true)" do + subject(:item) { Built } + + it "raises an error if the existing item is from a different provider" do + item_name = "ItemFromAnotherProvider" + existing_item = OpenHAB::Core::Items::StringItem.new(item_name) + generic_item_provider = OpenHAB::OSGi.service("org.openhab.core.items.ItemProvider") + allow(OpenHAB::DSL.items).to receive(:key?).with(item_name).and_return(true) + allow(OpenHAB::DSL.items).to receive(:[]).with(item_name).and_return(existing_item) + allow(existing_item).to receive(:provider).and_return(generic_item_provider) + expect { items.build { number_item item_name } }.to raise_error(FrozenError) + end + + # + # Create an item, then create it again with build(update: true) + # + # @param [Hash] org_config Config to create the original item + # @param [Hash] new_config Config to create the new item + # @param [:new_item,:old_item] item_to_keep Whether the new item should replace the old item or not + # @return [Array] + # An array of [original item, updated item]. These are unproxied raw items + # + def build_and_update(org_config, new_config, item_to_keep: :new_item, &block) + org_config = org_config.dup + new_config = new_config.dup + org_item = items.build do + send("#{org_config.delete(:type) || "number"}_item", "Built", org_config.delete(:label), **org_config) + end + # The proxy is cached based on uid, so when the proxy for the new item with the same uid/name is "created", + # it will reuse the existing proxy for the old item to hold the new item, overwriting the underlying object. + # So we must unwrap the actual item here before the new item is created. + org_item = org_item.__getobj__ + expect(Built.__getobj__).to be org_item + yield :original, org_item if block + + new_item = items.build do + send("#{new_config.delete(:type) || "number"}_item", "Built", new_config.delete(:label), **new_config) + end + new_item = new_item.__getobj__ + expect(Built.__getobj__).to be new_item + yield :updated, new_item if block + + case item_to_keep + when :new_item then expect(new_item).not_to be org_item + when :old_item then expect(new_item).to be org_item + end + + [org_item, new_item] + end + + context "with changes" do + it "replaces the old item when the item type is different" do + build_and_update({ type: "switch" }, { type: "string" }) + expect(item).to be_a_string_item + end + + it "replaces the old item when the label is different" do + build_and_update({ label: "Old Label" }, { label: "New Label" }) + expect(item.label).to eql "New Label" + end + + it "replaces the old item when the dimension is different" do + build_and_update({ dimension: "Power" }, { dimension: "Length" }) + expect(item.dimension.ruby_class).to be javax.measure.quantity.Length + end + + it "replaces the old item when a unit is added" do + build_and_update({}, { unit: "W" }) + expect(item.unit.to_s).to eql "W" + end + + it "replaces the old item when a unit is removed" do + build_and_update({ unit: "W" }, {}) + expect(item.unit).to be_nil + end + + it "replaces the old item when the groups changed" do + build_and_update({ group: "Group1" }, { groups: %w[Group1 Group2] }) + expect(item.group_names).to match_array %w[Group1 Group2] + end + + it "replaces the old item when the tags changed" do + build_and_update({ tags: %w[Tag1 Tag2 Light] }, { tags: %w[Tag1 Tag2] }) do |built, item| + expect(item.metadata[:semantics]).not_to be_nil if built == :original + end + expect(item.tags).to match_array %w[Tag1 Tag2] + expect(item.metadata[:semantics]).to be_nil + end + + it "replaces the old item when icon is different" do + build_and_update({ icon: :light }, {}) do |built, item| + case built + when :original then expect(item.category).to eql "light" + when :updated then expect(item.category).to be_nil + end + end + end + + it "keeps the old item but updates its links when the channels are different" do + things.build do + thing "a:b:c" do + channel "old", "number" + channel "new", "number" + end + end + build_and_update({ channel: "a:b:c:old" }, { channel: "a:b:c:new" }, item_to_keep: :old_item) + expect(item.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 + things.build do + thing "a:b:c" do + channel "d", "number" + end + end + build_and_update({ channel: ["a:b:c:d", { foo: "bar" }] }, + { channel: ["a:b:c:d", { foo: "qux" }] }, + item_to_keep: :old_item) + link = item.links.first + expect(link.linked_uid).to eql things["a:b:c"].channels["d"].uid + expect(link.configuration).to eq({ "foo" => "qux" }) + end + + it "replaces the old item when necessary but verifies that channels remain the same" do + things.build do + thing "a:b:c" do + channel "d", "number" + end + end + channel_config = { "foo" => "bar" }.freeze + build_and_update({ channel: ["a:b:c:d", channel_config] }, + { channel: ["a:b:c:d", channel_config], label: "x" }) + link = item.links.first + expect(link.linked_uid).to eql things["a:b:c"].channels["d"].uid + expect(link.configuration).to eq channel_config + end + + it "keeps the old item but delete its metadata when the new item has no metadata" do + build_and_update({ metadata: { foo: "baz" } }, {}, item_to_keep: :old_item) + expect(item.metadata.key?(:foo)).to be false + end + + it "keeps the old item but reset to the new metadata" do + build_and_update({ metadata: { foo: "bar", moo: "cow" } }, + { metadata: { foo: "baz", qux: "quux" } }, + item_to_keep: :old_item) + expect(item.metadata[:foo].value).to eq "baz" + expect(item.metadata[:qux].value).to eq "quux" + expect(item.metadata.key?(:moo)).to be false + end + + it "works when there's a special semantics (unmanaged) metadata" do + build_and_update({ tags: "Lightbulb", metadata: { qux: "quux" } }, + { tags: "Lightbulb", metadata: { foo: "bar" } }, + item_to_keep: :old_item) do |type, item| + expect(item.metadata[:semantics]).not_to be_nil if type == :original + end + expect(item.metadata.key?(:qux)).to be false + expect(item.metadata[:foo].value).to eql "bar" + end + + it "keeps the old item when only the format (state description metadata) is different" do + build_and_update({ format: "OLD %s" }, { format: "NEW %s" }, item_to_keep: :old_item) + expect(item.state_description.pattern).to eql "NEW %s" + end + + it "keeps the old item when autoupdate (metadata) is different" do + build_and_update({ autoupdate: true }, { autoupdate: false }, item_to_keep: :old_item) + expect(item.metadata[:autoupdate].value).to eql "false" + end + + it "keeps the old item but remove autoupdate (metadata)" do + build_and_update({ autoupdate: true }, {}, item_to_keep: :old_item) + expect(item.metadata.key?(:autoupdate)).to be false + end + + it "keeps the old item but add expire (metadata)" do + build_and_update({}, { expire: "5s" }, item_to_keep: :old_item) + expect(item.metadata[:expire].value).to eql "5s" + end + end + + context "with no changes" do + it "keeps the old item" do + properties = { + label: "Just a label", + tags: "Light", + unit: "W", + format: "%f", + ga: "Light", + groups: "Group1", + icon: :light, + autoupdate: true, + expire: "1h", + channel: "a:b:c:old" + }.freeze + + build_and_update(properties, properties, item_to_keep: :old_item) + end + + it "keeps the old item but updates to the new state" do + build_and_update({ state: 1 }, { state: 2 }, item_to_keep: :old_item) + expect(item.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..6bb4f84531 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 "with update: false, complains if you try to create a sitemap with the same name" do + sitemaps.build(update: false) { sitemap "default" } + expect { sitemaps.build(update: false) { sitemap "default" } }.to raise_error(ArgumentError) + end + + it "allows you to redefine a sitemap with the same name by default" do + sitemaps.build { sitemap "default" } + sitemaps.build { 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..8d95f9a02e 100644 --- a/spec/openhab/dsl/things/builder_spec.rb +++ b/spec/openhab/dsl/things/builder_spec.rb @@ -10,6 +10,138 @@ 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 update: false argument" 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(update: false) { thing uid, "new label" } }.to raise_error(ArgumentError) + end + + it "fails creating the new thing but doesn't cause any changes in the original thing" 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(update: false) 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 default arguments (update: true)" do + subject(:thing) { things[uid] } + + let(:uid) { "a:b:c" } + + it "refuses to update an existing thing from a different provider" do + uid = "a:b:file-thing" + existing_thing = OpenHAB::DSL::Things::ThingBuilder.new(uid).build + allow(OpenHAB::DSL.things).to receive(:key?).with(uid).and_return(true) + allow(OpenHAB::DSL.things).to receive(:[]).with(uid).and_return(existing_thing) + expect { things.build { thing uid } }.to raise_error(FrozenError) + end + + it "can create a new thing" do + things.build 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 + + # @return [Array] + # An array of unproxied things [original, updated] + def build_and_update(org_config, new_config, thing_to_keep: :new_thing, &block) + org_config = org_config.dup + new_config = new_config.dup + default_uid = uid + org_thing = things.build do + thing(org_config.delete(:uid) || default_uid, org_config.delete(:label), **org_config) + end + # Unwrap the thing object now before creating the new thing. + # See the comment in items/builder_spec.rb for more details. + org_thing = org_thing.__getobj__ + yield :original, org_thing if block + + new_thing = things.build do + thing(new_config.delete(:uid) || default_uid, new_config.delete(:label), **new_config) + end + new_thing = new_thing.__getobj__ + yield :updated, new_thing if block + + case thing_to_keep + when :new_thing then expect(new_thing).not_to be org_thing + when :old_thing then expect(new_thing).to be org_thing + end + + [org_thing, new_thing] + end + + context "with changes" do + it "replaces the old thing when the label is different" do + build_and_update({ label: "Old Label" }, { label: "New Label" }) + expect(thing.label).to eql "New Label" + end + + it "replaces the old thing when the bridge is different" do + build_and_update({ bridge: "a:b:bridge_a" }, { bridge: "a:b:bridge_b" }) + expect(thing.bridge_uid).to eq "a:b:bridge_b" + end + + it "replaces the old thing when the location is different" do + build_and_update({ location: "location a" }, {}) + expect(thing.location).to be_nil + end + + it "replaces the old thing when the config is different" do + build_and_update({ config: { ipAddress: "1" } }, { config: { ipAddress: "2" } }) + expect(thing.configuration[:ipAddress]).to eq "2" + end + + it "keeps the old thing when nothing is different" do + build_and_update({}, {}, thing_to_keep: :old_thing) + end + + it "keeps the old thing but update the state" do + build_and_update({}, { enabled: false }, thing_to_keep: :old_thing) + expect(thing).not_to be_enabled + end + end + end end it "can create a thing with separate binding and type params" do