[MQTT.Homeassistant] process errors in MQTT message handlers during components discovery (#11315)

Signed-off-by: Anton Kharuzhy <publicantroids@gmail.com>

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/DiscoverComponents.java

@@ -29,6 +29,8 @@ import org.openhab.binding.mqtt.generic.TransformationServiceProvider;
 import org.openhab.binding.mqtt.generic.utils.FutureCollector;
 import org.openhab.binding.mqtt.homeassistant.internal.component.AbstractComponent;
 import org.openhab.binding.mqtt.homeassistant.internal.component.ComponentFactory;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.UnsupportedComponentException;
 import org.openhab.core.io.transport.mqtt.MqttBrokerConnection;
 import org.openhab.core.io.transport.mqtt.MqttMessageSubscriber;
 import org.openhab.core.thing.ThingUID;
@@ -97,18 +99,27 @@ public class DiscoverComponents implements MqttMessageSubscriber {
         AbstractComponent<?> component = null;
 
         if (config.length() > 0) {
+            try {
                 component = ComponentFactory.createComponent(thingUID, haID, config, updateListener, tracker, scheduler,
                         gson, transformationServiceProvider);
-        }
-        if (component != null) {
                 component.setConfigSeen();
 
                 logger.trace("Found HomeAssistant thing {} component {}", haID.objectID, haID.component);
+
                 if (discoveredListener != null) {
                     discoveredListener.componentDiscovered(haID, component);
                 }
+            } catch (UnsupportedComponentException e) {
+                logger.warn("HomeAssistant discover error: thing {} component type is unsupported: {}", haID.objectID,
+                        haID.component);
+            } catch (ConfigurationException e) {
+                logger.warn("HomeAssistant discover error: invalid configuration of thing {} component {}: {}",
+                        haID.objectID, haID.component, e.getMessage());
+            } catch (Exception e) {
+                logger.warn("HomeAssistant discover error: {}", e.getMessage());
+            }
         } else {
-            logger.debug("Configuration of HomeAssistant thing {} invalid: {}", haID.objectID, config);
+            logger.warn("Configuration of HomeAssistant thing {} is empty", haID.objectID);
         }
     }
 

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

@@ -21,6 +21,8 @@ import org.openhab.binding.mqtt.generic.ChannelStateUpdateListener;
 import org.openhab.binding.mqtt.generic.TransformationServiceProvider;
 import org.openhab.binding.mqtt.homeassistant.internal.HaID;
 import org.openhab.binding.mqtt.homeassistant.internal.config.dto.AbstractChannelConfiguration;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.UnsupportedComponentException;
 import org.openhab.core.thing.ThingUID;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -48,14 +50,12 @@ public class ComponentFactory {
      * @param updateListener A channel state update listener
      * @return A HA MQTT Component
      */
-    public static @Nullable AbstractComponent<?> createComponent(ThingUID thingUID, HaID haID,
+    public static AbstractComponent<?> createComponent(ThingUID thingUID, HaID haID, String channelConfigurationJSON,
-            String channelConfigurationJSON, ChannelStateUpdateListener updateListener, AvailabilityTracker tracker,
+            ChannelStateUpdateListener updateListener, AvailabilityTracker tracker, ScheduledExecutorService scheduler,
-            ScheduledExecutorService scheduler, Gson gson,
+            Gson gson, TransformationServiceProvider transformationServiceProvider) throws ConfigurationException {
-            TransformationServiceProvider transformationServiceProvider) {
         ComponentConfiguration componentConfiguration = new ComponentConfiguration(thingUID, haID,
                 channelConfigurationJSON, gson, updateListener, tracker, scheduler)
                         .transformationProvider(transformationServiceProvider);
-        try {
         switch (haID.component) {
             case "alarm_control_panel":
                 return new AlarmControlPanel(componentConfiguration);
@@ -77,11 +77,9 @@ public class ComponentFactory {
                 return new Sensor(componentConfiguration);
             case "switch":
                 return new Switch(componentConfiguration);
+            default:
+                throw new UnsupportedComponentException("Component '" + haID + "' is unsupported!");
         }
-        } catch (UnsupportedOperationException e) {
-            LOGGER.warn("Not supported", e);
-        }
-        return null;
     }
 
     protected static class ComponentConfiguration {

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

@@ -16,6 +16,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.mqtt.generic.values.OnOffValue;
 import org.openhab.binding.mqtt.homeassistant.internal.config.dto.AbstractChannelConfiguration;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
 
 import com.google.gson.annotations.SerializedName;
 
@@ -53,7 +54,7 @@ public class Lock extends AbstractComponent<Lock.ChannelConfiguration> {
 
         // We do not support all HomeAssistant quirks
         if (channelConfiguration.optimistic && !channelConfiguration.stateTopic.isBlank()) {
-            throw new UnsupportedOperationException("Component:Lock does not support forced optimistic mode");
+            throw new ConfigurationException("Component:Lock does not support forced optimistic mode");
         }
 
         buildChannel(SWITCH_CHANNEL_ID,

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

@@ -16,6 +16,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.mqtt.generic.values.OnOffValue;
 import org.openhab.binding.mqtt.homeassistant.internal.config.dto.AbstractChannelConfiguration;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
 
 import com.google.gson.annotations.SerializedName;
 
@@ -65,7 +66,7 @@ public class Switch extends AbstractComponent<Switch.ChannelConfiguration> {
                 : channelConfiguration.stateTopic.isBlank();
 
         if (optimistic && !channelConfiguration.stateTopic.isBlank()) {
-            throw new UnsupportedOperationException("Component:Switch does not support forced optimistic mode");
+            throw new ConfigurationException("Component:Switch does not support forced optimistic mode");
         }
 
         String stateOn = channelConfiguration.stateOn != null ? channelConfiguration.stateOn

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java

@@ -37,16 +37,18 @@ public class ConnectionDeserializer implements JsonDeserializer<Connection> {
             throws JsonParseException {
         JsonArray list;
         if (json == null) {
-            throw new JsonParseException("JSON element is null");
+            throw new JsonParseException("JSON element is null, but must be connection definition.");
         }
         try {
             list = json.getAsJsonArray();
         } catch (IllegalStateException e) {
-            throw new JsonParseException("Cannot parse JSON array", e);
+            throw new JsonParseException("Cannot parse JSON array. Each connection must be defined as array with two "
+                    + "elements: connection_type, connection identifier. For example: \"connections\": [[\"mac\", "
+                    + "\"02:5b:26:a8:dc:12\"]]", e);
         }
         if (list.size() != 2) {
-            throw new JsonParseException(
+            throw new JsonParseException("Connection information must be a tuple, but has " + list.size()
-                    "Connection information must be a tuple, but has " + list.size() + " elements!");
+                    + " elements! For example: " + "\"connections\": [[\"mac\", \"02:5b:26:a8:dc:12\"]]");
         }
         return new Connection(list.get(0).getAsString(), list.get(1).getAsString());
     }

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/config/dto/AbstractChannelConfiguration.java

@@ -14,14 +14,15 @@ package org.openhab.binding.mqtt.homeassistant.internal.config.dto;
 
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.util.UIDUtils;
 
 import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
 import com.google.gson.annotations.SerializedName;
 
 /**
@@ -199,6 +200,15 @@ public abstract class AbstractChannelConfiguration {
      */
     public static <C extends AbstractChannelConfiguration> C fromString(final String configJSON, final Gson gson,
             final Class<C> clazz) {
-        return Objects.requireNonNull(gson.fromJson(configJSON, clazz));
+        try {
+            @Nullable
+            final C config = gson.fromJson(configJSON, clazz);
+            if (config == null) {
+                throw new ConfigurationException("Channel configuration is empty");
+            }
+            return config;
+        } catch (JsonSyntaxException e) {
+            throw new ConfigurationException("Cannot parse channel configuration JSON", e);
+        }
     }
 }

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java

@@ -36,6 +36,7 @@ import org.openhab.binding.mqtt.homeassistant.internal.HaID;
 import org.openhab.binding.mqtt.homeassistant.internal.HandlerConfiguration;
 import org.openhab.binding.mqtt.homeassistant.internal.config.ChannelConfigurationTypeAdapterFactory;
 import org.openhab.binding.mqtt.homeassistant.internal.config.dto.AbstractChannelConfiguration;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
 import org.openhab.core.config.discovery.DiscoveryResult;
 import org.openhab.core.config.discovery.DiscoveryResultBuilder;
 import org.openhab.core.config.discovery.DiscoveryService;
@@ -146,14 +147,15 @@ public class HomeAssistantDiscovery extends AbstractMQTTDiscovery {
         }
         this.future = scheduler.schedule(this::publishResults, 2, TimeUnit.SECONDS);
 
-        AbstractChannelConfiguration config = AbstractChannelConfiguration
-                .fromString(new String(payload, StandardCharsets.UTF_8), gson);
-
         // We will of course find multiple of the same unique Thing IDs, for each different component another one.
         // Therefore the components are assembled into a list and given to the DiscoveryResult label for the user to
         // easily recognize object capabilities.
-
         HaID haID = new HaID(topic);
+
+        try {
+            AbstractChannelConfiguration config = AbstractChannelConfiguration
+                    .fromString(new String(payload, StandardCharsets.UTF_8), gson);
+
             final String thingID = config.getThingId(haID.objectID);
 
             final ThingTypeUID typeID = new ThingTypeUID(MqttBindingConstants.BINDING_ID,
@@ -183,6 +185,12 @@ public class HomeAssistantDiscovery extends AbstractMQTTDiscovery {
                     DiscoveryResultBuilder.create(thingUID).withProperties(properties)
                             .withRepresentationProperty("deviceId").withBridge(connectionBridge)
                             .withLabel(config.getThingName() + " (" + componentNames + ")").build());
+        } catch (ConfigurationException e) {
+            logger.warn("HomeAssistant discover error: invalid configuration of thing {} component {}: {}",
+                    haID.objectID, haID.component, e.getMessage());
+        } catch (Exception e) {
+            logger.warn("HomeAssistant discover error: {}", e.getMessage());
+        }
     }
 
     protected void publishResults() {

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java

@@ -0,0 +1,28 @@
+/**
+ * Copyright (c) 2010-2021 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.mqtt.homeassistant.internal.exception;
+
+/**
+ * Exception class for errors in HomeAssistant components configurations
+ *
+ * @author Anton Kharuzhy - Initial contribution
+ */
+public class ConfigurationException extends RuntimeException {
+    public ConfigurationException(String message) {
+        super(message);
+    }
+
+    public ConfigurationException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java

@@ -0,0 +1,28 @@
+/**
+ * Copyright (c) 2010-2021 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.mqtt.homeassistant.internal.exception;
+
+/**
+ * Exception class for unsupported components
+ *
+ * @author Anton Kharuzhy - Initial contribution
+ */
+public class UnsupportedComponentException extends ConfigurationException {
+    public UnsupportedComponentException(String message) {
+        super(message);
+    }
+
+    public UnsupportedComponentException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java

@@ -40,6 +40,7 @@ import org.openhab.binding.mqtt.homeassistant.internal.HandlerConfiguration;
 import org.openhab.binding.mqtt.homeassistant.internal.component.AbstractComponent;
 import org.openhab.binding.mqtt.homeassistant.internal.component.ComponentFactory;
 import org.openhab.binding.mqtt.homeassistant.internal.config.ChannelConfigurationTypeAdapterFactory;
+import org.openhab.binding.mqtt.homeassistant.internal.exception.ConfigurationException;
 import org.openhab.core.io.transport.mqtt.MqttBrokerConnection;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
@@ -153,15 +154,14 @@ public class HomeAssistantThingHandler extends AbstractMQTTThingHandler
             if (channelConfigurationJSON == null) {
                 logger.warn("Provided channel does not have a 'config' configuration key!");
             } else {
+                try {
                     component = ComponentFactory.createComponent(thingUID, haID, channelConfigurationJSON, this, this,
                             scheduler, gson, transformationServiceProvider);
-            }
-
-            if (component != null) {
                     haComponents.put(component.getGroupUID().getId(), component);
                     component.addChannelTypes(channelTypeProvider);
-            } else {
+                } catch (ConfigurationException e) {
-                logger.warn("Could not restore component {}", thing);
+                    logger.error("Cannot not restore component {}: {}", thing, e.getMessage());
+                }
             }
         }
         updateThingType();

bundles/org.openhab.binding.mqtt.homeassistant/src/test/java/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandlerTests.java

@@ -22,6 +22,7 @@ import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -152,4 +153,34 @@ public class HomeAssistantThingHandlerTests extends AbstractHomeAssistantTests {
         // Expect channel group types removed, 1 for each component
         verify(channelTypeProvider, times(2)).removeChannelGroupType(any());
     }
+
+    @Test
+    public void testProcessMessageFromUnsupportedComponent() {
+        thingHandler.initialize();
+        thingHandler.discoverComponents.processMessage("homeassistant/unsupportedType/id_zigbee2mqtt/config",
+                "{}".getBytes(StandardCharsets.UTF_8));
+        // Ignore unsupported component
+        thingHandler.delayedProcessing.forceProcessNow();
+        assertThat(haThing.getChannels().size(), CoreMatchers.is(0));
+    }
+
+    @Test
+    public void testProcessMessageWithEmptyConfig() {
+        thingHandler.initialize();
+        thingHandler.discoverComponents.processMessage("homeassistant/sensor/id_zigbee2mqtt/config",
+                "".getBytes(StandardCharsets.UTF_8));
+        // Ignore component with empty config
+        thingHandler.delayedProcessing.forceProcessNow();
+        assertThat(haThing.getChannels().size(), CoreMatchers.is(0));
+    }
+
+    @Test
+    public void testProcessMessageWithBadFormatConfig() {
+        thingHandler.initialize();
+        thingHandler.discoverComponents.processMessage("homeassistant/sensor/id_zigbee2mqtt/config",
+                "{bad format}}".getBytes(StandardCharsets.UTF_8));
+        // Ignore component with bad format config
+        thingHandler.delayedProcessing.forceProcessNow();
+        assertThat(haThing.getChannels().size(), CoreMatchers.is(0));
+    }
 }