[MQTT.Homeassistant] process errors in MQTT message handlers during c…

…omponents discovery (openhab#11315) Signed-off-by: Anton Kharuzhy <publicantroids@gmail.com> Signed-off-by: Nick Waterton <n.waterton@outlook.com>
NickWaterton · Dec 30, 2021 · aa57504 · aa57504
1 parent 7531a06
commit aa57504
Show file tree

Hide file tree

Showing 11 changed files with 200 additions and 82 deletions.
diff --git a/...ant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/DiscoverComponents.java b/...ant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/DiscoverComponents.java
@@ -29,6 +29,8 @@
 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 void processMessage(String topic, byte[] payload) {
         AbstractComponent<?> component = null;
 
         if (config.length() > 0) {
-            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);
+            try {
+                component = ComponentFactory.createComponent(thingUID, haID, config, updateListener, tracker, scheduler,
+                        gson, transformationServiceProvider);
+                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);
         }
     }
 

diff --git a/...main/java/org/openhab/binding/mqtt/homeassistant/internal/component/ComponentFactory.java b/...main/java/org/openhab/binding/mqtt/homeassistant/internal/component/ComponentFactory.java
@@ -21,6 +21,8 @@
 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,40 +50,36 @@ public class ComponentFactory {
      * @param updateListener A channel state update listener
      * @return A HA MQTT Component
      */
-    public static @Nullable AbstractComponent<?> createComponent(ThingUID thingUID, HaID haID,
-            String channelConfigurationJSON, ChannelStateUpdateListener updateListener, AvailabilityTracker tracker,
-            ScheduledExecutorService scheduler, Gson gson,
-            TransformationServiceProvider transformationServiceProvider) {
+    public static AbstractComponent<?> createComponent(ThingUID thingUID, HaID haID, String channelConfigurationJSON,
+            ChannelStateUpdateListener updateListener, AvailabilityTracker tracker, ScheduledExecutorService scheduler,
+            Gson gson, TransformationServiceProvider transformationServiceProvider) throws ConfigurationException {
         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);
-                case "binary_sensor":
-                    return new BinarySensor(componentConfiguration);
-                case "camera":
-                    return new Camera(componentConfiguration);
-                case "cover":
-                    return new Cover(componentConfiguration);
-                case "fan":
-                    return new Fan(componentConfiguration);
-                case "climate":
-                    return new Climate(componentConfiguration);
-                case "light":
-                    return new Light(componentConfiguration);
-                case "lock":
-                    return new Lock(componentConfiguration);
-                case "sensor":
-                    return new Sensor(componentConfiguration);
-                case "switch":
-                    return new Switch(componentConfiguration);
-            }
-        } catch (UnsupportedOperationException e) {
-            LOGGER.warn("Not supported", e);
+        switch (haID.component) {
+            case "alarm_control_panel":
+                return new AlarmControlPanel(componentConfiguration);
+            case "binary_sensor":
+                return new BinarySensor(componentConfiguration);
+            case "camera":
+                return new Camera(componentConfiguration);
+            case "cover":
+                return new Cover(componentConfiguration);
+            case "fan":
+                return new Fan(componentConfiguration);
+            case "climate":
+                return new Climate(componentConfiguration);
+            case "light":
+                return new Light(componentConfiguration);
+            case "lock":
+                return new Lock(componentConfiguration);
+            case "sensor":
+                return new Sensor(componentConfiguration);
+            case "switch":
+                return new Switch(componentConfiguration);
+            default:
+                throw new UnsupportedComponentException("Component '" + haID + "' is unsupported!");
         }
-        return null;
     }
 
     protected static class ComponentConfiguration {

diff --git a/...sistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Lock.java b/...sistant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Lock.java
@@ -16,6 +16,7 @@
 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 Lock(ComponentFactory.ComponentConfiguration componentConfiguration) {
 
         // 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,

diff --git a/...stant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Switch.java b/...stant/src/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/Switch.java
@@ -16,6 +16,7 @@
 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 Switch(ComponentFactory.ComponentConfiguration componentConfiguration) {
                 : 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

diff --git a/...n/java/org/openhab/binding/mqtt/homeassistant/internal/config/ConnectionDeserializer.java b/...n/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(
-                    "Connection information must be a tuple, but has " + list.size() + " elements!");
+            throw new JsonParseException("Connection information must be a tuple, but has " + list.size()
+                    + " elements! For example: " + "\"connections\": [[\"mac\", \"02:5b:26:a8:dc:12\"]]");
         }
         return new Connection(list.get(0).getAsString(), list.get(1).getAsString());
     }

diff --git a/.../openhab/binding/mqtt/homeassistant/internal/config/dto/AbstractChannelConfiguration.java b/.../openhab/binding/mqtt/homeassistant/internal/config/dto/AbstractChannelConfiguration.java
@@ -14,14 +14,15 @@
 
 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 Config() {
      */
     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);
+        }
     }
 }
diff --git a/...ava/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java b/...ava/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java
@@ -36,6 +36,7 @@
 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,43 +147,50 @@ public void receivedMessage(ThingUID connectionBridge, MqttBrokerConnection conn
         }
         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);
-        final String thingID = config.getThingId(haID.objectID);
 
-        final ThingTypeUID typeID = new ThingTypeUID(MqttBindingConstants.BINDING_ID,
-                MqttBindingConstants.HOMEASSISTANT_MQTT_THING.getId() + "_" + thingID);
+        try {
+            AbstractChannelConfiguration config = AbstractChannelConfiguration
+                    .fromString(new String(payload, StandardCharsets.UTF_8), gson);
+
+            final String thingID = config.getThingId(haID.objectID);
 
-        final ThingUID thingUID = new ThingUID(typeID, connectionBridge, thingID);
+            final ThingTypeUID typeID = new ThingTypeUID(MqttBindingConstants.BINDING_ID,
+                    MqttBindingConstants.HOMEASSISTANT_MQTT_THING.getId() + "_" + thingID);
 
-        thingIDPerTopic.put(topic, thingUID);
+            final ThingUID thingUID = new ThingUID(typeID, connectionBridge, thingID);
 
-        // We need to keep track of already found component topics for a specific thing
-        Set<HaID> components = componentsPerThingID.computeIfAbsent(thingID, key -> ConcurrentHashMap.newKeySet());
-        components.add(haID);
+            thingIDPerTopic.put(topic, thingUID);
 
-        final String componentNames = components.stream().map(id -> id.component)
-                .map(c -> HA_COMP_TO_NAME.getOrDefault(c, c)).collect(Collectors.joining(", "));
+            // We need to keep track of already found component topics for a specific thing
+            Set<HaID> components = componentsPerThingID.computeIfAbsent(thingID, key -> ConcurrentHashMap.newKeySet());
+            components.add(haID);
 
-        final List<String> topics = components.stream().map(HaID::toShortTopic).collect(Collectors.toList());
+            final String componentNames = components.stream().map(id -> id.component)
+                    .map(c -> HA_COMP_TO_NAME.getOrDefault(c, c)).collect(Collectors.joining(", "));
 
-        Map<String, Object> properties = new HashMap<>();
-        HandlerConfiguration handlerConfig = new HandlerConfiguration(haID.baseTopic, topics);
-        properties = handlerConfig.appendToProperties(properties);
-        properties = config.appendToProperties(properties);
-        properties.put("deviceId", thingID);
+            final List<String> topics = components.stream().map(HaID::toShortTopic).collect(Collectors.toList());
 
-        // Because we need the new properties map with the updated "components" list
-        results.put(thingUID.getAsString(),
-                DiscoveryResultBuilder.create(thingUID).withProperties(properties)
-                        .withRepresentationProperty("deviceId").withBridge(connectionBridge)
-                        .withLabel(config.getThingName() + " (" + componentNames + ")").build());
+            Map<String, Object> properties = new HashMap<>();
+            HandlerConfiguration handlerConfig = new HandlerConfiguration(haID.baseTopic, topics);
+            properties = handlerConfig.appendToProperties(properties);
+            properties = config.appendToProperties(properties);
+            properties.put("deviceId", thingID);
+
+            // Because we need the new properties map with the updated "components" list
+            results.put(thingUID.getAsString(),
+                    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() {

diff --git a/...ava/org/openhab/binding/mqtt/homeassistant/internal/exception/ConfigurationException.java b/...ava/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);
+    }
+}
diff --git a/.../openhab/binding/mqtt/homeassistant/internal/exception/UnsupportedComponentException.java b/.../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);
+    }
+}
diff --git a/...va/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java b/...va/org/openhab/binding/mqtt/homeassistant/internal/handler/HomeAssistantThingHandler.java
@@ -40,6 +40,7 @@
 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 void initialize() {
             if (channelConfigurationJSON == null) {
                 logger.warn("Provided channel does not have a 'config' configuration key!");
             } else {
-                component = ComponentFactory.createComponent(thingUID, haID, channelConfigurationJSON, this, this,
-                        scheduler, gson, transformationServiceProvider);
-            }
-
-            if (component != null) {
-                haComponents.put(component.getGroupUID().getId(), component);
-                component.addChannelTypes(channelTypeProvider);
-            } else {
-                logger.warn("Could not restore component {}", thing);
+                try {
+                    component = ComponentFactory.createComponent(thingUID, haID, channelConfigurationJSON, this, this,
+                            scheduler, gson, transformationServiceProvider);
+                    haComponents.put(component.getGroupUID().getId(), component);
+                    component.addChannelTypes(channelTypeProvider);
+                } catch (ConfigurationException e) {
+                    logger.error("Cannot not restore component {}: {}", thing, e.getMessage());
+                }
             }
         }
         updateThingType();