Skip to content

⚡ Fix unsafe async API usage in MedalShowCmd #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
acsoto wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚡ Fix unsafe async API usage in MedalShowCmd #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion modules/MedalCabinet/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@
<id>placeholderapi</id>
<url>https://repo.extendedclip.com/content/repositories/placeholderapi/</url>
</repository>
<repository>
<id>helpch</id>
<url>https://repo.helpch.at/releases/</url>
</repository>
Comment on lines +69 to +72
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helpch repository was added but none of the dependencies in the pom.xml appear to require it. All test dependencies (JUnit Jupiter, Mockito, Byte Buddy) are available from Maven Central (sonatype repository). Unless there's a specific reason for this repository, it should be removed to avoid unnecessary repository lookups during builds.

Suggested change
<repository>
<id>helpch</id>
<url>https://repo.helpch.at/releases/</url>
</repository>

Copilot uses AI. Check for mistakes.
</repositories>

<dependencies>
Expand All @@ -78,8 +82,38 @@
<dependency>
<groupId>me.clip</groupId>
<artifactId>placeholderapi</artifactId>
<version>2.11.1</version>
<version>2.11.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<version>5.9.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>5.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-inline</artifactId>
<version>5.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.14.19</version>
<scope>test</scope>
Comment on lines +109 to +110
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions that this is compatible with Java 21, but the pom.xml still specifies Java 1.8 (line 15). This creates a discrepancy between the claimed Java 21 support and the actual project configuration. If the dependencies are truly meant to be compatible with Java 21, consider updating the java.version property to 21 or at least to a more recent version that can leverage the features of these newer dependencies.

Copilot uses AI. Check for mistakes.
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy-agent</artifactId>
<version>1.14.19</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
24 changes: 12 additions & 12 deletions modules/MedalCabinet/src/main/java/com/mcatk/medalcabinet/command/MedalShowCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,55 +10,55 @@
import org.bukkit.entity.Player;

public class MedalShowCmd implements CommandExecutor {
private CommandSender sender;
private String[] args;

private final String prefix = "§7[§6勋章墙§7]§7 ";

private void printHelp() {
private void printHelp(CommandSender sender) {
sender.sendMessage("§e------------帮助------------");
sender.sendMessage("§a/medalshow all §2展示你全部的勋章（全服可见）");
sender.sendMessage("§a/medalshow me §2展示你全部的勋章（仅自己可见）");
}

@Override
public boolean onCommand(CommandSender sender, Command command, String label, String[] args) {
this.sender = sender;
this.args = args;
if (!(sender instanceof Player)) {
return false;
}
if (args.length == 0) {
printHelp();
printHelp(sender);
return true;
}
if ("all".equals(args[0])) {
showAll();
showAll(sender);
} else if ("me".equals(args[0])) {
showMe();
showMe(sender);
}
return false;
Comment on lines 31 to 36
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onCommand method returns false when args[0] is "all" or "me", which in Bukkit's CommandExecutor contract indicates the command was not handled successfully and will cause the usage message to be sent to the player. This should return true to indicate successful command execution. The return value should be true for all valid command cases.

Copilot uses AI. Check for mistakes.
Comment on lines 31 to 36
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When args[0] is provided but doesn't match "all" or "me", the command silently does nothing and returns false. Consider adding an else clause to show the help message or an error message for invalid arguments, similar to how printHelp is shown when args.length == 0. This would provide better user experience and feedback.

Copilot uses AI. Check for mistakes.
}

private void showAll() {
private void showAll(CommandSender sender) {
Bukkit.getScheduler().runTaskAsynchronously(MedalCabinet.getPlugin(), () -> {
StringBuilder stringBuilder = new StringBuilder(prefix).append("§e").append(sender.getName())
.append(" 展示了他的勋章：\n");
for (Medal medal : SQLManager.getInstance().getPlayerMedals(sender.getName())) {
stringBuilder.append(medal).append(" ");
Comment on lines 40 to 44
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid Bukkit API calls on async thread

The async task still dereferences the CommandSender to call sender.getName() (and again in the showMe async block). Accessing Bukkit API objects off the main thread is unsafe, which means this change still risks the same thread-safety violations the refactor intended to remove. This can surface when the async task runs while the player object is being mutated or has already disconnected. Capture the player name (or UUID) on the main thread before scheduling the async work, and only use that captured value inside the async task.

Useful? React with 👍 / 👎.

}
MedalCabinet.getPlugin().getServer().broadcastMessage(stringBuilder.toString());
Bukkit.getScheduler().runTask(MedalCabinet.getPlugin(), () -> {
MedalCabinet.getPlugin().getServer().broadcastMessage(stringBuilder.toString());
});
});
}

private void showMe() {
private void showMe(CommandSender sender) {
Bukkit.getScheduler().runTaskAsynchronously(MedalCabinet.getPlugin(), () -> {
StringBuilder stringBuilder = new StringBuilder(prefix).append("§e").append(sender.getName())
.append(" 的勋章：\n");
for (Medal medal : SQLManager.getInstance().getPlayerMedals(sender.getName())) {
stringBuilder.append(medal).append(" ");
}
sender.sendMessage(stringBuilder.toString());
Bukkit.getScheduler().runTask(MedalCabinet.getPlugin(), () -> {
sender.sendMessage(stringBuilder.toString());
});
});
}

Expand Down
7 changes: 6 additions & 1 deletion modules/MedalCabinet/src/main/java/com/mcatk/medalcabinet/sql/SQLManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ public static SQLManager getInstance() {
return instance == null ? instance = new SQLManager() : instance;
}

private SQLManager() {
// For testing
public static void setInstance(SQLManager instance) {
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setInstance method allows replacing the singleton instance, which could lead to unexpected behavior in production if accidentally called. Consider adding appropriate safeguards such as making this package-private or adding a check to ensure it's only called during testing (e.g., checking for test environment). Alternatively, consider using a proper dependency injection framework or test-specific subclasses.

Suggested change
public static void setInstance(SQLManager instance) {
static void setInstance(SQLManager instance) {

Copilot uses AI. Check for mistakes.
SQLManager.instance = instance;
}

protected SQLManager() {
connectMySQL();
}

Expand Down
105 changes: 105 additions & 0 deletions modules/MedalCabinet/src/test/java/com/mcatk/medalcabinet/command/MedalShowCmdTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package com.mcatk.medalcabinet.command;

import com.mcatk.medalcabinet.MedalCabinet;
import com.mcatk.medalcabinet.medal.Medal;
import com.mcatk.medalcabinet.sql.SQLManager;
import org.bukkit.Bukkit;
import org.bukkit.Server;
import org.bukkit.command.Command;
import org.bukkit.entity.Player;
import org.bukkit.scheduler.BukkitScheduler;
import org.bukkit.scheduler.BukkitTask;
import org.bukkit.plugin.Plugin;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.MockedStatic;

import java.lang.reflect.Field;
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Field import (line 19) is unused and should be removed to keep the imports clean.

Suggested change
import java.lang.reflect.Field;

Copilot uses AI. Check for mistakes.
import java.util.ArrayList;
import java.util.Collections;
Comment on lines +12 to +21
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several imports appear to be unused in the test: Plugin (line 12), Collections (line 21). These should be removed to keep the code clean and avoid confusion.

Copilot uses AI. Check for mistakes.

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.*;

public class MedalShowCmdTest {

private MockedStatic<Bukkit> mockedBukkit;
private MockedStatic<SQLManager> mockedSQLManagerStatic;
private MockedStatic<MedalCabinet> mockedMedalCabinetStatic;
private SQLManager mockedSQLManager;
private MedalCabinet mockedPlugin;
private Server mockedServer;
private BukkitScheduler mockedScheduler;
private Player mockedPlayer;

@BeforeEach
public void setUp() {
mockedBukkit = mockStatic(Bukkit.class);
mockedSQLManagerStatic = mockStatic(SQLManager.class);
mockedMedalCabinetStatic = mockStatic(MedalCabinet.class);

mockedPlugin = mock(MedalCabinet.class);
mockedServer = mock(Server.class);
mockedScheduler = mock(BukkitScheduler.class);
mockedPlayer = mock(Player.class);
mockedSQLManager = mock(SQLManager.class);

// Setup Bukkit statics
mockedBukkit.when(Bukkit::getServer).thenReturn(mockedServer);
mockedBukkit.when(Bukkit::getScheduler).thenReturn(mockedScheduler);

// Setup SQLManager singleton
mockedSQLManagerStatic.when(SQLManager::getInstance).thenReturn(mockedSQLManager);
Comment on lines +54 to +55
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test uses mockStatic for SQLManager but never actually uses the setInstance method that was added in this PR. The test correctly mocks SQLManager.getInstance() to return the mocked instance, but the setInstance method (lines 19-21 in SQLManager.java) is not utilized in the test. This suggests the setInstance method may not be necessary for testing purposes, or the test could be simplified by using it directly instead of mocking the static getInstance call.

Copilot uses AI. Check for mistakes.

// Setup Plugin
mockedMedalCabinetStatic.when(MedalCabinet::getPlugin).thenReturn(mockedPlugin);
when(mockedPlugin.getServer()).thenReturn(mockedServer);

// Setup Player
when(mockedPlayer.getName()).thenReturn("TestPlayer");
}

@AfterEach
public void tearDown() {
mockedBukkit.close();
mockedSQLManagerStatic.close();
mockedMedalCabinetStatic.close();
}

@Test
public void testShowAll() {
MedalShowCmd cmd = new MedalShowCmd();
String[] args = {"all"};

// Mock SQL return
ArrayList<Medal> medals = new ArrayList<>();
medals.add(new Medal("1", "TestMedal", "STONE", "Desc"));
when(mockedSQLManager.getPlayerMedals("TestPlayer")).thenReturn(medals);

// Capture the async task
ArgumentCaptor<Runnable> asyncTaskCaptor = ArgumentCaptor.forClass(Runnable.class);
when(mockedScheduler.runTaskAsynchronously(eq(mockedPlugin), asyncTaskCaptor.capture()))
.thenReturn(mock(BukkitTask.class));

// Execute command
cmd.onCommand(mockedPlayer, mock(Command.class), "medalshow", args);

// Run the captured async task
Runnable asyncTask = asyncTaskCaptor.getValue();
asyncTask.run();

// Verify that runTask (sync) was called
ArgumentCaptor<Runnable> syncTaskCaptor = ArgumentCaptor.forClass(Runnable.class);
verify(mockedScheduler).runTask(eq(mockedPlugin), syncTaskCaptor.capture());

// Run the captured sync task
Runnable syncTask = syncTaskCaptor.getValue();
syncTask.run();

// Verify broadcastMessage is called
verify(mockedServer).broadcastMessage(contains("TestPlayer 展示了他的勋章"));
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test verifies that broadcastMessage is called on the server (mockedServer.broadcastMessage), but the actual implementation calls MedalCabinet.getPlugin().getServer().broadcastMessage(). While both should work since mockedPlugin.getServer() returns mockedServer, it would be more accurate to also verify the call on Bukkit.broadcastMessage() or document why the server's broadcastMessage is verified instead, as Bukkit.broadcastMessage() is a static convenience method that delegates to the server.

Copilot uses AI. Check for mistakes.
}
Comment on lines +72 to +104
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test only covers the "all" command case (testShowAll) but does not test the "me" command case which uses sender.sendMessage() instead of broadcastMessage(). Both code paths should be tested to ensure comprehensive coverage of the async-to-sync scheduling fix. Consider adding a testShowMe() method that verifies the "me" command also properly schedules the sendMessage call on the main thread.

Copilot uses AI. Check for mistakes.
}
Loading