Skip to content

Commit bd91621

Browse files
EvilPinguEvilPingu
committed
code review changes
Signed-off-by: Christian Kittel <ckittel@gmx.de>
1 parent e4107d8 commit bd91621

25 files changed

Lines changed: 550 additions & 166 deletions

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

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@ This binding provides the following Thing types. The Thing type IDs match the de
1919

2020
### `ntfyConnection` Bridge Configuration
2121

22-
| Name | Type | Description | Default | Required | Advanced |
23-
|-------------------|---------|-----------------------------------------|-----------------|----------|----------|
24-
| hostname | text | Hostname or base URL of the ntfy server | [https://ntfy.sh](https://ntfy.sh) | yes | no |
25-
| username | text | Optional username for basic auth | N/A | no | no |
26-
| password | text | Optional password for basic auth | N/A | no | no |
27-
| connectionTimeout | integer | WebSocket / HTTP connection timeout ms | 60000 | no | yes |
22+
| Name | Type | Description | Default | Required | Advanced |
23+
|-------------------|---------|-----------------------------------------|------------------------------------|----------|----------|
24+
| hostname | text | Base URL of the ntfy server | [https://ntfy.sh](https://ntfy.sh) | yes | no |
25+
| username | text | Optional username for basic auth | N/A | no | no |
26+
| password | text | Optional password for basic auth | N/A | no | no |
27+
| connectionTimeout | integer | WebSocket / HTTP connection timeout ms | 60000 | no | yes |
2828

2929
Configure the `ntfyConnection` as a bridge to hold shared server and authentication settings. Topic Things reference the bridge to reuse these settings.
3030

@@ -38,10 +38,11 @@ Configure the `ntfyConnection` as a bridge to hold shared server and authenticat
3838

3939
### `ntfyTopic` Channels
4040

41-
| Channel | Type | Read/Write | Description |
42-
|-------------|----------|------------|---------------------------------------------|
43-
| lastMessage | String | R | Last received message payload (read-only) |
44-
| messageTime | DateTime | R | Timestamp of the last message (read-only) |
41+
| Channel | Type | Read/Write | Description |
42+
|-----------------|----------|------------|---------------------------------------------|
43+
| lastMessage | String | R | Last received message payload (read-only) |
44+
| lastMessageTime | DateTime | R | Timestamp of the last message (read-only) |
45+
| lastMessageId | String | R | ID of the last message (read-only) |
4546

4647
## Full Example
4748

@@ -92,22 +93,23 @@ Important: Always check that the returned actions object is not null (the Thing
9293

9394
Below is a quick reference of the builder-style methods provided by the binding actions. Use these to construct messages before calling `send()`.
9495

95-
| Method | Parameters | Description | Ntfy docs |
96-
|--------|------------|-------------|----------|
97-
| withMessage | (String message) | Set the main message text. | [publish](https://docs.ntfy.sh/publish/#message-title) |
98-
| withPriority | (int priority) | Set message priority (0-5). | [priority](https://ntfy.sh/docs/publish/#priority) |
99-
| withTag | (String tag) | Add a tag to the message. | [tags](https://ntfy.sh/docs/publish/#tags) |
100-
| withIcon | (String url) | Set an icon URL for the notification. | [icons](https://ntfy.sh/docs/publish/#icons) |
101-
| 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) |
102-
| withViewAction | (String label, Boolean clearNotification, String url) | Add a view action (opens URL) with optional clear flag. | [actions](https://docs.ntfy.sh/publish/#open-websiteapp) |
103-
| withCopyAction | (String label, Boolean clearNotification, String value) | Add an action that copies text to clipboard on the client. | [actions](https://docs.ntfy.sh/publish/#copy-to-clipboard) |
104-
| withHttpAction | (String label, Boolean clearNotification, String url, String method, String headers, String body) | Add an HTTP action with optional method, headers and body. | [actions](https://docs.ntfy.sh/publish/#send-http-request) |
105-
| withBroadcastAction | (String label, Boolean clearNotification, String params) | Add a broadcast action to trigger local apps/handlers. | [actions](https://docs.ntfy.sh/publish/#send-android-broadcast) |
106-
| withDelay | (String delay) | Assign a delay. | [delay](https://docs.ntfy.sh/publish/#scheduled-delivery) |
107-
| withSequenceId | (String sequenceId) | Assign a sequence id for later reference (useful for delete/update). | [sequence id](https://docs.ntfy.sh/publish/#updating-deleting-notifications) |
108-
| send | () | Send the built message; returns a message ID string. | [publish](https://ntfy.sh/docs/publish/) |
109-
| send | (String file, String filename, String sequenceId) | Send a file; returns a message ID string. | [publish_local_file](https://docs.ntfy.sh/publish/#attach-local-file) |
110-
| delete | (String sequenceId) | Delete a message previously sent with the given sequence id. | [delete](https://ntfy.sh/docs/publish/#deleting-notifications) |
96+
| Method | Parameters | Description | Ntfy docs |
97+
|---------------------|---------------------------------------------------------------------------------------------------|----------------------------------------------------------------------|------------------------------------------------------------------------------|
98+
| withMessage | (String message) | Set the main message text. | [publish](https://docs.ntfy.sh/publish) |
99+
| withPriority | (int priority) | Set message priority (1-5). | [priority](https://ntfy.sh/docs/publish/#priority) |
100+
| withTitle | (String title) | Set title. | [title](https://docs.ntfy.sh/publish/#message-title) |
101+
| withTag | (String tag) | Add a tag to the message. | [tags](https://ntfy.sh/docs/publish/#tags) |
102+
| withIcon | (String url) | Set an icon URL for the notification. | [icons](https://ntfy.sh/docs/publish/#icons) |
103+
| 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) |
104+
| withViewAction | (String label, Boolean clearNotification, String url) | Add a view action (opens URL) with optional clear flag. | [actions](https://docs.ntfy.sh/publish/#open-websiteapp) |
105+
| withCopyAction | (String label, Boolean clearNotification, String value) | Add an action that copies text to clipboard on the client. | [actions](https://docs.ntfy.sh/publish/#copy-to-clipboard) |
106+
| withHttpAction | (String label, Boolean clearNotification, String url, String method, String headers, String body) | Add an HTTP action with optional method, headers and body. | [actions](https://docs.ntfy.sh/publish/#send-http-request) |
107+
| withBroadcastAction | (String label, Boolean clearNotification, String params) | Add a broadcast action to trigger local apps/handlers. | [actions](https://docs.ntfy.sh/publish/#send-android-broadcast) |
108+
| withDelay | (String delay) | Assign a delay. | [delay](https://docs.ntfy.sh/publish/#scheduled-delivery) |
109+
| withSequenceId | (String sequenceId) | Assign a sequence id for later reference (useful for delete/update). | [sequence id](https://docs.ntfy.sh/publish/#updating-deleting-notifications) |
110+
| send | () | Send the built message; returns a message ID string. | [publish](https://ntfy.sh/docs/publish/) |
111+
| send | (String file, String filename, String sequenceId) | Send a file; returns a message ID string. | [publish_local_file](https://docs.ntfy.sh/publish/#attach-local-file) |
112+
| delete | (String sequenceId) | Delete a message previously sent with the given sequence id. | [delete](https://ntfy.sh/docs/publish/#deleting-notifications) |
111113

112114
For more information about ntfy features and the notification format, see the ntfy project documentation: [https://ntfy.sh/docs/](https://ntfy.sh/docs/)
113115

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,11 @@ public class NtfyBindingConstants {
2727
private static final String BINDING_ID = "ntfy";
2828

2929
// List of all Thing Type UIDs
30-
public static final ThingTypeUID NTFY_CONNECTION_THING = new ThingTypeUID(BINDING_ID, "ntfyConnection");
31-
public static final ThingTypeUID NTFY_TOPIC_THING = new ThingTypeUID(BINDING_ID, "ntfyTopic");
30+
public static final ThingTypeUID NTFY_CONNECTION_THING = new ThingTypeUID(BINDING_ID, "server");
31+
public static final ThingTypeUID NTFY_TOPIC_THING = new ThingTypeUID(BINDING_ID, "ntfy-topic");
3232

3333
// List of all Channel ids
34-
public static final String CHANNEL_NTFY_LASTMESSAGE = "lastMessage";
35-
public static final String CHANNEL_NTFY_LASTMESSAGETIME = "lastMessageTime";
36-
public static final String CHANNEL_NTFY_LASTMESSAGEID = "lastMessageMessageId";
34+
public static final String CHANNEL_LASTMESSAGE = "last-message";
35+
public static final String CHANNEL_LASTMESSAGETIME = "last-message-time";
36+
public static final String CHANNEL_LASTMESSAGEID = "last-message-id";
3737
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
public class NtfyConnectionConfiguration {
2828

2929
/**
30-
* Hostname of the ntfy server, e.g. "ntfy.sh"
30+
* Hostname of the ntfy server, e.g. "https://ntfy.sh"
3131
*/
3232
public String hostname = "";
3333

@@ -54,6 +54,8 @@ public class NtfyConnectionConfiguration {
5454
* {@code false} otherwise
5555
*/
5656
public boolean isAuthHeaderNeeded() {
57+
final @Nullable String username = this.username;
58+
final @Nullable String password = this.password;
5759
return username != null && !username.isBlank() && password != null && !password.isBlank();
5860
}
5961

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

Lines changed: 98 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,16 @@
1212
*/
1313
package org.openhab.binding.ntfy.internal;
1414

15+
import java.io.IOException;
1516
import java.net.URI;
17+
import java.net.URISyntaxException;
1618
import java.util.HashMap;
1719
import java.util.Map;
20+
import java.util.concurrent.ScheduledFuture;
1821
import java.util.concurrent.TimeUnit;
1922

2023
import org.eclipse.jdt.annotation.NonNullByDefault;
24+
import org.eclipse.jdt.annotation.Nullable;
2125
import org.eclipse.jetty.websocket.client.ClientUpgradeRequest;
2226
import org.eclipse.jetty.websocket.client.WebSocketClient;
2327
import org.openhab.binding.ntfy.internal.network.NtfyWebSocket;
@@ -48,6 +52,8 @@ public class NtfyConnectionHandler extends BaseBridgeHandler {
4852
private NtfyConnectionConfiguration config;
4953
private Map<ThingUID, WebSocketClient> webSocketConnections = new HashMap<>();
5054
private WebSocketFactory webSocketFactory;
55+
private @Nullable ScheduledFuture<?> retryConnectionFuture;
56+
private @Nullable String scheme;
5157

5258
/**
5359
* Creates a new {@link NtfyConnectionHandler} for the given bridge (connection) thing.
@@ -67,11 +73,31 @@ public void handleCommand(ChannelUID channelUID, Command command) {
6773

6874
@Override
6975
public void initialize() {
70-
updateStatus(ThingStatus.UNKNOWN);
76+
final @Nullable ScheduledFuture<?> retryConnectionFuture = this.retryConnectionFuture;
77+
if (retryConnectionFuture != null) {
78+
retryConnectionFuture.cancel(true);
79+
}
7180

7281
config = getConfigAs(NtfyConnectionConfiguration.class);
7382

74-
scheduler.execute(() -> updateStatus(ThingStatus.ONLINE));
83+
updateStatus(ThingStatus.UNKNOWN);
84+
85+
@Nullable
86+
String scheme;
87+
try {
88+
scheme = (new URI(config.hostname)).getScheme();
89+
} catch (URISyntaxException e) {
90+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getLocalizedMessage());
91+
return;
92+
}
93+
if (scheme == null || scheme.isEmpty() || (!"http".equals(scheme) && !"https".equals(scheme))) {
94+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
95+
"URI scheme is missing in hostname or unsupported URI scheme (only http or https are supported): "
96+
+ config.hostname);
97+
}
98+
99+
this.scheme = scheme;
100+
updateStatus(ThingStatus.ONLINE);
75101
}
76102

77103
/**
@@ -91,8 +117,9 @@ public synchronized void createAndRegisterWebSocketClient(Thing thing) {
91117
try {
92118
client.stop();
93119
} catch (Exception e) {
94-
logger.error("Error stopping WebSocketConnection", e);
120+
logger.warn("Error stopping WebSocketConnection", e);
95121
}
122+
webSocketConnections.remove(thing.getUID());
96123
}
97124

98125
webSocketConnections.put(thing.getUID(), newClient);
@@ -122,28 +149,64 @@ public synchronized boolean startWebSocketConnection(Thing topicThing, NtfyWebSo
122149
String topicName = getTopicNameFromThing(topicThing);
123150

124151
if (client != null) {
125-
try {
126-
if (client.isStarted()) {
152+
if (client.isStarted()) {
153+
try {
127154
client.stop();
155+
} catch (InterruptedException e) {
156+
Thread.currentThread().interrupt();
157+
logger.error("WebSocket connection was interrupted", e);
158+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getLocalizedMessage());
159+
return false;
160+
} catch (Exception e) {
161+
logger.warn("Error stopping WebSocket connection - ignore it and continue", e);
128162
}
163+
}
164+
try {
129165
client.start();
130-
client.setMaxIdleTimeout(config.connectionTimeout);
166+
} catch (InterruptedException e) {
167+
Thread.currentThread().interrupt();
168+
logger.error("WebSocket connection was interrupted", e);
169+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getLocalizedMessage());
170+
return false;
171+
} catch (Exception e) {
172+
logger.error("Error starting WebSocketConnection", e);
173+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getLocalizedMessage());
174+
return false;
175+
}
176+
177+
client.setMaxIdleTimeout(config.connectionTimeout);
178+
179+
final String scheme = java.util.Objects.requireNonNull(this.scheme);
180+
String webSocketScheme = "wss";
181+
switch (scheme) {
182+
case "http":
183+
webSocketScheme = "ws";
184+
break;
185+
case "https":
186+
webSocketScheme = "wss";
187+
break;
188+
default:
189+
throw new IllegalArgumentException("Unsupported URI scheme");
190+
}
131191

192+
try {
132193
client.connect(ntfyWebSocket,
133-
new URI("wss:"
194+
new URI(webSocketScheme + ":"
134195
+ (new URI(config.hostname + "/" + topicName + "/ws")).getRawSchemeSpecificPart()),
135196
setupRequest());
136-
} catch (Exception e) {
137-
logger.error("Error creating WebSocketConnection", e);
138-
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getLocalizedMessage());
197+
} catch (IOException | URISyntaxException e) {
198+
logger.error("Error establishing WebSocket connection", e);
199+
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getLocalizedMessage());
139200
return false;
140201
}
202+
updateStatus(ThingStatus.ONLINE);
203+
141204
}
142205
return true;
143206
}
144207

145208
private String getTopicNameFromThing(Thing topicThing) {
146-
return topicThing.getConfiguration().as(NtfyTopicConfiguration.class).topicname;
209+
return topicThing.getConfiguration().as(NtfyTopicConfiguration.class).topicName;
147210
}
148211

149212
/**
@@ -154,6 +217,29 @@ private String getTopicNameFromThing(Thing topicThing) {
154217
*/
155218
public void connectionError(Throwable cause) {
156219
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, cause.getLocalizedMessage());
157-
scheduler.schedule(() -> updateStatus(ThingStatus.ONLINE), 30, TimeUnit.SECONDS);
220+
retryConnectionFuture = scheduler.schedule(() -> {
221+
retryConnectionFuture = null;
222+
updateStatus(ThingStatus.UNKNOWN);
223+
}, 30, TimeUnit.SECONDS);
224+
}
225+
226+
@Override
227+
public void dispose() {
228+
super.dispose();
229+
230+
final @Nullable ScheduledFuture<?> retryConnectionFuture = this.retryConnectionFuture;
231+
if (retryConnectionFuture != null) {
232+
retryConnectionFuture.cancel(true);
233+
}
234+
235+
webSocketConnections.values().forEach(client -> {
236+
try {
237+
if (client.isRunning()) {
238+
client.stop();
239+
}
240+
} catch (Exception e) {
241+
logger.warn("Error stopping WebSocketConnection", e);
242+
}
243+
});
158244
}
159245
}

‎bundles/org.openhab.binding.ntfy/src/main/java/org/openhab/binding/ntfy/internal/NtfyHandlerFactory.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
public class NtfyHandlerFactory extends BaseThingHandlerFactory {
4343

4444
private static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(NTFY_CONNECTION_THING, NTFY_TOPIC_THING);
45-
private HttpClient httpClient;
45+
private HttpClientFactory httpClientFactory;
4646
private WebSocketFactory webSocketFactory;
4747

4848
/**
@@ -55,7 +55,7 @@ public class NtfyHandlerFactory extends BaseThingHandlerFactory {
5555
@Activate
5656
public NtfyHandlerFactory(@Reference HttpClientFactory httpClientFactory,
5757
@Reference WebSocketFactory webSocketFactory) {
58-
this.httpClient = httpClientFactory.getCommonHttpClient();
58+
this.httpClientFactory = httpClientFactory;
5959
this.webSocketFactory = webSocketFactory;
6060
}
6161

@@ -73,7 +73,7 @@ public boolean supportsThingType(ThingTypeUID thingTypeUID) {
7373
}
7474

7575
if (NTFY_TOPIC_THING.equals(thingTypeUID)) {
76-
return new NtfyTopicHandler(thing, httpClient);
76+
return new NtfyTopicHandler(thing, httpClientFactory.getCommonHttpClient());
7777
}
7878

7979
return null;

0 commit comments

Comments
 (0)