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
 
@@ -94,8 +95,9 @@ 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) |
+| withMessage | (String message) | Set the main message text. | [publish](https://docs.ntfy.sh/publish) |
+| withPriority | (int priority) | Set message priority (1-5). | [priority](https://ntfy.sh/docs/publish/#priority) |
+| withTitle | (String title) | Set title. | [title](https://docs.ntfy.sh/publish/#message-title) |
 | 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

@@ -18,6 +18,7 @@
 import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.jetty.websocket.client.ClientUpgradeRequest;
 import org.eclipse.jetty.websocket.client.WebSocketClient;
 import org.openhab.binding.ntfy.internal.network.NtfyWebSocket;
@@ -31,6 +32,7 @@
 import org.openhab.core.thing.binding.BaseBridgeHandler;
 import org.openhab.core.thing.util.ThingWebClientUtil;
 import org.openhab.core.types.Command;
+import org.osgi.service.cm.ConfigurationException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -93,6 +95,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 +132,30 @@ public synchronized boolean startWebSocketConnection(Thing topicThing, NtfyWebSo
                 client.start();
                 client.setMaxIdleTimeout(config.connectionTimeout);
 
+                @Nullable
+                String scheme = (new URI(config.hostname)).getScheme();
+                if (scheme == null || scheme.isEmpty()) {
+                    throw new ConfigurationException("hostname", "URI scheme is missing in hostname" + config.hostname);
+                }
+
+                String webSocketScheme = "wss";
+                switch (scheme) {
+                    case "http":
+                        webSocketScheme = "ws";
+                        break;
+                    case "https":
+                        webSocketScheme = "wss";
+                        break;
+                    default:
+                        throw new IllegalArgumentException(
+                                "Unsupported URI scheme: " + scheme + " - 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());
@@ -154,6 +177,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);
     }
 }