From afee5b6dc17bc4fdaa2cd0ef03e3b77430564639 Mon Sep 17 00:00:00 2001 From: Sami Salonen Date: Sat, 12 Nov 2022 13:46:43 +0200 Subject: [PATCH] [mqtt.homeassistant] Fix binding crash when home assistant discovery topics update with content (#13518) * [mqtt.homeassistant] Fix for discovery topics that update with content Fixes #13517 Possibly resolves #9711 and #12295 as well. * [mqtt.homeassistant] Sort channels before changing thing * [mqtt.homeassistant] logging + removed unnecessary synchronization * Resolve bunch of warnings in homeassistant bundle * [mqtt.homeassistant] Handling null warnings and unnecessary null checks * [mqtt.homeassistant] Removing unnecessary null checks Signed-off-by: Sami Salonen Co-Authored-by: @antroids github handle --- .../internal/component/Climate.java | 7 +- .../internal/component/Vacuum.java | 5 +- ...hannelConfigurationTypeAdapterFactory.java | 2 + .../config/ConnectionDeserializer.java | 5 +- .../discovery/HomeAssistantDiscovery.java | 15 +-- .../exception/ConfigurationException.java | 2 + .../UnsupportedComponentException.java | 2 + .../handler/HomeAssistantThingHandler.java | 48 ++++++-- .../internal/AbstractHomeAssistantTests.java | 17 ++- .../component/AbstractComponentTests.java | 29 ++--- .../component/AlarmControlPanelTests.java | 2 +- .../internal/component/ClimateTests.java | 4 +- .../internal/component/CoverTests.java | 2 +- .../internal/component/FanTests.java | 2 +- .../component/HAConfigurationTests.java | 1 + .../internal/component/LockTests.java | 2 +- .../internal/component/SensorTests.java | 2 +- .../internal/component/SwitchTests.java | 3 +- .../internal/component/VacuumTests.java | 3 +- .../HomeAssistantDiscoveryTests.java | 2 +- .../HomeAssistantThingHandlerTests.java | 108 +++++++++++++++++- 21 files changed, 203 insertions(+), 60 deletions(-) diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Climate.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Climate.java index 41531396a6122..df63de2d2a647 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Climate.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Climate.java @@ -241,9 +241,10 @@ public Climate(ComponentFactory.ComponentConfiguration componentConfiguration) { updateListener, channelConfiguration.fanModeCommandTemplate, channelConfiguration.fanModeCommandTopic, channelConfiguration.fanModeStateTemplate, channelConfiguration.fanModeStateTopic, commandFilter); - if (channelConfiguration.holdModes != null && !channelConfiguration.holdModes.isEmpty()) { - buildOptionalChannel(HOLD_CH_ID, new TextValue(channelConfiguration.holdModes.toArray(new String[0])), - updateListener, channelConfiguration.holdCommandTemplate, channelConfiguration.holdCommandTopic, + List holdModes = channelConfiguration.holdModes; + if (holdModes != null && !holdModes.isEmpty()) { + buildOptionalChannel(HOLD_CH_ID, new TextValue(holdModes.toArray(new String[0])), updateListener, + channelConfiguration.holdCommandTemplate, channelConfiguration.holdCommandTopic, channelConfiguration.holdStateTemplate, channelConfiguration.holdStateTopic, commandFilter); } diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Vacuum.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Vacuum.java index f4b526da894ed..ad9766892c57a 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Vacuum.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Vacuum.java @@ -197,9 +197,10 @@ public Vacuum(ComponentFactory.ComponentConfiguration componentConfiguration) { final var allowedSupportedFeatures = channelConfiguration.schema == Schema.LEGACY ? LEGACY_SUPPORTED_FEATURES : STATE_SUPPORTED_FEATURES; - final var configSupportedFeatures = channelConfiguration.supportedFeatures == null + final var supportedFeatures = channelConfiguration.supportedFeatures; + final var configSupportedFeatures = supportedFeatures == null ? channelConfiguration.schema == Schema.LEGACY ? LEGACY_DEFAULT_FEATURES : STATE_DEFAULT_FEATURES - : channelConfiguration.supportedFeatures; + : supportedFeatures; List deviceSupportedFeatures = Collections.emptyList(); if (!configSupportedFeatures.isEmpty()) { diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ChannelConfigurationTypeAdapterFactory.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ChannelConfigurationTypeAdapterFactory.java index b1418c2d13eb2..139fd2aa5372c 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ChannelConfigurationTypeAdapterFactory.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ChannelConfigurationTypeAdapterFactory.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.lang.reflect.Field; import java.util.Arrays; +import java.util.Objects; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -119,6 +120,7 @@ private void expandTidleInTopics(AbstractChannelConfiguration config) { String parentTopic = config.getParentTopic(); while (type != Object.class) { + Objects.requireNonNull(type, "Bug: type is null"); // Should not happen? Making compiler happy Arrays.stream(type.getDeclaredFields()).filter(this::isMqttTopicField) .forEach(field -> replacePlaceholderByParentTopic(config, field, parentTopic)); type = type.getSuperclass(); diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java index a35be7ca4221f..490de07a8a632 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java @@ -33,8 +33,9 @@ */ @NonNullByDefault public class ConnectionDeserializer implements JsonDeserializer { - public @Nullable Connection deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) - throws JsonParseException { + @Override + public @Nullable Connection deserialize(@Nullable JsonElement json, Type typeOfT, + JsonDeserializationContext context) throws JsonParseException { JsonArray list; if (json == null) { throw new JsonParseException("JSON element is null, but must be connection definition."); diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java index 3d90235cb8165..aaab4b549a58c 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java @@ -66,7 +66,6 @@ @Component(service = DiscoveryService.class, configurationPid = "discovery.mqttha") @NonNullByDefault public class HomeAssistantDiscovery extends AbstractMQTTDiscovery { - @SuppressWarnings("unused") private final Logger logger = LoggerFactory.getLogger(HomeAssistantDiscovery.class); protected final Map> componentsPerThingID = new TreeMap<>(); protected final Map thingIDPerTopic = new TreeMap<>(); @@ -260,14 +259,16 @@ public void topicVanished(ThingUID connectionBridge, MqttBrokerConnection connec } if (thingIDPerTopic.containsKey(topic)) { ThingUID thingUID = thingIDPerTopic.remove(topic); - final String thingID = thingUID.getId(); + if (thingUID != null) { + final String thingID = thingUID.getId(); - HaID haID = new HaID(topic); + HaID haID = new HaID(topic); - Set components = componentsPerThingID.getOrDefault(thingID, Collections.emptySet()); - components.remove(haID); - if (components.isEmpty()) { - thingRemoved(thingUID); + Set components = componentsPerThingID.getOrDefault(thingID, Collections.emptySet()); + components.remove(haID); + if (components.isEmpty()) { + thingRemoved(thingUID); + } } } } diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java index 111ca895c049d..5998e9e3db287 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java @@ -21,6 +21,8 @@ */ @NonNullByDefault public class ConfigurationException extends RuntimeException { + private static final long serialVersionUID = -4828651603869498942L; + public ConfigurationException(String message) { super(message); } diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java index 27341e9e7aa3a..ff4ff4ce536e1 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java @@ -21,6 +21,8 @@ */ @NonNullByDefault public class UnsupportedComponentException extends ConfigurationException { + private static final long serialVersionUID = 5134690914728956232L; + public UnsupportedComponentException(String message) { super(message); } diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java index 6e147f20a36fb..89f560ab2e65a 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java @@ -12,7 +12,8 @@ */ package org.openhab.binding.mqtt.homeassistant.internal.handler; -import java.util.Collection; +import java.util.ArrayList; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -49,6 +50,7 @@ import org.openhab.core.thing.ThingStatusDetail; import org.openhab.core.thing.ThingTypeUID; import org.openhab.core.thing.ThingUID; +import org.openhab.core.thing.binding.builder.ThingBuilder; import org.openhab.core.thing.type.ChannelDefinition; import org.openhab.core.thing.type.ChannelGroupDefinition; import org.openhab.core.thing.type.ChannelGroupType; @@ -80,6 +82,8 @@ public class HomeAssistantThingHandler extends AbstractMQTTThingHandler implements ComponentDiscovered, Consumer>> { public static final String AVAILABILITY_CHANNEL = "availability"; + private static final Comparator CHANNEL_COMPARATOR_BY_UID = Comparator + .comparing(channel -> channel.getUID().toString());; private final Logger logger = LoggerFactory.getLogger(HomeAssistantThingHandler.class); @@ -120,13 +124,12 @@ public HomeAssistantThingHandler(Thing thing, MqttChannelTypeProvider channelTyp this.transformationServiceProvider); } - @SuppressWarnings({ "null", "unused" }) @Override public void initialize() { started = false; config = getConfigAs(HandlerConfiguration.class); - if (config.topics == null || config.topics.isEmpty()) { + if (config.topics.isEmpty()) { updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Device topics unknown"); return; } @@ -222,7 +225,6 @@ protected void stop() { super.stop(); } - @SuppressWarnings({ "null", "unused" }) @Override public @Nullable ChannelState getChannelState(ChannelUID channelUID) { String groupID = channelUID.getGroupId(); @@ -255,7 +257,6 @@ public void componentDiscovered(HaID homeAssistantTopicID, AbstractComponent * Callback of {@link DelayedBatchProcessing}. * Add all newly discovered components to the Thing and start the components. */ - @SuppressWarnings("null") @Override public void accept(List> discoveredComponentsList) { MqttBrokerConnection connection = this.connection; @@ -288,13 +289,44 @@ public void accept(List> discoveredComponentsList) { return null; }); - Collection channels = discovered.getChannelMap().values().stream() + List discoveredChannels = discovered.getChannelMap().values().stream() .map(ComponentChannel::getChannel).collect(Collectors.toList()); - ThingHelper.addChannelsToThing(thing, channels); + if (known != null) { + // We had previously known component with different config hash + // We remove all conflicting old channels, they will be re-added below based on the new discovery + logger.debug( + "Received component {} with slightly different config. Making sure we re-create conflicting channels...", + discovered.getGroupUID()); + removeJustRediscoveredChannels(discoveredChannels); + } + + // Add newly discovered channels. We sort the channels + // for (mostly) consistent jsondb serialization + discoveredChannels.sort(CHANNEL_COMPARATOR_BY_UID); + ThingHelper.addChannelsToThing(thing, discoveredChannels); } + updateThingType(); + } + } + + private void removeJustRediscoveredChannels(List discoveredChannels) { + ArrayList mutableChannels = new ArrayList<>(getThing().getChannels()); + Set newChannelUIDs = discoveredChannels.stream().map(Channel::getUID).collect(Collectors.toSet()); + // Take current channels but remove those channels that were just re-discovered + List existingChannelsWithNewlyDiscoveredChannelsRemoved = mutableChannels.stream() + .filter(existingChannel -> !newChannelUIDs.contains(existingChannel.getUID())) + .collect(Collectors.toList()); + if (existingChannelsWithNewlyDiscoveredChannelsRemoved.size() < mutableChannels.size()) { + // We sort the channels for (mostly) consistent jsondb serialization + existingChannelsWithNewlyDiscoveredChannelsRemoved.sort(CHANNEL_COMPARATOR_BY_UID); + updateThingChannels(existingChannelsWithNewlyDiscoveredChannelsRemoved); } + } - updateThingType(); + private void updateThingChannels(List channelList) { + ThingBuilder thingBuilder = editThing(); + thingBuilder.withChannels(channelList); + updateThing(thingBuilder.build()); } @Override diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/AbstractHomeAssistantTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/AbstractHomeAssistantTests.java index ae9eab0f475a9..1a867b68c8f6f 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/AbstractHomeAssistantTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/AbstractHomeAssistantTests.java @@ -12,19 +12,15 @@ */ package org.openhab.binding.mqtt.homeassistant.internal; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyBoolean; -import static org.mockito.Mockito.anyInt; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; import java.io.IOException; import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -64,7 +60,6 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings({ "ConstantConditions" }) @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @NonNullByDefault @@ -87,7 +82,6 @@ public abstract class AbstractHomeAssistantTests extends JavaTest { protected @Mock @NonNullByDefault({}) ThingTypeRegistry thingTypeRegistry; protected @Mock @NonNullByDefault({}) TransformationServiceProvider transformationServiceProvider; - @SuppressWarnings("NotNullFieldNotInitialized") protected @NonNullByDefault({}) MqttChannelTypeProvider channelTypeProvider; protected final Bridge bridgeThing = BridgeBuilder.create(BRIDGE_TYPE_UID, BRIDGE_UID).build(); @@ -126,7 +120,9 @@ protected void setupConnection() { final var subscriber = (MqttMessageSubscriber) invocation.getArgument(1); subscriptions.putIfAbsent(topic, ConcurrentHashMap.newKeySet()); - subscriptions.get(topic).add(subscriber); + Set subscribers = subscriptions.get(topic); + Objects.requireNonNull(subscribers); // Invariant, thanks to putIfAbsent above. To make compiler happy + subscribers.add(subscriber); return CompletableFuture.completedFuture(true); }).when(bridgeConnection).subscribe(any(), any()); @@ -154,6 +150,7 @@ protected void setupConnection() { * @param relativePath path from src/test/java/org/openhab/binding/mqtt/homeassistant/internal * @return path */ + @SuppressWarnings("null") protected Path getResourcePath(String relativePath) { try { return Paths.get(AbstractHomeAssistantTests.class.getResource(relativePath).toURI()); diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponentTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponentTests.java index 2336b80afe3f4..5679af518c930 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponentTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponentTests.java @@ -12,19 +12,11 @@ */ package org.openhab.binding.mqtt.homeassistant.internal.component; -import static org.hamcrest.CoreMatchers.instanceOf; -import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.*; import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import java.nio.charset.StandardCharsets; import java.util.List; @@ -38,6 +30,7 @@ import org.eclipse.jdt.annotation.Nullable; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.openhab.binding.mqtt.generic.MqttChannelTypeProvider; import org.openhab.binding.mqtt.generic.TransformationServiceProvider; @@ -59,7 +52,6 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings({ "ConstantConditions" }) @NonNullByDefault public abstract class AbstractComponentTests extends AbstractHomeAssistantTests { private static final int SUBSCRIBE_TIMEOUT = 10000; @@ -178,6 +170,7 @@ protected static void assertChannel(ComponentChannel stateChannel, String stateT * @param channelId channel * @param state expected state */ + @SuppressWarnings("null") protected static void assertState(AbstractComponent<@NonNull ? extends AbstractChannelConfiguration> component, String channelId, State state) { assertThat(component.getChannel(channelId).getState().getCache().getChannelState(), is(state)); @@ -190,8 +183,8 @@ protected static void assertState(AbstractComponent<@NonNull ? extends AbstractC * @param payload payload */ protected void assertPublished(String mqttTopic, String payload) { - verify(bridgeConnection).publish(eq(mqttTopic), eq(payload.getBytes(StandardCharsets.UTF_8)), anyInt(), - anyBoolean()); + verify(bridgeConnection).publish(eq(mqttTopic), ArgumentMatchers.eq(payload.getBytes(StandardCharsets.UTF_8)), + anyInt(), anyBoolean()); } /** @@ -202,8 +195,8 @@ protected void assertPublished(String mqttTopic, String payload) { * @param t payload must be published N times on given topic */ protected void assertPublished(String mqttTopic, String payload, int t) { - verify(bridgeConnection, times(t)).publish(eq(mqttTopic), eq(payload.getBytes(StandardCharsets.UTF_8)), - anyInt(), anyBoolean()); + verify(bridgeConnection, times(t)).publish(eq(mqttTopic), + ArgumentMatchers.eq(payload.getBytes(StandardCharsets.UTF_8)), anyInt(), anyBoolean()); } /** @@ -213,8 +206,8 @@ protected void assertPublished(String mqttTopic, String payload, int t) { * @param payload payload */ protected void assertNotPublished(String mqttTopic, String payload) { - verify(bridgeConnection, never()).publish(eq(mqttTopic), eq(payload.getBytes(StandardCharsets.UTF_8)), anyInt(), - anyBoolean()); + verify(bridgeConnection, never()).publish(eq(mqttTopic), + ArgumentMatchers.eq(payload.getBytes(StandardCharsets.UTF_8)), anyInt(), anyBoolean()); } /** diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AlarmControlPanelTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AlarmControlPanelTests.java index 9e1456267f496..8118312ce9037 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AlarmControlPanelTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/AlarmControlPanelTests.java @@ -27,11 +27,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class AlarmControlPanelTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "alarm_control_panel/0x0000000000000000_alarm_control_panel_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void testAlarmControlPanel() { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/ClimateTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/ClimateTests.java index 1f39a197d8bbe..d13d542b213f8 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/ClimateTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/ClimateTests.java @@ -34,11 +34,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class ClimateTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "climate/0x847127fffe11dd6a_climate_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void testTS0601Climate() { var component = discoverComponent(configTopicToMqtt(CONFIG_TOPIC), "{" @@ -102,6 +102,7 @@ public void testTS0601Climate() { assertPublished("zigbee2mqtt/th1/set/current_heating_setpoint", "25"); } + @SuppressWarnings("null") @Test public void testTS0601ClimateNotSendIfOff() { var component = discoverComponent(configTopicToMqtt(CONFIG_TOPIC), "{" @@ -185,6 +186,7 @@ public void testTS0601ClimateNotSendIfOff() { assertPublished("zigbee2mqtt/th1/set/current_heating_setpoint", "25"); } + @SuppressWarnings("null") @Test public void testClimate() { var component = discoverComponent(configTopicToMqtt(CONFIG_TOPIC), diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/CoverTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/CoverTests.java index 0bfeee34a5405..a3230625de88d 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/CoverTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/CoverTests.java @@ -28,11 +28,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class CoverTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "cover/0x0000000000000000_cover_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void test() throws InterruptedException { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/FanTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/FanTests.java index a0390dad9ba28..bc8cc4da5f27b 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/FanTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/FanTests.java @@ -27,11 +27,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ALL") @NonNullByDefault public class FanTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "fan/0x0000000000000000_fan_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void test() throws InterruptedException { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/HAConfigurationTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/HAConfigurationTests.java index ef518cff1312c..83c60419457fd 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/HAConfigurationTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/HAConfigurationTests.java @@ -148,6 +148,7 @@ public void testDeviceSingleStringConfig() { } } + @SuppressWarnings("null") @Test public void testTS0601ClimateConfig() { String json = readTestJson("configTS0601ClimateThermostat.json"); diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/LockTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/LockTests.java index 0a8d4de2eff1e..f9920c0b843e8 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/LockTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/LockTests.java @@ -27,11 +27,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ALL") @NonNullByDefault public class LockTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "lock/0x0000000000000000_lock_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void test() throws InterruptedException { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SensorTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SensorTests.java index 8b5e412a608c7..2ddab24751ad7 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SensorTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SensorTests.java @@ -31,11 +31,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class SensorTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "sensor/0x0000000000000000_sensor_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void test() throws InterruptedException { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SwitchTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SwitchTests.java index 28738436fdff8..c832ed3d11290 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SwitchTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/SwitchTests.java @@ -27,11 +27,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class SwitchTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "switch/0x847127fffe11dd6a_auto_lock_zigbee2mqtt"; + @SuppressWarnings("null") @Test public void testSwitchWithStateAndCommand() { var component = discoverComponent(configTopicToMqtt(CONFIG_TOPIC), @@ -92,6 +92,7 @@ public void testSwitchWithState() { assertState(component, Switch.SWITCH_CHANNEL_ID, OnOffType.ON); } + @SuppressWarnings("null") @Test public void testSwitchWithCommand() { var component = discoverComponent(configTopicToMqtt(CONFIG_TOPIC), diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/VacuumTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/VacuumTests.java index 2b0a0f5f20231..df831459c34a3 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/VacuumTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/component/VacuumTests.java @@ -32,11 +32,11 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings("ConstantConditions") @NonNullByDefault public class VacuumTests extends AbstractComponentTests { public static final String CONFIG_TOPIC = "vacuum/rockrobo_vacuum"; + @SuppressWarnings("null") @Test public void testRoborockValetudo() { // @formatter:off @@ -157,6 +157,7 @@ public void testRoborockValetudo() { assertState(component, Vacuum.JSON_ATTRIBUTES_CH_ID, new StringType(jsonValue)); } + @SuppressWarnings("null") @Test public void testLegacySchema() { // @formatter:off diff --git a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscoveryTests.java b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscoveryTests.java index 3ac574196c82e..ea46a42fc55f3 100644 --- a/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscoveryTests.java +++ b/bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscoveryTests.java @@ -44,7 +44,7 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings({ "ConstantConditions", "unchecked" }) +@SuppressWarnings({ "unchecked" }) @ExtendWith(MockitoExtension.class) @NonNullByDefault public class HomeAssistantDiscoveryTests extends AbstractHomeAssistantTests { 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 4b851692607d8..060540d325072 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 @@ -19,6 +19,7 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; @@ -32,7 +33,9 @@ import org.openhab.binding.mqtt.homeassistant.internal.HaID; import org.openhab.binding.mqtt.homeassistant.internal.HandlerConfiguration; import org.openhab.binding.mqtt.homeassistant.internal.component.Climate; +import org.openhab.binding.mqtt.homeassistant.internal.component.Sensor; import org.openhab.binding.mqtt.homeassistant.internal.component.Switch; +import org.openhab.core.thing.Channel; import org.openhab.core.thing.binding.ThingHandlerCallback; /** @@ -40,7 +43,6 @@ * * @author Anton Kharuzhy - Initial contribution */ -@SuppressWarnings({ "ConstantConditions" }) @ExtendWith(MockitoExtension.class) @NonNullByDefault public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests { @@ -60,6 +62,7 @@ public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests { private @Mock @NonNullByDefault({}) ThingHandlerCallback callbackMock; private @NonNullByDefault({}) HomeAssistantThingHandler thingHandler; + private @NonNullByDefault({}) HomeAssistantThingHandler nonSpyThingHandler; @BeforeEach public void setup() { @@ -74,6 +77,7 @@ public void setup() { SUBSCRIBE_TIMEOUT, ATTRIBUTE_RECEIVE_TIMEOUT); thingHandler.setConnection(bridgeConnection); thingHandler.setCallback(callbackMock); + nonSpyThingHandler = thingHandler; thingHandler = spy(thingHandler); } @@ -117,6 +121,108 @@ public void testInitialize() { verify(channelTypeProvider, times(2)).setChannelGroupType(any(), any()); } + /** + * Test where the same component is published twice to MQTT. The binding should handle this. + * + * @throws InterruptedException + */ + @Test + public void testDuplicateComponentPublish() throws InterruptedException { + 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()); + + // Expect subscription on each topic from config + MQTT_TOPICS.forEach(t -> { + verify(bridgeConnection, timeout(SUBSCRIBE_TIMEOUT)).subscribe(eq(t), any()); + }); + + verify(thingHandler, never()).componentDiscovered(any(), any()); + assertThat(haThing.getChannels().size(), CoreMatchers.is(0)); + + // + // + // Publish sensor components with identical payload except for + // change in "name" field. The binding should respect the latest discovery result. + // + // This simulates how multiple OpenMQTTGateway devices would publish + // the same discovery topics for a particular Bluetooth sensor, and thus "competing" with similar but slightly + // different discovery topics. + // + // In fact, only difference is actually "via_device" additional metadata field telling which OpenMQTTGateway + // published the discovery topic. + // + // + + // + // 1. publish corridor temperature sensor + // + var configTopicTempCorridor = "homeassistant/sensor/tempCorridor/config"; + thingHandler.discoverComponents.processMessage(configTopicTempCorridor, new String("{"// + + "\"temperature_state_topic\": \"+/+/BTtoMQTT/mysensor\","// + + "\"temperature_state_template\": \"{{ value_json.temperature }}\", "// + + "\"name\": \"CorridorTemp\", "// + + "\"unit_of_measurement\": \"°C\" "// + + "}").getBytes(StandardCharsets.UTF_8)); + verify(thingHandler, times(1)).componentDiscovered(eq(new HaID(configTopicTempCorridor)), any(Sensor.class)); + thingHandler.delayedProcessing.forceProcessNow(); + waitForAssert(() -> { + assertThat("1 channel created", thingHandler.getThing().getChannels().size() == 1); + }); + + // + // 2. publish outside temperature sensor + // + var configTopicTempOutside = "homeassistant/sensor/tempOutside/config"; + thingHandler.discoverComponents.processMessage(configTopicTempOutside, new String("{"// + + "\"temperature_state_topic\": \"+/+/BTtoMQTT/mysensor\","// + + "\"temperature_state_template\": \"{{ value_json.temperature }}\", " // + + "\"name\": \"OutsideTemp\", "// + + "\"source\": \"gateway2\" "// + + "}").getBytes(StandardCharsets.UTF_8)); + thingHandler.delayedProcessing.forceProcessNow(); + verify(thingHandler, times(1)).componentDiscovered(eq(new HaID(configTopicTempOutside)), any(Sensor.class)); + waitForAssert(() -> { + assertThat("2 channel created", thingHandler.getThing().getChannels().size() == 2); + }); + + // + // 3. publish corridor temperature sensor, this time with different name (openHAB channel label) + // + thingHandler.discoverComponents.processMessage(configTopicTempCorridor, new String("{"// + + "\"temperature_state_topic\": \"+/+/BTtoMQTT/mysensor\","// + + "\"temperature_state_template\": \"{{ value_json.temperature }}\", "// + + "\"name\": \"CorridorTemp NEW\", "// + + "\"unit_of_measurement\": \"°C\" "// + + "}").getBytes(StandardCharsets.UTF_8)); + thingHandler.delayedProcessing.forceProcessNow(); + + waitForAssert(() -> { + assertThat("2 channel created", thingHandler.getThing().getChannels().size() == 2); + }); + + // + // verify that both channels are there and the label corresponds to newer discovery topic payload + // + Channel corridorTempChannel = nonSpyThingHandler.getThing().getChannel("tempCorridor_5Fsensor#sensor"); + assertThat("Corridor temperature channel is created", corridorTempChannel, CoreMatchers.notNullValue()); + Objects.requireNonNull(corridorTempChannel); // for compiler + assertThat("Corridor temperature channel is having the updated label from 2nd discovery topic publish", + corridorTempChannel.getLabel(), CoreMatchers.is("CorridorTemp NEW")); + + Channel outsideTempChannel = nonSpyThingHandler.getThing().getChannel("tempOutside_5Fsensor#sensor"); + assertThat("Outside temperature channel is created", outsideTempChannel, CoreMatchers.notNullValue()); + + verify(thingHandler, times(2)).componentDiscovered(eq(new HaID(configTopicTempCorridor)), any(Sensor.class)); + + waitForAssert(() -> { + assertThat("2 channel created", thingHandler.getThing().getChannels().size() == 2); + }); + } + @Test public void testDispose() { thingHandler.initialize();