Skip to content

Commit 546bb56

Browse files
ccutrerccutrer
authored
[mqtt.homeassistant] Fix duplicate component resolution when unique_id is set (openhab#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 <[email protected]>
1 parent d966d1e commit 546bb56

File tree

2 files changed

+180
-9
lines changed

2 files changed

+180
-9
lines changed

‎bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

+8-3
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public AbstractComponent(ComponentFactory.ComponentConfiguration componentConfig
118118
groupId = null;
119119
componentId = "";
120120
}
121-
uniqueId = this.haID.getGroupId(channelConfiguration.getUniqueId(), false);
121+
uniqueId = haID.component + "_" + haID.getGroupId(channelConfiguration.getUniqueId(), false);
122122

123123
this.configSeen = false;
124124

@@ -182,8 +182,13 @@ protected void finalizeChannels() {
182182
}
183183

184184
public void resolveConflict() {
185-
componentId = this.haID.getGroupId(channelConfiguration.getUniqueId(), newStyleChannels);
186-
channels.values().forEach(c -> c.resetUID(buildChannelUID(c.getChannel().getUID().getIdWithoutGroup())));
185+
if (newStyleChannels && channels.size() == 1) {
186+
componentId = componentId + "_" + haID.component;
187+
channels.values().forEach(c -> c.resetUID(buildChannelUID(componentId)));
188+
} else {
189+
groupId = componentId = componentId + "_" + haID.component;
190+
channels.values().forEach(c -> c.resetUID(buildChannelUID(c.getChannel().getUID().getIdWithoutGroup())));
191+
}
187192
}
188193

189194
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

+172-6
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,11 @@ public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests {
6060
private static final int ATTRIBUTE_RECEIVE_TIMEOUT = 2000;
6161

6262
private static final List<String> CONFIG_TOPICS = Arrays.asList("climate/0x847127fffe11dd6a_climate_zigbee2mqtt",
63-
"switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt",
64-
65-
"sensor/0x1111111111111111_test_sensor_zigbee2mqtt", "camera/0x1111111111111111_test_camera_zigbee2mqtt",
66-
67-
"cover/0x2222222222222222_test_cover_zigbee2mqtt", "fan/0x2222222222222222_test_fan_zigbee2mqtt",
68-
"light/0x2222222222222222_test_light_zigbee2mqtt", "lock/0x2222222222222222_test_lock_zigbee2mqtt");
63+
"switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt", "sensor/0x1111111111111111_test_sensor_zigbee2mqtt",
64+
"camera/0x1111111111111111_test_camera_zigbee2mqtt", "cover/0x2222222222222222_test_cover_zigbee2mqtt",
65+
"fan/0x2222222222222222_test_fan_zigbee2mqtt", "light/0x2222222222222222_test_light_zigbee2mqtt",
66+
"lock/0x2222222222222222_test_lock_zigbee2mqtt", "binary_sensor/abc/activeEnergyReports",
67+
"number/abc/activeEnergyReports", "sensor/abc/activeEnergyReports");
6968

7069
private static final List<String> MQTT_TOPICS = CONFIG_TOPICS.stream()
7170
.map(AbstractHomeAssistantTests::configTopicToMqtt).collect(Collectors.toList());
@@ -354,4 +353,171 @@ public void testRestoreComponentFromChannelConfig() {
354353
assertThat(thingHandler.getComponents().keySet().iterator().next(), is("switch"));
355354
assertThat(thingHandler.getComponents().values().iterator().next().getClass(), is(Switch.class));
356355
}
356+
357+
@Test
358+
public void testDuplicateChannelId() {
359+
thingHandler.initialize();
360+
361+
verify(callbackMock).statusUpdated(eq(haThing), any());
362+
// Expect a call to the bridge status changed, the start, the propertiesChanged method
363+
verify(thingHandler).bridgeStatusChanged(any());
364+
verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any());
365+
366+
MQTT_TOPICS.forEach(t -> {
367+
verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any());
368+
});
369+
370+
verify(thingHandler, never()).componentDiscovered(any(), any());
371+
assertThat(haThing.getChannels().size(), is(0));
372+
373+
thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """
374+
{
375+
"name":"ActiveEnergyReports",
376+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
377+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
378+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
379+
"value_template":"{{ value_json.activeEnergyReports }}"
380+
}
381+
""".getBytes(StandardCharsets.UTF_8));
382+
thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """
383+
{
384+
"command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports",
385+
"max":32767,
386+
"min":0,
387+
"name":"ActiveEnergyReports",
388+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
389+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
390+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
391+
"value_template":"{{ value_json.activeEnergyReports }}"
392+
}
393+
""".getBytes(StandardCharsets.UTF_8));
394+
thingHandler.delayedProcessing.forceProcessNow();
395+
waitForAssert(() -> {
396+
assertThat("2 channels created", nonSpyThingHandler.getThing().getChannels().size() == 2);
397+
});
398+
399+
Channel numberChannel = nonSpyThingHandler.getThing()
400+
.getChannel("0x04cd15fffedb7f81_5FactiveEnergyReports_5Fzigbee2mqtt_number#number");
401+
assertThat("Number channel is created", numberChannel, notNullValue());
402+
403+
Channel sensorChannel = nonSpyThingHandler.getThing()
404+
.getChannel("0x04cd15fffedb7f81_5FactiveEnergyReports_5Fzigbee2mqtt_sensor#sensor");
405+
assertThat("Sensor channel is created", sensorChannel, notNullValue());
406+
}
407+
408+
@Test
409+
public void testDuplicateChannelIdNewStyleChannels() {
410+
haThing.setProperty("newStyleChannels", "true");
411+
thingHandler = new HomeAssistantThingHandler(haThing, channelTypeProvider, stateDescriptionProvider,
412+
channelTypeRegistry, new Jinjava(), SUBSCRIBE_TIMEOUT, ATTRIBUTE_RECEIVE_TIMEOUT);
413+
thingHandler.setConnection(bridgeConnection);
414+
thingHandler.setCallback(callbackMock);
415+
nonSpyThingHandler = thingHandler;
416+
thingHandler = spy(thingHandler);
417+
418+
thingHandler.initialize();
419+
420+
verify(callbackMock).statusUpdated(eq(haThing), any());
421+
// Expect a call to the bridge status changed, the start, the propertiesChanged method
422+
verify(thingHandler).bridgeStatusChanged(any());
423+
verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any());
424+
425+
MQTT_TOPICS.forEach(t -> {
426+
verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any());
427+
});
428+
429+
verify(thingHandler, never()).componentDiscovered(any(), any());
430+
assertThat(haThing.getChannels().size(), is(0));
431+
432+
thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """
433+
{
434+
"name":"ActiveEnergyReports",
435+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
436+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
437+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
438+
"value_template":"{{ value_json.activeEnergyReports }}"
439+
}
440+
""".getBytes(StandardCharsets.UTF_8));
441+
thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """
442+
{
443+
"command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports",
444+
"max":32767,
445+
"min":0,
446+
"name":"ActiveEnergyReports",
447+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
448+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
449+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
450+
"value_template":"{{ value_json.activeEnergyReports }}"
451+
}
452+
""".getBytes(StandardCharsets.UTF_8));
453+
thingHandler.delayedProcessing.forceProcessNow();
454+
waitForAssert(() -> {
455+
assertThat("2 channels created", nonSpyThingHandler.getThing().getChannels().size() == 2);
456+
});
457+
458+
Channel numberChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_number");
459+
assertThat("Number channel is created", numberChannel, notNullValue());
460+
461+
Channel sensorChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_sensor");
462+
assertThat("Sensor channel is created", sensorChannel, notNullValue());
463+
}
464+
465+
@Test
466+
public void testDuplicateChannelIdNewStyleChannelsComplex() {
467+
haThing.setProperty("newStyleChannels", "true");
468+
thingHandler = new HomeAssistantThingHandler(haThing, channelTypeProvider, stateDescriptionProvider,
469+
channelTypeRegistry, new Jinjava(), SUBSCRIBE_TIMEOUT, ATTRIBUTE_RECEIVE_TIMEOUT);
470+
thingHandler.setConnection(bridgeConnection);
471+
thingHandler.setCallback(callbackMock);
472+
nonSpyThingHandler = thingHandler;
473+
thingHandler = spy(thingHandler);
474+
475+
thingHandler.initialize();
476+
477+
verify(callbackMock).statusUpdated(eq(haThing), any());
478+
// Expect a call to the bridge status changed, the start, the propertiesChanged method
479+
verify(thingHandler).bridgeStatusChanged(any());
480+
verify(thingHandler, timeout(SUBSCRIBE_TIMEOUT)).start(any());
481+
482+
MQTT_TOPICS.forEach(t -> {
483+
verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any());
484+
});
485+
486+
verify(thingHandler, never()).componentDiscovered(any(), any());
487+
assertThat(haThing.getChannels().size(), is(0));
488+
489+
thingHandler.discoverComponents.processMessage("homeassistant/number/abc/activeEnergyReports/config", """
490+
{
491+
"name":"ActiveEnergyReports",
492+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
493+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
494+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
495+
"value_template":"{{ value_json.activeEnergyReports }}",
496+
"json_attributes_topic":"somewhere"
497+
}
498+
""".getBytes(StandardCharsets.UTF_8));
499+
thingHandler.discoverComponents.processMessage("homeassistant/sensor/abc/activeEnergyReports/config", """
500+
{
501+
"command_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)/set/activeEnergyReports",
502+
"max":32767,
503+
"min":0,
504+
"name":"ActiveEnergyReports",
505+
"object_id":"mud_room_cans_switch_(garage)_activeEnergyReports",
506+
"state_topic":"zigbee2mqtt/Mud Room Cans Switch (Garage)",
507+
"unique_id":"0x04cd15fffedb7f81_activeEnergyReports_zigbee2mqtt",
508+
"value_template":"{{ value_json.activeEnergyReports }}",
509+
"json_attributes_topic":"somewhere"
510+
}
511+
""".getBytes(StandardCharsets.UTF_8));
512+
thingHandler.delayedProcessing.forceProcessNow();
513+
waitForAssert(() -> {
514+
assertThat("4 channels created", nonSpyThingHandler.getThing().getChannels().size() == 4);
515+
});
516+
517+
Channel numberChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_number#number");
518+
assertThat("Number channel is created", numberChannel, notNullValue());
519+
520+
Channel sensorChannel = nonSpyThingHandler.getThing().getChannel("activeEnergyReports_sensor#sensor");
521+
assertThat("Sensor channel is created", sensorChannel, notNullValue());
522+
}
357523
}

0 commit comments

Comments
 (0)