code review changes

Signed-off-by: Christian Kittel <ckittel@gmx.de>

Author: EvilPingu

bundles/org.openhab.binding.ntfy/README.md

@@ -21,7 +21,7 @@ This binding provides the following Thing types. The Thing type IDs match the de
 
 | Name              | Type    | Description                             | Default         | Required | Advanced |
 |-------------------|---------|-----------------------------------------|-----------------|----------|----------|
-| hostname          | text    | Hostname or base URL of the ntfy server | [https://ntfy.sh](https://ntfy.sh) | yes      | no       |
+| hostname          | text    | Base URL of the ntfy server | [https://ntfy.sh](https://ntfy.sh) | yes      | no       |
 | username          | text    | Optional username for basic auth        | N/A             | no       | no       |
 | password          | text    | Optional password for basic auth        | N/A             | no       | no       |
 | connectionTimeout | integer | WebSocket / HTTP connection timeout ms  | 60000           | no       | yes      |
@@ -38,10 +38,11 @@ Configure the `ntfyConnection` as a bridge to hold shared server and authenticat
 
 ### `ntfyTopic` Channels
 
-| Channel     | Type     | Read/Write | Description                                 |
-|-------------|----------|------------|---------------------------------------------|
-| lastMessage | String   | R          | Last received message payload (read-only)   |
-| messageTime | DateTime | R          | Timestamp of the last message (read-only)   |
+| Channel         | Type     | Read/Write | Description                                 |
+|-----------------|----------|------------|---------------------------------------------|
+| lastMessage     | String   | R          | Last received message payload (read-only)   |
+| lastMessageTime | DateTime | R          | Timestamp of the last message (read-only)   |
+| lastMessageId   | String   | R          | ID of the last message (read-only)          |
 
 ## Full Example
 
@@ -95,7 +96,7 @@ Below is a quick reference of the builder-style methods provided by the binding
 | Method | Parameters | Description | Ntfy docs |
 |--------|------------|-------------|----------|
 | withMessage | (String message) | Set the main message text. | [publish](https://docs.ntfy.sh/publish/#message-title) |
-| withPriority | (int priority) | Set message priority (0-5). | [priority](https://ntfy.sh/docs/publish/#priority) |
+| withPriority | (int priority) | Set message priority (1-5). | [priority](https://ntfy.sh/docs/publish/#priority) |
 | withTag | (String tag) | Add a tag to the message. | [tags](https://ntfy.sh/docs/publish/#tags) |
 | withIcon | (String url) | Set an icon URL for the notification. | [icons](https://ntfy.sh/docs/publish/#icons) |
 | withAttachment | (String url, String filename) | Attach a file or resource URL with optional filename. | [attachments](https://docs.ntfy.sh/publish/#attach-file-from-a-url) |

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/NtfyBindingConstants.java

@@ -33,5 +33,5 @@ public class NtfyBindingConstants {
     // List of all Channel ids
     public static final String CHANNEL_NTFY_LASTMESSAGE = "lastMessage";
     public static final String CHANNEL_NTFY_LASTMESSAGETIME = "lastMessageTime";
-    public static final String CHANNEL_NTFY_LASTMESSAGEID = "lastMessageMessageId";
+    public static final String CHANNEL_NTFY_LASTMESSAGEID = "lastMessageId";
 }

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/NtfyConnectionConfiguration.java

@@ -27,7 +27,7 @@
 public class NtfyConnectionConfiguration {
 
     /**
-     * Hostname of the ntfy server, e.g. "ntfy.sh"
+     * Hostname of the ntfy server, e.g. "https://ntfy.sh"
      */
     public String hostname = "";
 

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/NtfyConnectionHandler.java

@@ -93,6 +93,7 @@ public synchronized void createAndRegisterWebSocketClient(Thing thing) {
             } catch (Exception e) {
                 logger.error("Error stopping WebSocketConnection", e);
             }
+            webSocketConnections.remove(thing.getUID());
         }
 
         webSocketConnections.put(thing.getUID(), newClient);
@@ -129,10 +130,24 @@ public synchronized boolean startWebSocketConnection(Thing topicThing, NtfyWebSo
                 client.start();
                 client.setMaxIdleTimeout(config.connectionTimeout);
 
+                String webSocketScheme = "wss";
+                switch ((new URI(config.hostname)).getScheme()) {
+                    case "http":
+                        webSocketScheme = "ws";
+                        break;
+                    case "https":
+                        webSocketScheme = "wss";
+                        break;
+                    default:
+                        throw new IllegalArgumentException("Unsupported URI scheme: "
+                                + (new URI(config.hostname)).getScheme() + " - only http and https are supported");
+                }
+
                 client.connect(ntfyWebSocket,
-                        new URI("wss:"
+                        new URI(webSocketScheme + ":"
                                 + (new URI(config.hostname + "/" + topicName + "/ws")).getRawSchemeSpecificPart()),
                         setupRequest());
+                updateStatus(ThingStatus.ONLINE);
             } catch (Exception e) {
                 logger.error("Error creating WebSocketConnection", e);
                 updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getLocalizedMessage());
@@ -142,6 +157,22 @@ public synchronized boolean startWebSocketConnection(Thing topicThing, NtfyWebSo
         return true;
     }
 
+    private synchronized void restartRegisteredWebSocketClients() {
+        for (Map.Entry<ThingUID, WebSocketClient> entry : webSocketConnections.entrySet()) {
+            WebSocketClient client = entry.getValue();
+            try {
+                if (client.isStarted()) {
+                    client.stop();
+                }
+                client.start();
+                updateStatus(ThingStatus.ONLINE);
+
+            } catch (Exception e) {
+                logger.debug("Error restarting WebSocketConnection for thing {}", entry.getKey(), e);
+            }
+        }
+    }
+
     private String getTopicNameFromThing(Thing topicThing) {
         return topicThing.getConfiguration().as(NtfyTopicConfiguration.class).topicname;
     }
@@ -154,6 +185,21 @@ private String getTopicNameFromThing(Thing topicThing) {
      */
     public void connectionError(Throwable cause) {
         updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, cause.getLocalizedMessage());
-        scheduler.schedule(() -> updateStatus(ThingStatus.ONLINE), 30, TimeUnit.SECONDS);
+        scheduler.schedule(() -> updateStatus(ThingStatus.UNKNOWN), 30, TimeUnit.SECONDS);
+    }
+
+    @Override
+    public void dispose() {
+        super.dispose();
+
+        webSocketConnections.values().forEach(client -> {
+            try {
+                if (client.isRunning()) {
+                    client.stop();
+                }
+            } catch (Exception e) {
+                logger.error("Error stopping WebSocketConnection", e);
+            }
+        });
     }
 }

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/NtfyTopicHandler.java

@@ -28,6 +28,7 @@
 import org.openhab.binding.ntfy.internal.network.WebSocketConnectionListener;
 import org.openhab.core.library.types.DateTimeType;
 import org.openhab.core.library.types.StringType;
+import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingStatus;
@@ -48,9 +49,10 @@
 public class NtfyTopicHandler extends BaseThingHandler implements WebSocketConnectionListener {
 
     private NtfyWebSocket ntfyWebSocket;
-    private NtfySender ntfySender;
+    private @Nullable NtfySender ntfySender;
     private boolean isInitializing;
     private @Nullable MessageEvent lastMessageEvent;
+    private HttpClient httpClient;
 
     /**
      * Creates a new {@link NtfyTopicHandler} for the provided topic thing.
@@ -61,14 +63,19 @@ public class NtfyTopicHandler extends BaseThingHandler implements WebSocketConne
     public NtfyTopicHandler(Thing thing, HttpClient httpClient) {
         super(thing);
 
+        this.httpClient = httpClient;
         this.ntfyWebSocket = new NtfyWebSocket(this);
-        this.ntfySender = new NtfySender(this.getConfigAs(NtfyTopicConfiguration.class).topicname, httpClient,
-                this::getBridgeHandler);
     }
 
-    private NtfyConnectionHandler getBridgeHandler() {
-        return (NtfyConnectionHandler) (java.util.Objects
-                .requireNonNull(java.util.Objects.requireNonNull(getBridge()).getHandler()));
+    private @Nullable NtfyConnectionHandler getBridgeHandler() {
+        @Nullable
+        Bridge bridge = getBridge();
+
+        if (bridge == null) {
+            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
+            return null;
+        }
+        return (NtfyConnectionHandler) (bridge.getHandler());
     }
 
     @Override
@@ -80,12 +87,22 @@ public Collection<Class<? extends ThingHandlerService>> getServices() {
     public void thingUpdated(Thing thing) {
         super.thingUpdated(thing);
 
-        getBridgeHandler().createAndRegisterWebSocketClient(thing);
+        @Nullable
+        NtfyConnectionHandler bridgeHandler = getBridgeHandler();
+
+        if (bridgeHandler == null) {
+            return;
+        }
+
+        ntfySender = new NtfySender(this.getConfigAs(NtfyTopicConfiguration.class).topicname, httpClient,
+                this::getBridgeHandler);
+        bridgeHandler.createAndRegisterWebSocketClient(thing);
+        startConnection();
     }
 
     @Override
     public void bridgeStatusChanged(ThingStatusInfo bridgeStatusInfo) {
-        if (bridgeStatusInfo.getStatus() == ThingStatus.ONLINE
+        if ((bridgeStatusInfo.getStatus() == ThingStatus.ONLINE || bridgeStatusInfo.getStatus() == ThingStatus.UNKNOWN)
                 && getThing().getStatusInfo().getStatusDetail() == ThingStatusDetail.BRIDGE_OFFLINE) {
             updateStatus(ThingStatus.UNKNOWN);
             initialize();
@@ -112,7 +129,14 @@ public void initialize() {
 
         scheduler.execute(() -> {
             try {
-                getBridgeHandler().createAndRegisterWebSocketClient(thing);
+                ntfySender = new NtfySender(this.getConfigAs(NtfyTopicConfiguration.class).topicname, httpClient,
+                        this::getBridgeHandler);
+                @Nullable
+                NtfyConnectionHandler bridgeHandler = getBridgeHandler();
+                if (bridgeHandler == null) {
+                    return;
+                }
+                bridgeHandler.createAndRegisterWebSocketClient(thing);
                 startConnection();
             } finally {
                 this.isInitializing = false;
@@ -122,7 +146,12 @@ public void initialize() {
 
     private void startConnection() {
         if (getThing().getStatus() != ThingStatus.ONLINE) {
-            if (!getBridgeHandler().startWebSocketConnection(thing, ntfyWebSocket)) {
+            @Nullable
+            NtfyConnectionHandler bridgeHandler = getBridgeHandler();
+            if (bridgeHandler == null) {
+                return;
+            }
+            if (!bridgeHandler.startWebSocketConnection(thing, ntfyWebSocket)) {
                 updateStatus(ThingStatus.OFFLINE);
             }
         }
@@ -135,15 +164,22 @@ public void connectionEstablished() {
 
     @Override
     public void connectionLost(String reason) {
+        updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
+                reason.isBlank() ? "WebSocket connection lost" : reason);
     }
 
     @Override
     public void connectionError(Throwable cause) {
-        getBridgeHandler().connectionError(cause);
+        @Nullable
+        NtfyConnectionHandler bridgeHandler = getBridgeHandler();
+        if (bridgeHandler == null) {
+            return;
+        }
+        bridgeHandler.connectionError(cause);
     }
 
     @Override
-    public void messageRecieved(BaseEvent event) {
+    public void messageReceived(BaseEvent event) {
         if (event instanceof MessageEvent message) {
             this.lastMessageEvent = message;
             updateChannels();
@@ -169,19 +205,40 @@ private void updateChannels() {
      * @throws URISyntaxException
      */
     public String sendMessage(NtfyMessage ntfyMessage) throws URISyntaxException {
+        final @Nullable NtfySender sender = ntfySender;
+        if (sender == null) {
+            return "";
+        }
+
         @Nullable
-        MessageEvent sendMessage = ntfySender.sendMessage(ntfyMessage);
+        MessageEvent sendMessage = sender.sendMessage(ntfyMessage);
 
         if (sendMessage == null) {
             return "";
         }
         return sendMessage.getId();
     }
 
+    /**
+     * Uploads a local file to the configured topic via the underlying
+     * {@link NtfySender#sendFile(String, String, String)} and returns the
+     * created message id on success.
+     *
+     * @param file the filesystem path to the file to upload
+     * @param filename optional filename to present to recipients (may be null)
+     * @param sequenceId optional sequence id to associate with the uploaded message
+     * @return the created message id on success, or an empty string on failure
+     * @throws URISyntaxException when the constructed request URI is invalid
+     */
     public String sendFile(String file, @Nullable String filename, @Nullable String sequenceId)
             throws URISyntaxException {
+        final @Nullable NtfySender sender = ntfySender;
+        if (sender == null) {
+            return "";
+        }
+
         @Nullable
-        MessageEvent sendMessage = ntfySender.sendFile(file, filename, sequenceId);
+        MessageEvent sendMessage = sender.sendFile(file, filename, sequenceId);
 
         if (sendMessage == null) {
             return "";
@@ -197,6 +254,10 @@ public String sendFile(String file, @Nullable String filename, @Nullable String
      * @throws URISyntaxException when the underlying request URI could not be constructed
      */
     public boolean deleteMessage(String sequenceId) throws URISyntaxException {
-        return ntfySender.deleteMessage(sequenceId);
+        final @Nullable NtfySender sender = ntfySender;
+        if (sender == null) {
+            return false;
+        }
+        return sender.deleteMessage(sequenceId);
     }
 }

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/action/NtfyActions.java

@@ -267,13 +267,13 @@ public static ThingActions withViewAction(ThingActions actions, String label, Bo
      * @param actions the ThingActions instance
      * @param label the action label
      * @param clearNotification whether to clear the notification
-     * @param url the value to copy
+     * @param value the value to copy
      * @return the modified ThingActions builder instance
      * @throws MalformedURLException never thrown by this wrapper but declared for API compatibility
      */
-    public static ThingActions withCopyAction(ThingActions actions, String label, Boolean clearNotification, String url)
-            throws MalformedURLException {
-        return ((NtfyActions) actions).withCopyAction(label, clearNotification, url);
+    public static ThingActions withCopyAction(ThingActions actions, String label, Boolean clearNotification,
+            String value) throws MalformedURLException {
+        return ((NtfyActions) actions).withCopyAction(label, clearNotification, value);
     }
 
     /**
@@ -412,6 +412,12 @@ public static ThingActions withSequenceId(ThingActions actions, @Nullable String
         if (handler == null) {
             return "";
         }
+
+        if (!ntfyMessage.hasMessage() || ntfyMessage.getMessage().isBlank()) {
+            throw new IllegalStateException(
+                    "Cannot send message without content. Please set a message via withMessage() before sending.");
+        }
+
         return handler.sendMessage(ntfyMessage);
     }
 

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/network/GsonDeserializer.java

@@ -20,6 +20,7 @@
 
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
+import com.google.gson.JsonSyntaxException;
 
 /**
  * Helper wrapper around Gson used to deserialize incoming ntfy JSON messages
@@ -39,7 +40,7 @@ public class GsonDeserializer {
      * @param message the raw JSON message
      * @return the deserialized {@link BaseEvent} or {@code null} when parsing failed
      */
-    public static @Nullable BaseEvent deserialize(String message) {
+    public static @Nullable BaseEvent deserialize(String message) throws JsonSyntaxException {
         return GSON.fromJson(message, BaseEvent.class);
     }
 

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/network/HttpActionButton.java

@@ -42,9 +42,10 @@ public class HttpActionButton extends ActionButtonBase {
      * @param headers optional headers to include (may be null)
      * @param body optional request body (may be null)
      * @throws MalformedURLException when the provided URL is not a valid URL
+     * @throws IllegalArgumentException when the provided URL is not valid
      */
     public HttpActionButton(String label, Boolean clearNotification, String url, @Nullable String method,
-            @Nullable String headers, @Nullable String body) throws MalformedURLException {
+            @Nullable String headers, @Nullable String body) throws IllegalArgumentException, MalformedURLException {
         super(label, clearNotification);
 
         this.url = URI.create(url).toURL();

bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/network/NtfyMessage.java

@@ -15,7 +15,7 @@
 import java.net.MalformedURLException;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Set;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
@@ -31,8 +31,8 @@ public class NtfyMessage {
 
     private @Nullable String message;
     private int priority = 3;
-    private HashSet<String> tags = new HashSet<>();
-    private HashSet<ActionButtonBase> actions = new HashSet<>();
+    private LinkedHashSet<String> tags = new LinkedHashSet<>();
+    private LinkedHashSet<ActionButtonBase> actions = new LinkedHashSet<>();
     private @Nullable URI clickAction;
     private @Nullable URI icon;
     private @Nullable URI attachment;
@@ -76,7 +76,7 @@ public boolean hasMessage() {
      */
     public void setPriority(int priority) {
         if (priority < 1 || priority > 5) {
-            throw new IllegalArgumentException();
+            throw new IllegalArgumentException("Invalid priority " + priority + ". Allowed range is 1..5.");
         }
         this.priority = priority;
     }