[unifi] Various stability improvements (#14249)

* [unifi] Various stability improvements

- Fixed bug, causing nullpointer, in devices not reporting voltage/ampere values on PoE ports.
- Added some null checks to avoid having null pointer exceptions.
- Handled timeout exception that is in ExecutionException.
- Improved handling case where server returns data, but it's not from the api, and therefor can't be parsed.
- Improved adding additional text to the error messages by adding the reported exception message too.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiCommunicationException.java

@@ -22,7 +22,11 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 @NonNullByDefault
 public class UniFiCommunicationException extends UniFiException {
 
-    private static final long serialVersionUID = -7261308872245069364L;
+    private static final long serialVersionUID = 1L;
+
+    public UniFiCommunicationException(final String message) {
+        super(message);
+    }
 
     public UniFiCommunicationException(final Throwable cause) {
         super(cause);

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiController.java

@@ -178,7 +178,7 @@ public class UniFiController {
     public boolean poeMode(final UniFiDevice device, final List<JsonObject> data) throws UniFiException {
         // Safety check to make sure no empty data is send to avoid corrupting override data on the device.
         if (data.isEmpty() || data.stream().anyMatch(p -> p.entrySet().isEmpty())) {
-            logger.info("Not overriding port for '{}', because port data contains empty json: {}", device.getName(),
+            logger.info("Not overriding port for '{}', because port data contains empty JSON: {}", device.getName(),
                     poeGson.toJson(data));
             return false;
         } else {

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiControllerRequest.java

@@ -47,6 +47,7 @@ import org.slf4j.LoggerFactory;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.google.gson.JsonObject;
+import com.google.gson.JsonParseException;
 import com.google.gson.JsonParser;
 
 /**
@@ -59,6 +60,8 @@ import com.google.gson.JsonParser;
 @NonNullByDefault
 class UniFiControllerRequest<T> {
 
+    private static final String CONTROLLER_PARSE_ERROR = "@text/error.controller.parse_error";
+
     private static final String CONTENT_TYPE_APPLICATION_JSON_UTF_8 = MimeTypes.Type.APPLICATION_JSON_UTF_8.asString();
 
     private static final long TIMEOUT_SECONDS = 5;
@@ -128,10 +131,20 @@ class UniFiControllerRequest<T> {
         final String json = getContent();
         // mgb: only try and unmarshall non-void result types
         if (!Void.class.equals(resultType)) {
-            final JsonObject jsonObject = JsonParser.parseString(json).getAsJsonObject();
+            try {
+                final JsonObject jsonObject = JsonParser.parseString(json).getAsJsonObject();
 
-            if (jsonObject.has(PROPERTY_DATA) && jsonObject.get(PROPERTY_DATA).isJsonArray()) {
+                if (jsonObject.has(PROPERTY_DATA) && jsonObject.get(PROPERTY_DATA).isJsonArray()) {
-                result = (T) gson.fromJson(jsonObject.getAsJsonArray(PROPERTY_DATA), resultType);
+                    result = (T) gson.fromJson(jsonObject.getAsJsonArray(PROPERTY_DATA), resultType);
+                }
+            } catch (final JsonParseException e) {
+                logger.debug(
+                        "Could not parse content retrieved from the server. Is the configuration pointing to the right server/port?, {}",
+                        e.getMessage());
+                if (logger.isTraceEnabled()) {
+                    prettyPrintJson(json);
+                }
+                throw new UniFiCommunicationException(CONTROLLER_PARSE_ERROR);
             }
         }
         return result;
@@ -182,7 +195,9 @@ class UniFiControllerRequest<T> {
         } catch (final ExecutionException e) {
             // mgb: unwrap the cause and try to cleanly handle it
             final Throwable cause = e.getCause();
-            if (cause instanceof UnknownHostException) {
+            if (cause instanceof TimeoutException) {
+                throw new UniFiCommunicationException(e);
+            } else if (cause instanceof UnknownHostException) {
                 // invalid hostname
                 throw new UniFiInvalidHostException(cause);
             } else if (cause instanceof ConnectException) {
@@ -242,14 +257,15 @@ class UniFiControllerRequest<T> {
         return request;
     }
 
-    private static String prettyPrintJson(final String content) {
+    private String prettyPrintJson(final String content) {
         try {
             final JsonObject json = JsonParser.parseString(content).getAsJsonObject();
             final Gson prettyGson = new GsonBuilder().setPrettyPrinting().create();
 
             return prettyGson.toJson(json);
         } catch (final RuntimeException e) {
-            // If could not parse the string as json, just return the string
+            logger.debug("RuntimeException pretty printing JSON. Returning the raw content.", e);
+            // If could not parse the string as JSON, just return the string
             return content;
         }
     }

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiCache.java

@@ -16,7 +16,9 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -103,13 +105,12 @@ abstract class UniFiCache<T extends @Nullable HasId> {
 
     public final void putAll(final T @Nullable [] values) {
         if (values != null) {
-            logger.debug("Put #{} entries in {}: {}", values.length, getClass().getSimpleName(),
+            if (logger.isDebugEnabled()) {
-                    lazyFormatAsList(values));
+                logger.debug("Put #{} entries in {}: {}", values.length, getClass().getSimpleName(),
-            for (final T value : values) {
+                        Stream.of(values).filter(Objects::nonNull).map(Object::toString)
-                if (value != null) {
+                                .collect(Collectors.joining(System.lineSeparator() + " - ")));
-                    put(value.getId(), value);
-                }
             }
+            Stream.of(values).filter(Objects::nonNull).forEach(value -> put(value.getId(), value));
         }
     }
 
@@ -133,18 +134,4 @@ abstract class UniFiCache<T extends @Nullable HasId> {
     }
 
     protected abstract @Nullable String getSuffix(T value, Prefix prefix);
-
-    private static Object lazyFormatAsList(final Object[] arr) {
-        return new Object() {
-
-            @Override
-            public String toString() {
-                String value = "";
-                for (final Object o : arr) {
-                    value += "\n - " + o.toString();
-                }
-                return value;
-            }
-        };
-    }
 }

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiControllerCache.java

@@ -17,7 +17,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
@@ -138,8 +138,8 @@ public class UniFiControllerCache {
         return clientsCache.values();
     }
 
-    public long countClients(final UniFiSite site, final Function<UniFiClient, Boolean> filter) {
+    public long countClients(final UniFiSite site, final Predicate<UniFiClient> filter) {
-        return getClients().stream().filter(c -> site.isSite(c.getSite())).filter(filter::apply).count();
+        return getClients().stream().filter(c -> site.isSite(c.getSite())).filter(filter::test).count();
     }
 
     public @Nullable UniFiClient getClient(@Nullable final String cid) {

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiClient.java

@@ -101,10 +101,10 @@ public abstract class UniFiClient implements HasId {
         return blocked;
     }
 
-    public abstract Boolean isWired();
+    public abstract boolean isWired();
 
-    public final Boolean isWireless() {
+    public final boolean isWireless() {
-        return isWired() == null ? null : Boolean.FALSE.equals(isWired());
+        return !isWired();
     }
 
     protected abstract String getDeviceMac();

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiPortTuple.java

@@ -14,7 +14,7 @@ package org.openhab.binding.unifi.internal.api.dto;
 
 /**
  * Tuple to store both the {@link UniFiPortTable}, which contains the all information related to the port,
- * and the {@link UnfiPortOverrideJsonObject}, which contains the raw json data of the port override.
+ * and the {@link UnfiPortOverrideJsonObject}, which contains the raw JSON data of the port override.
  *
  * @author Hilbrand Bouwkamp - Initial contribution
  */

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiSwitchPorts.java

@@ -79,13 +79,13 @@ public class UniFiSwitchPorts {
     }
 
     /**
-     * Returns the override data as list with json objects after calling the updateMethod on the data for the given
+     * Returns the override data as list with JSON objects after calling the updateMethod on the data for the given
      * portIdx.
      * The update method changes the data in the internal structure.
      *
      * @param portIdx port to call updateMethod for
      * @param updateMethod method to call to update data for a specific port
-     * @return Returns a list of json objects of all override data
+     * @return Returns a list of JSON objects of all override data
      */
     public List<JsonObject> updatedList(final int portIdx, final Consumer<UnfiPortOverrideJsonObject> updateMethod) {
         @SuppressWarnings("null")
@@ -103,7 +103,7 @@ public class UniFiSwitchPorts {
      * Set the port override object. If it's for a specific port set bind it to the port data, otherwise store it as
      * generic data.
      *
-     * @param jsonObject json object to set
+     * @param jsonObject JSON object to set
      */
     public void setOverride(final JsonObject jsonObject) {
         if (UnfiPortOverrideJsonObject.hasPortIdx(jsonObject)) {

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiUnknownClient.java

@@ -29,12 +29,12 @@ public class UniFiUnknownClient extends UniFiClient {
     }
 
     @Override
-    public Boolean isWired() {
+    public boolean isWired() {
-        return null; // mgb: no is_wired property in the json
+        return false; // mgb: no is_wired property in the JSON
     }
 
     @Override
     public String getDeviceMac() {
-        return null; // mgb: no device mac in the json
+        return null; // mgb: no device mac in the JSON
     }
 }

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWiredClient.java

@@ -30,7 +30,7 @@ public class UniFiWiredClient extends UniFiClient {
     }
 
     @Override
-    public Boolean isWired() {
+    public boolean isWired() {
         return true;
     }
 

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWirelessClient.java

@@ -38,7 +38,7 @@ public class UniFiWirelessClient extends UniFiClient {
     }
 
     @Override
-    public Boolean isWired() {
+    public boolean isWired() {
         return false;
     }
 

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiClientThingHandler.java

@@ -165,7 +165,7 @@ public class UniFiClientThingHandler extends UniFiBaseThingHandler<UniFiClient,
     protected State getChannelState(final UniFiClient client, final String channelId) {
         final boolean clientHome = isClientHome(client);
         final UniFiDevice device = client.getDevice();
-        final UniFiSite site = (device == null ? null : device.getSite());
+        final UniFiSite site = device == null ? null : device.getSite();
         State state = getDefaultState(channelId);
 
         switch (channelId) {

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiControllerThingHandler.java

@@ -59,6 +59,7 @@ public class UniFiControllerThingHandler extends BaseBridgeHandler {
     private static final String STATUS_DESCRIPTION_SSL_ERROR = "@text/error.bridge.offline.ssl_error";
     private static final String STATUS_DESCRIPTION_INVALID_CREDENTIALS = "@text/error.bridge.offline.invalid_credentials";
     private static final String STATUS_DESCRIPTION_INVALID_HOSTNAME = "@text/error.bridge.offline.invalid_hostname";
+    private static final String I18N_STATUS_WITH_ARGUMENTS = "%s [\"%s\"]";
 
     private final Logger logger = LoggerFactory.getLogger(UniFiControllerThingHandler.class);
 
@@ -138,14 +139,14 @@ public class UniFiControllerThingHandler extends BaseBridgeHandler {
             uc.start();
             startRefresh = true;
         } catch (final UniFiCommunicationException e) {
-            updateStatus(OFFLINE, COMMUNICATION_ERROR, STATUS_DESCRIPTION_COMMUNICATION_ERROR);
+            updateStatusOffline(COMMUNICATION_ERROR, STATUS_DESCRIPTION_COMMUNICATION_ERROR, e.getMessage());
             startRefresh = true;
         } catch (final UniFiInvalidHostException e) {
-            updateStatus(OFFLINE, CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_HOSTNAME);
+            updateStatusOffline(CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_HOSTNAME, e.getMessage());
         } catch (final UniFiSSLException e) {
-            updateStatus(OFFLINE, CONFIGURATION_ERROR, STATUS_DESCRIPTION_SSL_ERROR);
+            updateStatusOffline(CONFIGURATION_ERROR, STATUS_DESCRIPTION_SSL_ERROR, e.getMessage());
         } catch (final UniFiInvalidCredentialsException e) {
-            updateStatus(OFFLINE, CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_CREDENTIALS);
+            updateStatusOffline(CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_CREDENTIALS, e.getMessage());
         } catch (final UniFiException e) {
             logger.debug("Unknown error while configuring the UniFi Controller", e);
             updateStatus(OFFLINE, CONFIGURATION_ERROR, e.getMessage());
@@ -174,15 +175,20 @@ public class UniFiControllerThingHandler extends BaseBridgeHandler {
             refresh();
             updateStatus(ONLINE);
         } catch (final UniFiCommunicationException e) {
-            updateStatus(OFFLINE, COMMUNICATION_ERROR, STATUS_DESCRIPTION_COMMUNICATION_ERROR);
+            updateStatusOffline(COMMUNICATION_ERROR, STATUS_DESCRIPTION_COMMUNICATION_ERROR, e.getMessage());
         } catch (final UniFiInvalidCredentialsException e) {
-            updateStatus(OFFLINE, CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_CREDENTIALS);
+            updateStatusOffline(CONFIGURATION_ERROR, STATUS_DESCRIPTION_INVALID_CREDENTIALS, e.getMessage());
         } catch (final RuntimeException | UniFiException e) {
             logger.debug("Unhandled exception while refreshing the UniFi Controller {}", getThing().getUID(), e);
             updateStatus(OFFLINE, COMMUNICATION_ERROR, e.getMessage());
         }
     }
 
+    private void updateStatusOffline(final ThingStatusDetail thingStatusDetail, final String i18nKey,
+            final @Nullable String argument) {
+        updateStatus(OFFLINE, thingStatusDetail, String.format(I18N_STATUS_WITH_ARGUMENTS, i18nKey, argument));
+    }
+
     private void refresh() throws UniFiException {
         final UniFiController uc = controller;
 

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiPoePortThingHandler.java

@@ -25,9 +25,8 @@ import static org.openhab.binding.unifi.internal.UniFiBindingConstants.CHANNEL_P
 import static org.openhab.binding.unifi.internal.UniFiBindingConstants.CHANNEL_PORT_POE_VOLTAGE;
 import static org.openhab.core.library.unit.MetricPrefix.MILLI;
 
-import javax.measure.quantity.ElectricCurrent;
+import javax.measure.Quantity;
-import javax.measure.quantity.ElectricPotential;
+import javax.measure.Unit;
-import javax.measure.quantity.Power;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -126,13 +125,13 @@ public class UniFiPoePortThingHandler extends UniFiBaseThingHandler<UniFiSwitchP
                 state = StringType.valueOf(port.getPoeMode());
                 break;
             case CHANNEL_PORT_POE_POWER:
-                state = new QuantityType<Power>(Double.valueOf(port.getPoePower()), Units.WATT);
+                state = safeDouble(port.getPoePower(), Units.WATT);
                 break;
             case CHANNEL_PORT_POE_VOLTAGE:
-                state = new QuantityType<ElectricPotential>(Double.valueOf(port.getPoeVoltage()), Units.VOLT);
+                state = safeDouble(port.getPoeVoltage(), Units.VOLT);
                 break;
             case CHANNEL_PORT_POE_CURRENT:
-                state = new QuantityType<ElectricCurrent>(Double.valueOf(port.getPoeCurrent()), MILLI(Units.AMPERE));
+                state = safeDouble(port.getPoeCurrent(), MILLI(Units.AMPERE));
                 break;
             default:
                 state = UnDefType.UNDEF;
@@ -140,6 +139,15 @@ public class UniFiPoePortThingHandler extends UniFiBaseThingHandler<UniFiSwitchP
         return state;
     }
 
+    private <Q extends Quantity<Q>> State safeDouble(final String value, final Unit<Q> unit) {
+        try {
+            return value == null ? UnDefType.UNDEF : QuantityType.valueOf(Double.parseDouble(value), unit);
+        } catch (final NumberFormatException e) {
+            logger.debug("Could not parse value '{}' for unit {}", value, unit);
+            return UnDefType.UNDEF;
+        }
+    }
+
     private State setOfflineOnNoPoEPortData() {
         if (getThing().getStatus() != ThingStatus.OFFLINE) {
             updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiThingDiscoveryService.java

@@ -127,8 +127,9 @@ public class UniFiThingDiscoveryService extends AbstractDiscoveryService
         for (final UniFiWlan wlan : cache.getWlans()) {
             final ThingUID thingUID = new ThingUID(UniFiBindingConstants.THING_TYPE_WLAN, bridgeUID,
                     stripIdShort(wlan.getId()));
-            final Map<String, Object> properties = Map.of(PARAMETER_WID, wlan.getId(), PARAMETER_SITE,
+            final String siteName = wlan.getSite() == null ? "" : wlan.getSite().getName();
-                    wlan.getSite().getName(), PARAMETER_WIFI_NAME, wlan.getName());
+            final Map<String, Object> properties = Map.of(PARAMETER_WID, wlan.getId(), PARAMETER_SITE, siteName,
+                    PARAMETER_WIFI_NAME, wlan.getName());
 
             thingDiscovered(DiscoveryResultBuilder.create(thingUID).withThingType(UniFiBindingConstants.THING_TYPE_WLAN)
                     .withBridge(bridgeUID).withRepresentationProperty(PARAMETER_WID).withTTL(TTL_SECONDS)
@@ -157,7 +158,7 @@ public class UniFiThingDiscoveryService extends AbstractDiscoveryService
      * @return shortened id or if to short the original id
      */
     private static String stripIdShort(final String id) {
-        return id.length() > THING_ID_LENGTH ? id.substring(id.length() - THING_ID_LENGTH) : id;
+        return id != null && id.length() > THING_ID_LENGTH ? id.substring(id.length() - THING_ID_LENGTH) : id;
     }
 
     private void discoverPoePorts(final UniFiControllerCache cache, final ThingUID bridgeUID) {

bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiWlanThingHandler.java

@@ -24,7 +24,7 @@ import static org.openhab.binding.unifi.internal.UniFiBindingConstants.CHANNEL_W
 import static org.openhab.binding.unifi.internal.UniFiBindingConstants.CHANNEL_WPAENC;
 import static org.openhab.binding.unifi.internal.UniFiBindingConstants.CHANNEL_WPAMODE;
 
-import java.util.function.Function;
+import java.util.function.Predicate;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -48,6 +48,7 @@ import org.openhab.core.types.State;
 import org.openhab.core.types.UnDefType;
 
 /**
+ * The {@link UniFiWlanThingHandler} is responsible for handling commands and status updates for a wireless network.
  *
  * @author Hilbrand Bouwkamp - Initial contribution
  */
@@ -127,10 +128,17 @@ public class UniFiWlanThingHandler extends UniFiBaseThingHandler<UniFiWlan, UniF
         return state;
     }
 
-    private static State countClients(final UniFiWlan wlan, final Function<UniFiClient, Boolean> filter) {
+    private static State countClients(final UniFiWlan wlan, final Predicate<UniFiClient> filter) {
         final UniFiSite site = wlan.getSite();
-        return new DecimalType(site.getCache().countClients(site, c -> c instanceof UniFiWirelessClient
+
-                && wlan.getName().equals(((UniFiWirelessClient) c).getEssid()) && filter.apply(c)));
+        if (site == null) {
+            return UnDefType.UNDEF;
+        } else {
+            return new DecimalType(site.getCache().countClients(site,
+                    c -> c instanceof UniFiWirelessClient
+                            && (wlan.getName() != null && wlan.getName().equals(((UniFiWirelessClient) c).getEssid()))
+                            && filter.test(c)));
+        }
     }
 
     /**
@@ -161,7 +169,7 @@ public class UniFiWlanThingHandler extends UniFiBaseThingHandler<UniFiWlan, UniF
             final ChannelUID channelUID, final Command command) throws UniFiException {
         final String channelID = channelUID.getId();
 
-        if (CHANNEL_ENABLE.equals(channelID) && command instanceof OnOffType) {
+        if (CHANNEL_ENABLE.equals(channelID) && command instanceof OnOffType && entity.getSite() != null) {
             controller.enableWifi(entity, OnOffType.ON == command);
             return true;
         }

bundles/org.openhab.binding.unifi/src/main/resources/OH-INF/i18n/unifi.properties

@@ -132,10 +132,11 @@ channel-type.config.unifi.poeEnable.mode.option.passthrough = Passthrough
 
 # status messages
 
-error.bridge.offline.communication_error = Error communicating with the UniFi controller.
+error.bridge.offline.communication_error = Error communicating with the UniFi controller: {0}
-error.bridge.offline.invalid_credentials = Invalid username and/or password - please double-check your configuration.
+error.bridge.offline.invalid_credentials = Invalid username and/or password - please double-check your configuration: {0}
-error.bridge.offline.invalid_hostname = Invalid hostname - please double-check your configuration.
+error.bridge.offline.invalid_hostname = Invalid hostname - please double-check your configuration: {0}
-error.bridge.offline.ssl_error = Error establishing an SSL connection with the UniFi controller.
+error.bridge.offline.ssl_error = Error establishing an SSL connection with the UniFi controller: {0}
+error.controller.parse_error = Could not parse JSON from the UniFi Controller. Is the bridge configured with the correct host and/or port?
 error.thing.client.offline.configuration_error = You must define a MAC address, IP address, hostname or alias for this thing.
 error.thing.offline.bridge_offline = The UniFi Controller is currently offline.
 error.thing.offline.configuration_error = You must choose a UniFi Controller for this thing.