[openhabcloud] Code improvements (#9131)

* Code improvements
* Add util method to create random alphanimeric string

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

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);

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<Integer, Request> runningRequests;
+    private final Map<Integer, Request> 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<String, List<String>> headers = (Map<String, List<String>>) args[0];
-                        headers.put("uuid", Arrays.asList(uuid));
+                        headers.put("uuid", List.of(uuid));
-                        headers.put("secret", Arrays.asList(secret));
+                        headers.put("secret", List.of(secret));
-                        headers.put("openhabversion", Arrays.asList(OpenHAB.getVersion()));
+                        headers.put("openhabversion", List.of(OpenHAB.getVersion()));
-                        headers.put("clientversion", Arrays.asList(CloudService.clientVersion));
+                        headers.put("clientversion", List.of(CloudService.clientVersion));
-                        headers.put("remoteaccess", Arrays.asList(((Boolean) remoteAccessEnabled).toString()));
+                        headers.put("remoteaccess", List.of(((Boolean) remoteAccessEnabled).toString()));
                     }
                 });
             }
@@ -242,10 +239,8 @@ public class CloudClient {
                 this.localBaseUrl);
         isConnected = false;
         // And clean up the list of running requests
-        if (runningRequests != null) {
         runningRequests.clear();
     }
-    }
 
     /**
      * Callback method for socket.io client which is called when an error occurs
@@ -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<String> 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<String> 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);
+            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...
             }
         }
     }

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

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.nio.charset.StandardCharsets;
-import java.io.OutputStream;
+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.eclipse.jdt.annotation.Nullable;
-import org.apache.commons.lang.RandomStringUtils;
-import org.apache.commons.lang.StringUtils;
 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;
+    private final HttpClient httpClient;
-    protected ItemRegistry itemRegistry = null;
+    protected final ItemRegistry itemRegistry;
-    protected EventPublisher eventPublisher = null;
+    protected final EventPublisher eventPublisher;
 
     private boolean remoteAccessEnabled = true;
     private Set<String> 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<String, ?> 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<String> lines = null;
-        try (InputStream fis = new FileInputStream(file)) {
+        try {
-            lines = IOUtils.readLines(fis);
+            lines = Files.readAllLines(file.toPath(), StandardCharsets.UTF_8);
-        } catch (IOException ioe) {
+        } 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)) {
+        try {
-            IOUtils.write(content, fos);
+            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 <code>userdata/openhabcloud</code>
      * directory. An existing <code>secret</code> 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,11 +322,8 @@ 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))) {
@@ -330,53 +343,18 @@ public class CloudService implements ActionService, CloudClientListener, EventSu
             }
             if (command != null) {
                 logger.debug("Received command '{}' for item '{}'", commandString, itemName);
-                            this.eventPublisher.post(ItemEventFactory.createCommandEvent(itemName, command));
+                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);
-                }
-            } else {
-                return;
-            }
         } 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<String> getSubscribedEventTypes() {
-        return Collections.singleton(ItemStateEvent.TYPE);
+        return Set.of(ItemStateEvent.TYPE);
     }
 
     @Override