From 9bbcb85f59dcbe86a6fd1303b39a7087f2470096 Mon Sep 17 00:00:00 2001 From: Jacob Laursen Date: Sat, 9 Sep 2023 12:05:38 +0200 Subject: [PATCH] [hue] Fix and improve error logging and status descriptions for API v2 (#15512) * Provide detailed error information on failed commands * Log as info when command succeeds * Revert collect(Collectors.toList()) refactoring * Provide exception message in status description Fixes #15511 --------- Signed-off-by: Jacob Laursen --- .../hue/internal/connection/Clip2Bridge.java | 13 +- .../hue/internal/dto/clip2/Resources.java | 4 + .../internal/handler/Clip2BridgeHandler.java | 115 ++++++++---------- .../internal/handler/Clip2ThingHandler.java | 7 +- .../main/resources/OH-INF/i18n/hue.properties | 2 +- 5 files changed, 69 insertions(+), 72 deletions(-) diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java index 102b5170e..6c2f8d843 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java @@ -1081,10 +1081,11 @@ public class Clip2Bridge implements Closeable { * accessing the session while it is being recreated. * * @param resource the resource to put. + * @return the resource, which may contain errors. * @throws ApiException if something fails. * @throws InterruptedException */ - public void putResource(Resource resource) throws ApiException, InterruptedException { + public Resources putResource(Resource resource) throws ApiException, InterruptedException { Stream stream = null; try (Throttler throttler = new Throttler(MAX_CONCURRENT_STREAMS); SessionSynchronizer sessionSynchronizer = new SessionSynchronizer(false)) { @@ -1106,17 +1107,17 @@ public class Clip2Bridge implements Closeable { String contentType = contentStreamListener.getContentType(); int status = contentStreamListener.getStatus(); LOGGER.trace("HTTP/2 {} (Content-Type: {}) << {}", status, contentType, contentJson); - if (status != HttpStatus.OK_200) { + if (!HttpStatus.isSuccess(status)) { throw new ApiException(String.format("Unexpected HTTP status '%d'", status)); } if (!MediaType.APPLICATION_JSON.equals(contentType)) { throw new ApiException("Unexpected Content-Type: " + contentType); } + if (contentJson.isEmpty()) { + throw new ApiException("Response payload is empty"); + } try { - Resources resources = Objects.requireNonNull(jsonParser.fromJson(contentJson, Resources.class)); - if (LOGGER.isDebugEnabled()) { - resources.getErrors().forEach(error -> LOGGER.debug("putResource() resources error:{}", error)); - } + return Objects.requireNonNull(jsonParser.fromJson(contentJson, Resources.class)); } catch (JsonParseException e) { LOGGER.debug("putResource() parsing error json:{}", contentJson, e); throw new ApiException("Parsing error", e); diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resources.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resources.java index a20618893..ce554136d 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resources.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resources.java @@ -32,6 +32,10 @@ public class Resources { return errors.stream().map(Error::getDescription).collect(Collectors.toList()); } + public boolean hasErrors() { + return !errors.isEmpty(); + } + public List getResources() { return data; } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java index d7903bce6..aabdf9735 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java @@ -161,73 +161,50 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { private synchronized void checkConnection() { logger.debug("checkConnection()"); - // check connection to the hub - ThingStatusDetail thingStatus; + boolean retryApplicationKey = false; + boolean retryConnection = false; + try { checkAssetsLoaded(); getClip2Bridge().testConnectionState(); - thingStatus = ThingStatusDetail.NONE; - } catch (HttpUnauthorizedException e) { - logger.debug("checkConnection() {}", e.getMessage(), e); - thingStatus = ThingStatusDetail.CONFIGURATION_ERROR; + updateSelf(); // go online + } catch (HttpUnauthorizedException unauthorizedException) { + logger.debug("checkConnection() {}", unauthorizedException.getMessage(), unauthorizedException); + if (applKeyRetriesRemaining > 0) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, + "@text/offline.api2.conf-error.press-pairing-button"); + try { + registerApplicationKey(); + retryApplicationKey = true; + } catch (HttpUnauthorizedException e) { + retryApplicationKey = true; + } catch (ApiException e) { + setStatusOfflineWithCommunicationError(e); + } catch (IllegalStateException e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, + "@text/offline.api2.conf-error.read-only"); + } catch (AssetNotLoadedException e) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "@text/offline.api2.conf-error.assets-not-loaded"); + } catch (InterruptedException e) { + return; + } + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, + "@text/offline.api2.conf-error.not-authorized"); + } } catch (ApiException e) { logger.debug("checkConnection() {}", e.getMessage(), e); - thingStatus = ThingStatusDetail.COMMUNICATION_ERROR; + setStatusOfflineWithCommunicationError(e); + retryConnection = connectRetriesRemaining > 0; } catch (AssetNotLoadedException e) { logger.debug("checkConnection() {}", e.getMessage(), e); - thingStatus = ThingStatusDetail.HANDLER_INITIALIZING_ERROR; + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "@text/offline.api2.conf-error.assets-not-loaded"); } catch (InterruptedException e) { return; } - // update the thing status - boolean retryApplicationKey = false; - boolean retryConnection = false; - switch (thingStatus) { - case CONFIGURATION_ERROR: - if (applKeyRetriesRemaining > 0) { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, - "@text/offline.api2.conf-error.press-pairing-button"); - try { - registerApplicationKey(); - retryApplicationKey = true; - } catch (HttpUnauthorizedException e) { - retryApplicationKey = true; - } catch (ApiException e) { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.communication-error"); - } catch (IllegalStateException e) { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, - "@text/offline.api2.conf-error.read-only"); - } catch (AssetNotLoadedException e) { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.api2.conf-error.assets-not-loaded"); - } catch (InterruptedException e) { - return; - } - } else { - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, - "@text/offline.api2.conf-error.not-authorized"); - } - break; - - case COMMUNICATION_ERROR: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.communication-error"); - retryConnection = connectRetriesRemaining > 0; - break; - - case HANDLER_INITIALIZING_ERROR: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.api2.conf-error.assets-not-loaded"); - break; - - case NONE: - default: - updateSelf(); // go online - break; - } - int milliSeconds; if (retryApplicationKey) { // short delay used during attempts to create or validate an application key @@ -250,6 +227,18 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { checkConnectionTask = scheduler.schedule(() -> checkConnection(), milliSeconds, TimeUnit.MILLISECONDS); } + private void setStatusOfflineWithCommunicationError(Exception e) { + Throwable cause = e.getCause(); + String causeMessage = cause == null ? null : cause.getMessage(); + if (causeMessage == null || causeMessage.isEmpty()) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + "\"]"); + } else { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + " -> " + causeMessage + "\"]"); + } + } + /** * If a child thing has been added, and the bridge is online, update the child's data. */ @@ -467,8 +456,7 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { } } catch (IOException e) { logger.trace("initializeAssets() communication error on '{}'", ipAddress, e); - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + "\"]"); + setStatusOfflineWithCommunicationError(e); return; } @@ -491,8 +479,7 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { clip2Bridge = new Clip2Bridge(httpClientFactory, this, ipAddress, applicationKey); } catch (ApiException e) { logger.trace("initializeAssets() communication error on '{}'", ipAddress, e); - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + "\"]"); + setStatusOfflineWithCommunicationError(e); return; } @@ -555,14 +542,15 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { * Execute an HTTP PUT to send a Resource object to the server. * * @param resource the resource to put. + * @return the resource, which may contain errors. * @throws ApiException if a communication error occurred. * @throws AssetNotLoadedException if one of the assets is not loaded. * @throws InterruptedException */ - public void putResource(Resource resource) throws ApiException, AssetNotLoadedException, InterruptedException { + public Resources putResource(Resource resource) throws ApiException, AssetNotLoadedException, InterruptedException { logger.debug("putResource() {}", resource); checkAssetsLoaded(); - getClip2Bridge().putResource(resource); + return getClip2Bridge().putResource(resource); } /** @@ -684,8 +672,7 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { getClip2Bridge().open(); } catch (ApiException e) { logger.trace("updateSelf() {}", e.getMessage(), e); - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + "\"]"); + setStatusOfflineWithCommunicationError(e); onConnectionOffline(); } catch (AssetNotLoadedException e) { logger.trace("updateSelf() {}", e.getMessage(), e); diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java index 84586791b..76d7b4c5c 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java @@ -44,6 +44,7 @@ import org.openhab.binding.hue.internal.dto.clip2.MirekSchema; import org.openhab.binding.hue.internal.dto.clip2.ProductData; import org.openhab.binding.hue.internal.dto.clip2.Resource; import org.openhab.binding.hue.internal.dto.clip2.ResourceReference; +import org.openhab.binding.hue.internal.dto.clip2.Resources; import org.openhab.binding.hue.internal.dto.clip2.enums.ActionType; import org.openhab.binding.hue.internal.dto.clip2.enums.EffectType; import org.openhab.binding.hue.internal.dto.clip2.enums.RecallAction; @@ -507,7 +508,11 @@ public class Clip2ThingHandler extends BaseThingHandler { logger.debug("{} -> handleCommand() put resource {}", resourceId, putResource); try { - getBridgeHandler().putResource(putResource); + Resources resources = getBridgeHandler().putResource(putResource); + if (resources.hasErrors()) { + logger.info("Command '{}' for thing '{}', channel '{}' succeeded with errors: {}", command, + thing.getUID(), channelUID, String.join("; ", resources.getErrors())); + } } catch (ApiException | AssetNotLoadedException e) { if (logger.isDebugEnabled()) { logger.debug("{} -> handleCommand() error {}", resourceId, e.getMessage(), e); diff --git a/bundles/org.openhab.binding.hue/src/main/resources/OH-INF/i18n/hue.properties b/bundles/org.openhab.binding.hue/src/main/resources/OH-INF/i18n/hue.properties index 1835b97ab..b79fd4ddb 100644 --- a/bundles/org.openhab.binding.hue/src/main/resources/OH-INF/i18n/hue.properties +++ b/bundles/org.openhab.binding.hue/src/main/resources/OH-INF/i18n/hue.properties @@ -232,7 +232,7 @@ offline.group-removed = Hue Bridge reports group as removed. # api v2 offline configuration error descriptions offline.api2.comm-error.zigbee-connectivity-issue = Zigbee connectivity issue. -offline.api2.comm-error.exception = An unexpected exception ''{0}'' occurred. +offline.api2.comm-error.exception = An unexpected exception occurred: {0} offline.api2.conf-error.certificate-load = Certificate loading failed. Please check your configuration settings (network address, type of certificate). offline.api2.conf-error.assets-not-loaded = Bridge/Thing handler assets not loaded. offline.api2.conf-error.press-pairing-button = Not authenticated. Press pairing button on the Hue Bridge or set a valid application key in configuration.