From bd23bcc87d46f445ba2778687375fbcc56a8b5bc Mon Sep 17 00:00:00 2001 From: Hilbrand Bouwkamp Date: Fri, 20 Jan 2023 13:12:57 +0100 Subject: [PATCH] [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 --- .../api/UniFiCommunicationException.java | 6 +++- .../unifi/internal/api/UniFiController.java | 2 +- .../internal/api/UniFiControllerRequest.java | 28 +++++++++++++++---- .../unifi/internal/api/cache/UniFiCache.java | 27 +++++------------- .../api/cache/UniFiControllerCache.java | 6 ++-- .../unifi/internal/api/dto/UniFiClient.java | 6 ++-- .../internal/api/dto/UniFiPortTuple.java | 2 +- .../internal/api/dto/UniFiSwitchPorts.java | 6 ++-- .../internal/api/dto/UniFiUnknownClient.java | 6 ++-- .../internal/api/dto/UniFiWiredClient.java | 2 +- .../internal/api/dto/UniFiWirelessClient.java | 2 +- .../handler/UniFiClientThingHandler.java | 2 +- .../handler/UniFiControllerThingHandler.java | 18 ++++++++---- .../handler/UniFiPoePortThingHandler.java | 20 +++++++++---- .../handler/UniFiThingDiscoveryService.java | 7 +++-- .../handler/UniFiWlanThingHandler.java | 18 ++++++++---- .../resources/OH-INF/i18n/unifi.properties | 9 +++--- 17 files changed, 99 insertions(+), 68 deletions(-) diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiCommunicationException.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiCommunicationException.java index b7674aec6..eb8edd04b 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiCommunicationException.java +++ b/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); diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiController.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiController.java index 59e72b4f7..09f363489 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiController.java +++ b/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 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 { diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiControllerRequest.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiControllerRequest.java index 60b033707..0e450dd09 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/UniFiControllerRequest.java +++ b/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 { + 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 { 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()) { - result = (T) gson.fromJson(jsonObject.getAsJsonArray(PROPERTY_DATA), resultType); + if (jsonObject.has(PROPERTY_DATA) && jsonObject.get(PROPERTY_DATA).isJsonArray()) { + 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 { } 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 { 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; } } diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiCache.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiCache.java index 0286fd72c..04dc87963 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiCache.java +++ b/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 { public final void putAll(final T @Nullable [] values) { if (values != null) { - logger.debug("Put #{} entries in {}: {}", values.length, getClass().getSimpleName(), - lazyFormatAsList(values)); - for (final T value : values) { - if (value != null) { - put(value.getId(), value); - } + if (logger.isDebugEnabled()) { + logger.debug("Put #{} entries in {}: {}", values.length, getClass().getSimpleName(), + Stream.of(values).filter(Objects::nonNull).map(Object::toString) + .collect(Collectors.joining(System.lineSeparator() + " - "))); } + Stream.of(values).filter(Objects::nonNull).forEach(value -> put(value.getId(), value)); } } @@ -133,18 +134,4 @@ abstract class UniFiCache { } 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; - } - }; - } } diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiControllerCache.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiControllerCache.java index fda7918fa..df0299f45 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/cache/UniFiControllerCache.java +++ b/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 filter) { - return getClients().stream().filter(c -> site.isSite(c.getSite())).filter(filter::apply).count(); + public long countClients(final UniFiSite site, final Predicate filter) { + return getClients().stream().filter(c -> site.isSite(c.getSite())).filter(filter::test).count(); } public @Nullable UniFiClient getClient(@Nullable final String cid) { diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiClient.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiClient.java index 9c5f16316..109ad1efc 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiClient.java +++ b/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() { - return isWired() == null ? null : Boolean.FALSE.equals(isWired()); + public final boolean isWireless() { + return !isWired(); } protected abstract String getDeviceMac(); diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiPortTuple.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiPortTuple.java index 624281f6d..e0311d585 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiPortTuple.java +++ b/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 */ diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiSwitchPorts.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiSwitchPorts.java index c39227108..4680f1766 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiSwitchPorts.java +++ b/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 updatedList(final int portIdx, final Consumer 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)) { diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiUnknownClient.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiUnknownClient.java index 4c37855f9..079b30ebd 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiUnknownClient.java +++ b/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() { - return null; // mgb: no is_wired property in the json + public boolean isWired() { + 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 } } diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWiredClient.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWiredClient.java index 55ed933e4..043c7cf9f 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWiredClient.java +++ b/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; } diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWirelessClient.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWirelessClient.java index 9b1898dd6..a27afc6fc 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/api/dto/UniFiWirelessClient.java +++ b/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; } diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiClientThingHandler.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiClientThingHandler.java index 2ba0c7e83..6d01b4829 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiClientThingHandler.java +++ b/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(Double.valueOf(port.getPoePower()), Units.WATT); + state = safeDouble(port.getPoePower(), Units.WATT); break; case CHANNEL_PORT_POE_VOLTAGE: - state = new QuantityType(Double.valueOf(port.getPoeVoltage()), Units.VOLT); + state = safeDouble(port.getPoeVoltage(), Units.VOLT); break; case CHANNEL_PORT_POE_CURRENT: - state = new QuantityType(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> State safeDouble(final String value, final Unit 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, diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiThingDiscoveryService.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiThingDiscoveryService.java index aa9140dfc..5568b3ec4 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiThingDiscoveryService.java +++ b/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 properties = Map.of(PARAMETER_WID, wlan.getId(), PARAMETER_SITE, - wlan.getSite().getName(), PARAMETER_WIFI_NAME, wlan.getName()); + final String siteName = wlan.getSite() == null ? "" : wlan.getSite().getName(); + final Map 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) { diff --git a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiWlanThingHandler.java b/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiWlanThingHandler.java index 4cd65f2fa..72f2fc4ae 100644 --- a/bundles/org.openhab.binding.unifi/src/main/java/org/openhab/binding/unifi/internal/handler/UniFiWlanThingHandler.java +++ b/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 filter) { + private static State countClients(final UniFiWlan wlan, final Predicate 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