diff --git a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java index bad6444..b0db53e 100644 --- a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java +++ b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java @@ -2,8 +2,6 @@ import java.util.List; -import org.eclipse.jdt.annotation.Nullable; - import world.bentobox.bentobox.api.commands.CompositeCommand; import world.bentobox.bentobox.api.user.User; import world.bentobox.bentobox.database.objects.Island; @@ -14,8 +12,6 @@ */ public class IslandChatCommand extends CompositeCommand { - private @Nullable Island island; - public IslandChatCommand(Chat addon, CompositeCommand parent, String label) { super(addon, parent, label); } @@ -30,15 +26,23 @@ public void setup() { @Override public boolean canExecute(User user, String label, List args) { - - island = this.getIslands().getIslandAt(user.getLocation()).orElse(null); - return island != null; + // Command instances are shared across all players, so the resolved island must not be + // cached on an instance field here (e.g. player A's island could otherwise leak into + // player B's execute() call) - it is recomputed per-player in execute() instead. + return this.getIslands().getIslandAt(user.getLocation()).isPresent(); } @Override public boolean execute(User user, String label, List args) { Chat addon = this.getAddon(); + // Resolve the island fresh for this specific invocation, since this command + // instance is shared by all players and must not rely on state set in canExecute(). + Island island = this.getIslands().getIslandAt(user.getLocation()).orElse(null); + if (island == null) { + return false; + } + // Send the message directly into island chat without the need of toggling it // if there is existence of more arguments if (!args.isEmpty()) { diff --git a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java index 0501a88..02dce89 100644 --- a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java +++ b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java @@ -271,6 +271,68 @@ public void testOnChatIslandChatPlayerNotRegistered() { assertFalse(event.isCancelled()); } + // ----------------------------------------------------------------------- + // onChat asynchronous tests - regression tests for repeating message bug + // ----------------------------------------------------------------------- + + /** + * Stubs the scheduler so that {@code callSyncMethod} runs the given callable + * immediately (as if the next server tick had already happened) and returns + * an already completed future, mimicking the real Bukkit scheduler behaviour + * closely enough for the async {@link ChatListener#onChat(AsyncPlayerChatEvent)} + * code path to be exercised in a unit test. + */ + private void stubSyncScheduler() { + when(sch.callSyncMethod(any(), any())).thenAnswer(invocation -> { + java.util.concurrent.Callable callable = invocation.getArgument(1); + try { + return java.util.concurrent.CompletableFuture.completedFuture(callable.call()); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + }); + } + + @Test + public void testOnChatAsyncIslandChatUsesCurrentMessageNotStale() { + stubSyncScheduler(); + + ChatListener spyListener = org.mockito.Mockito.spy(listener); + spyListener.toggleIslandChat(island, player); + when(im.getIslandAt(any())).thenReturn(Optional.of(island)); + + // First async chat message + AsyncPlayerChatEvent event1 = new AsyncPlayerChatEvent(true, player, "aaaa", Collections.emptySet()); + spyListener.onChat(event1); + assertTrue(event1.isCancelled()); + verify(spyListener).islandChat(island, player, "aaaa"); + + // Second async chat message must use its own content, not the previous one + AsyncPlayerChatEvent event2 = new AsyncPlayerChatEvent(true, player, "test", Collections.emptySet()); + spyListener.onChat(event2); + assertTrue(event2.isCancelled()); + verify(spyListener).islandChat(island, player, "test"); + } + + @Test + public void testOnChatAsyncTeamChatUsesCurrentMessageNotStale() { + stubSyncScheduler(); + + ChatListener spyListener = org.mockito.Mockito.spy(listener); + spyListener.togglePlayerTeamChat(uuid); + when(im.inTeam(any(World.class), any(UUID.class))).thenReturn(true); + + AsyncPlayerChatEvent event1 = new AsyncPlayerChatEvent(true, player, "aaaa", Collections.emptySet()); + spyListener.onChat(event1); + assertTrue(event1.isCancelled()); + verify(spyListener).teamChat(world, player, "aaaa"); + + AsyncPlayerChatEvent event2 = new AsyncPlayerChatEvent(true, player, "test", Collections.emptySet()); + spyListener.onChat(event2); + assertTrue(event2.isCancelled()); + verify(spyListener).teamChat(world, player, "test"); + } + // ----------------------------------------------------------------------- // teamChat tests // -----------------------------------------------------------------------