Skip to content

Commit fa9f28b

Browse files
Copilotdmccoystephenson
Copilot
and
dmccoystephenson
committed
Address review comments: thread safety, URL trimming, isEnabled guard, connection cleanup, test verifications
- Capture player.getName() before async task in JoinHandler and QuitHandler - Trim whitespace-only URLs in isEnabled() check - Add isEnabled() guard at start of sendJoinNotification/sendQuitNotification - Properly disconnect HttpURLConnection and consume response streams - Add Mockito verify() assertions to tests and whitespace URL test case - Use buffered read for response stream consumption Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
1 parent 877151a commit fa9f28b

File tree

4 files changed

+95
-31
lines changed

4 files changed

+95
-31
lines changed

‎src/main/java/dansplugins/activitytracker/eventhandlers/JoinHandler.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ private void sendDiscordJoinNotification(Player player) {
6060
if (discordWebhookService.isStaffOnly() && !player.hasPermission(STAFF_PERMISSION)) {
6161
return;
6262
}
63+
final String playerName = player.getName();
6364
plugin.getServer().getScheduler().runTaskAsynchronously(plugin, new Runnable() {
6465
@Override
6566
public void run() {
66-
discordWebhookService.sendJoinNotification(player.getName());
67+
discordWebhookService.sendJoinNotification(playerName);
6768
}
6869
});
6970
}

‎src/main/java/dansplugins/activitytracker/eventhandlers/QuitHandler.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,11 @@ private void sendDiscordQuitNotification(Player player) {
7575
if (discordWebhookService.isStaffOnly() && !player.hasPermission(STAFF_PERMISSION)) {
7676
return;
7777
}
78+
final String playerName = player.getName();
7879
plugin.getServer().getScheduler().runTaskAsynchronously(plugin, new Runnable() {
7980
@Override
8081
public void run() {
81-
discordWebhookService.sendQuitNotification(player.getName());
82+
discordWebhookService.sendQuitNotification(playerName);
8283
}
8384
});
8485
}

‎src/main/java/dansplugins/activitytracker/services/DiscordWebhookService.java‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dansplugins.activitytracker.services;
22

33
import java.io.IOException;
4+
import java.io.InputStream;
45
import java.io.OutputStream;
56
import java.net.HttpURLConnection;
67
import java.net.URL;
@@ -30,7 +31,11 @@ public boolean isEnabled() {
3031
return false;
3132
}
3233
String url = configService.getString("discordWebhookUrl");
33-
return url != null && !url.isEmpty();
34+
if (url == null) {
35+
return false;
36+
}
37+
String trimmed = url.trim();
38+
return !trimmed.isEmpty();
3439
}
3540

3641
/**
@@ -46,6 +51,9 @@ public boolean isStaffOnly() {
4651
* @param playerName The name of the player who joined.
4752
*/
4853
public void sendJoinNotification(String playerName) {
54+
if (!isEnabled()) {
55+
return;
56+
}
4957
String template = configService.getString("discordWebhookJoinMessage");
5058
if (template == null || template.isEmpty()) {
5159
return;
@@ -59,6 +67,9 @@ public void sendJoinNotification(String playerName) {
5967
* @param playerName The name of the player who quit.
6068
*/
6169
public void sendQuitNotification(String playerName) {
70+
if (!isEnabled()) {
71+
return;
72+
}
6273
String template = configService.getString("discordWebhookQuitMessage");
6374
if (template == null || template.isEmpty()) {
6475
return;
@@ -73,13 +84,14 @@ public void sendQuitNotification(String playerName) {
7384
*/
7485
private void sendWebhookMessage(String content) {
7586
String webhookUrl = configService.getString("discordWebhookUrl");
76-
if (webhookUrl == null || webhookUrl.isEmpty()) {
87+
if (webhookUrl == null || webhookUrl.trim().isEmpty()) {
7788
return;
7889
}
7990

91+
HttpURLConnection connection = null;
8092
try {
81-
URL url = new URL(webhookUrl);
82-
HttpURLConnection connection = (HttpURLConnection) url.openConnection();
93+
URL url = new URL(webhookUrl.trim());
94+
connection = (HttpURLConnection) url.openConnection();
8395
connection.setRequestMethod("POST");
8496
connection.setRequestProperty("Content-Type", "application/json");
8597
connection.setConnectTimeout(5000);
@@ -100,8 +112,25 @@ private void sendWebhookMessage(String content) {
100112
if (responseCode < 200 || responseCode >= 300) {
101113
logger.log("Discord webhook returned error code: " + responseCode);
102114
}
115+
116+
// Consume response stream to free up the connection
117+
InputStream is = (responseCode >= 200 && responseCode < 300)
118+
? connection.getInputStream()
119+
: connection.getErrorStream();
120+
if (is != null) {
121+
try {
122+
byte[] buffer = new byte[1024];
123+
while (is.read(buffer) != -1) { }
124+
} finally {
125+
is.close();
126+
}
127+
}
103128
} catch (IOException e) {
104129
logger.log("Failed to send Discord webhook message: " + e.getMessage());
130+
} finally {
131+
if (connection != null) {
132+
connection.disconnect();
133+
}
105134
}
106135
}
107136

‎src/test/java/dansplugins/activitytracker/services/DiscordWebhookServiceTest.java‎

Lines changed: 58 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ public void testIsEnabledReturnsFalseWhenUrlIsNull() {
5151
assertFalse(discordWebhookService.isEnabled());
5252
}
5353

54+
@Test
55+
public void testIsEnabledReturnsFalseWhenUrlIsWhitespaceOnly() {
56+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
57+
when(configService.getString("discordWebhookUrl")).thenReturn(" ");
58+
59+
assertFalse(discordWebhookService.isEnabled());
60+
}
61+
5462
@Test
5563
public void testIsEnabledReturnsTrueWhenConfigured() {
5664
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
@@ -68,71 +76,96 @@ public void testIsStaffOnlyReturnsConfigValue() {
6876
assertFalse(discordWebhookService.isStaffOnly());
6977
}
7078

79+
@Test
80+
public void testSendJoinNotificationSkipsWhenDisabled() {
81+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(false);
82+
83+
discordWebhookService.sendJoinNotification("TestPlayer");
84+
85+
// Should not attempt to read join message template when disabled
86+
verify(configService, never()).getString("discordWebhookJoinMessage");
87+
}
88+
89+
@Test
90+
public void testSendQuitNotificationSkipsWhenDisabled() {
91+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(false);
92+
93+
discordWebhookService.sendQuitNotification("TestPlayer");
94+
95+
// Should not attempt to read quit message template when disabled
96+
verify(configService, never()).getString("discordWebhookQuitMessage");
97+
}
98+
7199
@Test
72100
public void testSendJoinNotificationSkipsWhenTemplateIsEmpty() {
101+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
102+
when(configService.getString("discordWebhookUrl")).thenReturn("https://discord.com/api/webhooks/test");
73103
when(configService.getString("discordWebhookJoinMessage")).thenReturn("");
74104

75-
// Should not throw and should not attempt to send
76105
discordWebhookService.sendJoinNotification("TestPlayer");
106+
107+
// Should check the template but not attempt to fetch URL for sending
108+
verify(configService).getString("discordWebhookJoinMessage");
109+
verify(configService, times(1)).getString("discordWebhookUrl");
77110
}
78111

79112
@Test
80113
public void testSendJoinNotificationSkipsWhenTemplateIsNull() {
114+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
115+
when(configService.getString("discordWebhookUrl")).thenReturn("https://discord.com/api/webhooks/test");
81116
when(configService.getString("discordWebhookJoinMessage")).thenReturn(null);
82117

83-
// Should not throw and should not attempt to send
84118
discordWebhookService.sendJoinNotification("TestPlayer");
119+
120+
verify(configService).getString("discordWebhookJoinMessage");
121+
verify(configService, times(1)).getString("discordWebhookUrl");
85122
}
86123

87124
@Test
88125
public void testSendQuitNotificationSkipsWhenTemplateIsEmpty() {
126+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
127+
when(configService.getString("discordWebhookUrl")).thenReturn("https://discord.com/api/webhooks/test");
89128
when(configService.getString("discordWebhookQuitMessage")).thenReturn("");
90129

91-
// Should not throw and should not attempt to send
92130
discordWebhookService.sendQuitNotification("TestPlayer");
131+
132+
verify(configService).getString("discordWebhookQuitMessage");
133+
verify(configService, times(1)).getString("discordWebhookUrl");
93134
}
94135

95136
@Test
96137
public void testSendQuitNotificationSkipsWhenTemplateIsNull() {
138+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
139+
when(configService.getString("discordWebhookUrl")).thenReturn("https://discord.com/api/webhooks/test");
97140
when(configService.getString("discordWebhookQuitMessage")).thenReturn(null);
98141

99-
// Should not throw and should not attempt to send
100142
discordWebhookService.sendQuitNotification("TestPlayer");
101-
}
102-
103-
@Test
104-
public void testSendJoinNotificationSkipsWhenUrlIsEmpty() {
105-
when(configService.getString("discordWebhookJoinMessage")).thenReturn("**{player}** joined!");
106-
when(configService.getString("discordWebhookUrl")).thenReturn("");
107-
108-
// Should not throw
109-
discordWebhookService.sendJoinNotification("TestPlayer");
110-
}
111143

112-
@Test
113-
public void testSendQuitNotificationSkipsWhenUrlIsEmpty() {
114-
when(configService.getString("discordWebhookQuitMessage")).thenReturn("**{player}** left.");
115-
when(configService.getString("discordWebhookUrl")).thenReturn("");
116-
117-
// Should not throw
118-
discordWebhookService.sendQuitNotification("TestPlayer");
144+
verify(configService).getString("discordWebhookQuitMessage");
145+
verify(configService, times(1)).getString("discordWebhookUrl");
119146
}
120147

121148
@Test
122149
public void testSendJoinNotificationHandlesInvalidUrl() {
123-
when(configService.getString("discordWebhookJoinMessage")).thenReturn("**{player}** joined!");
150+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
124151
when(configService.getString("discordWebhookUrl")).thenReturn("not-a-valid-url");
152+
when(configService.getString("discordWebhookJoinMessage")).thenReturn("**{player}** joined!");
125153

126-
// Should not throw - errors are logged gracefully
127154
discordWebhookService.sendJoinNotification("TestPlayer");
155+
156+
// Error should be logged gracefully
157+
verify(logger).log(contains("Failed to send Discord webhook message"));
128158
}
129159

130160
@Test
131161
public void testSendQuitNotificationHandlesInvalidUrl() {
132-
when(configService.getString("discordWebhookQuitMessage")).thenReturn("**{player}** left.");
162+
when(configService.getBoolean("discordWebhookEnabled")).thenReturn(true);
133163
when(configService.getString("discordWebhookUrl")).thenReturn("not-a-valid-url");
164+
when(configService.getString("discordWebhookQuitMessage")).thenReturn("**{player}** left.");
134165

135-
// Should not throw - errors are logged gracefully
136166
discordWebhookService.sendQuitNotification("TestPlayer");
167+
168+
// Error should be logged gracefully
169+
verify(logger).log(contains("Failed to send Discord webhook message"));
137170
}
138171
}

0 commit comments

Comments
 (0)