diff --git a/src/main/java/tfmc/justin/activity/commands/ActivityCommand.java b/src/main/java/tfmc/justin/activity/commands/ActivityCommand.java index 47447a1..b6a6770 100644 --- a/src/main/java/tfmc/justin/activity/commands/ActivityCommand.java +++ b/src/main/java/tfmc/justin/activity/commands/ActivityCommand.java @@ -7,6 +7,7 @@ import org.bukkit.command.CommandSender; import org.bukkit.command.TabCompleter; import org.bukkit.entity.Player; +import tfmc.justin.activity.config.ActivityConfiguration; import tfmc.justin.activity.config.Messages; import tfmc.justin.activity.gui.ActivityGui; import tfmc.justin.activity.managers.ActivityManager; @@ -99,7 +100,8 @@ private void handleReset(CommandSender sender, String[] args) { } PlayerData data = manager.getStore().get(target.getUniqueId()); - data.reset(manager.getConfiguration().currentWeekKey(), manager.getConfiguration().currentDayKey()); + ActivityConfiguration.Keys keys = manager.getConfiguration().currentKeys(); + data.reset(keys.week(), keys.day()); manager.getStore().markDirty(); // The resolved name, not what was typed - casing and the cache decide diff --git a/src/main/java/tfmc/justin/activity/config/ActivityConfiguration.java b/src/main/java/tfmc/justin/activity/config/ActivityConfiguration.java index de6246a..7a0e215 100644 --- a/src/main/java/tfmc/justin/activity/config/ActivityConfiguration.java +++ b/src/main/java/tfmc/justin/activity/config/ActivityConfiguration.java @@ -9,7 +9,6 @@ import tfmc.justin.activity.utils.Weeks; import java.time.DayOfWeek; -import java.time.LocalDate; import java.time.LocalDateTime; import java.util.ArrayList; import java.util.Collections; @@ -77,6 +76,10 @@ public void load() { loadActivities(config.getConfigurationSection("activities")); rewardCommands = config.getStringList("rewards.commands"); + if (rewardCommands.isEmpty()) { + plugin.getLogger().warning("rewards.commands is empty - a full bar can be reached but nothing" + + " can ever be claimed."); + } rewardDisplay = config.getStringList("rewards.display"); guiTitle = config.getString("gui.title", "&8Weekly Activity"); @@ -112,15 +115,6 @@ private void loadActivities(ConfigurationSection section) { continue; } - // The old key: its meaning moved to 'every' with 'points' per award, - // so an unconverted entry would hand out its old 50-point award per - // single action. Loud and skipped rather than silently wrong. - if (entry.contains("daily-goal")) { - plugin.getLogger().warning("Activity '" + id + "' still uses 'daily-goal' - skipping it." - + " Convert to every/points/daily-cap (see the default config.yml)."); - continue; - } - int points = wholeNumber(entry, id, "points", 0); if (points <= 0) { plugin.getLogger().warning("Activity '" + id + "' is worth " + points @@ -231,16 +225,16 @@ public ActivityDef activity(String id) { } // ==================================== - // The keys "right now" falls in. Every caller used to spell out the same - // LocalDateTime.now() plus two Weeks calls; a reset boundary belongs to - // the config that defines it. + // Both keys off one clock reading. Asking for them separately can straddle + // a midnight tick and produce a day key from the new day with a week key + // from the old one, which reads as a rollover that never happened. // ==================================== - public String currentWeekKey() { - return Weeks.weekKey(LocalDateTime.now(), resetDay, resetHour); + public Keys currentKeys() { + LocalDateTime now = LocalDateTime.now(); + return new Keys(Weeks.weekKey(now, resetDay, resetHour), Weeks.dayKey(now.toLocalDate())); } - public String currentDayKey() { - return Weeks.dayKey(LocalDate.now()); + public record Keys(String week, String day) { } public DayOfWeek resetDay() { diff --git a/src/main/java/tfmc/justin/activity/gui/ActivityGui.java b/src/main/java/tfmc/justin/activity/gui/ActivityGui.java index e3dbd93..5816ba6 100644 --- a/src/main/java/tfmc/justin/activity/gui/ActivityGui.java +++ b/src/main/java/tfmc/justin/activity/gui/ActivityGui.java @@ -115,7 +115,7 @@ private ItemStack rewardItem(ActivityConfiguration config, Messages messages, Pl List lore = new ArrayList<>(); lore.add(messages.get("gui.reward-lore-header", "%every%", config.rewardEvery())); lore.addAll(Messages.colorize(config.rewardDisplay())); - lore.add(messages.get("gui.reward-lore-claimed", "%claimed%", data.claimed(), + lore.add(messages.get("gui.reward-lore-claimed", "%claimed%", data.claimedPoints() / config.rewardEvery(), "%total%", config.barMax() / config.rewardEvery())); lore.add(due > 0 ? messages.get("gui.reward-click", "%count%", due) @@ -152,9 +152,40 @@ public void onClick(InventoryClickEvent event) { return; } + // The permission is checked on /activity, but an inventory can outlive + // the permission that opened it - a revoked player must not still be + // able to click a reward out of a window they left open + if (!player.hasPermission("activity.use")) { + return; + } + ActivityConfiguration config = manager.getConfiguration(); if (manager.claim(player) == 0) { - player.sendMessage(config.messages().get("reward-nothing")); + // ==================================== + // A claim of 0 is either "nothing was due" or a refusal, and every + // refusal has already told the player why - saying "nothing to + // claim yet" on top of that contradicts it. So the only two lines + // sent here are the ones claim() cannot send itself: the missing + // reward commands, which it leaves silent on purpose, and the + // genuinely empty claim, which still owes the player an answer. + // ==================================== + // A store that never loaded refuses every claim and has already + // said so - what is or is not configured is beside the point + if (!manager.getStore().isLoaded()) { + return; + } + + if (config.rewardCommands().isEmpty()) { + player.sendMessage(config.messages().get("reward-unconfigured")); + return; + } + + // peek, not get: deciding which line to send must not create an + // entry or dirty the store + PlayerData data = manager.getStore().peek(player.getUniqueId()); + if (data == null || data.claimable(config.rewardEvery()) == 0) { + player.sendMessage(config.messages().get("reward-nothing")); + } return; } event.getView().getTopInventory().setItem(REWARD_SLOT, diff --git a/src/main/java/tfmc/justin/activity/hooks/PlaceholderHook.java b/src/main/java/tfmc/justin/activity/hooks/PlaceholderHook.java index a8fc343..3044875 100644 --- a/src/main/java/tfmc/justin/activity/hooks/PlaceholderHook.java +++ b/src/main/java/tfmc/justin/activity/hooks/PlaceholderHook.java @@ -61,7 +61,10 @@ public String onRequest(OfflinePlayer player, String params) { ActivityConfiguration config = manager.getConfiguration(); PlayerData data = manager.getStore().peek(player.getUniqueId()); - boolean currentWeek = data != null && data.weekKey().equals(config.currentWeekKey()); + // One clock reading for both keys: asked separately they can straddle + // a midnight tick and disagree about which day this week it is + ActivityConfiguration.Keys keys = config.currentKeys(); + boolean currentWeek = data != null && data.weekKey().equals(keys.week()); // Clamped here too: peek() never runs the clamp get() does, so a lowered // bar.max would otherwise read over 100% until the next action int points = currentWeek ? Math.min(data.points(), config.barMax()) : 0; @@ -99,7 +102,7 @@ public String onRequest(OfflinePlayer player, String params) { } // "Done" is the daily cap reached, or - for an uncapped activity - // at least one award earned today - int today = currentWeek && data.dayKey().equals(config.currentDayKey()) + int today = currentWeek && data.dayKey().equals(keys.day()) ? def.worth(data.count(def.id())) : 0; boolean done = def.dailyCap() > 0 ? today >= def.dailyCap() : today > 0; return Utils.colorize(config.messages().raw(done ? "placeholder.done" : "placeholder.not-done")); diff --git a/src/main/java/tfmc/justin/activity/listeners/VoteListener.java b/src/main/java/tfmc/justin/activity/listeners/VoteListener.java index f977744..fdf82a5 100644 --- a/src/main/java/tfmc/justin/activity/listeners/VoteListener.java +++ b/src/main/java/tfmc/justin/activity/listeners/VoteListener.java @@ -8,8 +8,10 @@ import org.bukkit.event.EventHandler; import org.bukkit.event.EventPriority; import org.bukkit.event.Listener; +import org.bukkit.plugin.IllegalPluginAccessException; import org.bukkit.plugin.java.JavaPlugin; import tfmc.justin.activity.managers.ActivityManager; +import tfmc.justin.activity.utils.Utils; import java.util.UUID; @@ -46,35 +48,45 @@ public void onVote(PlayerVoteEvent event) { return; } - UUID uuid = resolve(event); - if (uuid == null) { - return; - } + // Only plain data is read off the vote thread; every lookup that + // touches server state happens on the tick below + VotingPluginUser user = event.getVotingPluginUser(); + UUID javaUuid = user == null ? null : user.getJavaUUID(); + String name = event.getPlayer(); // The scheduler throws for a disabled plugin, and a vote arriving // during shutdown has nowhere to be scored anyway - if (manager == null || !plugin.isEnabled()) { + if (!plugin.isEnabled()) { return; } - Bukkit.getScheduler().runTask(plugin, () -> manager.recordAction(uuid, "vote", 1)); + // isEnabled() was true a line ago, but a disable landing in between + // would throw on VotingPlugin's thread rather than ours + try { + Bukkit.getScheduler().runTask(plugin, () -> { + UUID uuid = resolve(javaUuid, name); + if (uuid != null) { + manager.recordAction(uuid, "vote", 1); + } + }); + } catch (IllegalPluginAccessException | IllegalStateException e) { + // Plugin is being disabled, vote dropped + } } // ==================================== - // Every fallback here reads a local cache only, which is what makes this - // safe on the async vote thread. Bukkit.getOfflinePlayer(String) is not - // used at all: it blocks on a Mojang lookup and invents a UUID for names - // that have never played, which would quietly fill players.yml with junk. + // Main thread only: Bukkit.getPlayerExact and the offline-player cache are + // not safe off a tick. Bukkit.getOfflinePlayer(String) is not used at all: + // it blocks on a Mojang lookup and invents a UUID for names that have + // never played, which would quietly fill players.yml with junk. // ==================================== - private UUID resolve(PlayerVoteEvent event) { - VotingPluginUser user = event.getVotingPluginUser(); - if (user != null && user.getJavaUUID() != null) { - return user.getJavaUUID(); + private UUID resolve(UUID javaUuid, String name) { + if (javaUuid != null) { + return javaUuid; } // Proxy-forwarded votes can arrive with no user attached; the name is // always set - String name = event.getPlayer(); if (name == null || name.isBlank()) { plugin.getLogger().warning("Ignoring a vote with neither a user nor a player name."); return null; @@ -90,7 +102,8 @@ private UUID resolve(PlayerVoteEvent event) { return cached.getUniqueId(); } - plugin.getLogger().info("Dropping a vote from '" + name + "' - no cached player by that name."); + plugin.getLogger().info("Dropping a vote from '" + Utils.safeForLog(name) + + "' - no cached player by that name."); return null; } } diff --git a/src/main/java/tfmc/justin/activity/managers/ActivityManager.java b/src/main/java/tfmc/justin/activity/managers/ActivityManager.java index 9c64143..29f53b5 100644 --- a/src/main/java/tfmc/justin/activity/managers/ActivityManager.java +++ b/src/main/java/tfmc/justin/activity/managers/ActivityManager.java @@ -11,6 +11,7 @@ import tfmc.justin.activity.store.PlayerStore; import tfmc.justin.activity.utils.Utils; +import java.util.List; import java.util.UUID; import java.util.regex.Pattern; @@ -28,12 +29,18 @@ public class ActivityManager { // ==================================== private static final Pattern SAFE_NAME = Pattern.compile("^[A-Za-z0-9_.]{1,16}$"); - private static ActivityManager instance; + // Nulled by shutdown() on the main thread while a vote can still be in + // flight on VotingPlugin's own thread + private static volatile ActivityManager instance; private final JavaPlugin plugin; private final ActivityConfiguration config; private final PlayerStore store; + // A store that never loaded refuses every save for the rest of the + // session, so saying so once per session is enough + private boolean warnedStoreNotLoaded; + private ActivityManager(JavaPlugin plugin) { this.plugin = plugin; this.config = new ActivityConfiguration(plugin); @@ -130,73 +137,184 @@ public boolean recordAction(UUID uuid, String activityId, int amount) { // ==================================== // Hands over every milestone the player has reached but not yet claimed. // Only ever called for an online player (a GUI click), so %player% always - // resolves. Returns how many milestones were paid out. + // resolves. Returns how many milestones were actually paid: 0 covers both + // "nothing was due" and every refusal below, all of which leave the + // thresholds there to claim once an operator has fixed what broke. + // + // The order is the whole point, because a reward has real in-game value: + // + // 1. Every reason to refuse is checked before anything is touched. The + // unsafe-name case especially - that is what a Bedrock player clicking + // the chest over and over hits, and it must not cost a disk write. + // 2. The thresholds are burned and written to disk BEFORE the first + // command runs: a crash between the give and the next autosave would + // otherwise hand the whole lot out again on restart, and a reward + // command that loops back in here must find them already paid. + // 3. A save that failed pays nothing at all. The tick blocks on one file + // write, which is the price of never paying twice. + // 4. If the dispatch stops early - and it can, since a reward command + // belongs to another plugin and is free to throw - only what actually + // went out stays burned; the rest goes back, never below where it + // started. + // + // The command list is snapshotted once, so a reward command that runs + // /activity reload cannot change what the rest of the loop hands out. // ==================================== public int claim(Player player) { + Messages messages = config.messages(); + + // Nothing may be paid while nothing can be persisted: a store that + // never loaded refuses every write for the rest of the session, so a + // payout here would last only until the next restart and then repeat + if (!store.isLoaded()) { + if (!warnedStoreNotLoaded) { + warnedStoreNotLoaded = true; + plugin.getLogger().severe("Refusing every reward claim: " + PlayerStore.FILE + + " was never loaded, so nothing handed over could be saved."); + } + player.sendMessage(messages.get("reward-failed")); + return 0; + } + PlayerData data = store.get(player.getUniqueId()); - int due = data.claimable(config.rewardEvery()); + int every = config.rewardEvery(); + int due = data.claimable(every); if (due == 0) { return 0; } + List commands = List.copyOf(config.rewardCommands()); + + // Nothing configured to hand over: burning the thresholds here would + // pay the player in silence. Load already warned about this. + if (commands.isEmpty()) { + return 0; + } + + // A name no command can take is a failure the player should hear + // about, and it is answered here - before any mutation or save - + // because it is the one refusal a player can trigger at click rate + if (!canRunAnyRewardCommand(commands, player.getName())) { + plugin.getLogger().warning("No reward command could be run for '" + + Utils.safeForLog(player.getName()) + "': the name cannot be safely pasted into a console" + + " command. Use %uuid%-based reward commands to support Bedrock/unsafe names."); + player.sendMessage(messages.get("reward-failed")); + return 0; + } + + int claimedBefore = data.claimedPoints(); + int claimedAfter = nextClaimedPoints(data.points(), every); + data.setClaimedPoints(claimedAfter); + store.markDirty(); + + if (!store.saveNow()) { + data.setClaimedPoints(claimedBefore); + store.markDirty(); + plugin.getLogger().severe("Handed nothing to " + player.getUniqueId() + ": claimed-points could" + + " not be saved before the rewards ran (would have gone " + claimedBefore + " -> " + + claimedAfter + "), so nothing was dispatched."); + player.sendMessage(messages.get("reward-failed")); + return 0; + } + + int paid = 0; + for (int i = 0; i < due; i++) { + if (!dispatchRewards(player, commands)) { + break; + } + paid++; + } + // ==================================== - // Marked and written before the commands run: a reward command that - // loops back into this plugin must find the milestones already paid - // out, and a crash between the give and the next autosave tick would - // otherwise hand the whole lot out a second time. + // Only what actually went out stays paid for. The second save is the + // one that can leave memory and disk disagreeing, and the only honest + // thing to do about it is to say exactly what each holds so an + // operator can put the file right by hand. // ==================================== - data.setClaimed(data.claimed() + due); - store.markDirty(); - store.saveSoon(); - - // An empty rewards.commands list means there is nothing configured to - // hand over - not a failure, so it stays silent. A name no command can - // take is a failure the player should hear about, not just the console. - if (!config.rewardCommands().isEmpty()) { - for (int i = 0; i < due; i++) { - if (!dispatchRewards(player)) { - plugin.getLogger().warning("No reward command could be run for '" + player.getName() - + "': the name cannot be safely pasted into a console command. Use %uuid%-based" - + " reward commands to support Bedrock/unsafe names."); - player.sendMessage(config.messages().get("reward-failed")); - return due; - } + if (paid < due) { + int rolledBack = rollbackClaimedPoints(claimedBefore, paid, every); + data.setClaimedPoints(rolledBack); + store.markDirty(); + if (!store.saveNow()) { + plugin.getLogger().severe("Reward payout for " + player.getUniqueId() + " is out of sync:" + + " paid " + paid + " of " + due + " milestones, claimed-points is " + rolledBack + + " in memory but " + claimedAfter + " on disk. Repair " + PlayerStore.FILE + " by hand."); } + player.sendMessage(messages.get("reward-failed")); + return paid; } - player.sendMessage(config.messages().get("reward-claimed", "%count%", due)); + player.sendMessage(messages.get("reward-claimed", "%count%", paid)); playSound(player, config.barCompleteSound()); - return due; + return paid; + } + + // The threshold that every point on the bar has now been paid for + static int nextClaimedPoints(int points, int every) { + return points / every * every; + } + + // What stays burned when only part of the payout went out. Built up from + // claimedBefore rather than down from the new threshold, so a failure can + // never hand back a milestone that was paid before this click. + static int rollbackClaimedPoints(int claimedBefore, int paid, int every) { + return claimedBefore + paid * every; + } + + private static boolean canRunAnyRewardCommand(List commands, String name) { + for (String command : commands) { + if (canRunRewardCommand(command, name)) { + return true; + } + } + return false; } // ==================================== // True only if at least one command actually ran. The name check is per // command rather than per player: an unsafe name skips the ones that paste - // it and leaves the %uuid%-only ones working. + // it and leaves the %uuid%-only ones working. The list is handed in so + // every milestone of one claim pays out of the same snapshot. // ==================================== - private boolean dispatchRewards(Player player) { + private boolean dispatchRewards(Player player, List commands) { String name = player.getName(); String uuid = player.getUniqueId().toString(); boolean ranAny = false; - for (String command : config.rewardCommands()) { + for (String command : commands) { if (!canRunRewardCommand(command, name)) { - plugin.getLogger().warning("Skipping reward command '" + command + "' for '" + name - + "': the name is not one that can be safely pasted into a console command."); + plugin.getLogger().warning("Skipping reward command '" + command + "' for '" + + Utils.safeForLog(name) + "': the name is not one that can be safely pasted into a" + + " console command."); continue; } - Bukkit.dispatchCommand(Bukkit.getConsoleSender(), - command.replace("%player%", name).replace("%uuid%", uuid)); - ranAny = true; + // ==================================== + // A reward command runs somebody else's plugin, which is free to + // throw. Letting that unwind would leave claim() with the + // thresholds already burned and saved and no chance to put them + // back, so a throwing command counts as "did not run" and stops + // the payout at the milestone it broke on. + // ==================================== + try { + Bukkit.dispatchCommand(Bukkit.getConsoleSender(), + command.replace("%player%", name).replace("%uuid%", uuid)); + ranAny = true; + } catch (Throwable t) { + plugin.getLogger().severe("Reward command '" + command + "' threw for " + + Utils.safeForLog(name) + ": " + t); + } } return ranAny; } + // A joiner who has never scored anything gets no entry: rolled() reads + // what is already there rather than creating a row per player who ever + // logged in public void onJoin(Player player) { - PlayerData data = store.get(player.getUniqueId()); - if (data.claimable(config.rewardEvery()) > 0) { + PlayerData data = store.rolled(player.getUniqueId()); + if (data != null && data.claimable(config.rewardEvery()) > 0) { player.sendMessage(config.messages().get("reward-ready")); } } diff --git a/src/main/java/tfmc/justin/activity/models/PlayerData.java b/src/main/java/tfmc/justin/activity/models/PlayerData.java index c17ae10..38cbf1c 100644 --- a/src/main/java/tfmc/justin/activity/models/PlayerData.java +++ b/src/main/java/tfmc/justin/activity/models/PlayerData.java @@ -20,19 +20,23 @@ public class PlayerData { private volatile int points; private volatile String weekKey; private volatile String dayKey; - // Milestones already paid out this week - private volatile int claimed; + // ==================================== + // The highest point threshold already paid out, not a milestone count: + // lowering bar.reward-every and reloading must not make thresholds that + // were already handed over claimable a second time. + // ==================================== + private volatile int claimedPoints; public PlayerData(String weekKey, String dayKey) { this.weekKey = weekKey; this.dayKey = dayKey; } - public PlayerData(int points, String weekKey, String dayKey, int claimed, Map daily) { + public PlayerData(int points, String weekKey, String dayKey, int claimedPoints, Map daily) { this.points = points; this.weekKey = weekKey; this.dayKey = dayKey; - this.claimed = claimed; + this.claimedPoints = claimedPoints; this.daily.putAll(daily); } @@ -46,7 +50,7 @@ public boolean roll(String weekKey, String dayKey) { if (!this.weekKey.equals(weekKey)) { points = 0; - claimed = 0; + claimedPoints = 0; daily.clear(); this.weekKey = weekKey; changed = true; @@ -100,12 +104,12 @@ public boolean clamp(int max) { } public int claimable(int rewardEvery) { - return Math.max(0, points / rewardEvery - claimed); + return Math.max(0, points / rewardEvery - claimedPoints / rewardEvery); } public void reset(String weekKey, String dayKey) { points = 0; - claimed = 0; + claimedPoints = 0; daily.clear(); this.weekKey = weekKey; this.dayKey = dayKey; @@ -131,11 +135,11 @@ public String dayKey() { return dayKey; } - public int claimed() { - return claimed; + public int claimedPoints() { + return claimedPoints; } - public void setClaimed(int claimed) { - this.claimed = claimed; + public void setClaimedPoints(int claimedPoints) { + this.claimedPoints = claimedPoints; } } diff --git a/src/main/java/tfmc/justin/activity/store/PlayerStore.java b/src/main/java/tfmc/justin/activity/store/PlayerStore.java index ae5aa33..07605b6 100644 --- a/src/main/java/tfmc/justin/activity/store/PlayerStore.java +++ b/src/main/java/tfmc/justin/activity/store/PlayerStore.java @@ -33,7 +33,8 @@ // ==================================== public class PlayerStore { - private static final String FILE = "players.yml"; + // Public so a log line elsewhere can name the file it is talking about + public static final String FILE = "players.yml"; private final JavaPlugin plugin; private final ActivityConfiguration config; @@ -50,7 +51,7 @@ public class PlayerStore { // Nothing may be written before load() has said what is already on disk, // or an enable that failed early would save an empty store over a real file - private boolean loaded; + private volatile boolean loaded; // Set on the main thread by shutdown; volatile so a save path the // scheduler runs late still sees that the final write has been taken @@ -77,20 +78,26 @@ public PlayerStore(JavaPlugin plugin, ActivityConfiguration config) { public void load() { players.clear(); - if (file.exists()) { - read(); + // A file that could not even be copied aside must stay untouched: with + // loaded left false, write() and shutdown() both refuse to save, so the + // original survives until somebody looks at it + if (file.exists() && !read()) { + plugin.getLogger().severe("Saving is disabled until an operator moves " + FILE + + " aside by hand. Nothing earned from now on will be kept."); + return; } // Only now may anything be written back over the file loaded = true; } - private void read() { + // Returns false only when the existing file could neither be used nor + // safely copied aside + private boolean read() { YamlConfiguration yaml = new YamlConfiguration(); try { yaml.load(file); } catch (Exception e) { - quarantine("could not be parsed (" + e.getMessage() + ")"); - return; + return quarantine("could not be parsed (" + e.getMessage() + ")"); } ConfigurationSection root = yaml.getConfigurationSection("players"); @@ -102,16 +109,21 @@ private void read() { Set stray = yaml.getKeys(false); stray.remove("players"); if (!stray.isEmpty()) { - quarantine("has no 'players' section"); + return quarantine("has no 'players' section"); } - return; + return true; } for (String key : root.getKeys(false)) { readEntry(root, key); } + return true; } + // ==================================== + // Section and key are resolved once, here, because this is also the side + // that has a logger to say why a row was skipped. + // ==================================== private void readEntry(ConfigurationSection root, String key) { ConfigurationSection entry = root.getConfigurationSection(key); if (entry == null) { @@ -126,25 +138,52 @@ private void readEntry(ConfigurationSection root, String key) { return; } + players.put(uuid, parse(entry, config.barMax())); + } + + // ==================================== + // Test seam over the instance path above: the same "skip this row" rules - + // a missing section or a key that is not a UUID - reported as null rather + // than as a warning, so they can be exercised without a JavaPlugin. + // ==================================== + static PlayerData readEntry(ConfigurationSection root, String key, int barMax) { + ConfigurationSection entry = root.getConfigurationSection(key); + if (entry == null) { + return null; + } + + try { + UUID.fromString(key); + } catch (IllegalArgumentException e) { + return null; + } + + return parse(entry, barMax); + } + + // ==================================== + // One entry's values, with the key already dealt with by the caller. + // ==================================== + private static PlayerData parse(ConfigurationSection entry, int barMax) { Map daily = new HashMap<>(); ConfigurationSection dailySection = entry.getConfigurationSection("daily"); if (dailySection != null) { for (String id : dailySection.getKeys(false)) { - daily.put(id, dailySection.getInt(id)); + daily.put(id, Math.max(0, dailySection.getInt(id))); } } - // ponytail: pre-milestone files stored a 0-100 bar and a boolean - // 'rewarded'. Points are clamped and every milestone those points - // reach counts as already claimed, so an upgrade never makes anything - // instantly claimable - the old scale was paid out under old rules. - int points = Math.min(entry.getInt("points"), config.barMax()); - int claimed = entry.contains("rewarded") - ? points / config.rewardEvery() - : entry.getInt("claimed"); - - players.put(uuid, new PlayerData(points, entry.getString("week", ""), entry.getString("day", ""), - claimed, daily)); + // ==================================== + // Nothing read off disk is trusted: a hand-edited or garbled file must + // not be able to produce a negative bar, or claimed-points behind the + // points that were supposedly paid for, which would read as free + // rewards nobody earned. + // ==================================== + int points = Math.max(0, Math.min(entry.getInt("points"), barMax)); + int claimedPoints = Math.max(0, Math.min(entry.getInt("claimed-points"), points)); + + return new PlayerData(points, entry.getString("week", ""), entry.getString("day", ""), + claimedPoints, daily); } // ==================================== @@ -152,15 +191,16 @@ private void readEntry(ConfigurationSection root, String key) { // our data, is copied aside and reported loudly rather than being silently // overwritten by the next save. // ==================================== - private void quarantine(String reason) { + private boolean quarantine(String reason) { File backup = new File(file.getParentFile(), FILE + ".corrupt-" + System.currentTimeMillis()); try { Files.copy(file.toPath(), backup.toPath(), StandardCopyOption.REPLACE_EXISTING); plugin.getLogger().severe(FILE + " " + reason + " - kept a copy as " + backup.getName() + " and started from an empty store."); + return true; } catch (IOException e) { - plugin.getLogger().severe(FILE + " " + reason + ", and copying it aside failed: " + e.getMessage() - + " - started from an empty store."); + plugin.getLogger().severe(FILE + " " + reason + ", and copying it aside failed: " + e.getMessage()); + return false; } } @@ -170,20 +210,33 @@ private void quarantine(String reason) { // entry if this is the player's first action. // ==================================== public PlayerData get(UUID uuid) { - String week = config.currentWeekKey(); - String day = config.currentDayKey(); + PlayerData data = rolled(uuid); + if (data != null) { + return data; + } + + ActivityConfiguration.Keys keys = config.currentKeys(); + data = new PlayerData(keys.week(), keys.day()); + players.put(uuid, data); + dirty = true; + return data; + } + // ==================================== + // get() without the create: for callers that want current numbers for a + // player who may never have scored anything, and must not put them in the + // file just for asking. Main thread only, since it mutates what it finds. + // ==================================== + public PlayerData rolled(UUID uuid) { PlayerData data = players.get(uuid); if (data == null) { - data = new PlayerData(week, day); - players.put(uuid, data); - dirty = true; - return data; + return null; } + ActivityConfiguration.Keys keys = config.currentKeys(); // Clamp after the roll: a reload that lowered bar.max must not leave a // player above it - if (data.roll(week, day) | data.clamp(config.barMax())) { + if (data.roll(keys.week(), keys.day()) | data.clamp(config.barMax())) { dirty = true; } return data; @@ -245,11 +298,22 @@ public void saveSoon() { // dirty only clears on a write that actually landed - a failed write must // leave it set so the next autosave tick tries again instead of the // in-memory changes silently going unsaved. + // + // The result is returned rather than swallowed: the claim path pays out + // only once the thresholds it burned are known to be on disk. // ==================================== - private void saveNow() { - if (write(snapshot(), saveSeq.incrementAndGet())) { - dirty = false; + public boolean saveNow() { + if (!write(snapshot(), saveSeq.incrementAndGet())) { + return false; } + dirty = false; + return true; + } + + // Whether anything written now would actually reach disk - false means a + // corrupt file was never read and every save for this session is refused + public boolean isLoaded() { + return loaded; } public void shutdown() { @@ -275,15 +339,29 @@ public void shutdown() { } private YamlConfiguration snapshot() { + return snapshot(players); + } + + // ==================================== + // Pure over the map it is handed, so the "an empty player is no player" + // rule can be exercised without a store. + // ==================================== + static YamlConfiguration snapshot(Map players) { YamlConfiguration yaml = new YamlConfiguration(); for (Map.Entry entry : players.entrySet()) { PlayerData data = entry.getValue(); + // A player who has nothing is the same as a player with no entry, + // and writing one per joiner grows the file for no reason + if (data.points() == 0 && data.claimedPoints() == 0 && data.daily().isEmpty()) { + continue; + } + String path = "players." + entry.getKey(); yaml.set(path + ".points", data.points()); yaml.set(path + ".week", data.weekKey()); yaml.set(path + ".day", data.dayKey()); - yaml.set(path + ".claimed", data.claimed()); + yaml.set(path + ".claimed-points", data.claimedPoints()); yaml.set(path + ".daily", new LinkedHashMap<>(data.daily())); } return yaml; diff --git a/src/main/java/tfmc/justin/activity/utils/Utils.java b/src/main/java/tfmc/justin/activity/utils/Utils.java index 65d01b1..6200871 100644 --- a/src/main/java/tfmc/justin/activity/utils/Utils.java +++ b/src/main/java/tfmc/justin/activity/utils/Utils.java @@ -15,4 +15,16 @@ public static String colorize(String msg) { } return ChatColor.translateAlternateColorCodes('&', msg); } + + // ==================================== + // A player-supplied name on its way into the log. A name carrying a + // newline or an ANSI escape can forge log lines or repaint the console, + // so every control character is replaced before it is written. + // + // Not \p{Cntrl}, which is ASCII only: a Bedrock name can carry U+2028 or + // the right-to-left override U+202E, which break a log line just as well. + // ==================================== + public static String safeForLog(String name) { + return name == null ? "null" : name.replaceAll("[\\p{Cc}\\p{Cf}\\p{Zl}\\p{Zp}]", "?"); + } } diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index c942618..eca53dd 100644 --- a/src/main/resources/config.yml +++ b/src/main/resources/config.yml @@ -38,9 +38,9 @@ activities: # A command containing %player% is skipped with a warning when the name is not # [A-Za-z0-9_.]{1,16} (e.g. a Bedrock name with a space); commands that only # use %uuid% always run. If none of the commands below can run for a given -# player (their name is unsafe and every command pastes %player%), the -# milestone is still marked claimed - there is no retry, since the name will -# never become safe. To reward Bedrock/Floodgate or other unsafe names, write +# player (their name is unsafe and every command pastes %player%), nothing is +# marked claimed and the reward stays claimable, so fixing the commands below +# still pays it out. To reward Bedrock/Floodgate or other unsafe names, write # reward commands using only %uuid% (e.g. via a plugin that can look a player # up by UUID) instead of %player%. rewards: diff --git a/src/main/resources/messages.yml b/src/main/resources/messages.yml index 4851f0b..3156c83 100644 --- a/src/main/resources/messages.yml +++ b/src/main/resources/messages.yml @@ -3,6 +3,7 @@ reward-ready: "&a&lREWARD READY! &7Open &f/activity &7to claim." reward-claimed: "&a&lCLAIMED &7%count% reward(s) granted." reward-nothing: "&7Nothing to claim yet." reward-failed: "&cYour rewards could not be handed over - tell an admin." +reward-unconfigured: "&cRewards are not configured - tell an admin." gui: bar-name: "&fWeekly Activity: &a%points%&7/%max%" reward-name: "&6Rewards" diff --git a/src/test/java/tfmc/justin/activity/managers/ActivityManagerTest.java b/src/test/java/tfmc/justin/activity/managers/ActivityManagerTest.java index fe941e7..52a1e08 100644 --- a/src/test/java/tfmc/justin/activity/managers/ActivityManagerTest.java +++ b/src/test/java/tfmc/justin/activity/managers/ActivityManagerTest.java @@ -2,6 +2,7 @@ import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -66,4 +67,69 @@ void aCommandPastingTheNameRunsForASafeName() { void aNullCommandRunsNothing() { assertFalse(ActivityManager.canRunRewardCommand(null, "Notch")); } + + // ==================================== + // The claim arithmetic. A full claim burns every threshold the bar has + // reached; a partial one gives back only what never went out. + // ==================================== + @Test + void aFullClaimBurnsEveryReachedThreshold() { + assertEquals(20, ActivityManager.nextClaimedPoints(20, 10)); + // Points between thresholds do not pay for the next one + assertEquals(20, ActivityManager.nextClaimedPoints(27, 10)); + assertEquals(0, ActivityManager.nextClaimedPoints(9, 10)); + } + + @Test + void aPartialRollbackKeepsOnlyWhatWasPaid() { + // Claimed 10 before, 3 milestones were due, 1 went out: 10 + 1 * 10 + assertEquals(20, ActivityManager.rollbackClaimedPoints(10, 1, 10)); + assertEquals(30, ActivityManager.rollbackClaimedPoints(10, 2, 10)); + } + + @Test + void aRollbackNeverGoesBelowWhereItStarted() { + for (int paid = 0; paid <= 3; paid++) { + assertTrue(ActivityManager.rollbackClaimedPoints(10, paid, 10) >= 10); + } + } + + @Test + void payingNothingRollsBackToExactlyWhereItStarted() { + assertEquals(10, ActivityManager.rollbackClaimedPoints(10, 0, 10)); + assertEquals(0, ActivityManager.rollbackClaimedPoints(0, 0, 10)); + } + + // ==================================== + // The two bounds that make a partial payout safe, over every shape the + // numbers can take - including a claimedBefore that is not a multiple of + // every, which is what a reward-every change mid-week leaves behind: + // + // never below claimedBefore - a failure cannot hand back a milestone + // that was paid before this click + // always below a full claim - what did not go out stays claimable + // ==================================== + @Test + void aPartialRollbackStaysBetweenWhereItStartedAndAFullClaim() { + for (int points : new int[] {0, 7, 10, 19, 20, 45}) { + for (int every : new int[] {5, 10, 20}) { + for (int claimedBefore : new int[] {0, 7, 10}) { + int due = points / every - claimedBefore / every; + if (due <= 0) { + continue; + } + + int full = ActivityManager.nextClaimedPoints(points, every); + for (int paid = 0; paid < due; paid++) { + int rolledBack = ActivityManager.rollbackClaimedPoints(claimedBefore, paid, every); + String shape = "points=" + points + " every=" + every + + " claimedBefore=" + claimedBefore + " paid=" + paid; + + assertTrue(rolledBack >= claimedBefore, shape); + assertTrue(rolledBack < full, shape); + } + } + } + } + } } diff --git a/src/test/java/tfmc/justin/activity/models/PlayerDataTest.java b/src/test/java/tfmc/justin/activity/models/PlayerDataTest.java index d79950e..dd758f4 100644 --- a/src/test/java/tfmc/justin/activity/models/PlayerDataTest.java +++ b/src/test/java/tfmc/justin/activity/models/PlayerDataTest.java @@ -117,13 +117,24 @@ void claimingRemovesFromClaimable() { PlayerData data = data(); record(data, UNCAPPED, 10); - data.setClaimed(1); + data.setClaimedPoints(10); assertEquals(0, data.claimable(EVERY)); record(data, UNCAPPED, 10); assertEquals(1, data.claimable(EVERY)); } + // A paid threshold stays paid: with a milestone counter, halving + // reward-every used to make every already-claimed milestone claimable again + @Test + void loweringRewardEveryDoesNotReviveAPaidClaim() { + PlayerData data = data(); + record(data, UNCAPPED, 10); + data.setClaimedPoints(10); + + assertEquals(0, data.claimable(EVERY / 2)); + } + @Test void addPointsClampsNegativeValuesAtZero() { PlayerData data = data(); @@ -166,12 +177,12 @@ void clampPullsLegacyPointsDownToTheBar() { void newWeekWipesEverything() { PlayerData data = data(); record(data, UNCAPPED, 10); - data.setClaimed(1); + data.setClaimedPoints(10); assertTrue(data.roll("2026-09-14", "2026-09-14")); assertEquals(0, data.points()); - assertEquals(0, data.claimed()); + assertEquals(0, data.claimedPoints()); assertEquals(0, data.count("free")); } @@ -179,12 +190,12 @@ void newWeekWipesEverything() { void newDayWipesOnlyTheDailyCounters() { PlayerData data = data(); record(data, VOTE, 5); - data.setClaimed(1); + data.setClaimedPoints(10); assertTrue(data.roll(WEEK, "2026-09-10")); assertEquals(5, data.points()); - assertEquals(1, data.claimed()); + assertEquals(10, data.claimedPoints()); assertEquals(0, data.count("vote")); } @@ -203,13 +214,85 @@ void sameWeekAndDayChangeNothing() { void resetClearsTheWeekAndMovesTheKeys() { PlayerData data = data(); record(data, UNCAPPED, 10); - data.setClaimed(1); + data.setClaimedPoints(10); data.reset("2026-09-14", "2026-09-14"); assertEquals(0, data.points()); - assertEquals(0, data.claimed()); + assertEquals(0, data.claimedPoints()); assertEquals(0, data.count("free")); assertEquals("2026-09-14", data.weekKey()); } + + @Test + void claimableFollowsTheDivisionFormulaDirectly() { + PlayerData data = data(); + record(data, UNCAPPED, 15); + data.setClaimedPoints(10); + + // 15 / 5 - 10 / 5 = 3 - 2 = 1 + assertEquals(1, data.claimable(5)); + } + + @Test + void doublingRewardEveryAfterAFullClaimDoesNotReviveIt() { + PlayerData data = data(); + record(data, UNCAPPED, MAX); + // Claimed everything at EVERY (10): claimedPoints = (20/10)*10 = 20 + data.setClaimedPoints((data.points() / EVERY) * EVERY); + + assertEquals(0, data.claimable(EVERY * 2)); + } + + @Test + void nonDivisorRewardEveryAfterAFullClaimDoesNotReviveIt() { + PlayerData data = data(); + record(data, UNCAPPED, MAX); + data.setClaimedPoints((data.points() / EVERY) * EVERY); + + // 7 does not evenly divide bar.max (20) + assertEquals(0, data.claimable(7)); + } + + @Test + void changingRewardEveryStillExposesAReachableUnclaimedMilestone() { + PlayerData data = data(); + record(data, UNCAPPED, 10); + data.setClaimedPoints((data.points() / EVERY) * EVERY); + record(data, UNCAPPED, 7); + + // points=17, claimed=10. Under every=7: 17/7 - 10/7 = 2 - 1 = 1 + assertEquals(1, data.claimable(7)); + } + + // ==================================== + // claimable() is the gap between two thresholds, not points/every: a + // player who has already been paid for part of the bar must be owed only + // what is left, or every claim would hand out the whole bar again. + // ==================================== + @Test + void partlyClaimedPointsOnlyOweTheRemainder() { + PlayerData data = new PlayerData(20, WEEK, DAY, 10, java.util.Map.of()); + + // 20 / 5 - 10 / 5 = 4 - 2 = 2, not 20 / 5 = 4 and not 3 + assertEquals(2, data.claimable(5)); + } + + @Test + void pointsShortOfAThresholdDoNotOweIt() { + PlayerData data = new PlayerData(45, WEEK, DAY, 0, java.util.Map.of()); + + // 45 / 20 - 0 / 20 = 2: the last 5 points have not reached the third + assertEquals(2, data.claimable(20)); + } + + @Test + void claimableNeverGoesNegativeWhenClaimedPointsExceedsPoints() { + PlayerData data = data(); + record(data, UNCAPPED, 5); + + data.setClaimedPoints(100); + + assertEquals(0, data.claimable(EVERY)); + } } diff --git a/src/test/java/tfmc/justin/activity/store/PlayerStoreTest.java b/src/test/java/tfmc/justin/activity/store/PlayerStoreTest.java new file mode 100644 index 0000000..374bab2 --- /dev/null +++ b/src/test/java/tfmc/justin/activity/store/PlayerStoreTest.java @@ -0,0 +1,123 @@ +package tfmc.justin.activity.store; + +import org.bukkit.configuration.ConfigurationSection; +import org.bukkit.configuration.file.YamlConfiguration; +import org.junit.jupiter.api.Test; +import tfmc.justin.activity.models.PlayerData; + +import java.util.Map; +import java.util.UUID; + +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; + +// ==================================== +// PlayerStore's parsing/snapshot rules are pure logic over a +// ConfigurationSection / PlayerData, but PlayerStore itself needs a live +// JavaPlugin + ActivityConfiguration to construct, which this suite cannot +// build. Both rules are therefore exercised through the package-private +// statics the instance methods delegate to: +// +// - readEntry(ConfigurationSection root, String key, int barMax), returning +// null for a missing section or a key that is not a UUID +// - snapshot(Map) +// ==================================== +class PlayerStoreTest { + + private static final int BAR_MAX = 20; + + @Test + void pointsAreFlooredAtZeroAndCappedAtBarMax() { + ConfigurationSection root = new YamlConfiguration().createSection("players"); + UUID id = UUID.randomUUID(); + ConfigurationSection entry = root.createSection(id.toString()); + entry.set("points", 999); + entry.set("claimed-points", 0); + + PlayerData data = PlayerStore.readEntry(root, id.toString(), BAR_MAX); + assertEquals(BAR_MAX, data.points()); + + UUID negativeId = UUID.randomUUID(); + ConfigurationSection negative = root.createSection(negativeId.toString()); + negative.set("points", -5); + negative.set("claimed-points", 0); + PlayerData negativeData = PlayerStore.readEntry(root, negativeId.toString(), BAR_MAX); + assertEquals(0, negativeData.points()); + } + + @Test + void claimedPointsIsFlooredAtZeroAndCappedAtPoints() { + ConfigurationSection root = new YamlConfiguration().createSection("players"); + UUID id = UUID.randomUUID(); + ConfigurationSection entry = root.createSection(id.toString()); + entry.set("points", 10); + entry.set("claimed-points", 999); + + PlayerData data = PlayerStore.readEntry(root, id.toString(), BAR_MAX); + assertEquals(10, data.claimedPoints()); + + UUID negId = UUID.randomUUID(); + ConfigurationSection negEntry = root.createSection(negId.toString()); + negEntry.set("points", 10); + negEntry.set("claimed-points", -5); + PlayerData negData = PlayerStore.readEntry(root, negId.toString(), BAR_MAX); + assertEquals(0, negData.claimedPoints()); + } + + @Test + void everyDailyValueIsFlooredAtZero() { + ConfigurationSection root = new YamlConfiguration().createSection("players"); + UUID id = UUID.randomUUID(); + ConfigurationSection entry = root.createSection(id.toString()); + entry.set("points", 5); + entry.set("claimed-points", 0); + entry.set("daily.vote", -3); + entry.set("daily.quest", 2); + + PlayerData data = PlayerStore.readEntry(root, id.toString(), BAR_MAX); + assertEquals(0, data.count("vote")); + assertEquals(2, data.count("quest")); + } + + @Test + void malformedUuidKeyIsSkippedNotFatal() { + ConfigurationSection root = new YamlConfiguration().createSection("players"); + ConfigurationSection entry = root.createSection("not-a-uuid"); + entry.set("points", 5); + + PlayerData data = PlayerStore.readEntry(root, "not-a-uuid", BAR_MAX); + assertNull(data); + } + + @Test + void aMissingSectionIsSkipped() { + ConfigurationSection root = new YamlConfiguration().createSection("players"); + + assertNull(PlayerStore.readEntry(root, UUID.randomUUID().toString(), BAR_MAX)); + } + + @Test + void emptyEntryIsOmittedFromTheSnapshot() { + UUID id = UUID.randomUUID(); + PlayerData empty = new PlayerData("2026-09-07", "2026-09-09"); + + YamlConfiguration yaml = PlayerStore.snapshot(Map.of(id, empty)); + assertFalse(yaml.contains("players." + id)); + } + + @Test + void nonZeroEntryIsWrittenWithAllExpectedKeys() { + UUID id = UUID.randomUUID(); + PlayerData data = new PlayerData(5, "2026-09-07", "2026-09-09", 0, Map.of("vote", 1)); + + YamlConfiguration yaml = PlayerStore.snapshot(Map.of(id, data)); + String path = "players." + id; + assertTrue(yaml.isSet(path + ".points")); + assertTrue(yaml.isSet(path + ".week")); + assertTrue(yaml.isSet(path + ".day")); + assertTrue(yaml.isSet(path + ".claimed-points")); + assertTrue(yaml.isSet(path + ".daily")); + } +} diff --git a/src/test/java/tfmc/justin/activity/utils/WeeksTest.java b/src/test/java/tfmc/justin/activity/utils/WeeksTest.java index 5657193..aeef83d 100644 --- a/src/test/java/tfmc/justin/activity/utils/WeeksTest.java +++ b/src/test/java/tfmc/justin/activity/utils/WeeksTest.java @@ -3,6 +3,7 @@ import org.junit.jupiter.api.Test; import java.time.DayOfWeek; +import java.time.LocalDate; import java.time.LocalDateTime; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -58,4 +59,28 @@ void sevenDaysAfterABoundaryStartsTheNextWeek() { void dayKeyIsTheIsoDate() { assertEquals("2026-09-09", Weeks.dayKey(LocalDateTime.of(2026, 9, 9, 23, 59).toLocalDate())); } + + // One clock, two keys: on both sides of the reset boundary the day key + // must belong to the week key derived from the same moment, i.e. + // weekKey(dayKey's date) still resolves to that same week key. + @Test + void dayAndWeekKeysStayConsistentAcrossTheMidnightBoundary() { + LocalDateTime beforeMidnight = LocalDateTime.of(2026, 9, 6, 23, 59, 59); + LocalDateTime afterMidnight = LocalDateTime.of(2026, 9, 7, 0, 0, 1); + + String weekBefore = Weeks.weekKey(beforeMidnight, DayOfWeek.MONDAY, 0); + String dayBefore = Weeks.dayKey(beforeMidnight.toLocalDate()); + String weekAfter = Weeks.weekKey(afterMidnight, DayOfWeek.MONDAY, 0); + String dayAfter = Weeks.dayKey(afterMidnight.toLocalDate()); + + assertEquals("2026-08-31", weekBefore); + assertEquals("2026-09-06", dayBefore); + assertEquals("2026-09-07", weekAfter); + assertEquals("2026-09-07", dayAfter); + + // The day key, re-fed as "now" at the start of that day, must resolve + // to the same week key that was derived alongside it. + assertEquals(weekBefore, Weeks.weekKey(LocalDate.parse(dayBefore).atStartOfDay(), DayOfWeek.MONDAY, 0)); + assertEquals(weekAfter, Weeks.weekKey(LocalDate.parse(dayAfter).atStartOfDay(), DayOfWeek.MONDAY, 0)); + } }