[mqtt.homeassistant] Fix binding crash when home assistant discovery …

…topics update with content (openhab#13518)

* [mqtt.homeassistant] Fix for discovery topics that update with content

Fixes openhab#13517
Possibly resolves openhab#9711 and openhab#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 <ssalonen@gmail.com>
Co-Authored-by: @antroids github handle

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<String> 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);
         }
 

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<String> deviceSupportedFeatures = Collections.emptyList();
 
         if (!configSupportedFeatures.isEmpty()) {

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();

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<Connection> {
-    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.");

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<String, Set<HaID>> componentsPerThingID = new TreeMap<>();
     protected final Map<String, ThingUID> 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<HaID> components = componentsPerThingID.getOrDefault(thingID, Collections.emptySet());
-            components.remove(haID);
-            if (components.isEmpty()) {
-                thingRemoved(thingUID);
+                Set<HaID> components = componentsPerThingID.getOrDefault(thingID, Collections.emptySet());
+                components.remove(haID);
+                if (components.isEmpty()) {
+                    thingRemoved(thingUID);
+                }
             }
         }
     }

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);
     }

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);
     }

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<List<AbstractComponent<?>>> {
     public static final String AVAILABILITY_CHANNEL = "availability";
+    private static final Comparator<Channel> 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<AbstractComponent<?>> discoveredComponentsList) {
         MqttBrokerConnection connection = this.connection;
@@ -288,13 +289,44 @@ public void accept(List<AbstractComponent<?>> discoveredComponentsList) {
                     return null;
                 });
 
-                Collection<Channel> channels = discovered.getChannelMap().values().stream()
+                List<Channel> 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<Channel> discoveredChannels) {
+        ArrayList<Channel> mutableChannels = new ArrayList<>(getThing().getChannels());
+        Set<ChannelUID> newChannelUIDs = discoveredChannels.stream().map(Channel::getUID).collect(Collectors.toSet());
+        // Take current channels but remove those channels that were just re-discovered
+        List<Channel> 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<Channel> channelList) {
+        ThingBuilder thingBuilder = editThing();
+        thingBuilder.withChannels(channelList);
+        updateThing(thingBuilder.build());
     }
 
     @Override

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<MqttMessageSubscriber> 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());

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());
     }
 
     /**

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

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),