[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>

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);
+        if (newStyleChannels && channels.size() == 1) {
-        channels.values().forEach(c -> c.resetUID(buildChannelUID(c.getChannel().getUID().getIdWithoutGroup())));
+            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,

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",
+            "switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt", "sensor/0x1111111111111111_test_sensor_zigbee2mqtt",
-
+            "camera/0x1111111111111111_test_camera_zigbee2mqtt", "cover/0x2222222222222222_test_cover_zigbee2mqtt",
-            "sensor/0x1111111111111111_test_sensor_zigbee2mqtt", "camera/0x1111111111111111_test_camera_zigbee2mqtt",
+            "fan/0x2222222222222222_test_fan_zigbee2mqtt", "light/0x2222222222222222_test_light_zigbee2mqtt",
-
+            "lock/0x2222222222222222_test_lock_zigbee2mqtt", "binary_sensor/abc/activeEnergyReports",
-            "cover/0x2222222222222222_test_cover_zigbee2mqtt", "fan/0x2222222222222222_test_fan_zigbee2mqtt",
+            "number/abc/activeEnergyReports", "sensor/abc/activeEnergyReports");
-            "light/0x2222222222222222_test_light_zigbee2mqtt", "lock/0x2222222222222222_test_lock_zigbee2mqtt");
 
     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());
+    }
 }