From 5b74af7acf3754c93f89f6302214a79470feab30 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:13:22 +0000 Subject: [PATCH 1/5] fix: require marketblock.admin for /marketblock Any player could run /marketblock add and create a trade that pays any price they typed, including Infinity, as well as delete, reset and reload trades. The command now needs marketblock.admin (default op), checked in plugin.yml, in the executor and during the chat prompts. The add prompts also reject non-finite and out-of-range numbers, can be stopped by typing "cancel", and end when the player quits. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../marketblock/manager/CommandManager.java | 10 +- .../manager/commands/ChatListener.java | 67 ++++++++----- .../manager/commands/ConversationManager.java | 6 +- .../manager/commands/TabCompletion.java | 2 + .../manager/commands/TradeInput.java | 45 +++++++++ src/main/resources/plugin.yml | 10 +- .../manager/CommandManagerTest.java | 89 +++++++++++++++++ .../marketblock/manager/FakePlayer.java | 67 +++++++++++++ .../manager/commands/ChatListenerTest.java | 98 +++++++++++++++++++ .../manager/commands/TradeInputTest.java | 48 +++++++++ 10 files changed, 413 insertions(+), 29 deletions(-) create mode 100644 src/main/java/net/tfminecraft/marketblock/manager/commands/TradeInput.java create mode 100644 src/test/java/net/tfminecraft/marketblock/manager/CommandManagerTest.java create mode 100644 src/test/java/net/tfminecraft/marketblock/manager/FakePlayer.java create mode 100644 src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java create mode 100644 src/test/java/net/tfminecraft/marketblock/manager/commands/TradeInputTest.java diff --git a/src/main/java/net/tfminecraft/marketblock/manager/CommandManager.java b/src/main/java/net/tfminecraft/marketblock/manager/CommandManager.java index dd7b7a3..35d43c2 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/CommandManager.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/CommandManager.java @@ -16,12 +16,20 @@ public class CommandManager implements CommandExecutor { + public static final String ADMIN_PERMISSION = "marketblock.admin"; + public String cmd1 = "marketblock"; @Override public boolean onCommand(CommandSender sender, Command command, String label, String[] args) { if (!command.getName().equalsIgnoreCase(cmd1)) return true; + // plugin.yml also guards the command, but every subcommand here edits live trades. + if (!sender.hasPermission(ADMIN_PERMISSION)) { + sender.sendMessage("§cYou do not have permission to use this command."); + return true; + } + if (args.length == 0) { sender.sendMessage("§cUsage: /marketblock add OR /marketblock delete "); return true; @@ -52,7 +60,7 @@ public boolean onCommand(CommandSender sender, Command command, String label, St MarketblockConversation convo = new MarketblockConversation(player, item); ConversationManager.startConversation(player, convo); - player.sendMessage("§aPlease enter the trade ID in chat:"); + player.sendMessage("§aPlease enter the trade ID in chat, or type §ecancel§a to stop:"); return true; } diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java index 64da1f4..6ea3301 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java @@ -4,10 +4,12 @@ import org.bukkit.event.EventHandler; import org.bukkit.event.Listener; import org.bukkit.event.player.AsyncPlayerChatEvent; +import org.bukkit.event.player.PlayerQuitEvent; import net.tfminecraft.tlibs.TLibs; import net.tfminecraft.marketblock.loader.CategoryLoader; import net.tfminecraft.marketblock.loader.TradeLoader; +import net.tfminecraft.marketblock.manager.CommandManager; import net.tfminecraft.marketblock.trade.Category; import net.tfminecraft.marketblock.trade.Trade; @@ -22,9 +24,21 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { if (convo == null) return; + // Permission may have been removed since /marketblock add; let the message reach chat. + if (!player.hasPermission(CommandManager.ADMIN_PERMISSION)) { + ConversationManager.endConversation(player); + return; + } + event.setCancelled(true); String message = event.getMessage(); + if (TradeInput.isCancel(message)) { + ConversationManager.endConversation(player); + player.sendMessage("§eTrade creation cancelled."); + return; + } + switch (convo.getStep()) { case 0 -> { if (TradeLoader.getTradeById(message) != null) { @@ -37,15 +51,15 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { player.sendMessage("§aEnter the demand limit:"); } case 1 -> { - try { - double demandLimit = Double.parseDouble(message); - convo.setDemandLimit(demandLimit); - player.sendMessage("§aDemand limit set to: " + demandLimit); - convo.nextStep(); - player.sendMessage("§aEnter the group:"); - } catch (NumberFormatException e) { - player.sendMessage("§cInvalid number. Please enter a valid demand limit."); + Double demandLimit = TradeInput.demandLimit(message); + if (demandLimit == null) { + player.sendMessage("§cInvalid number. Enter a demand limit of at least 1."); + return; } + convo.setDemandLimit(demandLimit); + player.sendMessage("§aDemand limit set to: " + demandLimit); + convo.nextStep(); + player.sendMessage("§aEnter the group:"); } case 2 -> { try { @@ -59,26 +73,26 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { } } case 3 -> { - try { - double priceChange = Double.parseDouble(message); - convo.setPriceChange(priceChange); - player.sendMessage("§aPrice change set to: " + priceChange); - convo.nextStep(); - player.sendMessage("§aEnter the resting price:"); - } catch (NumberFormatException e) { - player.sendMessage("§cInvalid number. Please enter a valid price change."); + Double priceChange = TradeInput.priceChange(message); + if (priceChange == null) { + player.sendMessage("§cInvalid number. Enter a price change of 0 or more."); + return; } + convo.setPriceChange(priceChange); + player.sendMessage("§aPrice change set to: " + priceChange); + convo.nextStep(); + player.sendMessage("§aEnter the resting price:"); } case 4 -> { - try { - double restingPrice = Double.parseDouble(message); - convo.setRestingPrice(restingPrice); - player.sendMessage("§aResting price set to: " + restingPrice); - convo.nextStep(); - player.sendMessage("§aEnter the category:"); - } catch (NumberFormatException e) { - player.sendMessage("§cInvalid number. Please enter a valid resting price."); + Double restingPrice = TradeInput.restingPrice(message); + if (restingPrice == null) { + player.sendMessage("§cInvalid number. Enter a resting price above 0."); + return; } + convo.setRestingPrice(restingPrice); + player.sendMessage("§aResting price set to: " + restingPrice); + convo.nextStep(); + player.sendMessage("§aEnter the category:"); } case 5 -> { Category cat = CategoryLoader.getByString(message); @@ -95,6 +109,11 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { } } + @EventHandler + public void onQuit(PlayerQuitEvent event) { + ConversationManager.endConversation(event.getPlayer()); + } + private boolean saveTrade(Player p, MarketblockConversation convo) { String path = TLibs.getItemAPI().getChecker().getAsStringPath(convo.getItem()); if(path == null) { diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java index 4aed0bb..635677d 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java @@ -2,12 +2,14 @@ import org.bukkit.entity.Player; -import java.util.HashMap; +import java.util.Map; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; public class ConversationManager { - private static final HashMap conversations = new HashMap<>(); + // Read from the async chat thread and written from the main thread. + private static final Map conversations = new ConcurrentHashMap<>(); public static void startConversation(Player player, MarketblockConversation convo) { conversations.put(player.getUniqueId(), convo); diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/TabCompletion.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/TabCompletion.java index 875e90c..6c5b373 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/TabCompletion.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/TabCompletion.java @@ -1,6 +1,7 @@ package net.tfminecraft.marketblock.manager.commands; import net.tfminecraft.marketblock.loader.TradeLoader; +import net.tfminecraft.marketblock.manager.CommandManager; import org.bukkit.command.Command; import org.bukkit.command.CommandSender; import org.bukkit.command.TabCompleter; @@ -15,6 +16,7 @@ public List onTabComplete(CommandSender sender, Command command, String List completions = new ArrayList<>(); if (!command.getName().equalsIgnoreCase("marketblock")) return completions; + if (!sender.hasPermission(CommandManager.ADMIN_PERMISSION)) return completions; if (args.length == 1) { String prefix = args[0].toLowerCase(); diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/TradeInput.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/TradeInput.java new file mode 100644 index 0000000..ed71eca --- /dev/null +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/TradeInput.java @@ -0,0 +1,45 @@ +package net.tfminecraft.marketblock.manager.commands; + +/** + * Parses the numbers typed during /marketblock add. Each method returns null when the text is not + * a usable value, so a typo or a value such as NaN or Infinity cannot reach a saved trade. + */ +public final class TradeInput { + + public static final String CANCEL = "cancel"; + + private TradeInput() { + } + + public static boolean isCancel(String message) { + return message != null && message.trim().equalsIgnoreCase(CANCEL); + } + + /** Demand limit: a finite number of at least 1, the lowest a trade allows. */ + public static Double demandLimit(String message) { + Double value = finite(message); + return value != null && value >= 1 ? value : null; + } + + /** Price change: a finite number of at least 0. */ + public static Double priceChange(String message) { + Double value = finite(message); + return value != null && value >= 0 ? value : null; + } + + /** Resting price: a finite number above 0. */ + public static Double restingPrice(String message) { + Double value = finite(message); + return value != null && value > 0 ? value : null; + } + + private static Double finite(String message) { + if (message == null) return null; + try { + double value = Double.parseDouble(message.trim()); + return Double.isFinite(value) ? value : null; + } catch (NumberFormatException e) { + return null; + } + } +} diff --git a/src/main/resources/plugin.yml b/src/main/resources/plugin.yml index a0367ad..6b0ae0d 100644 --- a/src/main/resources/plugin.yml +++ b/src/main/resources/plugin.yml @@ -9,5 +9,11 @@ softdepend: [Cooking] commands: marketblock: - useage: / - description: MarketBlock Command \ No newline at end of file + usage: / + description: MarketBlock Command + permission: marketblock.admin + +permissions: + marketblock.admin: + description: Add, delete, reset and reload market trades + default: op diff --git a/src/test/java/net/tfminecraft/marketblock/manager/CommandManagerTest.java b/src/test/java/net/tfminecraft/marketblock/manager/CommandManagerTest.java new file mode 100644 index 0000000..e06621a --- /dev/null +++ b/src/test/java/net/tfminecraft/marketblock/manager/CommandManagerTest.java @@ -0,0 +1,89 @@ +package net.tfminecraft.marketblock.manager; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.StandardCharsets; +import java.util.List; + +import org.bukkit.command.Command; +import org.bukkit.command.CommandSender; +import org.bukkit.configuration.file.YamlConfiguration; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import net.tfminecraft.marketblock.manager.commands.ConversationManager; +import net.tfminecraft.marketblock.manager.commands.TabCompletion; + +class CommandManagerTest { + private final CommandManager commands = new CommandManager(); + private final Command command = new Command("marketblock") { + @Override + public boolean execute(CommandSender sender, String label, String[] args) { + return false; + } + }; + private FakePlayer tester; + + @AfterEach + void endConversation() { + if (tester != null) ConversationManager.endConversation(tester.player); + } + + @Test + void aPlayerWithoutPermissionCannotStartAddingATrade() { + tester = new FakePlayer(false); + + assertTrue(commands.onCommand(tester.player, command, "marketblock", new String[] { "add" })); + + assertNull(ConversationManager.getConversation(tester.player)); + assertEquals("§cYou do not have permission to use this command.", tester.lastMessage()); + } + + @Test + void everySubcommandIsRefusedWithoutPermission() { + // reload would reach JavaPlugin.getPlugin and throw here if the check did not stop it first. + for (String sub : List.of("add", "delete", "reset", "resetall", "reload")) { + tester = new FakePlayer(false); + + commands.onCommand(tester.player, command, "marketblock", new String[] { sub, "stone" }); + + assertEquals(List.of("§cYou do not have permission to use this command."), tester.messages, sub); + } + } + + @Test + void anAdminGetsPastThePermissionCheck() { + tester = new FakePlayer(true); + + commands.onCommand(tester.player, command, "marketblock", new String[] { "add" }); + + assertEquals("§cYou must hold an item in your hand to add a trade.", tester.lastMessage()); + } + + @Test + void tabCompletionIsEmptyWithoutPermission() { + TabCompletion completion = new TabCompletion(); + + assertTrue(completion.onTabComplete(new FakePlayer(false).player, command, "marketblock", new String[] { "" }).isEmpty()); + assertEquals(List.of("add", "delete", "reload", "reset", "resetall"), + completion.onTabComplete(new FakePlayer(true).player, command, "marketblock", new String[] { "" })); + } + + @Test + void pluginYmlRequiresTheAdminPermission() throws Exception { + YamlConfiguration yaml; + try (Reader reader = new InputStreamReader( + getClass().getClassLoader().getResourceAsStream("plugin.yml"), StandardCharsets.UTF_8)) { + yaml = YamlConfiguration.loadConfiguration(reader); + } + + assertEquals(CommandManager.ADMIN_PERMISSION, yaml.getString("commands.marketblock.permission")); + assertNotNull(yaml.getConfigurationSection("permissions." + CommandManager.ADMIN_PERMISSION)); + assertEquals("op", yaml.getString("permissions." + CommandManager.ADMIN_PERMISSION + ".default")); + } +} diff --git a/src/test/java/net/tfminecraft/marketblock/manager/FakePlayer.java b/src/test/java/net/tfminecraft/marketblock/manager/FakePlayer.java new file mode 100644 index 0000000..23c5e61 --- /dev/null +++ b/src/test/java/net/tfminecraft/marketblock/manager/FakePlayer.java @@ -0,0 +1,67 @@ +package net.tfminecraft.marketblock.manager; + +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +import org.bukkit.entity.Player; +import org.bukkit.inventory.PlayerInventory; + +/** A Player with a fixed permission answer and an empty hand, recording the messages it is sent. */ +public final class FakePlayer { + public final UUID id = UUID.randomUUID(); + public final List messages = new ArrayList<>(); + public boolean admin; + public final Player player; + + public FakePlayer(boolean admin) { + this.admin = admin; + PlayerInventory inventory = proxy(PlayerInventory.class, (name, args) -> null); + this.player = proxy(Player.class, (name, args) -> switch (name) { + case "getUniqueId" -> id; + case "getName" -> "tester"; + case "hasPermission" -> this.admin && CommandManager.ADMIN_PERMISSION.equals(args[0]); + case "getInventory" -> inventory; + case "sendMessage" -> { + if (args[0] instanceof String message) messages.add(message); + yield null; + } + default -> null; + }); + } + + public String lastMessage() { + return messages.isEmpty() ? null : messages.get(messages.size() - 1); + } + + private interface Answer { + Object answer(String name, Object[] args); + } + + @SuppressWarnings("unchecked") + private static T proxy(Class type, Answer answer) { + return (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type }, (self, method, args) -> { + switch (method.getName()) { + case "equals": return self == args[0]; + case "hashCode": return System.identityHashCode(self); + case "toString": return "Fake" + type.getSimpleName(); + default: + } + Object value = answer.answer(method.getName(), args == null ? new Object[0] : args); + return value != null ? value : zero(method.getReturnType()); + }); + } + + private static Object zero(Class type) { + if (!type.isPrimitive() || type == void.class) return null; + if (type == boolean.class) return false; + if (type == char.class) return '\0'; + if (type == long.class) return 0L; + if (type == float.class) return 0f; + if (type == double.class) return 0d; + if (type == byte.class) return (byte) 0; + if (type == short.class) return (short) 0; + return 0; + } +} diff --git a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java new file mode 100644 index 0000000..a50d70f --- /dev/null +++ b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java @@ -0,0 +1,98 @@ +package net.tfminecraft.marketblock.manager.commands; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.HashSet; + +import org.bukkit.event.player.AsyncPlayerChatEvent; +import org.bukkit.event.player.PlayerQuitEvent; +import org.bukkit.event.player.PlayerQuitEvent.QuitReason; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import net.kyori.adventure.text.Component; +import net.tfminecraft.marketblock.manager.FakePlayer; + +@SuppressWarnings("deprecation") +class ChatListenerTest { + private final ChatListener listener = new ChatListener(); + private final FakePlayer tester = new FakePlayer(true); + + @AfterEach + void endConversation() { + ConversationManager.endConversation(tester.player); + } + + @Test + void typingCancelEndsTheConversation() { + start(0); + + AsyncPlayerChatEvent event = chat("cancel"); + + assertTrue(event.isCancelled()); + assertNull(ConversationManager.getConversation(tester.player)); + assertEquals("§eTrade creation cancelled.", tester.lastMessage()); + } + + @Test + void numbersThatAreNotFiniteAreRejected() { + MarketblockConversation convo = start(1); + + for (String text : new String[] { "NaN", "Infinity", "0" }) { + chat(text); + assertEquals(1, convo.getStep(), text); + } + chat("20"); + + assertEquals(2, convo.getStep()); + assertEquals(20.0, convo.getDemandLimit()); + } + + @Test + void anInfiniteRestingPriceIsRejected() { + MarketblockConversation convo = start(4); + + chat("Infinity"); + + assertEquals(4, convo.getStep()); + assertEquals("§cInvalid number. Enter a resting price above 0.", tester.lastMessage()); + } + + @Test + void losingPermissionEndsTheConversationAndLeavesChatAlone() { + start(0); + tester.admin = false; + + AsyncPlayerChatEvent event = chat("hello"); + + assertFalse(event.isCancelled()); + assertNull(ConversationManager.getConversation(tester.player)); + } + + @Test + void quittingEndsTheConversation() { + start(0); + + listener.onQuit(new PlayerQuitEvent(tester.player, Component.text("left"), QuitReason.DISCONNECTED)); + + assertNull(ConversationManager.getConversation(tester.player)); + } + + private MarketblockConversation start(int step) { + MarketblockConversation convo = new MarketblockConversation(tester.player, null); + for (int i = 0; i < step; i++) convo.nextStep(); + ConversationManager.startConversation(tester.player, convo); + assertSame(convo, ConversationManager.getConversation(tester.player)); + return convo; + } + + private AsyncPlayerChatEvent chat(String message) { + AsyncPlayerChatEvent event = new AsyncPlayerChatEvent(true, tester.player, message, new HashSet<>()); + listener.onPlayerChat(event); + return event; + } +} diff --git a/src/test/java/net/tfminecraft/marketblock/manager/commands/TradeInputTest.java b/src/test/java/net/tfminecraft/marketblock/manager/commands/TradeInputTest.java new file mode 100644 index 0000000..2893e81 --- /dev/null +++ b/src/test/java/net/tfminecraft/marketblock/manager/commands/TradeInputTest.java @@ -0,0 +1,48 @@ +package net.tfminecraft.marketblock.manager.commands; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class TradeInputTest { + + @Test + void rejectsValuesThatAreNotFiniteNumbers() { + for (String text : new String[] { "NaN", "Infinity", "-Infinity", "1e400", "", "twenty", null }) { + assertNull(TradeInput.demandLimit(text), text); + assertNull(TradeInput.priceChange(text), text); + assertNull(TradeInput.restingPrice(text), text); + } + } + + @Test + void demandLimitMustBeAtLeastOne() { + assertNull(TradeInput.demandLimit("0.5")); + assertEquals(1.0, TradeInput.demandLimit("1")); + assertEquals(20.0, TradeInput.demandLimit(" 20 ")); + } + + @Test + void priceChangeMayBeZeroButNotNegative() { + assertNull(TradeInput.priceChange("-1")); + assertEquals(0.0, TradeInput.priceChange("0")); + assertEquals(8.0, TradeInput.priceChange("8")); + } + + @Test + void restingPriceMustBeAboveZero() { + assertNull(TradeInput.restingPrice("0")); + assertNull(TradeInput.restingPrice("-4")); + assertEquals(0.5, TradeInput.restingPrice("0.5")); + } + + @Test + void cancelIgnoresCaseAndSpaces() { + assertTrue(TradeInput.isCancel(" Cancel ")); + assertFalse(TradeInput.isCancel("cancelled")); + assertFalse(TradeInput.isCancel(null)); + } +} From 8e956fcfa29953bc4bfb19bca6a94f72348ee0ff Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:40:19 +0000 Subject: [PATCH 2/5] fix: save a new trade only if its answer beat a quit The final /marketblock add answer arrives on the async chat thread while a quit is handled on the main thread, so a quit could clear the conversation and the trade still be saved. The final answer now claims the conversation with an atomic remove and saves only if it won, and the save runs on the main thread, which owns the trade map and trades.yml. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../manager/commands/ChatListener.java | 13 +++++++--- .../manager/commands/ConversationManager.java | 8 ++++++ .../manager/commands/ChatListenerTest.java | 26 +++++++++++++++++++ 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java index 6ea3301..2f21717 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java @@ -1,5 +1,6 @@ package net.tfminecraft.marketblock.manager.commands; +import org.bukkit.Bukkit; import org.bukkit.entity.Player; import org.bukkit.event.EventHandler; import org.bukkit.event.Listener; @@ -7,6 +8,7 @@ import org.bukkit.event.player.PlayerQuitEvent; import net.tfminecraft.tlibs.TLibs; +import net.tfminecraft.marketblock.MarketBlock; import net.tfminecraft.marketblock.loader.CategoryLoader; import net.tfminecraft.marketblock.loader.TradeLoader; import net.tfminecraft.marketblock.manager.CommandManager; @@ -101,10 +103,13 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { } convo.setCategory(cat); player.sendMessage("§aCategory set to: " + cat.getId()); - // Done! - ConversationManager.endConversation(player); - if (!saveTrade(player, convo)) return; - player.sendMessage("§aTrade successfully created!"); + // Done! Claim the conversation first, so a quit that got there first stops the save. + if (!ConversationManager.finishConversation(player, convo)) return; + // Chat arrives on an async thread; trades and trades.yml belong to the main thread. + Bukkit.getScheduler().runTask(MarketBlock.plugin, () -> { + if (!saveTrade(player, convo)) return; + player.sendMessage("§aTrade successfully created!"); + }); } } } diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java index 635677d..f52d4b9 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ConversationManager.java @@ -22,5 +22,13 @@ public static MarketblockConversation getConversation(Player player) { public static void endConversation(Player player) { conversations.remove(player.getUniqueId()); } + + /** + * Ends this exact conversation and reports whether this call did so. Only one caller can win, + * so a trade is saved only if its final answer arrived before the player quit or cancelled. + */ + public static boolean finishConversation(Player player, MarketblockConversation convo) { + return conversations.remove(player.getUniqueId(), convo); + } } diff --git a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java index a50d70f..c0a4dfc 100644 --- a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java +++ b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java @@ -82,6 +82,32 @@ void quittingEndsTheConversation() { assertNull(ConversationManager.getConversation(tester.player)); } + @Test + void aConversationEndedByQuittingCannotThenBeFinished() { + MarketblockConversation convo = start(5); + + listener.onQuit(new PlayerQuitEvent(tester.player, Component.text("left"), QuitReason.DISCONNECTED)); + + assertFalse(ConversationManager.finishConversation(tester.player, convo)); + } + + @Test + void onlyOneCallerCanFinishAConversation() { + MarketblockConversation convo = start(5); + + assertTrue(ConversationManager.finishConversation(tester.player, convo)); + assertFalse(ConversationManager.finishConversation(tester.player, convo)); + } + + @Test + void finishingDoesNotEndANewerConversation() { + MarketblockConversation old = start(5); + MarketblockConversation current = start(0); + + assertFalse(ConversationManager.finishConversation(tester.player, old)); + assertSame(current, ConversationManager.getConversation(tester.player)); + } + private MarketblockConversation start(int step) { MarketblockConversation convo = new MarketblockConversation(tester.player, null); for (int i = 0; i < step; i++) convo.nextStep(); From dc561201ac2dc55625ddc81f681366afb9dc9fe0 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:58:13 +0000 Subject: [PATCH 3/5] fix: handle one /marketblock add answer at a time Answers for a conversation now run one at a time under its lock and are ignored once it is no longer current. The final answer claims the conversation before setting the category, and cancel removes only the conversation it belongs to. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../manager/commands/ChatListener.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java index 2f21717..92987b8 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java @@ -35,12 +35,20 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { event.setCancelled(true); String message = event.getMessage(); - if (TradeInput.isCancel(message)) { - ConversationManager.endConversation(player); - player.sendMessage("§eTrade creation cancelled."); - return; + // One answer at a time per conversation, and none after it has been cancelled or finished. + synchronized (convo) { + if (ConversationManager.getConversation(player) != convo) return; + if (TradeInput.isCancel(message)) { + if (ConversationManager.finishConversation(player, convo)) { + player.sendMessage("§eTrade creation cancelled."); + } + return; + } + answer(player, convo, message); } + } + private void answer(Player player, MarketblockConversation convo, String message) { switch (convo.getStep()) { case 0 -> { if (TradeLoader.getTradeById(message) != null) { @@ -97,14 +105,14 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { player.sendMessage("§aEnter the category:"); } case 5 -> { + // Done! Claim the conversation first, so a quit that got there first stops the save. + if (!ConversationManager.finishConversation(player, convo)) return; Category cat = CategoryLoader.getByString(message); if (cat == null || cat.getId().equalsIgnoreCase("unknown")) { player.sendMessage("§cWarning, no category found, default selected"); } convo.setCategory(cat); player.sendMessage("§aCategory set to: " + cat.getId()); - // Done! Claim the conversation first, so a quit that got there first stops the save. - if (!ConversationManager.finishConversation(player, convo)) return; // Chat arrives on an async thread; trades and trades.yml belong to the main thread. Bukkit.getScheduler().runTask(MarketBlock.plugin, () -> { if (!saveTrade(player, convo)) return; From 41dc88a99a36d656df29544092f2e0ec4e437977 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:06:53 +0000 Subject: [PATCH 4/5] fix: let a late chat message through instead of dropping it A message whose conversation was cancelled, finished or replaced before its handler took the lock is no longer cancelled, so it reaches chat instead of disappearing. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../manager/commands/ChatListener.java | 13 +++++++--- .../manager/commands/ChatListenerTest.java | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java index 92987b8..2f8cbab 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java @@ -32,12 +32,19 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { return; } - event.setCancelled(true); - String message = event.getMessage(); + answerIfCurrent(event, player, convo); + } - // One answer at a time per conversation, and none after it has been cancelled or finished. + /** + * One answer at a time per conversation. A late message for a conversation that has already + * been cancelled, finished or replaced is not an answer, so it goes to chat as normal. + */ + @SuppressWarnings("deprecation") + void answerIfCurrent(AsyncPlayerChatEvent event, Player player, MarketblockConversation convo) { + String message = event.getMessage(); synchronized (convo) { if (ConversationManager.getConversation(player) != convo) return; + event.setCancelled(true); if (TradeInput.isCancel(message)) { if (ConversationManager.finishConversation(player, convo)) { player.sendMessage("§eTrade creation cancelled."); diff --git a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java index c0a4dfc..14f1680 100644 --- a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java +++ b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java @@ -73,6 +73,31 @@ void losingPermissionEndsTheConversationAndLeavesChatAlone() { assertNull(ConversationManager.getConversation(tester.player)); } + @Test + void aLateMessageForAFinishedConversationReachesChat() { + MarketblockConversation old = start(5); + ConversationManager.finishConversation(tester.player, old); + + // The handler looked the conversation up before another answer finished it. + AsyncPlayerChatEvent event = new AsyncPlayerChatEvent(true, tester.player, "hello", new HashSet<>()); + listener.answerIfCurrent(event, tester.player, old); + + assertFalse(event.isCancelled()); + assertTrue(tester.messages.isEmpty()); + } + + @Test + void aLateMessageForAReplacedConversationLeavesTheNewOneAlone() { + MarketblockConversation old = start(2); + MarketblockConversation current = start(0); + + AsyncPlayerChatEvent event = new AsyncPlayerChatEvent(true, tester.player, "cancel", new HashSet<>()); + listener.answerIfCurrent(event, tester.player, old); + + assertFalse(event.isCancelled()); + assertSame(current, ConversationManager.getConversation(tester.player)); + } + @Test void quittingEndsTheConversation() { start(0); From 83c6551e9ba66a52c027a82b219bba8dd772fca1 Mon Sep 17 00:00:00 2001 From: Ryan Barlow <7389646+ryanbarlow97@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:16:24 +0000 Subject: [PATCH 5/5] fix: recheck the admin permission under the conversation lock The mid-prompt permission check now runs inside the lock, after the conversation is confirmed current, and ends only that conversation, so a late message cannot remove a newer one. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../marketblock/manager/commands/ChatListener.java | 11 +++++------ .../manager/commands/ChatListenerTest.java | 13 +++++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java index 2f8cbab..9577627 100644 --- a/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java +++ b/src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java @@ -26,12 +26,6 @@ public void onPlayerChat(AsyncPlayerChatEvent event) { if (convo == null) return; - // Permission may have been removed since /marketblock add; let the message reach chat. - if (!player.hasPermission(CommandManager.ADMIN_PERMISSION)) { - ConversationManager.endConversation(player); - return; - } - answerIfCurrent(event, player, convo); } @@ -44,6 +38,11 @@ void answerIfCurrent(AsyncPlayerChatEvent event, Player player, MarketblockConve String message = event.getMessage(); synchronized (convo) { if (ConversationManager.getConversation(player) != convo) return; + // Permission may have been removed since /marketblock add; let the message reach chat. + if (!player.hasPermission(CommandManager.ADMIN_PERMISSION)) { + ConversationManager.finishConversation(player, convo); + return; + } event.setCancelled(true); if (TradeInput.isCancel(message)) { if (ConversationManager.finishConversation(player, convo)) { diff --git a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java index 14f1680..a6debf8 100644 --- a/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java +++ b/src/test/java/net/tfminecraft/marketblock/manager/commands/ChatListenerTest.java @@ -86,6 +86,19 @@ void aLateMessageForAFinishedConversationReachesChat() { assertTrue(tester.messages.isEmpty()); } + @Test + void losingPermissionDuringALateMessageLeavesTheNewConversationAlone() { + MarketblockConversation old = start(2); + MarketblockConversation current = start(0); + tester.admin = false; + + AsyncPlayerChatEvent event = new AsyncPlayerChatEvent(true, tester.player, "hello", new HashSet<>()); + listener.answerIfCurrent(event, tester.player, old); + + assertFalse(event.isCancelled()); + assertSame(current, ConversationManager.getConversation(tester.player)); + } + @Test void aLateMessageForAReplacedConversationLeavesTheNewOneAlone() { MarketblockConversation old = start(2);