From f103c8dfbf114e1e5ec3dd42e4e6e701f93e62d7 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Mon, 25 Nov 2024 14:04:29 -0700 Subject: [PATCH] [mqtt.homeassistant] Fix duplicate component resolution when unique_id is set (#17803) * [mqtt.homeassistant] fix duplicate component resolution when unique_id is set unique_id is only unique per component type. so we need to a) take that into account, and b) use that when resolving duplicates Signed-off-by: Cody Cutrer Signed-off-by: Ciprian Pascu --- .../internal/component/AbstractComponent.java | 11 +- .../HomeAssistantThingHandlerTests.java | 178 +++++++++++++++++- 2 files changed, 180 insertions(+), 9 deletions(-) diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java index e6f0577045e..28e17c01a1f 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java @@ -118,7 +118,7 @@ public abstract class AbstractComponent groupId = null; componentId = ""; } - uniqueId = this.haID.getGroupId(channelConfiguration.getUniqueId(), false); + uniqueId = haID.component + "_" + haID.getGroupId(channelConfiguration.getUniqueId(), false); this.configSeen = false; @@ -182,8 +182,13 @@ public abstract class AbstractComponent } public void resolveConflict() { - componentId = this.haID.getGroupId(channelConfiguration.getUniqueId(), newStyleChannels); - channels.values().forEach(c -> c.resetUID(buildChannelUID(c.getChannel().getUID().getIdWithoutGroup()))); + if (newStyleChannels && channels.size() == 1) { + componentId = componentId + "_" + haID.component; + channels.values().forEach(c -> c.resetUID(buildChannelUID(componentId))); + } else { + groupId = componentId = componentId + "_" + haID.component; + channels.values().forEach(c -> c.resetUID(buildChannelUID(c.getChannel().getUID().getIdWithoutGroup()))); + } } protected ComponentChannel.Builder buildChannel(String channelID, ComponentChannelType channelType, diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandlerTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandlerTests.java index 8d207b8918b..3f4c066239a 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandlerTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandlerTests.java @@ -60,12 +60,11 @@ public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests { private static final int ATTRIBUTE_RECEIVE_TIMEOUT = 2000; private static final List CONFIG_TOPICS = Arrays.asList("climate/0x847127fffe11dd6a_climate_zigbee2mqtt", - "switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt", - - "sensor/0x1111111111111111_test_sensor_zigbee2mqtt", "camera/0x1111111111111111_test_camera_zigbee2mqtt", - - "cover/0x2222222222222222_test_cover_zigbee2mqtt", "fan/0x2222222222222222_test_fan_zigbee2mqtt", - "light/0x2222222222222222_test_light_zigbee2mqtt", "lock/0x2222222222222222_test_lock_zigbee2mqtt"); + "switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt", "sensor/0x1111111111111111_test_sensor_zigbee2mqtt", + "camera/0x1111111111111111_test_camera_zigbee2mqtt", "cover/0x2222222222222222_test_cover_zigbee2mqtt", + "fan/0x2222222222222222_test_fan_zigbee2mqtt", "light/0x2222222222222222_test_light_zigbee2mqtt", + "lock/0x2222222222222222_test_lock_zigbee2mqtt", "binary_sensor/abc/activeEnergyReports", + "number/abc/activeEnergyReports", "sensor/abc/activeEnergyReports"); private static final List MQTT_TOPICS = CONFIG_TOPICS.stream() .map(AbstractHomeAssistantTests::configTopicToMqtt).collect(Collectors.toList()); @@ -354,4 +353,171 @@ public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests { assertThat(thingHandler.getComponents().keySet().iterator().next(), is("switch")); assertThat(thingHandler.getComponents().values().iterator().next().getClass(), is(Switch.class)); } + + @Test + public void testDuplicateChannelId() { + thingHandler.initialize(); + + verify(callbackMock).statusUpdated(eq(haThing), any()); + // Expect a call to the bridge status changed, the start, the propertiesChanged method + verify(thingHandler).bridgeStatusChanged(any()); + verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any()); + + MQTT_TOPICS.forEach(t -> { + verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any()); + }); + + verify(thingHandler, never()).componentDiscovered(any(), any()); + assertThat(haThing.getChannels().size(), is(0)); + + thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """ + { + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """ + { + "command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports", + "max":32767, + "min":0, + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.delayedProcessing.forceProcessNow(); + waitForAssert(() -> { + assertThat("2 channels created", nonSpyThingHandler.getThing().getChannels().size() == 2); + }); + + Channel numberChannel = nonSpyThingHandler.getThing() + .getChannel("0x04cd15fffedb7f81_5FactiveEnergyReports_5Fzigbee2mqtt_number#number"); + assertThat("Number channel is created", numberChannel, notNullValue()); + + Channel sensorChannel = nonSpyThingHandler.getThing() + .getChannel("0x04cd15fffedb7f81_5FactiveEnergyReports_5Fzigbee2mqtt_sensor#sensor"); + assertThat("Sensor channel is created", sensorChannel, notNullValue()); + } + + @Test + public void testDuplicateChannelIdNewStyleChannels() { + haThing.setProperty("newStyleChannels", "true"); + thingHandler = new HomeAssistantThingHandler(haThing, channelTypeProvider, stateDescriptionProvider, + channelTypeRegistry, new Jinjava(), SUBSCRIBE_TIMEOUT, ATTRIBUTE_RECEIVE_TIMEOUT); + thingHandler.setConnection(bridgeConnection); + thingHandler.setCallback(callbackMock); + nonSpyThingHandler = thingHandler; + thingHandler = spy(thingHandler); + + thingHandler.initialize(); + + verify(callbackMock).statusUpdated(eq(haThing), any()); + // Expect a call to the bridge status changed, the start, the propertiesChanged method + verify(thingHandler).bridgeStatusChanged(any()); + verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any()); + + MQTT_TOPICS.forEach(t -> { + verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any()); + }); + + verify(thingHandler, never()).componentDiscovered(any(), any()); + assertThat(haThing.getChannels().size(), is(0)); + + thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """ + { + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """ + { + "command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports", + "max":32767, + "min":0, + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.delayedProcessing.forceProcessNow(); + waitForAssert(() -> { + assertThat("2 channels created", nonSpyThingHandler.getThing().getChannels().size() == 2); + }); + + Channel numberChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_number"); + assertThat("Number channel is created", numberChannel, notNullValue()); + + Channel sensorChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_sensor"); + assertThat("Sensor channel is created", sensorChannel, notNullValue()); + } + + @Test + public void testDuplicateChannelIdNewStyleChannelsComplex() { + haThing.setProperty("newStyleChannels", "true"); + thingHandler = new HomeAssistantThingHandler(haThing, channelTypeProvider, stateDescriptionProvider, + channelTypeRegistry, new Jinjava(), SUBSCRIBE_TIMEOUT, ATTRIBUTE_RECEIVE_TIMEOUT); + thingHandler.setConnection(bridgeConnection); + thingHandler.setCallback(callbackMock); + nonSpyThingHandler = thingHandler; + thingHandler = spy(thingHandler); + + thingHandler.initialize(); + + verify(callbackMock).statusUpdated(eq(haThing), any()); + // Expect a call to the bridge status changed, the start, the propertiesChanged method + verify(thingHandler).bridgeStatusChanged(any()); + verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any()); + + MQTT_TOPICS.forEach(t -> { + verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any()); + }); + + verify(thingHandler, never()).componentDiscovered(any(), any()); + assertThat(haThing.getChannels().size(), is(0)); + + thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """ + { + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}", + "json_attributes_topic":"somewhere" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """ + { + "command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports", + "max":32767, + "min":0, + "name":"ActiveEnergyReports", + "object_id":"mud_room_cans_switch_(garage)_activeEnergyReports", + "state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)", + "unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt", + "value_template":"{{ value_json.activeEnergyReports }}", + "json_attributes_topic":"somewhere" + } + """.getBytes(StandardCharsets.UTF_8)); + thingHandler.delayedProcessing.forceProcessNow(); + waitForAssert(() -> { + assertThat("4 channels created", nonSpyThingHandler.getThing().getChannels().size() == 4); + }); + + Channel numberChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_number#number"); + assertThat("Number channel is created", numberChannel, notNullValue()); + + Channel sensorChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_sensor#sensor"); + assertThat("Sensor channel is created", sensorChannel, notNullValue()); + } }