From 546bb566e2236cade81e2f4b39ceca1a2f7643ec Mon Sep 17 00:00:00 2001
From: Cody Cutrer <cody@cutrer.us>
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 <cody@cutrer.us>
---
.../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<C extends AbstractChannelConfiguration>
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<C extends AbstractChannelConfiguration>
}
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<String> 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<String> 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());
+ }
}