fix: carry-over fixes — per-type cooldowns, PM cooldown, nickname persistence, thread-safe deques
- PlayerServiceImpl: separate cooldown map per chat type (fixes cross-blocking between local chat and RP commands) - PrivateMessageServiceImpl: enforce chat.pm.cooldown (honors chat.bypass.cooldown) - NickServiceImpl: persist nicknames async on every change/reset - FloodFilter/SpamFilter: ConcurrentLinkedDeque instead of ArrayDeque - tests: PlayerServiceCooldownTest
This commit is contained in:
parent
b1b0c9082b
commit
cda89c78e4
8 changed files with 114 additions and 28 deletions
20
TODO.md
20
TODO.md
|
|
@ -99,26 +99,26 @@
|
||||||
|
|
||||||
**Fix:** Extract `FilePersistenceUtil` or base class
|
**Fix:** Extract `FilePersistenceUtil` or base class
|
||||||
|
|
||||||
### 2. Persist nicknames on change
|
### 2. Persist nicknames on change ✅
|
||||||
`NickServiceImpl.java:77` mutates only in-memory map; `save()` only on shutdown → nicknames lost on crash (async save after mutation).
|
`NickServiceImpl` now saves asynchronously (`FoliaUtil.runAsync → save()`) on every `setNickname`/`resetNickname` — nicknames survive crashes.
|
||||||
|
|
||||||
### 3. DIP: Service Locator Anti-Pattern
|
### 3. DIP: Service Locator Anti-Pattern
|
||||||
`ServiceRegistry.get(Xxx.class)` used everywhere instead of constructor DI. Big refactor.
|
`ServiceRegistry.get(Xxx.class)` used everywhere instead of constructor DI. Big refactor.
|
||||||
|
|
||||||
### 4. DRY: PM Send Logic Duplicated
|
### 4. DRY: PM Send Logic ✅
|
||||||
`MsgCommand` + `ReplyCommand` still have near-identical send logic.
|
PM cooldown + actual send centralized in `PrivateMessageServiceImpl.sendPrivateMessage`; both `/msg` and `/reply` delegate to it.
|
||||||
|
|
||||||
### 5. Fix cooldown map collision
|
### 5. Fix cooldown map collision ✅
|
||||||
`PlayerServiceImpl.java:80-82` — all non-global types (local chat + RP `/me /do /try /roll`) share one map → cross-blocking cooldowns. Split per-type maps.
|
`PlayerServiceImpl` now keeps a separate `ConcurrentHashMap` per chat type (`global`, `local`, `rp_me`, `rp_do`, `rp_try`, `pm`) — no more cross-blocking. Covered by `PlayerServiceCooldownTest`.
|
||||||
|
|
||||||
### 6. Thread-safe flood/spam deques
|
### 6. Thread-safe flood/spam deques ✅
|
||||||
`FloodFilter.java:32`, `SpamFilter.java:30` use plain `ArrayDeque` written from async threads, read from main → use synchronized/thread-safe structures.
|
`FloodFilter`/`SpamFilter` now use `ConcurrentLinkedDeque` instead of plain `ArrayDeque`.
|
||||||
|
|
||||||
### 7. Tests
|
### 7. Tests
|
||||||
Only 11 test files for 155 main files. Very low coverage (JaCoCo wired, CI uploads report).
|
Only 11 test files for 155 main files. Very low coverage (JaCoCo wired, CI uploads report).
|
||||||
|
|
||||||
### 8. No PM cooldown
|
### 8. No PM cooldown ✅
|
||||||
`chat.pm.cooldown: 2` in config.yml is never read → `/msg` spam unthrottled.
|
`chat.pm.cooldown` (default 2s) is now enforced in `PrivateMessageServiceImpl.sendPrivateMessage`, honoring `chat.bypass.cooldown`.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -5,18 +5,17 @@ import com.loki.lochat.core.filter.FilterResult;
|
||||||
|
|
||||||
import org.bukkit.entity.Player;
|
import org.bukkit.entity.Player;
|
||||||
|
|
||||||
import java.util.ArrayDeque;
|
|
||||||
import java.util.Deque;
|
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
import java.util.concurrent.ConcurrentHashMap;
|
import java.util.concurrent.ConcurrentHashMap;
|
||||||
|
import java.util.concurrent.ConcurrentLinkedDeque;
|
||||||
|
|
||||||
public class FloodFilter {
|
public class FloodFilter {
|
||||||
private static final String FLOOD_BLOCK_MESSAGE = "&#CF6679Не флудите!";
|
private static final String FLOOD_BLOCK_MESSAGE = "&#CF6679Не флудите!";
|
||||||
|
|
||||||
private final int maxMessages;
|
private final int maxMessages;
|
||||||
private final int timePeriod;
|
private final int timePeriod;
|
||||||
private final Map<UUID, Deque<Long>> floodTracker = new ConcurrentHashMap<>();
|
private final Map<UUID, ConcurrentLinkedDeque<Long>> floodTracker = new ConcurrentHashMap<>();
|
||||||
|
|
||||||
public FloodFilter(FiltersConfig filters) {
|
public FloodFilter(FiltersConfig filters) {
|
||||||
this.maxMessages = filters.getFloodMaxMessages();
|
this.maxMessages = filters.getFloodMaxMessages();
|
||||||
|
|
@ -31,8 +30,8 @@ public class FloodFilter {
|
||||||
long now = System.currentTimeMillis();
|
long now = System.currentTimeMillis();
|
||||||
long windowMs = timePeriod * 1000L;
|
long windowMs = timePeriod * 1000L;
|
||||||
|
|
||||||
Deque<Long> timestamps = floodTracker.computeIfAbsent(
|
ConcurrentLinkedDeque<Long> timestamps = floodTracker.computeIfAbsent(
|
||||||
player.getUniqueId(), k -> new ArrayDeque<>());
|
player.getUniqueId(), k -> new ConcurrentLinkedDeque<>());
|
||||||
|
|
||||||
// Удаляем старые timestamp'ы
|
// Удаляем старые timestamp'ы
|
||||||
while (!timestamps.isEmpty() && now - timestamps.peekFirst() > windowMs) {
|
while (!timestamps.isEmpty() && now - timestamps.peekFirst() > windowMs) {
|
||||||
|
|
|
||||||
|
|
@ -5,17 +5,16 @@ import com.loki.lochat.core.filter.FilterResult;
|
||||||
|
|
||||||
import org.bukkit.entity.Player;
|
import org.bukkit.entity.Player;
|
||||||
|
|
||||||
import java.util.ArrayDeque;
|
|
||||||
import java.util.Deque;
|
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
import java.util.concurrent.ConcurrentHashMap;
|
import java.util.concurrent.ConcurrentHashMap;
|
||||||
|
import java.util.concurrent.ConcurrentLinkedDeque;
|
||||||
|
|
||||||
public class SpamFilter {
|
public class SpamFilter {
|
||||||
private final int maxSimilar;
|
private final int maxSimilar;
|
||||||
private final int threshold;
|
private final int threshold;
|
||||||
private final String blockMessage;
|
private final String blockMessage;
|
||||||
private final Map<UUID, Deque<String>> spamTracker = new ConcurrentHashMap<>();
|
private final Map<UUID, ConcurrentLinkedDeque<String>> spamTracker = new ConcurrentHashMap<>();
|
||||||
|
|
||||||
public SpamFilter(FiltersConfig filters) {
|
public SpamFilter(FiltersConfig filters) {
|
||||||
this(filters.getSpamMaxSimilarMessages(), filters.getSpamSimilarityThreshold(),
|
this(filters.getSpamMaxSimilarMessages(), filters.getSpamSimilarityThreshold(),
|
||||||
|
|
@ -33,8 +32,8 @@ public class SpamFilter {
|
||||||
return FilterResult.ok(message);
|
return FilterResult.ok(message);
|
||||||
}
|
}
|
||||||
|
|
||||||
Deque<String> history = spamTracker.computeIfAbsent(
|
ConcurrentLinkedDeque<String> history = spamTracker.computeIfAbsent(
|
||||||
player.getUniqueId(), k -> new ArrayDeque<>());
|
player.getUniqueId(), k -> new ConcurrentLinkedDeque<>());
|
||||||
|
|
||||||
// Считаем похожие сообщения
|
// Считаем похожие сообщения
|
||||||
long similarCount = history.stream()
|
long similarCount = history.stream()
|
||||||
|
|
|
||||||
|
|
@ -52,7 +52,7 @@ public class ServiceRegistry {
|
||||||
|
|
||||||
register(ChatService.class, new ChatServiceImpl(plugin, messageService));
|
register(ChatService.class, new ChatServiceImpl(plugin, messageService));
|
||||||
|
|
||||||
PrivateMessageServiceImpl pmService = new PrivateMessageServiceImpl((LoChat) plugin);
|
PrivateMessageServiceImpl pmService = new PrivateMessageServiceImpl((LoChat) plugin, playerService);
|
||||||
MessagingService messagingService = ServiceFactory.createMessagingService(plugin, messageConfig, pmService);
|
MessagingService messagingService = ServiceFactory.createMessagingService(plugin, messageConfig, pmService);
|
||||||
pmService.init(messagingService);
|
pmService.init(messagingService);
|
||||||
register(PrivateMessageService.class, pmService);
|
register(PrivateMessageService.class, pmService);
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ import com.loki.lochat.api.service.NickService;
|
||||||
import com.loki.lochat.config.RatConfig;
|
import com.loki.lochat.config.RatConfig;
|
||||||
import com.loki.lochat.utils.format.ChatFormatter;
|
import com.loki.lochat.utils.format.ChatFormatter;
|
||||||
import com.loki.lochat.utils.persistence.FilePersistence;
|
import com.loki.lochat.utils.persistence.FilePersistence;
|
||||||
|
import com.loki.lochat.utils.platform.FoliaUtil;
|
||||||
|
|
||||||
import net.kyori.adventure.text.Component;
|
import net.kyori.adventure.text.Component;
|
||||||
import net.kyori.adventure.text.serializer.plain.PlainTextComponentSerializer;
|
import net.kyori.adventure.text.serializer.plain.PlainTextComponentSerializer;
|
||||||
|
|
@ -76,6 +77,9 @@ public class NickServiceImpl implements NickService {
|
||||||
// Устанавливаем ник
|
// Устанавливаем ник
|
||||||
nicknames.put(player, nickname);
|
nicknames.put(player, nickname);
|
||||||
|
|
||||||
|
// Персистим сразу — не теряем ники при падении сервера
|
||||||
|
FoliaUtil.runAsync(plugin, this::save);
|
||||||
|
|
||||||
// Обновляем display
|
// Обновляем display
|
||||||
Player onlinePlayer = Bukkit.getPlayer(player);
|
Player onlinePlayer = Bukkit.getPlayer(player);
|
||||||
if (onlinePlayer != null) {
|
if (onlinePlayer != null) {
|
||||||
|
|
@ -89,6 +93,9 @@ public class NickServiceImpl implements NickService {
|
||||||
public void resetNickname(UUID player) {
|
public void resetNickname(UUID player) {
|
||||||
nicknames.remove(player);
|
nicknames.remove(player);
|
||||||
|
|
||||||
|
// Персистим сразу
|
||||||
|
FoliaUtil.runAsync(plugin, this::save);
|
||||||
|
|
||||||
// Сбрасываем display
|
// Сбрасываем display
|
||||||
Player onlinePlayer = Bukkit.getPlayer(player);
|
Player onlinePlayer = Bukkit.getPlayer(player);
|
||||||
if (onlinePlayer != null) {
|
if (onlinePlayer != null) {
|
||||||
|
|
|
||||||
|
|
@ -20,9 +20,8 @@ public class PlayerServiceImpl implements PlayerService {
|
||||||
|
|
||||||
private final JavaPlugin plugin;
|
private final JavaPlugin plugin;
|
||||||
|
|
||||||
// Cooldown state
|
// Cooldown state — separate map per chat type (global, local, rp_me, rp_do, rp_try, pm...)
|
||||||
private final Map<UUID, Long> globalCooldowns = new ConcurrentHashMap<>();
|
private final Map<String, Map<UUID, Long>> cooldowns = new ConcurrentHashMap<>();
|
||||||
private final Map<UUID, Long> localCooldowns = new ConcurrentHashMap<>();
|
|
||||||
|
|
||||||
// Statistics state
|
// Statistics state
|
||||||
private final Map<UUID, Long> playerMessages = new ConcurrentHashMap<>();
|
private final Map<UUID, Long> playerMessages = new ConcurrentHashMap<>();
|
||||||
|
|
@ -74,12 +73,11 @@ public class PlayerServiceImpl implements PlayerService {
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void removeCooldown(UUID player) {
|
public void removeCooldown(UUID player) {
|
||||||
globalCooldowns.remove(player);
|
cooldowns.values().forEach(map -> map.remove(player));
|
||||||
localCooldowns.remove(player);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private Map<UUID, Long> getCooldownMap(String type) {
|
private Map<UUID, Long> getCooldownMap(String type) {
|
||||||
return "global".equals(type) ? globalCooldowns : localCooldowns;
|
return cooldowns.computeIfAbsent(type, k -> new ConcurrentHashMap<>());
|
||||||
}
|
}
|
||||||
|
|
||||||
// ========== Statistics Implementation ==========
|
// ========== Statistics Implementation ==========
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ package com.loki.lochat.core.service.messaging;
|
||||||
|
|
||||||
import com.loki.lochat.LoChat;
|
import com.loki.lochat.LoChat;
|
||||||
import com.loki.lochat.api.service.MessagingService;
|
import com.loki.lochat.api.service.MessagingService;
|
||||||
|
import com.loki.lochat.api.service.PlayerService;
|
||||||
import com.loki.lochat.api.service.pm.PrivateMessageService;
|
import com.loki.lochat.api.service.pm.PrivateMessageService;
|
||||||
import com.loki.lochat.utils.format.ChatFormatter;
|
import com.loki.lochat.utils.format.ChatFormatter;
|
||||||
import com.loki.lochat.utils.player.PlayerUtil;
|
import com.loki.lochat.utils.player.PlayerUtil;
|
||||||
|
|
@ -23,10 +24,12 @@ public class PrivateMessageServiceImpl implements PrivateMessageService {
|
||||||
|
|
||||||
private final Map<UUID, UUID> lastConversation = new ConcurrentHashMap<>();
|
private final Map<UUID, UUID> lastConversation = new ConcurrentHashMap<>();
|
||||||
private final LoChat plugin;
|
private final LoChat plugin;
|
||||||
|
private final PlayerService playerService;
|
||||||
private MessagingService messagingService;
|
private MessagingService messagingService;
|
||||||
|
|
||||||
public PrivateMessageServiceImpl(LoChat plugin) {
|
public PrivateMessageServiceImpl(LoChat plugin, PlayerService playerService) {
|
||||||
this.plugin = plugin;
|
this.plugin = plugin;
|
||||||
|
this.playerService = playerService;
|
||||||
}
|
}
|
||||||
|
|
||||||
public void init(MessagingService messagingService) {
|
public void init(MessagingService messagingService) {
|
||||||
|
|
@ -56,6 +59,19 @@ public class PrivateMessageServiceImpl implements PrivateMessageService {
|
||||||
@Override
|
@Override
|
||||||
public void sendPrivateMessage(CommandSender sender, Player target, String message) {
|
public void sendPrivateMessage(CommandSender sender, Player target, String message) {
|
||||||
Player playerSender = (Player) sender;
|
Player playerSender = (Player) sender;
|
||||||
|
|
||||||
|
int cooldown = plugin.getConfig().getInt("chat.pm.cooldown", 2);
|
||||||
|
if (cooldown > 0 && !playerSender.hasPermission("chat.bypass.cooldown")
|
||||||
|
&& playerService.isOnCooldown(playerSender.getUniqueId(), "pm", cooldown)) {
|
||||||
|
int remaining = playerService.getRemainingCooldown(
|
||||||
|
playerSender.getUniqueId(), "pm", cooldown);
|
||||||
|
String cooldownMessage = plugin.getConfigManager().getMessagesConfig()
|
||||||
|
.getCooldownMessage().replace("{remaining}", String.valueOf(remaining));
|
||||||
|
sender.sendMessage(ChatFormatter.parse(cooldownMessage));
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
playerService.setCooldown(playerSender.getUniqueId(), "pm");
|
||||||
|
|
||||||
sender.sendMessage(ChatFormatter.formatPmSentNew(
|
sender.sendMessage(ChatFormatter.formatPmSentNew(
|
||||||
plugin.getMessageConfig().getPmFormatSent(), playerSender, target, message));
|
plugin.getMessageConfig().getPmFormatSent(), playerSender, target, message));
|
||||||
target.sendMessage(ChatFormatter.formatPmReceivedNew(
|
target.sendMessage(ChatFormatter.formatPmReceivedNew(
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,67 @@
|
||||||
|
package com.loki.lochat.core.service;
|
||||||
|
|
||||||
|
import com.loki.lochat.utils.persistence.FilePersistence;
|
||||||
|
|
||||||
|
import org.bukkit.configuration.file.YamlConfiguration;
|
||||||
|
import org.bukkit.plugin.java.JavaPlugin;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.mockito.MockedStatic;
|
||||||
|
import org.mockito.Mockito;
|
||||||
|
|
||||||
|
import java.io.File;
|
||||||
|
import java.util.UUID;
|
||||||
|
|
||||||
|
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||||
|
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||||
|
import static org.mockito.Mockito.mock;
|
||||||
|
import static org.mockito.Mockito.when;
|
||||||
|
|
||||||
|
class PlayerServiceCooldownTest {
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void cooldown_typesAreIndependent() {
|
||||||
|
try (MockedStatic<FilePersistence> fp = Mockito.mockStatic(FilePersistence.class)) {
|
||||||
|
JavaPlugin plugin = mock(JavaPlugin.class);
|
||||||
|
fp.when(() -> FilePersistence.getFile(plugin, "data/statistics.yml"))
|
||||||
|
.thenReturn(new File("/tmp/nonexistent-statistics.yml"));
|
||||||
|
fp.when(() -> FilePersistence.loadYaml(plugin, "data/statistics.yml"))
|
||||||
|
.thenReturn(new YamlConfiguration());
|
||||||
|
PlayerServiceImpl service = new PlayerServiceImpl(plugin);
|
||||||
|
|
||||||
|
UUID player = UUID.randomUUID();
|
||||||
|
service.setCooldown(player, "rp_me");
|
||||||
|
service.setCooldown(player, "local");
|
||||||
|
|
||||||
|
// rp_me на кулдауне
|
||||||
|
assertTrue(service.isOnCooldown(player, "rp_me", 5));
|
||||||
|
// local на кулдауне
|
||||||
|
assertTrue(service.isOnCooldown(player, "local", 5));
|
||||||
|
// rp_try НЕ на кулдауне (разные типы не блокируют друг друга)
|
||||||
|
assertFalse(service.isOnCooldown(player, "rp_try", 5));
|
||||||
|
// global НЕ на кулдауне
|
||||||
|
assertFalse(service.isOnCooldown(player, "global", 5));
|
||||||
|
// pm НЕ на кулдауне
|
||||||
|
assertFalse(service.isOnCooldown(player, "pm", 5));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void removeCooldown_clearsAllTypes() {
|
||||||
|
try (MockedStatic<FilePersistence> fp = Mockito.mockStatic(FilePersistence.class)) {
|
||||||
|
JavaPlugin plugin = mock(JavaPlugin.class);
|
||||||
|
fp.when(() -> FilePersistence.getFile(plugin, "data/statistics.yml"))
|
||||||
|
.thenReturn(new File("/tmp/nonexistent-statistics.yml"));
|
||||||
|
fp.when(() -> FilePersistence.loadYaml(plugin, "data/statistics.yml"))
|
||||||
|
.thenReturn(new YamlConfiguration());
|
||||||
|
PlayerServiceImpl service = new PlayerServiceImpl(plugin);
|
||||||
|
|
||||||
|
UUID player = UUID.randomUUID();
|
||||||
|
service.setCooldown(player, "global");
|
||||||
|
service.setCooldown(player, "rp_do");
|
||||||
|
service.removeCooldown(player);
|
||||||
|
|
||||||
|
assertFalse(service.isOnCooldown(player, "global", 5));
|
||||||
|
assertFalse(service.isOnCooldown(player, "rp_do", 5));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Add table
Add a link
Reference in a new issue