From aa7204455458a565322dcb5dce3977332cd264fa Mon Sep 17 00:00:00 2001 From: Christoph Weitkamp Date: Thu, 26 Nov 2020 23:14:20 +0100 Subject: [PATCH] [openhabcloud] Code improvements (#9131) * Code improvements * Add util method to create random alphanimeric string Signed-off-by: Christoph Weitkamp --- .../io/openhabcloud/NotificationAction.java | 14 +- .../io/openhabcloud/internal/CloudClient.java | 44 ++--- .../internal/CloudClientListener.java | 4 +- .../openhabcloud/internal/CloudService.java | 160 ++++++++---------- 4 files changed, 96 insertions(+), 126 deletions(-) diff --git a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java index 2a0851b2d48..f257d40e629 100644 --- a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java +++ b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/NotificationAction.java @@ -12,6 +12,8 @@ */ package org.openhab.io.openhabcloud; +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.core.model.script.engine.action.ActionDoc; import org.openhab.io.openhabcloud.internal.CloudService; import org.slf4j.Logger; @@ -23,14 +25,13 @@ import org.slf4j.LoggerFactory; * * @author Victor Belov - Initial contribution * @author Kai Kreuzer - migrated code to ESH APIs - * */ - +@NonNullByDefault public class NotificationAction { private static final Logger logger = LoggerFactory.getLogger(NotificationAction.class); - public static CloudService cloudService = null; + public static @Nullable CloudService cloudService; /** * Sends a simple push notification to mobile devices of user @@ -54,7 +55,8 @@ public class NotificationAction { * */ @ActionDoc(text = "Sends a push notification to mobile devices of user with userId") - public static void sendNotification(String userId, String message, String icon, String severity) { + public static void sendNotification(String userId, String message, @Nullable String icon, + @Nullable String severity) { logger.debug("sending notification '{}' to user {}", message, userId); if (cloudService != null) { cloudService.sendNotification(userId, message, icon, severity); @@ -83,7 +85,7 @@ public class NotificationAction { * */ @ActionDoc(text = "Sends a log notification which is shown in notifications log to all account users") - public static void sendLogNotification(String message, String icon, String severity) { + public static void sendLogNotification(String message, @Nullable String icon, @Nullable String severity) { logger.debug("sending log notification '{}'", message); if (cloudService != null) { cloudService.sendLogNotification(message, icon, severity); @@ -112,7 +114,7 @@ public class NotificationAction { * */ @ActionDoc(text = "Sends a push notification to mobile devices of user with userId") - public static void sendBroadcastNotification(String message, String icon, String severity) { + public static void sendBroadcastNotification(String message, @Nullable String icon, @Nullable String severity) { logger.debug("sending broadcast notification '{}' to all users", message); if (cloudService != null) { cloudService.sendBroadcastNotification(message, icon, severity); diff --git a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClient.java b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClient.java index 2d0a31386d5..742df823580 100644 --- a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClient.java +++ b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClient.java @@ -19,14 +19,14 @@ import java.net.URISyntaxException; import java.net.URL; import java.net.URLEncoder; import java.nio.ByteBuffer; -import java.util.Arrays; -import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; +import org.eclipse.jdt.annotation.Nullable; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Request.FailureListener; @@ -63,14 +63,12 @@ import io.socket.engineio.client.Transport; * * @author Victor Belov - Initial contribution * @author Kai Kreuzer - migrated code to new Jetty client and ESH APIs - * */ - public class CloudClient { /* * Logger for this class */ - private Logger logger = LoggerFactory.getLogger(CloudClient.class); + private final Logger logger = LoggerFactory.getLogger(CloudClient.class); /* * This variable holds base URL for the openHAB Cloud connections @@ -98,9 +96,9 @@ public class CloudClient { private final HttpClient jettyClient; /* - * This hashmap holds HTTP requests to local openHAB which are currently running + * This map holds HTTP requests to local openHAB which are currently running */ - private Map runningRequests; + private final Map runningRequests = new ConcurrentHashMap<>(); /* * This variable indicates if connection to the openHAB Cloud is currently in an established state @@ -147,7 +145,6 @@ public class CloudClient { this.localBaseUrl = localBaseUrl; this.remoteAccessEnabled = remoteAccessEnabled; this.exposedItems = exposedItems; - runningRequests = new HashMap<>(); this.jettyClient = httpClient; } @@ -176,11 +173,11 @@ public class CloudClient { logger.trace("Transport.EVENT_REQUEST_HEADERS"); @SuppressWarnings("unchecked") Map> headers = (Map>) args[0]; - headers.put("uuid", Arrays.asList(uuid)); - headers.put("secret", Arrays.asList(secret)); - headers.put("openhabversion", Arrays.asList(OpenHAB.getVersion())); - headers.put("clientversion", Arrays.asList(CloudService.clientVersion)); - headers.put("remoteaccess", Arrays.asList(((Boolean) remoteAccessEnabled).toString())); + headers.put("uuid", List.of(uuid)); + headers.put("secret", List.of(secret)); + headers.put("openhabversion", List.of(OpenHAB.getVersion())); + headers.put("clientversion", List.of(CloudService.clientVersion)); + headers.put("remoteaccess", List.of(((Boolean) remoteAccessEnabled).toString())); } }); } @@ -242,9 +239,7 @@ public class CloudClient { this.localBaseUrl); isConnected = false; // And clean up the list of running requests - if (runningRequests != null) { - runningRequests.clear(); - } + runningRequests.clear(); } /** @@ -294,7 +289,6 @@ public class CloudClient { JSONObject requestQueryJson = data.getJSONObject("query"); // Create URI builder with base request URI of openHAB and path from request String newPath = URIUtil.addPaths(localBaseUrl, requestPath); - @SuppressWarnings("unchecked") Iterator queryIterator = requestQueryJson.keys(); // Add query parameters to URI builder, if any newPath += "?"; @@ -345,7 +339,6 @@ public class CloudClient { } private void setRequestHeaders(Request request, JSONObject requestHeadersJson) { - @SuppressWarnings("unchecked") Iterator headersIterator = requestHeadersJson.keys(); // Convert JSONObject of headers into Header ArrayList while (headersIterator.hasNext()) { @@ -368,8 +361,8 @@ public class CloudClient { int requestId = data.getInt("id"); logger.debug("Received cancel for request {}", requestId); // Find and abort running request - if (runningRequests.containsKey(requestId)) { - Request request = runningRequests.get(requestId); + Request request = runningRequests.get(requestId); + if (request != null) { request.abort(new InterruptedException()); runningRequests.remove(requestId); } @@ -401,9 +394,8 @@ public class CloudClient { * @param message notification message text * @param icon name of the icon for this notification * @param severity severity name for this notification - * */ - public void sendNotification(String userId, String message, String icon, String severity) { + public void sendNotification(String userId, String message, @Nullable String icon, @Nullable String severity) { if (isConnected()) { JSONObject notificationMessage = new JSONObject(); try { @@ -426,9 +418,8 @@ public class CloudClient { * @param message notification message text * @param icon name of the icon for this notification * @param severity severity name for this notification - * */ - public void sendLogNotification(String message, String icon, String severity) { + public void sendLogNotification(String message, @Nullable String icon, @Nullable String severity) { if (isConnected()) { JSONObject notificationMessage = new JSONObject(); try { @@ -450,9 +441,8 @@ public class CloudClient { * @param message notification message text * @param icon name of the icon for this notification * @param severity severity name for this notification - * */ - public void sendBroadcastNotification(String message, String icon, String severity) { + public void sendBroadcastNotification(String message, @Nullable String icon, @Nullable String severity) { if (isConnected()) { JSONObject notificationMessage = new JSONObject(); try { @@ -621,8 +611,6 @@ public class CloudClient { } catch (JSONException e) { logger.debug("{}", e.getMessage()); } - } else { - // We should not send headers for the second time... } } } diff --git a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClientListener.java b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClientListener.java index fbca049806e..d3a0a4b8185 100644 --- a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClientListener.java +++ b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudClientListener.java @@ -12,6 +12,8 @@ */ package org.openhab.io.openhabcloud.internal; +import org.eclipse.jdt.annotation.NonNullByDefault; + /** * This interface provides callbacks from CloudClient * @@ -19,7 +21,7 @@ package org.openhab.io.openhabcloud.internal; * @author Kai Kreuzer - migrated code to ESH APIs * */ - +@NonNullByDefault public interface CloudClientListener { /** * This method receives command for an item from the openHAB Cloud client and should post it diff --git a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudService.java b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudService.java index 7f3f9999842..4e719bb5aeb 100644 --- a/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudService.java +++ b/bundles/org.openhab.io.openhabcloud/src/main/java/org/openhab/io/openhabcloud/internal/CloudService.java @@ -13,22 +13,18 @@ package org.openhab.io.openhabcloud.internal; import java.io.File; -import java.io.FileInputStream; import java.io.FileNotFoundException; -import java.io.FileOutputStream; import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.security.SecureRandom; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import org.apache.commons.io.IOUtils; -import org.apache.commons.lang.RandomStringUtils; -import org.apache.commons.lang.StringUtils; +import org.eclipse.jdt.annotation.Nullable; import org.eclipse.jetty.client.HttpClient; import org.openhab.core.OpenHAB; import org.openhab.core.config.core.ConfigurableService; @@ -59,8 +55,6 @@ import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; import org.osgi.service.component.annotations.Modified; import org.osgi.service.component.annotations.Reference; -import org.osgi.service.component.annotations.ReferenceCardinality; -import org.osgi.service.component.annotations.ReferencePolicy; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -84,21 +78,33 @@ public class CloudService implements ActionService, CloudClientListener, EventSu private static final int DEFAULT_LOCAL_OPENHAB_MAX_CONCURRENT_REQUESTS = 200; private static final int DEFAULT_LOCAL_OPENHAB_REQUEST_TIMEOUT = 30000; private static final String HTTPCLIENT_NAME = "openhabcloud"; + private static final String CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + private static final SecureRandom SR = new SecureRandom(); - private Logger logger = LoggerFactory.getLogger(CloudService.class); + private final Logger logger = LoggerFactory.getLogger(CloudService.class); public static String clientVersion = null; private CloudClient cloudClient; private String cloudBaseUrl = null; - private HttpClient httpClient; - protected ItemRegistry itemRegistry = null; - protected EventPublisher eventPublisher = null; + private final HttpClient httpClient; + protected final ItemRegistry itemRegistry; + protected final EventPublisher eventPublisher; private boolean remoteAccessEnabled = true; private Set exposedItems = null; private int localPort; - public CloudService() { + @Activate + public CloudService(final @Reference HttpClientFactory httpClientFactory, + final @Reference ItemRegistry itemRegistry, final @Reference EventPublisher eventPublisher) { + this.httpClient = httpClientFactory.createHttpClient(HTTPCLIENT_NAME); + this.httpClient.setStopTimeout(0); + this.httpClient.setMaxConnectionsPerDestination(DEFAULT_LOCAL_OPENHAB_MAX_CONCURRENT_REQUESTS); + this.httpClient.setConnectTimeout(DEFAULT_LOCAL_OPENHAB_REQUEST_TIMEOUT); + this.httpClient.setFollowRedirects(false); + + this.itemRegistry = itemRegistry; + this.eventPublisher = eventPublisher; } /** @@ -109,7 +115,7 @@ public class CloudService implements ActionService, CloudClientListener, EventSu * @param icon the {@link String} containing a name of the icon to be used with this notification * @param severity the {@link String} containing severity (good, info, warning, error) of notification */ - public void sendNotification(String userId, String message, String icon, String severity) { + public void sendNotification(String userId, String message, @Nullable String icon, @Nullable String severity) { logger.debug("Sending message '{}' to user id {}", message, userId); cloudClient.sendNotification(userId, message, icon, severity); } @@ -122,7 +128,7 @@ public class CloudService implements ActionService, CloudClientListener, EventSu * @param icon the {@link String} containing a name of the icon to be used with this notification * @param severity the {@link String} containing severity (good, info, warning, error) of notification */ - public void sendLogNotification(String message, String icon, String severity) { + public void sendLogNotification(String message, @Nullable String icon, @Nullable String severity) { logger.debug("Sending log message '{}'", message); cloudClient.sendLogNotification(message, icon, severity); } @@ -135,14 +141,19 @@ public class CloudService implements ActionService, CloudClientListener, EventSu * @param icon the {@link String} containing a name of the icon to be used with this notification * @param severity the {@link String} containing severity (good, info, warning, error) of notification */ - public void sendBroadcastNotification(String message, String icon, String severity) { + public void sendBroadcastNotification(String message, @Nullable String icon, @Nullable String severity) { logger.debug("Sending broadcast message '{}' to all users", message); cloudClient.sendBroadcastNotification(message, icon, severity); } + private String substringBefore(String str, String separator) { + int index = str.indexOf(separator); + return index == -1 ? str : str.substring(0, index); + } + @Activate protected void activate(BundleContext context, Map config) { - clientVersion = StringUtils.substringBefore(context.getBundle().getVersion().toString(), ".qualifier"); + clientVersion = substringBefore(context.getBundle().getVersion().toString(), ".qualifier"); localPort = HttpServiceUtil.getHttpServicePort(context); if (localPort == -1) { logger.warn("openHAB Cloud connector not started, since no local HTTP port could be determined"); @@ -221,9 +232,6 @@ public class CloudService implements ActionService, CloudClientListener, EventSu cloudClient.shutdown(); } - httpClient.setMaxConnectionsPerDestination(DEFAULT_LOCAL_OPENHAB_MAX_CONCURRENT_REQUESTS); - httpClient.setConnectTimeout(DEFAULT_LOCAL_OPENHAB_REQUEST_TIMEOUT); - httpClient.setFollowRedirects(false); if (!httpClient.isRunning()) { try { httpClient.start(); @@ -257,12 +265,12 @@ public class CloudService implements ActionService, CloudClientListener, EventSu private String readFirstLine(File file) { List lines = null; - try (InputStream fis = new FileInputStream(file)) { - lines = IOUtils.readLines(fis); - } catch (IOException ioe) { + try { + lines = Files.readAllLines(file.toPath(), StandardCharsets.UTF_8); + } catch (IOException e) { // no exception handling - we just return the empty String } - return lines != null && !lines.isEmpty() ? lines.get(0) : ""; + return lines == null || lines.isEmpty() ? "" : lines.get(0); } /** @@ -272,8 +280,8 @@ public class CloudService implements ActionService, CloudClientListener, EventSu private void writeFile(File file, String content) { // create intermediary directories file.getParentFile().mkdirs(); - try (OutputStream fos = new FileOutputStream(file)) { - IOUtils.write(content, fos); + try { + Files.writeString(file.toPath(), content, StandardCharsets.UTF_8); logger.debug("Created file '{}' with content '{}'", file.getAbsolutePath(), content); } catch (FileNotFoundException e) { logger.error("Couldn't create file '{}'.", file.getPath(), e); @@ -282,6 +290,14 @@ public class CloudService implements ActionService, CloudClientListener, EventSu } } + private String randomString(int length) { + StringBuilder sb = new StringBuilder(length); + for (int i = 0; i < length; i++) { + sb.append(CHARS.charAt(SR.nextInt(CHARS.length()))); + } + return sb.toString(); + } + /** * Creates a random secret and writes it to the userdata/openhabcloud * directory. An existing secret file won't be overwritten. @@ -292,7 +308,7 @@ public class CloudService implements ActionService, CloudClientListener, EventSu String newSecretString = ""; if (!file.exists()) { - newSecretString = RandomStringUtils.randomAlphanumeric(20); + newSecretString = randomString(20); logger.debug("New secret = {}", newSecretString); writeFile(file, newSecretString); } else { @@ -306,77 +322,39 @@ public class CloudService implements ActionService, CloudClientListener, EventSu @Override public void sendCommand(String itemName, String commandString) { try { - if (itemRegistry != null) { - Item item = itemRegistry.getItem(itemName); - Command command = null; - if (item != null) { - if (this.eventPublisher != null) { - if ("toggle".equalsIgnoreCase(commandString) - && (item instanceof SwitchItem || item instanceof RollershutterItem)) { - if (OnOffType.ON.equals(item.getStateAs(OnOffType.class))) { - command = OnOffType.OFF; - } - if (OnOffType.OFF.equals(item.getStateAs(OnOffType.class))) { - command = OnOffType.ON; - } - if (UpDownType.UP.equals(item.getStateAs(UpDownType.class))) { - command = UpDownType.DOWN; - } - if (UpDownType.DOWN.equals(item.getStateAs(UpDownType.class))) { - command = UpDownType.UP; - } - } else { - command = TypeParser.parseCommand(item.getAcceptedCommandTypes(), commandString); - } - if (command != null) { - logger.debug("Received command '{}' for item '{}'", commandString, itemName); - this.eventPublisher.post(ItemEventFactory.createCommandEvent(itemName, command)); - } else { - logger.warn("Received invalid command '{}' for item '{}'", commandString, itemName); - } - } - } else { - logger.warn("Received command '{}' for non-existent item '{}'", commandString, itemName); + Item item = itemRegistry.getItem(itemName); + Command command = null; + if ("toggle".equalsIgnoreCase(commandString) + && (item instanceof SwitchItem || item instanceof RollershutterItem)) { + if (OnOffType.ON.equals(item.getStateAs(OnOffType.class))) { + command = OnOffType.OFF; + } + if (OnOffType.OFF.equals(item.getStateAs(OnOffType.class))) { + command = OnOffType.ON; + } + if (UpDownType.UP.equals(item.getStateAs(UpDownType.class))) { + command = UpDownType.DOWN; + } + if (UpDownType.DOWN.equals(item.getStateAs(UpDownType.class))) { + command = UpDownType.UP; } } else { - return; + command = TypeParser.parseCommand(item.getAcceptedCommandTypes(), commandString); + } + if (command != null) { + logger.debug("Received command '{}' for item '{}'", commandString, itemName); + eventPublisher.post(ItemEventFactory.createCommandEvent(itemName, command)); + } else { + logger.warn("Received invalid command '{}' for item '{}'", commandString, itemName); } } catch (ItemNotFoundException e) { - logger.warn("Received command for a non-existent item '{}'", itemName); + logger.warn("Received command '{}' for a non-existent item '{}'", commandString, itemName); } } - @Reference - protected void setHttpClientFactory(HttpClientFactory httpClientFactory) { - this.httpClient = httpClientFactory.createHttpClient(HTTPCLIENT_NAME); - this.httpClient.setStopTimeout(0); - } - - protected void unsetHttpClientFactory(HttpClientFactory httpClientFactory) { - this.httpClient = null; - } - - @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.DYNAMIC) - public void setItemRegistry(ItemRegistry itemRegistry) { - this.itemRegistry = itemRegistry; - } - - public void unsetItemRegistry(ItemRegistry itemRegistry) { - this.itemRegistry = null; - } - - @Reference(cardinality = ReferenceCardinality.OPTIONAL, policy = ReferencePolicy.DYNAMIC) - public void setEventPublisher(EventPublisher eventPublisher) { - this.eventPublisher = eventPublisher; - } - - public void unsetEventPublisher(EventPublisher eventPublisher) { - this.eventPublisher = null; - } - @Override public Set getSubscribedEventTypes() { - return Collections.singleton(ItemStateEvent.TYPE); + return Set.of(ItemStateEvent.TYPE); } @Override