From db1719bd8c8759f20140c59b91c7f47089e84c01 Mon Sep 17 00:00:00 2001 From: darbyjack Date: Thu, 22 Oct 2026 05:38:35 +0000 Subject: [PATCH 1/7] fix: only read .json files as records in the JSON storage --- .../guilds/database/JsonFileUtils.java | 86 ++++++++ .../arenas/provider/ArenaJsonProvider.java | 34 +-- .../provider/ChallengeJsonProvider.java | 30 ++- .../provider/CooldownJsonProvider.java | 133 +++++++++-- .../guild/provider/GuildJsonProvider.java | 34 +-- .../guilds/database/LogCapture.java | 70 ++++++ .../guilds/database/TestGson.java | 40 ++++ .../provider/ArenaJsonProviderTest.java | 185 ++++++++++++++++ .../provider/ChallengeJsonProviderTest.java | 177 +++++++++++++++ .../provider/CooldownJsonProviderTest.java | 141 ++++++++++++ .../guild/provider/GuildJsonProviderTest.java | 207 ++++++++++++++++++ 11 files changed, 1080 insertions(+), 57 deletions(-) create mode 100644 src/test/java/me/glaremasters/guilds/database/LogCapture.java create mode 100644 src/test/java/me/glaremasters/guilds/database/TestGson.java create mode 100644 src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java create mode 100644 src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java create mode 100644 src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java create mode 100644 src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index 63f161fa5..930454124 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -1,6 +1,7 @@ package me.glaremasters.guilds.database; import com.google.gson.Gson; +import org.jetbrains.annotations.Nullable; import java.io.File; import java.io.IOException; @@ -11,16 +12,101 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; /** * Utility methods for JSON persistence files. */ public final class JsonFileUtils { + /** + * The extension every data file the plugin writes carries. + * + *

The writers produce the record id and this extension: {@code createGuild} and + * {@code updateGuild} write {@code id + ".json"}, and the arena and challenge providers do the + * same. Nothing else in a data folder is a record. + */ + public static final String DATA_FILE_EXTENSION = ".json"; + private JsonFileUtils() { throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); } + /** + * Lists the files in a folder that hold a record. + * + *

A data folder belongs to the plugin, but the operator can see it and can put things in + * it. It can therefore hold more than the files the plugin wrote: the temporary file a write + * that was interrupted left behind, a note, a backup folder. Treating every entry as a record + * made a stray file stand in for a guild that did not exist, and made the plugin complain + * about files it had never written. + * + *

The rule is narrow on purpose. A file is a record when it is a regular file whose name + * ends in {@code .json}. The temporary file that {@link #writeAtomically(File, String)} creates + * next to its target ends in {@code .tmp}, so an interrupted write is ignored. A file the + * operator put there is ignored as well. + * + *

Ignored files are left alone. They are not the plugin's to remove. + * + * @param folder the folder to look in + * @return the data files in the folder, in no particular order, empty if the folder cannot be read + */ + public static List listDataFiles(@Nullable File folder) { + final List dataFiles = new ArrayList<>(); + + if (folder == null) { + return dataFiles; + } + + final File[] files = folder.listFiles(); + + if (files == null) { + return dataFiles; + } + + for (File file : files) { + if (isDataFile(file)) { + dataFiles.add(file); + } + } + + return dataFiles; + } + + /** + * Checks whether a file holds a record. + * + * @param file the file to check + * @return true if the file is a regular file whose name ends in {@code .json} + */ + public static boolean isDataFile(@Nullable File file) { + if (file == null || !file.isFile()) { + return false; + } + + final String name = file.getName(); + return name.length() > DATA_FILE_EXTENSION.length() + && name.toLowerCase(Locale.ROOT).endsWith(DATA_FILE_EXTENSION); + } + + /** + * Gets the id of the record a file holds. + * + *

The id is the name of the file without its extension. That is what the writers use, and + * it is the key the SQL providers use for the same records, so the two backends have to agree + * on it. + * + * @param file the file to read the id from + * @return the id of the record + */ + public static String getRecordId(File file) { + final String name = file.getName(); + final int dotIndex = name.lastIndexOf('.'); + return dotIndex == -1 ? name : name.substring(0, dotIndex); + } + public static T readJson(File file, Gson gson, Class type) throws IOException { try (Reader reader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8)) { return gson.fromJson(reader, type); diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java index f02650fd6..ad941fa13 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java @@ -13,9 +13,7 @@ import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; -import java.util.Objects; public class ArenaJsonProvider implements ArenaProvider { private final File dataFolder; @@ -33,17 +31,16 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { @Override public boolean arenaExists(@Nullable String tablePrefix, @NotNull String id) throws IOException { - return Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .map(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName())) - .anyMatch(n -> n.equals(id)); + return JsonFileUtils.listDataFiles(this.dataFolder).stream() + .anyMatch(f -> JsonFileUtils.getRecordId(f).equals(id)); } @Override public List getAllArenaIds(@Nullable String tablePrefix) throws IOException { List loadedArenaIds = new ArrayList<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { - loadedArenaIds.add(com.google.common.io.Files.getNameWithoutExtension(file.getName())); + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { + loadedArenaIds.add(JsonFileUtils.getRecordId(file)); } return loadedArenaIds; @@ -53,14 +50,23 @@ public List getAllArenaIds(@Nullable String tablePrefix) throws IOExcept public List getAllArenas(@Nullable String tablePrefix) throws IOException { List loadedArenas = new ArrayList<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { try { Arena arena = JsonFileUtils.readJson(file, gson, Arena.class, "Arena"); - arena.getId(); + + // The id is the key the arena is stored and looked up under. A record without one + // cannot be saved, found or removed, so it is not an arena the plugin can work with. + if (arena == null || arena.getId() == null) { + LoggingUtils.severe("The Arena file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + continue; + } + loadedArenas.add(arena); } catch (Exception ex) { LoggingUtils.severe("There was an error loading an Arena from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Arena has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); } } @@ -69,8 +75,8 @@ public List getAllArenas(@Nullable String tablePrefix) throws IOException @Override public Arena getArena(@Nullable String tablePrefix, @NotNull String id) throws IOException { - File data = Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .filter(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName()).equals(id)) + File data = JsonFileUtils.listDataFiles(this.dataFolder).stream() + .filter(f -> JsonFileUtils.getRecordId(f).equals(id)) .findFirst() .orElse(null); @@ -82,12 +88,12 @@ public Arena getArena(@Nullable String tablePrefix, @NotNull String id) throws I @Override public void createArena(@Nullable String tablePrefix, String id, String data) throws IOException { if (arenaExists(tablePrefix, id)) return; - writeArenaFile(new File(dataFolder, id + ".json"), data); + writeArenaFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } @Override public void updateArena(@Nullable String tablePrefix, @NotNull String id, @NotNull String data) throws IOException { - writeArenaFile(new File(dataFolder, id + ".json"), data); + writeArenaFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } private void writeArenaFile(File file, String data) throws IOException { @@ -96,7 +102,7 @@ private void writeArenaFile(File file, String data) throws IOException { @Override public void deleteArena(@Nullable String tablePrefix, @NotNull String id) throws IOException { - deleteArena(new File(dataFolder, id + ".json")); + deleteArena(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } private void deleteArena(File file) { diff --git a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java index fa7c2db5d..ec790aedf 100644 --- a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java @@ -12,9 +12,7 @@ import java.io.File; import java.io.IOException; import java.nio.file.Files; -import java.util.Arrays; import java.util.HashSet; -import java.util.Objects; import java.util.Set; public class ChallengeJsonProvider implements ChallengeProvider { @@ -35,14 +33,23 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { public Set getAllChallenges(@Nullable String tablePrefix) { final Set challenges = new HashSet<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { try { GuildChallenge challenge = JsonFileUtils.readJson(file, gson, GuildChallenge.class, "GuildChallenge"); - challenge.getId(); + + // The id is what a challenge is looked up and removed by. A record without one + // cannot be worked with, so it is not loaded. + if (challenge == null || challenge.getId() == null) { + LoggingUtils.severe("The GuildChallenge file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + continue; + } + challenges.add(challenge); } catch (Exception ex) { LoggingUtils.severe("There was an error loading a GuildChallenge from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this GuildChallenge has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); } } @@ -51,15 +58,14 @@ public Set getAllChallenges(@Nullable String tablePrefix) { @Override public boolean challengeExists(@Nullable String tablePrefix, @NotNull String id) { - return Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .map(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName())) - .anyMatch(n -> n.equals(id)); + return JsonFileUtils.listDataFiles(this.dataFolder).stream() + .anyMatch(f -> JsonFileUtils.getRecordId(f).equals(id)); } @Override public GuildChallenge getChallenge(@Nullable String tablePrefix, @NotNull String id) throws IOException { - File data = Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .filter(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName()).equals(id)) + File data = JsonFileUtils.listDataFiles(this.dataFolder).stream() + .filter(f -> JsonFileUtils.getRecordId(f).equals(id)) .findFirst() .orElse(null); @@ -70,17 +76,17 @@ public GuildChallenge getChallenge(@Nullable String tablePrefix, @NotNull String @Override public void createChallenge(@Nullable String tablePrefix, String id, String data) throws IOException { - writeChallengeFile(new File(dataFolder, id + ".json"), data); + writeChallengeFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } @Override public void updateChallenge(@Nullable String tablePrefix, @NotNull String id, @NotNull String data) throws IOException { - writeChallengeFile(new File(dataFolder, id + ".json"), data); + writeChallengeFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } @Override public void deleteChallenge(@Nullable String tablePrefix, @NotNull String id) { - deleteChallenge(new File(dataFolder, id + ".json")); + deleteChallenge(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } private void writeChallengeFile(File file, String data) throws IOException { diff --git a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java index d0328d927..c00edd869 100644 --- a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java @@ -1,17 +1,21 @@ package me.glaremasters.guilds.database.cooldowns.provider; import com.google.gson.Gson; -import com.google.gson.reflect.TypeToken; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonParser; import me.glaremasters.guilds.Guilds; import me.glaremasters.guilds.cooldowns.Cooldown; import me.glaremasters.guilds.database.JsonFileUtils; import me.glaremasters.guilds.database.cooldowns.CooldownProvider; +import me.glaremasters.guilds.utils.LoggingUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.io.File; import java.io.IOException; -import java.lang.reflect.Type; +import java.io.Reader; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.sql.Timestamp; import java.util.ArrayList; @@ -22,13 +26,11 @@ public class CooldownJsonProvider implements CooldownProvider { private final File dataFolder; private final File cooldownFile; private Gson gson; - private Type cooldownCollectionType; public CooldownJsonProvider(File dataFolder) { this.dataFolder = dataFolder; this.cooldownFile = new File(dataFolder, "cooldowns.json"); this.gson = Guilds.getGson(); - this.cooldownCollectionType = new TypeToken>(){}.getType(); // needed to serialize a list properly } @Override @@ -47,29 +49,126 @@ public boolean cooldownExists(@Nullable String tablePrefix, @NotNull String cool @Override public List getAllCooldowns(@Nullable String tablePrefix) throws IOException { - List loadedCooldowns = JsonFileUtils.readJson(cooldownFile, gson, cooldownCollectionType, "cooldowns"); - - return loadedCooldowns == null ? new ArrayList<>() : loadedCooldowns; + return readCooldownFile().cooldowns; } @Override public void createCooldown(@Nullable String tablePrefix, @NotNull String id, @NotNull String cooldownType, @NotNull String cooldownOwner, @NotNull Timestamp cooldownExpiry) throws IOException { - List current = getAllCooldowns(tablePrefix); - if (cooldownExists(cooldownType, cooldownOwner, current)) return; - current.add(new Cooldown(UUID.fromString(id), Cooldown.Type.getByTypeName(cooldownType), UUID.fromString(cooldownOwner), cooldownExpiry.getTime())); - writeCooldownFile(cooldownFile, current); + final CooldownFile contents = readCooldownFile(); + + if (cooldownExists(cooldownType, cooldownOwner, contents.cooldowns)) return; + + contents.cooldowns.add(new Cooldown(UUID.fromString(id), Cooldown.Type.getByTypeName(cooldownType), UUID.fromString(cooldownOwner), cooldownExpiry.getTime())); + writeCooldownFile(contents); } @Override public void deleteCooldown(@Nullable String tablePrefix, @NotNull String cooldownType, @NotNull String cooldownOwner) throws IOException { - List current = getAllCooldowns(tablePrefix); - if (!cooldownExists(cooldownType, cooldownOwner, current)) return; - current.removeIf(c -> c.getCooldownType().getTypeName().equalsIgnoreCase(cooldownType) && c.getCooldownOwner().toString().equals(cooldownOwner)); - writeCooldownFile(cooldownFile, current); + final CooldownFile contents = readCooldownFile(); + + if (!cooldownExists(cooldownType, cooldownOwner, contents.cooldowns)) return; + + contents.cooldowns.removeIf(c -> c.getCooldownType().getTypeName().equalsIgnoreCase(cooldownType) && c.getCooldownOwner().toString().equals(cooldownOwner)); + writeCooldownFile(contents); + } + + /** + * The contents of the cooldown file. + * + *

One file holds a list of cooldowns, so one broken entry used to take the whole list with + * it: the file could not be parsed, every cooldown in it was lost for the session, and the + * plugin logged a single error with no way to tell which entry was at fault. + * + *

The entries that cannot be read are kept as they are written, so a later write puts them + * back unchanged instead of quietly dropping them. + */ + private static final class CooldownFile { + private final List cooldowns = new ArrayList<>(); + private final List unreadable = new ArrayList<>(); + } + + /** + * Reads the cooldown file. + * + *

An entry that cannot be read is skipped and logged, and the entries around it are still + * returned. The whole file being unreadable is still an error: there is nothing to salvage + * then, and the caller has to know that no cooldowns were loaded. + * + * @return the cooldowns in the file, and the entries that could not be read + * @throws IOException if the file cannot be read or does not hold a list of cooldowns + */ + private CooldownFile readCooldownFile() throws IOException { + final CooldownFile contents = new CooldownFile(); + + if (!this.cooldownFile.exists()) { + return contents; + } + + final JsonElement root; + try (Reader reader = Files.newBufferedReader(this.cooldownFile.toPath(), StandardCharsets.UTF_8)) { + root = new JsonParser().parse(reader); + } catch (RuntimeException ex) { + throw new IOException("Failed to parse cooldowns JSON file: " + this.cooldownFile.getAbsolutePath(), ex); + } + + // An empty file is an empty list of cooldowns, which is what the container starts as. + if (root == null || root.isJsonNull()) { + return contents; + } + + if (!root.isJsonArray()) { + throw new IOException("The cooldowns JSON file does not hold a list of cooldowns: " + this.cooldownFile.getAbsolutePath()); + } + + final JsonArray entries = root.getAsJsonArray(); + + for (int i = 0; i < entries.size(); i++) { + final JsonElement entry = entries.get(i); + + try { + final Cooldown cooldown = gson.fromJson(entry, Cooldown.class); + + // A cooldown without a type or an owner cannot be used, and asking for one used to + // throw where the cooldowns are used. + if (cooldown == null || cooldown.getCooldownType() == null || cooldown.getCooldownOwner() == null) { + contents.unreadable.add(entry); + LoggingUtils.severe("There was an error loading a Cooldown from the cooldowns file: " + this.cooldownFile.getAbsolutePath()); + LoggingUtils.severe("To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); + continue; + } + + contents.cooldowns.add(cooldown); + } catch (RuntimeException ex) { + contents.unreadable.add(entry); + LoggingUtils.severe("There was an error loading a Cooldown from the cooldowns file: " + this.cooldownFile.getAbsolutePath(), ex); + LoggingUtils.severe("To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); + } + } + + return contents; } - private void writeCooldownFile(File file, List cooldowns) throws IOException { - JsonFileUtils.writeAtomically(file, gson.toJson(cooldowns, cooldownCollectionType)); + /** + * Writes the cooldown file. + * + *

The file stays a JSON list of cooldowns, which is what it always was. The entries that + * could not be read are written back unchanged. + * + * @param contents the cooldowns to write, and the entries to keep + * @throws IOException if the file cannot be written + */ + private void writeCooldownFile(CooldownFile contents) throws IOException { + final JsonArray entries = new JsonArray(); + + for (Cooldown cooldown : contents.cooldowns) { + entries.add(gson.toJsonTree(cooldown, Cooldown.class)); + } + + for (JsonElement unreadable : contents.unreadable) { + entries.add(unreadable); + } + + JsonFileUtils.writeAtomically(this.cooldownFile, gson.toJson(entries)); } private boolean cooldownExists(@NotNull String cooldownType, @NotNull String cooldownOwner, @NotNull List current) throws IOException { diff --git a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java index 13db0d238..902161925 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java @@ -13,9 +13,7 @@ import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; -import java.util.Objects; /** * Created by GlareMasters @@ -38,17 +36,16 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { @Override public boolean guildExists(@Nullable String tablePrefix, @NotNull String id) { - return Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .map(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName())) - .anyMatch(n -> n.equals(id)); + return JsonFileUtils.listDataFiles(this.dataFolder).stream() + .anyMatch(f -> JsonFileUtils.getRecordId(f).equals(id)); } @Override public List getAllGuildIds(@Nullable String tablePrefix) { List loadedGuildIds = new ArrayList<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { - loadedGuildIds.add(com.google.common.io.Files.getNameWithoutExtension(file.getName())); + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { + loadedGuildIds.add(JsonFileUtils.getRecordId(file)); } return loadedGuildIds; @@ -58,14 +55,23 @@ public List getAllGuildIds(@Nullable String tablePrefix) { public List getAllGuilds(@Nullable String tablePrefix) { List loadedGuilds = new ArrayList<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { try { Guild guild = JsonFileUtils.readJson(file, gson, Guild.class, "Guild"); - guild.getId(); + + // The id is the key the guild is stored and looked up under. A record without one + // cannot be saved, found or removed, so it is not a guild the plugin can work with. + if (guild == null || guild.getId() == null) { + LoggingUtils.severe("The Guild file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + continue; + } + loadedGuilds.add(guild); } catch (Exception ex) { LoggingUtils.severe("There was an error loading a Guild from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Guild has been prevented from loading."); + LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); } } @@ -74,8 +80,8 @@ public List getAllGuilds(@Nullable String tablePrefix) { @Override public Guild getGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { - File data = Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) - .filter(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName()).equals(id)) + File data = JsonFileUtils.listDataFiles(this.dataFolder).stream() + .filter(f -> JsonFileUtils.getRecordId(f).equals(id)) .findFirst() .orElse(null); @@ -87,12 +93,12 @@ public Guild getGuild(@Nullable String tablePrefix, @NotNull String id) throws I @Override public void createGuild(@Nullable String tablePrefix, @NotNull String id, @NotNull String data) throws IOException { if (guildExists(tablePrefix, id)) return; - writeGuildFile(new File(dataFolder, id + ".json"), data); + writeGuildFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } @Override public void updateGuild(@Nullable String tablePrefix, @NotNull String id, @NotNull String data) throws IOException { - writeGuildFile(new File(dataFolder, id + ".json"), data); + writeGuildFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } private void writeGuildFile(File file, String data) throws IOException { @@ -101,7 +107,7 @@ private void writeGuildFile(File file, String data) throws IOException { @Override public void deleteGuild(@Nullable String tablePrefix, @NotNull String id) { - deleteGuild(new File(dataFolder, id + ".json")); + deleteGuild(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } private void deleteGuild(File file) { diff --git a/src/test/java/me/glaremasters/guilds/database/LogCapture.java b/src/test/java/me/glaremasters/guilds/database/LogCapture.java new file mode 100644 index 000000000..0e241854c --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/LogCapture.java @@ -0,0 +1,70 @@ +package me.glaremasters.guilds.database; + +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +/** + * Collects what the plugin logged while a block of code ran. + * + *

The plugin logs through {@code java.util.logging} under the logger name {@code Guilds}, so a + * handler on that logger is enough to see what it said. The tests use this to check that the + * plugin did not report a problem about a file it was supposed to ignore. + */ +public final class LogCapture implements AutoCloseable { + + private final Logger logger = Logger.getLogger("Guilds"); + private final Handler handler; + private final List records = new ArrayList<>(); + + public LogCapture() { + handler = new Handler() { + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() { + } + + @Override + public void close() { + } + }; + handler.setLevel(Level.ALL); + logger.addHandler(handler); + } + + /** + * @return the messages logged so far + */ + public List messages() { + final List messages = new ArrayList<>(); + for (LogRecord record : records) { + messages.add(record.getMessage()); + } + return messages; + } + + /** + * @param needle a fragment of the message to look for + * @return true if any logged message contains the fragment + */ + public boolean contains(String needle) { + for (String message : messages()) { + if (message.contains(needle)) { + return true; + } + } + return false; + } + + @Override + public void close() { + logger.removeHandler(handler); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/TestGson.java b/src/test/java/me/glaremasters/guilds/database/TestGson.java new file mode 100644 index 000000000..016938c6e --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/TestGson.java @@ -0,0 +1,40 @@ +package me.glaremasters.guilds.database; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import me.glaremasters.guilds.Guilds; + +import java.lang.reflect.Field; + +/** + * Puts a Gson where the JSON providers expect to find one. + * + *

The providers read their Gson from {@link Guilds#getGson()}, which is a static field filled in + * during plugin startup. A unit test has no plugin, so the field is set directly. The Gson built + * here is the same one the plugin builds, so the files the tests write are the files the plugin + * would write. + */ +public final class TestGson { + + private TestGson() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + } + + /** + * Installs the plugin's Gson into the plugin's static field. + * + * @throws ReflectiveOperationException if the field cannot be reached + */ + public static void install() throws ReflectiveOperationException { + final Field field = Guilds.class.getDeclaredField("gson"); + field.setAccessible(true); + field.set(null, new GsonBuilder().setPrettyPrinting().create()); + } + + /** + * @return a Gson identical to the one the plugin uses + */ + public static Gson gson() { + return new GsonBuilder().setPrettyPrinting().create(); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java new file mode 100644 index 000000000..4a0aa8c6e --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java @@ -0,0 +1,185 @@ +package me.glaremasters.guilds.database.arenas.provider; + +import me.glaremasters.guilds.arena.Arena; +import me.glaremasters.guilds.database.LogCapture; +import me.glaremasters.guilds.database.TestGson; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers which files the JSON arena storage treats as arenas. + * + *

The arena folder has the same problem the guild folder had: everything in it was taken to be + * an arena. See {@code GuildJsonProviderTest} for the full story. + */ +class ArenaJsonProviderTest { + + private static final String ARENA_ID = "33333333-3333-3333-3333-333333333333"; + + @TempDir + Path tempDir; + + private ArenaJsonProvider provider; + + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + + @BeforeEach + void setUp() throws IOException { + provider = new ArenaJsonProvider(tempDir.toFile()); + + write(ARENA_ID + ".json", arenaJson(ARENA_ID, "The Real Arena")); + write(ARENA_ID + ".json" + "1234567890" + ".tmp", arenaJson(ARENA_ID, "The Real Arena")); + write("notes.txt", "this is not an arena"); + write("README", "nothing to see here"); + + Files.createDirectory(tempDir.resolve("backup")); + write(tempDir.resolve("backup").resolve("old-arena.json"), arenaJson(ARENA_ID, "Old Arena")); + } + + private void write(String name, String content) throws IOException { + write(tempDir.resolve(name), content); + } + + private void write(Path path, String content) throws IOException { + Files.write(path, content.getBytes(StandardCharsets.UTF_8)); + } + + private static String arenaJson(String id, String name) { + return "{\n" + + " \"id\": \"" + id + "\",\n" + + " \"name\": \"" + name + "\",\n" + + " \"challenger\": null,\n" + + " \"defender\": null\n" + + "}\n"; + } + + @Test + @DisplayName("only the arena data files are enumerated") + void onlyDataFilesAreEnumerated() throws IOException { + assertEquals(List.of(ARENA_ID), provider.getAllArenaIds(null)); + } + + @Test + @DisplayName("only the arena data files are loaded as arenas") + void onlyDataFilesAreLoaded() throws IOException { + final List arenas = provider.getAllArenas(null); + + assertEquals(1, arenas.size()); + assertEquals(ARENA_ID, arenas.get(0).getId().toString()); + assertEquals("The Real Arena", arenas.get(0).getName()); + } + + @Test + @DisplayName("a file that is not an arena file is not reported as an error") + void ignoredFilesAreNotReportedAsErrors() throws IOException { + try (LogCapture log = new LogCapture()) { + provider.getAllArenas(null); + + assertFalse(log.contains("notes.txt"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("README"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("backup"), "the plugin complained about a folder it should ignore"); + } + } + + @Test + @DisplayName("only the arena data files count as an existing arena") + void onlyDataFilesExist() throws IOException { + assertTrue(provider.arenaExists(null, ARENA_ID)); + assertFalse(provider.arenaExists(null, "notes")); + assertFalse(provider.arenaExists(null, "README")); + assertFalse(provider.arenaExists(null, "backup")); + assertFalse(provider.arenaExists(null, "old-arena")); + } + + @Test + @DisplayName("an arena is only read from an arena data file") + void onlyDataFilesAreRead() throws IOException { + final Arena arena = provider.getArena(null, ARENA_ID); + + assertNotNull(arena); + assertEquals(ARENA_ID, arena.getId().toString()); + assertEquals("The Real Arena", arena.getName()); + + assertNull(provider.getArena(null, "notes")); + assertNull(provider.getArena(null, "README")); + assertNull(provider.getArena(null, "old-arena")); + } + + @Test + @DisplayName("the files the plugin ignores are left on disk") + void ignoredFilesAreLeftAlone() throws IOException { + provider.getAllArenas(null); + provider.getAllArenaIds(null); + provider.arenaExists(null, ARENA_ID); + provider.getArena(null, ARENA_ID); + + assertTrue(Files.exists(tempDir.resolve(ARENA_ID + ".json"))); + assertTrue(Files.exists(tempDir.resolve(ARENA_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(tempDir.resolve("notes.txt"))); + assertTrue(Files.exists(tempDir.resolve("README"))); + assertTrue(Files.isDirectory(tempDir.resolve("backup"))); + assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-arena.json"))); + } + + @Test + @DisplayName("an arena file that cannot be parsed does not take the other arenas with it") + void aBrokenArenaFileDoesNotTakeTheOthersWithIt() throws IOException { + write("broken-arena.json", "{ this is not json"); + + final List arenas = provider.getAllArenas(null); + + assertEquals(1, arenas.size()); + assertEquals(ARENA_ID, arenas.get(0).getId().toString()); + } + + @Test + @DisplayName("an arena file that cannot be parsed is left on disk") + void aBrokenArenaFileIsLeftOnDisk() throws IOException { + write("broken-arena.json", "{ this is not json"); + + provider.getAllArenas(null); + + assertTrue(Files.exists(tempDir.resolve("broken-arena.json"))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve("broken-arena.json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("an arena file without an id does not take the other arenas with it") + void anArenaFileWithoutAnIdDoesNotTakeTheOthersWithIt() throws IOException { + write("no-id-arena.json", "{ \"name\": \"No Id Here\" }"); + + final List arenas = provider.getAllArenas(null); + + assertEquals(1, arenas.size()); + assertEquals(ARENA_ID, arenas.get(0).getId().toString()); + } + + @Test + @DisplayName("an arena file without an id is left on disk") + void anArenaFileWithoutAnIdIsLeftOnDisk() throws IOException { + write("no-id-arena.json", "{ \"name\": \"No Id Here\" }"); + + provider.getAllArenas(null); + + assertTrue(Files.exists(tempDir.resolve("no-id-arena.json"))); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java new file mode 100644 index 000000000..b98bf58b5 --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java @@ -0,0 +1,177 @@ +package me.glaremasters.guilds.database.challenges.provider; + +import me.glaremasters.guilds.database.LogCapture; +import me.glaremasters.guilds.database.TestGson; +import me.glaremasters.guilds.guild.GuildChallenge; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers which files the JSON challenge storage treats as challenges. + * + *

The challenge folder has the same problem the guild folder had: everything in it was taken to + * be a challenge. See {@code GuildJsonProviderTest} for the full story. + */ +class ChallengeJsonProviderTest { + + private static final String CHALLENGE_ID = "44444444-4444-4444-4444-444444444444"; + + @TempDir + Path tempDir; + + private ChallengeJsonProvider provider; + + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + + @BeforeEach + void setUp() throws IOException { + provider = new ChallengeJsonProvider(tempDir.toFile()); + + write(CHALLENGE_ID + ".json", challengeJson(CHALLENGE_ID)); + write(CHALLENGE_ID + ".json" + "1234567890" + ".tmp", challengeJson(CHALLENGE_ID)); + write("notes.txt", "this is not a challenge"); + write("README", "nothing to see here"); + + Files.createDirectory(tempDir.resolve("backup")); + write(tempDir.resolve("backup").resolve("old-challenge.json"), challengeJson(CHALLENGE_ID)); + } + + private void write(String name, String content) throws IOException { + write(tempDir.resolve(name), content); + } + + private void write(Path path, String content) throws IOException { + Files.write(path, content.getBytes(StandardCharsets.UTF_8)); + } + + private static String challengeJson(String id) { + return "{\n" + + " \"id\": \"" + id + "\",\n" + + " \"initiateTime\": 0,\n" + + " \"isCompleted\": false,\n" + + " \"challengePlayers\": [],\n" + + " \"defendPlayers\": []\n" + + "}\n"; + } + + @Test + @DisplayName("only the challenge data files are loaded as challenges") + void onlyDataFilesAreLoaded() { + final Set challenges = provider.getAllChallenges(null); + + assertEquals(1, challenges.size()); + assertEquals(CHALLENGE_ID, challenges.iterator().next().getId().toString()); + } + + @Test + @DisplayName("a file that is not a challenge file is not reported as an error") + void ignoredFilesAreNotReportedAsErrors() { + try (LogCapture log = new LogCapture()) { + provider.getAllChallenges(null); + + assertFalse(log.contains("notes.txt"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("README"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("backup"), "the plugin complained about a folder it should ignore"); + } + } + + @Test + @DisplayName("only the challenge data files count as an existing challenge") + void onlyDataFilesExist() { + assertTrue(provider.challengeExists(null, CHALLENGE_ID)); + assertFalse(provider.challengeExists(null, "notes")); + assertFalse(provider.challengeExists(null, "README")); + assertFalse(provider.challengeExists(null, "backup")); + assertFalse(provider.challengeExists(null, "old-challenge")); + } + + @Test + @DisplayName("a challenge is only read from a challenge data file") + void onlyDataFilesAreRead() throws IOException { + final GuildChallenge challenge = provider.getChallenge(null, CHALLENGE_ID); + + assertNotNull(challenge); + assertEquals(CHALLENGE_ID, challenge.getId().toString()); + + assertNull(provider.getChallenge(null, "notes")); + assertNull(provider.getChallenge(null, "README")); + assertNull(provider.getChallenge(null, "old-challenge")); + } + + @Test + @DisplayName("the files the plugin ignores are left on disk") + void ignoredFilesAreLeftAlone() throws IOException { + provider.getAllChallenges(null); + provider.challengeExists(null, CHALLENGE_ID); + provider.getChallenge(null, CHALLENGE_ID); + + assertTrue(Files.exists(tempDir.resolve(CHALLENGE_ID + ".json"))); + assertTrue(Files.exists(tempDir.resolve(CHALLENGE_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(tempDir.resolve("notes.txt"))); + assertTrue(Files.exists(tempDir.resolve("README"))); + assertTrue(Files.isDirectory(tempDir.resolve("backup"))); + assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-challenge.json"))); + } + + @Test + @DisplayName("a challenge file that cannot be parsed does not take the other challenges with it") + void aBrokenChallengeFileDoesNotTakeTheOthersWithIt() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + final Set challenges = provider.getAllChallenges(null); + + assertEquals(1, challenges.size()); + assertEquals(CHALLENGE_ID, challenges.iterator().next().getId().toString()); + } + + @Test + @DisplayName("a challenge file that cannot be parsed is left on disk") + void aBrokenChallengeFileIsLeftOnDisk() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + provider.getAllChallenges(null); + + assertTrue(Files.exists(tempDir.resolve("broken-challenge.json"))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve("broken-challenge.json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("a challenge file without an id does not take the other challenges with it") + void aChallengeFileWithoutAnIdDoesNotTakeTheOthersWithIt() throws IOException { + write("no-id-challenge.json", "{ \"isCompleted\": false }"); + + final Set challenges = provider.getAllChallenges(null); + + assertEquals(1, challenges.size()); + assertEquals(CHALLENGE_ID, challenges.iterator().next().getId().toString()); + } + + @Test + @DisplayName("a challenge file without an id is left on disk") + void aChallengeFileWithoutAnIdIsLeftOnDisk() throws IOException { + write("no-id-challenge.json", "{ \"isCompleted\": false }"); + + provider.getAllChallenges(null); + + assertTrue(Files.exists(tempDir.resolve("no-id-challenge.json"))); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java new file mode 100644 index 000000000..9aa46d442 --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java @@ -0,0 +1,141 @@ +package me.glaremasters.guilds.database.cooldowns.provider; + +import com.google.gson.Gson; +import com.google.gson.reflect.TypeToken; +import me.glaremasters.guilds.cooldowns.Cooldown; +import me.glaremasters.guilds.database.TestGson; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.lang.reflect.Type; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.Timestamp; +import java.util.Arrays; +import java.util.List; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers what the JSON cooldown storage does with an entry it cannot read. + * + *

The cooldown file is the one data file that holds a list rather than a single record, so it + * is the one place where a single broken entry used to cost every other entry as well: the file + * could not be parsed, no cooldown was loaded, and the plugin logged one error that did not say + * which entry was at fault. + */ +class CooldownJsonProviderTest { + + private static final UUID COOLDOWN_ID = UUID.fromString("55555555-5555-5555-5555-555555555555"); + private static final UUID OWNER_ID = UUID.fromString("66666666-6666-6666-6666-666666666666"); + + @TempDir + Path tempDir; + + private CooldownJsonProvider provider; + + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + + @BeforeEach + void setUp() throws IOException { + provider = new CooldownJsonProvider(tempDir.toFile()); + provider.createContainer(null); + } + + private Path cooldownFile() { + return tempDir.resolve("cooldowns.json"); + } + + private void writeCooldowns(String content) throws IOException { + Files.write(cooldownFile(), content.getBytes(StandardCharsets.UTF_8)); + } + + private static String cooldownEntry(UUID id, Cooldown.Type type, UUID owner, long expiry) { + return " {\n" + + " \"cooldownId\": \"" + id + "\",\n" + + " \"cooldownType\": \"" + type.name() + "\",\n" + + " \"cooldownOwner\": \"" + owner + "\",\n" + + " \"cooldownExpiry\": " + expiry + "\n" + + " }"; + } + + @Test + @DisplayName("a cooldown file with one good entry is read") + void aGoodFileIsRead() throws IOException { + writeCooldowns("[\n" + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n]\n"); + + final List cooldowns = provider.getAllCooldowns(null); + + assertEquals(1, cooldowns.size()); + assertEquals(COOLDOWN_ID, cooldowns.get(0).getCooldownId()); + assertEquals(Cooldown.Type.Request, cooldowns.get(0).getCooldownType()); + assertEquals(OWNER_ID, cooldowns.get(0).getCooldownOwner()); + } + + @Test + @DisplayName("a broken entry does not take the other entries with it") + void aBrokenEntryDoesNotTakeTheOthersWithIt() throws IOException { + writeCooldowns("[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + ",\n" + + " {\n" + + " \"cooldownId\": \"77777777-7777-7777-7777-777777777777\",\n" + + " \"cooldownType\": \"NotAType\",\n" + + " \"cooldownOwner\": \"" + OWNER_ID + "\",\n" + + " \"cooldownExpiry\": 9999999999999\n" + + " }\n" + + "]\n"); + + final List cooldowns = provider.getAllCooldowns(null); + + assertEquals(1, cooldowns.size()); + assertEquals(COOLDOWN_ID, cooldowns.get(0).getCooldownId()); + } + + @Test + @DisplayName("a broken entry is still in the file after a cooldown is written") + void aBrokenEntrySurvivesAWrite() throws IOException { + writeCooldowns("[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n" + + "]\n"); + + provider.createCooldown(null, UUID.randomUUID().toString(), Cooldown.Type.Home.name(), OWNER_ID.toString(), new Timestamp(9999999999999L)); + + final String written = new String(Files.readAllBytes(cooldownFile()), StandardCharsets.UTF_8); + + assertTrue(written.contains("this is not a cooldown"), "the entry that could not be read was dropped"); + assertTrue(written.contains(COOLDOWN_ID.toString()), "the cooldown that could be read was dropped"); + assertTrue(written.contains(Cooldown.Type.Home.name()), "the new cooldown was not written"); + } + + @Test + @DisplayName("the cooldown file stays the list of cooldowns it always was") + void theFileFormatIsUnchanged() throws IOException { + final Cooldown cooldown = new Cooldown(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L); + + provider.createCooldown(null, COOLDOWN_ID.toString(), Cooldown.Type.Request.name(), OWNER_ID.toString(), new Timestamp(9999999999999L)); + + final Gson gson = TestGson.gson(); + final Type listType = new TypeToken>(){}.getType(); + final String expected = gson.toJson(Arrays.asList(cooldown), listType); + + assertEquals(expected, new String(Files.readAllBytes(cooldownFile()), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("an empty cooldown file holds no cooldowns") + void anEmptyFileHoldsNoCooldowns() throws IOException { + assertTrue(provider.getAllCooldowns(null).isEmpty()); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java new file mode 100644 index 000000000..57a4db64f --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java @@ -0,0 +1,207 @@ +package me.glaremasters.guilds.database.guild.provider; + +import me.glaremasters.guilds.database.LogCapture; +import me.glaremasters.guilds.database.TestGson; +import me.glaremasters.guilds.guild.Guild; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers which files the JSON guild storage treats as guilds. + * + *

The guild data folder is a folder an operator can see and touch. Anything can end up in it: + * a leftover temporary file from a write that was interrupted, a note, a backup folder. The + * provider used to take every entry it found and call it a guild, which let a stray file stand in + * for a real guild and made the plugin report a guild that did not exist. + */ +class GuildJsonProviderTest { + + private static final String GUILD_ID = "11111111-1111-1111-1111-111111111111"; + private static final String OTHER_GUILD_ID = "22222222-2222-2222-2222-222222222222"; + + @TempDir + Path tempDir; + + private GuildJsonProvider provider; + + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + + @BeforeEach + void setUp() throws IOException { + provider = new GuildJsonProvider(tempDir.toFile()); + + // The one real guild. + write(GUILD_ID + ".json", guildJson(GUILD_ID, "The Real Guild")); + + // What a write that was killed half way leaves behind: the plugin writes to a temporary + // file next to the target and moves it into place afterwards. + write(GUILD_ID + ".json" + "1234567890" + ".tmp", guildJson(GUILD_ID, "The Real Guild")); + + // Files an operator dropped in. + write("notes.txt", "this is not a guild"); + write("README", "nothing to see here"); + + // A folder, because people make backups that way. + Files.createDirectory(tempDir.resolve("backup")); + write(tempDir.resolve("backup").resolve("old-guild.json"), guildJson(OTHER_GUILD_ID, "Old Guild")); + } + + private void write(String name, String content) throws IOException { + write(tempDir.resolve(name), content); + } + + private void write(Path path, String content) throws IOException { + Files.write(path, content.getBytes(StandardCharsets.UTF_8)); + } + + private static String guildJson(String id, String name) { + return "{\n" + + " \"id\": \"" + id + "\",\n" + + " \"name\": \"" + name + "\",\n" + + " \"prefix\": \"TG\",\n" + + " \"motd\": \"A guild\",\n" + + " \"status\": \"Public\",\n" + + " \"balance\": 0.0,\n" + + " \"members\": [],\n" + + " \"invitedMembers\": [],\n" + + " \"allies\": [],\n" + + " \"pendingAllies\": [],\n" + + " \"codes\": [],\n" + + " \"vaults\": [],\n" + + " \"lastDefended\": 0,\n" + + " \"creationDate\": 0\n" + + "}\n"; + } + + @Test + @DisplayName("only the guild data files are enumerated") + void onlyDataFilesAreEnumerated() { + assertEquals(List.of(GUILD_ID), provider.getAllGuildIds(null)); + } + + @Test + @DisplayName("only the guild data files are loaded as guilds") + void onlyDataFilesAreLoaded() { + final List guilds = provider.getAllGuilds(null); + + assertEquals(1, guilds.size()); + assertEquals(GUILD_ID, guilds.get(0).getId().toString()); + assertEquals("The Real Guild", guilds.get(0).getName()); + } + + @Test + @DisplayName("a file that is not a guild file is not reported as an error") + void ignoredFilesAreNotReportedAsErrors() { + try (LogCapture log = new LogCapture()) { + provider.getAllGuilds(null); + + assertFalse(log.contains("notes.txt"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("README"), "the plugin complained about a file it should ignore"); + assertFalse(log.contains("backup"), "the plugin complained about a folder it should ignore"); + } + } + + @Test + @DisplayName("only the guild data files count as an existing guild") + void onlyDataFilesExist() { + assertTrue(provider.guildExists(null, GUILD_ID)); + assertFalse(provider.guildExists(null, "notes")); + assertFalse(provider.guildExists(null, "README")); + assertFalse(provider.guildExists(null, "backup")); + assertFalse(provider.guildExists(null, "old-guild")); + } + + @Test + @DisplayName("a guild is only read from a guild data file") + void onlyDataFilesAreRead() throws IOException { + final Guild guild = provider.getGuild(null, GUILD_ID); + + assertNotNull(guild); + assertEquals(GUILD_ID, guild.getId().toString()); + assertEquals("The Real Guild", guild.getName()); + + assertNull(provider.getGuild(null, "notes")); + assertNull(provider.getGuild(null, "README")); + assertNull(provider.getGuild(null, "old-guild")); + } + + @Test + @DisplayName("the files the plugin ignores are left on disk") + void ignoredFilesAreLeftAlone() throws IOException { + provider.getAllGuilds(null); + provider.getAllGuildIds(null); + provider.guildExists(null, GUILD_ID); + provider.getGuild(null, GUILD_ID); + + assertTrue(Files.exists(tempDir.resolve(GUILD_ID + ".json"))); + assertTrue(Files.exists(tempDir.resolve(GUILD_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(tempDir.resolve("notes.txt"))); + assertTrue(Files.exists(tempDir.resolve("README"))); + assertTrue(Files.isDirectory(tempDir.resolve("backup"))); + assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-guild.json"))); + } + + @Test + @DisplayName("a guild file that cannot be parsed does not take the other guilds with it") + void aBrokenGuildFileDoesNotTakeTheOthersWithIt() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + final List guilds = provider.getAllGuilds(null); + + assertEquals(1, guilds.size()); + assertEquals(GUILD_ID, guilds.get(0).getId().toString()); + } + + @Test + @DisplayName("a guild file that cannot be parsed is left on disk") + void aBrokenGuildFileIsLeftOnDisk() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + + assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("a guild file without an id does not take the other guilds with it") + void aGuildFileWithoutAnIdDoesNotTakeTheOthersWithIt() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); + + final List guilds = provider.getAllGuilds(null); + + assertEquals(1, guilds.size()); + assertEquals(GUILD_ID, guilds.get(0).getId().toString()); + } + + @Test + @DisplayName("a guild file without an id is left on disk") + void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); + + provider.getAllGuilds(null); + + assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertEquals("{ \"name\": \"No Id Here\" }", new String( + Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + } +} From aa8711cf8dd42964ed1bd6b9f19215a438df178d Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 06:11:59 +0000 Subject: [PATCH 2/7] fix: protect the guild files the plugin could not read --- .../guilds/database/JsonFileUtils.java | 9 ++- .../arenas/provider/ArenaJsonProvider.java | 50 +++++++++++++++++ .../provider/ChallengeJsonProvider.java | 49 ++++++++++++++--- .../guild/provider/GuildJsonProvider.java | 48 ++++++++++++++-- .../provider/ArenaJsonProviderTest.java | 37 +++++++++++++ .../provider/ChallengeJsonProviderTest.java | 30 +++++++++- .../guild/provider/GuildJsonProviderTest.java | 55 +++++++++++++++++-- 7 files changed, 256 insertions(+), 22 deletions(-) diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index 930454124..638f71943 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -51,9 +51,12 @@ private JsonFileUtils() { *

Ignored files are left alone. They are not the plugin's to remove. * * @param folder the folder to look in - * @return the data files in the folder, in no particular order, empty if the folder cannot be read + * @return the data files in the folder, in no particular order + * @throws IOException if the folder cannot be listed. A folder that cannot be read is a problem + * with the storage rather than with one record, and it has to say so: a folder that + * looks empty is a server that comes up with no guilds and no explanation. */ - public static List listDataFiles(@Nullable File folder) { + public static List listDataFiles(@Nullable File folder) throws IOException { final List dataFiles = new ArrayList<>(); if (folder == null) { @@ -63,7 +66,7 @@ public static List listDataFiles(@Nullable File folder) { final File[] files = folder.listFiles(); if (files == null) { - return dataFiles; + throw new IOException("The data folder could not be read: " + folder.getAbsolutePath()); } for (File file : files) { diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java index ad941fa13..7bbf70e5a 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java @@ -13,12 +13,24 @@ import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; public class ArenaJsonProvider implements ArenaProvider { private final File dataFolder; private Gson gson; + /** + * The arena files that could not be read when the arenas were loaded. + * + *

A file in here holds an arena the plugin could not use: one it could not parse, or one + * whose record has no id. The arena storage deletes the files it has no record for, so a file + * it could not read has to be kept out of that pass. The file is the only copy of that + * arena's data. + */ + private final Set unreadableFiles = new HashSet<>(); + public ArenaJsonProvider(File dataFolder) { this.dataFolder = dataFolder; this.gson = Guilds.getGson(); @@ -57,6 +69,8 @@ public List getAllArenas(@Nullable String tablePrefix) throws IOException // The id is the key the arena is stored and looked up under. A record without one // cannot be saved, found or removed, so it is not an arena the plugin can work with. if (arena == null || arena.getId() == null) { + // The file holds the only copy of that arena, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("The Arena file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); continue; @@ -64,6 +78,8 @@ public List getAllArenas(@Nullable String tablePrefix) throws IOException loadedArenas.add(arena); } catch (Exception ex) { + // The file holds the only copy of that arena, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("There was an error loading an Arena from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Arena has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); @@ -97,15 +113,49 @@ public void updateArena(@Nullable String tablePrefix, @NotNull String id, @NotNu } private void writeArenaFile(File file, String data) throws IOException { + refuseToOverwriteUnreadableFile(file); JsonFileUtils.writeAtomically(file, data); } + /** + * Refuses to write over an arena file that could not be read when the arenas were loaded. + * + *

That file is the only copy of the arena it holds. The plugin has no arena for it in + * memory, so anything written there would replace data the plugin never read with data it + * never had. + * + * @param file the file that is about to be written + * @throws IOException if the file is one the plugin could not read + */ + private void refuseToOverwriteUnreadableFile(File file) throws IOException { + if (!unreadableFiles.contains(file.getName())) { + return; + } + + throw new IOException("The arena file " + file.getAbsolutePath() + " could not be read when the plugin started, " + + "so it has not been changed. Fix the file and restart the server."); + } + @Override public void deleteArena(@Nullable String tablePrefix, @NotNull String id) throws IOException { deleteArena(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } + /** + * Deletes an arena file, unless it is one the plugin could not read. + * + *

The arena storage deletes the files it has no record for, one after the other, so a file + * that could not be read is skipped rather than refused: it is the only copy of that arena, + * and one unreadable file must not stop the rest of the batch from being cleaned up. The + * load already said which files could not be read. + * + * @param file the file to delete + */ private void deleteArena(File file) { + if (unreadableFiles.contains(file.getName())) { + return; + } + if (file.exists()) file.delete(); } } diff --git a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java index ec790aedf..4bfa6a8a1 100644 --- a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java @@ -19,6 +19,15 @@ public class ChallengeJsonProvider implements ChallengeProvider { private final File dataFolder; private final Gson gson; + /** + * The challenge files that could not be read when the challenges were loaded. + * + *

A file in here holds a challenge the plugin could not use: one it could not parse, or one + * whose record has no id. The file is the only copy of that challenge's data, so it is never + * written to or deleted. + */ + private final Set unreadableFiles = new HashSet<>(); + public ChallengeJsonProvider(File dataFolder) { this.dataFolder = dataFolder; this.gson = Guilds.getGson(); @@ -30,7 +39,7 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { } @Override - public Set getAllChallenges(@Nullable String tablePrefix) { + public Set getAllChallenges(@Nullable String tablePrefix) throws IOException { final Set challenges = new HashSet<>(); for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { @@ -40,6 +49,8 @@ public Set getAllChallenges(@Nullable String tablePrefix) { // The id is what a challenge is looked up and removed by. A record without one // cannot be worked with, so it is not loaded. if (challenge == null || challenge.getId() == null) { + // The file holds the only copy of that challenge, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("The GuildChallenge file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); continue; @@ -47,6 +58,8 @@ public Set getAllChallenges(@Nullable String tablePrefix) { challenges.add(challenge); } catch (Exception ex) { + // The file holds the only copy of that challenge, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("There was an error loading a GuildChallenge from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this GuildChallenge has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); @@ -57,7 +70,7 @@ public Set getAllChallenges(@Nullable String tablePrefix) { } @Override - public boolean challengeExists(@Nullable String tablePrefix, @NotNull String id) { + public boolean challengeExists(@Nullable String tablePrefix, @NotNull String id) throws IOException { return JsonFileUtils.listDataFiles(this.dataFolder).stream() .anyMatch(f -> JsonFileUtils.getRecordId(f).equals(id)); } @@ -84,16 +97,38 @@ public void updateChallenge(@Nullable String tablePrefix, @NotNull String id, @N writeChallengeFile(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION), data); } + private void writeChallengeFile(File file, String data) throws IOException { + refuseToOverwriteUnreadableFile(file); + JsonFileUtils.writeAtomically(file, data); + } + + /** + * Refuses to change a challenge file that could not be read when the challenges were loaded. + * + *

That file is the only copy of the challenge it holds. The plugin has no challenge for it + * in memory, so anything written there would replace data the plugin never read with data it + * never had. + * + * @param file the file that is about to be written or deleted + * @throws IOException if the file is one the plugin could not read + */ + private void refuseToOverwriteUnreadableFile(File file) throws IOException { + if (!unreadableFiles.contains(file.getName())) { + return; + } + + throw new IOException("The challenge file " + file.getAbsolutePath() + " could not be read when the plugin started, " + + "so it has not been changed. Fix the file and restart the server."); + } + @Override - public void deleteChallenge(@Nullable String tablePrefix, @NotNull String id) { + public void deleteChallenge(@Nullable String tablePrefix, @NotNull String id) throws IOException { deleteChallenge(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } - private void writeChallengeFile(File file, String data) throws IOException { - JsonFileUtils.writeAtomically(file, data); - } + private void deleteChallenge(File file) throws IOException { + refuseToOverwriteUnreadableFile(file); - private void deleteChallenge(File file) { if (file.exists()) file.delete(); } } diff --git a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java index 902161925..3a0a22c28 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java @@ -13,7 +13,9 @@ import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; /** * Created by GlareMasters @@ -24,6 +26,16 @@ public class GuildJsonProvider implements GuildProvider { private final File dataFolder; private Gson gson; + /** + * The guild files that could not be read when the guilds were loaded. + * + *

A file in here holds a guild the plugin could not use: one it could not parse, or one + * whose record has no id. Either way the file is the only copy of that guild's data, so the + * save must never write over it. The plugin does not have the guild in memory, so anything it + * wrote would be a guess at data it never read. + */ + private final Set unreadableFiles = new HashSet<>(); + public GuildJsonProvider(File dataFolder) { this.dataFolder = dataFolder; this.gson = Guilds.getGson(); @@ -35,13 +47,13 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { } @Override - public boolean guildExists(@Nullable String tablePrefix, @NotNull String id) { + public boolean guildExists(@Nullable String tablePrefix, @NotNull String id) throws IOException { return JsonFileUtils.listDataFiles(this.dataFolder).stream() .anyMatch(f -> JsonFileUtils.getRecordId(f).equals(id)); } @Override - public List getAllGuildIds(@Nullable String tablePrefix) { + public List getAllGuildIds(@Nullable String tablePrefix) throws IOException { List loadedGuildIds = new ArrayList<>(); for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { @@ -52,7 +64,7 @@ public List getAllGuildIds(@Nullable String tablePrefix) { } @Override - public List getAllGuilds(@Nullable String tablePrefix) { + public List getAllGuilds(@Nullable String tablePrefix) throws IOException { List loadedGuilds = new ArrayList<>(); for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { @@ -62,6 +74,8 @@ public List getAllGuilds(@Nullable String tablePrefix) { // The id is the key the guild is stored and looked up under. A record without one // cannot be saved, found or removed, so it is not a guild the plugin can work with. if (guild == null || guild.getId() == null) { + // The file holds the only copy of that guild, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("The Guild file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); continue; @@ -69,6 +83,8 @@ public List getAllGuilds(@Nullable String tablePrefix) { loadedGuilds.add(guild); } catch (Exception ex) { + // The file holds the only copy of that guild, so it is remembered and left alone. + unreadableFiles.add(file.getName()); LoggingUtils.severe("There was an error loading a Guild from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Guild has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); @@ -102,15 +118,37 @@ public void updateGuild(@Nullable String tablePrefix, @NotNull String id, @NotNu } private void writeGuildFile(File file, String data) throws IOException { + refuseToOverwriteUnreadableFile(file); JsonFileUtils.writeAtomically(file, data); } + /** + * Refuses to change a guild file that could not be read when the guilds were loaded. + * + *

That file is the only copy of the guild it holds. The plugin has no guild for it in + * memory, so anything written there would replace data the plugin never read with data it + * never had. + * + * @param file the file that is about to be written or deleted + * @throws IOException if the file is one the plugin could not read + */ + private void refuseToOverwriteUnreadableFile(File file) throws IOException { + if (!unreadableFiles.contains(file.getName())) { + return; + } + + throw new IOException("The guild file " + file.getAbsolutePath() + " could not be read when the plugin started, " + + "so it has not been changed. Fix the file and restart the server."); + } + @Override - public void deleteGuild(@Nullable String tablePrefix, @NotNull String id) { + public void deleteGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { deleteGuild(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } - private void deleteGuild(File file) { + private void deleteGuild(File file) throws IOException { + refuseToOverwriteUnreadableFile(file); + if (file.exists()) file.delete(); } } diff --git a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java index 4a0aa8c6e..156aee8f4 100644 --- a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -182,4 +183,40 @@ void anArenaFileWithoutAnIdIsLeftOnDisk() throws IOException { assertTrue(Files.exists(tempDir.resolve("no-id-arena.json"))); } + + @Test + @DisplayName("an arena file that could not be read is not written over") + void anArenaFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { + write("broken-arena.json", "{ this is not json"); + + provider.getAllArenas(null); + + assertThrows(IOException.class, () -> provider.updateArena(null, "broken-arena", arenaJson(ARENA_ID, "Replaced"))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve("broken-arena.json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("an arena file that could not be read is not deleted") + void anArenaFileThatCouldNotBeReadIsNotDeleted() throws IOException { + write("broken-arena.json", "{ this is not json"); + + provider.getAllArenas(null); + + // The arena storage deletes the files it has no record for, so a file it could not read + // has to survive that pass. + provider.deleteArena(null, "broken-arena"); + assertTrue(Files.exists(tempDir.resolve("broken-arena.json"))); + } + + @Test + @DisplayName("an arena file without an id is not deleted") + void anArenaFileWithoutAnIdIsNotDeleted() throws IOException { + write("no-id-arena.json", "{ \"name\": \"No Id Here\" }"); + + provider.getAllArenas(null); + + provider.deleteArena(null, "no-id-arena"); + assertTrue(Files.exists(tempDir.resolve("no-id-arena.json"))); + } } diff --git a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java index b98bf58b5..fa69479f9 100644 --- a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -74,7 +75,7 @@ private static String challengeJson(String id) { @Test @DisplayName("only the challenge data files are loaded as challenges") - void onlyDataFilesAreLoaded() { + void onlyDataFilesAreLoaded() throws IOException { final Set challenges = provider.getAllChallenges(null); assertEquals(1, challenges.size()); @@ -83,7 +84,7 @@ void onlyDataFilesAreLoaded() { @Test @DisplayName("a file that is not a challenge file is not reported as an error") - void ignoredFilesAreNotReportedAsErrors() { + void ignoredFilesAreNotReportedAsErrors() throws IOException { try (LogCapture log = new LogCapture()) { provider.getAllChallenges(null); @@ -95,7 +96,7 @@ void ignoredFilesAreNotReportedAsErrors() { @Test @DisplayName("only the challenge data files count as an existing challenge") - void onlyDataFilesExist() { + void onlyDataFilesExist() throws IOException { assertTrue(provider.challengeExists(null, CHALLENGE_ID)); assertFalse(provider.challengeExists(null, "notes")); assertFalse(provider.challengeExists(null, "README")); @@ -174,4 +175,27 @@ void aChallengeFileWithoutAnIdIsLeftOnDisk() throws IOException { assertTrue(Files.exists(tempDir.resolve("no-id-challenge.json"))); } + + @Test + @DisplayName("a challenge file that could not be read is not written over") + void aChallengeFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + provider.getAllChallenges(null); + + assertThrows(IOException.class, () -> provider.updateChallenge(null, "broken-challenge", challengeJson(CHALLENGE_ID))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve("broken-challenge.json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("a challenge file that could not be read is not deleted") + void aChallengeFileThatCouldNotBeReadIsNotDeleted() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + provider.getAllChallenges(null); + + assertThrows(IOException.class, () -> provider.deleteChallenge(null, "broken-challenge")); + assertTrue(Files.exists(tempDir.resolve("broken-challenge.json"))); + } } diff --git a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java index 57a4db64f..495dd4a83 100644 --- a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -93,13 +94,13 @@ private static String guildJson(String id, String name) { @Test @DisplayName("only the guild data files are enumerated") - void onlyDataFilesAreEnumerated() { + void onlyDataFilesAreEnumerated() throws IOException { assertEquals(List.of(GUILD_ID), provider.getAllGuildIds(null)); } @Test @DisplayName("only the guild data files are loaded as guilds") - void onlyDataFilesAreLoaded() { + void onlyDataFilesAreLoaded() throws IOException { final List guilds = provider.getAllGuilds(null); assertEquals(1, guilds.size()); @@ -109,7 +110,7 @@ void onlyDataFilesAreLoaded() { @Test @DisplayName("a file that is not a guild file is not reported as an error") - void ignoredFilesAreNotReportedAsErrors() { + void ignoredFilesAreNotReportedAsErrors() throws IOException { try (LogCapture log = new LogCapture()) { provider.getAllGuilds(null); @@ -121,7 +122,7 @@ void ignoredFilesAreNotReportedAsErrors() { @Test @DisplayName("only the guild data files count as an existing guild") - void onlyDataFilesExist() { + void onlyDataFilesExist() throws IOException { assertTrue(provider.guildExists(null, GUILD_ID)); assertFalse(provider.guildExists(null, "notes")); assertFalse(provider.guildExists(null, "README")); @@ -204,4 +205,50 @@ void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { assertEquals("{ \"name\": \"No Id Here\" }", new String( Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } + + @Test + @DisplayName("a guild file that could not be read is not written over") + void aGuildFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); + assertEquals("{ this is not json", new String( + Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("a guild file without an id is not written over") + void aGuildFileWithoutAnIdIsNotWrittenOver() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); + + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); + assertEquals("{ \"name\": \"No Id Here\" }", new String( + Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("a guild file that could not be read is not deleted") + void aGuildFileThatCouldNotBeReadIsNotDeleted() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); + assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + } + + @Test + @DisplayName("a guild file without an id is not deleted") + void aGuildFileWithoutAnIdIsNotDeleted() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); + + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); + assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + } } From 3015308df9d1e92e6c3d53a8f11de4be79f92af4 Mon Sep 17 00:00:00 2001 From: Chief of Staff Date: Thu, 22 Oct 2026 12:04:09 +0000 Subject: [PATCH 3/7] feat(backup): add the shared helper that copies unreadable data aside --- .../guilds/database/CorruptDataBackup.java | 258 ++++++++++++++++++ 1 file changed, 258 insertions(+) create mode 100644 src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java diff --git a/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java b/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java new file mode 100644 index 000000000..9ac65dcc0 --- /dev/null +++ b/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java @@ -0,0 +1,258 @@ +package me.glaremasters.guilds.database; + +import me.glaremasters.guilds.utils.LoggingUtils; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; + +/** + * Keeps a copy of data the plugin could not read. + * + *

A file the plugin cannot parse is often the only copy of that record. The plugin has no + * in-memory version of it, so anything it wrote in its place would be a guess. Rather than + * discarding the file or writing over it, the file is copied to a backup directory and the + * original is left exactly where the operator put it. + * + *

The backups live outside every data folder the plugin reads, under + * {@code plugins/Guilds/backups/corrupt///}. The plugin enumerates only its + * own data folders, so a backup is never loaded as a record, however it is named. + * + *

The timestamp comes from the file's own last-modified time rather than from the moment the + * backup was taken. An unchanged file therefore always lands in the same directory, and a second + * read of the same corruption finds the copy that is already there instead of making another one. + * The same holds across a restart. + */ +public final class CorruptDataBackup { + + /** + * The directory under the plugin's data folder that holds the backups. + */ + public static final String BACKUP_DIRECTORY = "backups/corrupt"; + + /** + * The timestamp format used for a backup directory. + */ + private static final DateTimeFormatter TIMESTAMP = + DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss").withZone(ZoneId.systemDefault()); + + private CorruptDataBackup() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + } + + /** + * What happened when a backup was attempted. + */ + public static final class Result { + + private final boolean created; + private final boolean skipped; + private final boolean failed; + private final File backupFile; + private final String failure; + + private Result(boolean created, boolean skipped, boolean failed, File backupFile, String failure) { + this.created = created; + this.skipped = skipped; + this.failed = failed; + this.backupFile = backupFile; + this.failure = failure; + } + + /** + * Whether a new copy was written. + * + * @return true when a backup file was created + */ + public boolean isCreated() { + return created; + } + + /** + * Whether an identical backup was already there. + * + * @return true when the corruption had already been backed up + */ + public boolean isSkipped() { + return skipped; + } + + /** + * Whether the backup could not be written. + * + * @return true when the backup failed + */ + public boolean isFailed() { + return failed; + } + + /** + * Where the backup is. + * + * @return the backup file, or null when the backup failed + */ + public File getBackupFile() { + return backupFile; + } + + /** + * Why the backup failed. + * + * @return the reason, or null when the backup succeeded + */ + public String getFailure() { + return failure; + } + } + + /** + * Copies a file the plugin could not read into the backup directory. + * + *

The copy is byte for byte. The original file is not moved, not truncated and not + * overwritten: it is left exactly as it was, and a backup is a safeguard rather than + * permission to replace it. + * + * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds} + * @param sourceFile the file that could not be read + * @param dataType the kind of record the file holds, used as a directory name + * @return what happened + */ + public static Result backup(File pluginDataFolder, File sourceFile, String dataType) { + if (pluginDataFolder == null || sourceFile == null || !sourceFile.isFile()) { + return new Result(false, false, true, null, + "there was no file to copy: " + (sourceFile == null ? "no file was given" : sourceFile.getAbsolutePath())); + } + + final File directory = backupDirectory(pluginDataFolder, sourceFile, dataType); + final File original = new File(directory, sourceFile.getName()); + + // The same corruption, read again, must not produce a second copy. + if (original.isFile() && sameContent(original, sourceFile)) { + return new Result(false, true, false, original, null); + } + + // Something with that name is already there and holds different data, so the copy needs a + // name of its own rather than the one that is taken. + File target = original; + if (target.exists()) { + target = freeName(directory, sourceFile.getName()); + if (target == null) { + return new Result(false, false, true, null, + "the backup directory holds a file with that name and it could not be replaced: " + directory.getAbsolutePath()); + } + } + + try { + final Path source = sourceFile.toPath(); + final Path destination = target.toPath(); + Files.createDirectories(destination.getParent()); + Files.copy(source, destination, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES); + return new Result(true, false, false, target, null); + } catch (IOException | RuntimeException ex) { + final String reason = ex.getMessage() == null ? ex.toString() : ex.getMessage(); + LoggingUtils.severe("The backup of " + sourceFile.getAbsolutePath() + " could not be written to " + + directory.getAbsolutePath() + ". The file has been left untouched: " + reason, ex); + return new Result(false, false, true, null, reason); + } + } + + /** + * The directory a backup of a file goes in. + * + *

The timestamp is the file's own last-modified time, so an unchanged file always maps to + * the same directory and is only ever backed up once. + * + * @param pluginDataFolder the plugin's data folder + * @param sourceFile the file being backed up + * @param dataType the kind of record the file holds + * @return the directory + */ + private static File backupDirectory(File pluginDataFolder, File sourceFile, String dataType) { + final long modified = sourceFile.lastModified(); + final String timestamp = modified <= 0L + ? TIMESTAMP.format(Instant.now()) + : TIMESTAMP.format(Instant.ofEpochMilli(modified)); + return new File(new File(new File(pluginDataFolder, BACKUP_DIRECTORY), timestamp), safeName(dataType)); + } + + /** + * A name that can be used as a directory name. + * + * @param dataType the kind of record + * @return the name + */ + private static String safeName(String dataType) { + if (dataType == null || dataType.trim().isEmpty()) { + return "unknown"; + } + return dataType.replaceAll("[^A-Za-z0-9._-]", "_"); + } + + /** + * A free name for a backup inside its directory. + * + *

The original name is kept, so the operator can tell what the file was. When something with + * that name is already there, a number is added until the name is free, so a backup never + * overwrites another one. + * + * @param directory the backup directory + * @param name the original file name + * @return the name to write, or null when none could be found + */ + private static File freeName(File directory, String name) { + for (int suffix = 1; suffix < 1000; suffix++) { + final File candidate = new File(directory, name + "." + suffix); + if (!candidate.exists()) { + return candidate; + } + } + + return null; + } + + /** + * Whether two files hold the same bytes. + * + * @param first the first file + * @param second the second file + * @return true when their contents are identical + */ + private static boolean sameContent(File first, File second) { + try { + return digest(first).equals(digest(second)); + } catch (IOException | RuntimeException ex) { + return false; + } + } + + /** + * The SHA-256 digest of a file's contents. + * + * @param file the file + * @return the digest, as hex + * @throws IOException if the file cannot be read + */ + private static String digest(File file) throws IOException { + final MessageDigest digest; + try { + digest = MessageDigest.getInstance("SHA-256"); + } catch (NoSuchAlgorithmException ex) { + throw new IOException("SHA-256 is not available", ex); + } + + final byte[] hash = digest.digest(Files.readAllBytes(file.toPath())); + final StringBuilder hex = new StringBuilder(hash.length * 2); + for (byte value : hash) { + hex.append(Character.forDigit((value >> 4) & 0xF, 16)); + hex.append(Character.forDigit(value & 0xF, 16)); + } + return hex.toString(); + } +} From faa9841557b292d8281687f54edcd90aee6ccf05 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 12:41:05 +0000 Subject: [PATCH 4/7] feat(backup): keep a copy of the data the plugin cannot read --- .../guilds/database/JsonFileUtils.java | 67 ++++++ .../guilds/database/arenas/ArenaAdapter.java | 2 +- .../arenas/provider/ArenaJsonProvider.java | 17 ++ .../database/challenges/ChallengeAdapter.java | 2 +- .../provider/ChallengeJsonProvider.java | 17 ++ .../database/cooldowns/CooldownAdapter.java | 2 +- .../provider/CooldownJsonProvider.java | 32 ++- .../guilds/database/guild/GuildAdapter.java | 2 +- .../guild/provider/GuildJsonProvider.java | 17 ++ .../guilds/database/CorruptBackups.java | 70 ++++++ .../database/JsonStorageBackupTest.java | 144 +++++++++++++ .../provider/ArenaJsonProviderTest.java | 189 ++++++++++++++-- .../provider/ChallengeJsonProviderTest.java | 188 ++++++++++++++-- .../provider/CooldownJsonProviderTest.java | 155 +++++++++++++- .../guild/provider/GuildJsonProviderTest.java | 202 ++++++++++++++++-- 15 files changed, 1045 insertions(+), 61 deletions(-) create mode 100644 src/test/java/me/glaremasters/guilds/database/CorruptBackups.java create mode 100644 src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index 638f71943..9cd05d0cb 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -1,6 +1,7 @@ package me.glaremasters.guilds.database; import com.google.gson.Gson; +import me.glaremasters.guilds.utils.LoggingUtils; import org.jetbrains.annotations.Nullable; import java.io.File; @@ -110,6 +111,72 @@ public static String getRecordId(File file) { return dotIndex == -1 ? name : name.substring(0, dotIndex); } + /** + * Keeps a copy of a file the plugin could not read and says where it is. + * + *

The copy is byte for byte. The original is left exactly as it was: a backup is a + * safeguard, not permission to replace the file. + * + *

The log line names the file that could not be read, why it could not be read and where + * the copy is, so an operator has all three in one place. + * + * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds} + * @param file the file that could not be read + * @param dataType the kind of record the file holds + * @param cause what went wrong while reading the file + */ + public static void backupCorruptFile(File pluginDataFolder, File file, String dataType, Throwable cause) { + backupCorruptFile(pluginDataFolder, file, dataType, describe(cause)); + } + + /** + * Keeps a copy of a file the plugin could not read and says where it is. + * + *

The copy is byte for byte. The original is left exactly as it was: a backup is a + * safeguard, not permission to replace the file. + * + *

The log line names the file that could not be read, why it could not be read and where + * the copy is, so an operator has all three in one place. + * + * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds} + * @param file the file that could not be read + * @param dataType the kind of record the file holds + * @param reason why the file could not be read + */ + public static void backupCorruptFile(File pluginDataFolder, File file, String dataType, String reason) { + final CorruptDataBackup.Result result = CorruptDataBackup.backup(pluginDataFolder, file, dataType); + + if (result.isFailed()) { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "), " + + "and its backup could not be written: " + result.getFailure() + ". The file has been left where it is."); + return; + } + + if (result.isSkipped()) { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "). " + + "A copy of it is already at " + result.getBackupFile().getAbsolutePath() + "."); + return; + } + + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "). " + + "A copy of it was written to " + result.getBackupFile().getAbsolutePath() + ". " + + "The file itself has been left where it is."); + } + + /** + * Describes what went wrong, in a form that fits in a log line. + * + * @param cause what went wrong + * @return the message of the cause, or the cause itself when it has no message + */ + private static String describe(Throwable cause) { + if (cause == null) { + return "the reason was not reported"; + } + + return cause.getMessage() == null ? cause.toString() : cause.getMessage(); + } + public static T readJson(File file, Gson gson, Class type) throws IOException { try (Reader reader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8)) { return gson.fromJson(reader, type); diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java b/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java index 3282b8685..a096fc9c3 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java @@ -33,7 +33,7 @@ public ArenaAdapter(Guilds guilds, DatabaseAdapter adapter) { default: case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "arenas"); - provider = new ArenaJsonProvider(fileDataFolder); + provider = new ArenaJsonProvider(fileDataFolder, guilds.getDataFolder()); break; case MYSQL: case SQLITE: diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java index 7bbf70e5a..c2090b21b 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java @@ -18,7 +18,13 @@ import java.util.Set; public class ArenaJsonProvider implements ArenaProvider { + /** + * The kind of record the arena files hold, used as the name of the backup folder. + */ + private static final String DATA_TYPE = "arenas"; + private final File dataFolder; + private final File pluginDataFolder; private Gson gson; /** @@ -32,7 +38,16 @@ public class ArenaJsonProvider implements ArenaProvider { private final Set unreadableFiles = new HashSet<>(); public ArenaJsonProvider(File dataFolder) { + this(dataFolder, dataFolder.getParentFile()); + } + + /** + * @param dataFolder the folder the arena files live in + * @param pluginDataFolder the plugin's data folder, where the backups are kept + */ + public ArenaJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; + this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); } @@ -73,6 +88,7 @@ public List getAllArenas(@Nullable String tablePrefix) throws IOException unreadableFiles.add(file.getName()); LoggingUtils.severe("The Arena file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, "the record has no id"); continue; } @@ -83,6 +99,7 @@ public List getAllArenas(@Nullable String tablePrefix) throws IOException LoggingUtils.severe("There was an error loading an Arena from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Arena has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, ex); } } diff --git a/src/main/java/me/glaremasters/guilds/database/challenges/ChallengeAdapter.java b/src/main/java/me/glaremasters/guilds/database/challenges/ChallengeAdapter.java index 2fbf938d9..45f9ff0aa 100644 --- a/src/main/java/me/glaremasters/guilds/database/challenges/ChallengeAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/challenges/ChallengeAdapter.java @@ -22,7 +22,7 @@ public ChallengeAdapter(Guilds guilds, DatabaseAdapter adapter) { default: case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "challenges"); - provider = new ChallengeJsonProvider(fileDataFolder); + provider = new ChallengeJsonProvider(fileDataFolder, guilds.getDataFolder()); break; case MYSQL: case SQLITE: diff --git a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java index 4bfa6a8a1..7915240ec 100644 --- a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java @@ -16,7 +16,13 @@ import java.util.Set; public class ChallengeJsonProvider implements ChallengeProvider { + /** + * The kind of record the challenge files hold, used as the name of the backup folder. + */ + private static final String DATA_TYPE = "challenges"; + private final File dataFolder; + private final File pluginDataFolder; private final Gson gson; /** @@ -29,7 +35,16 @@ public class ChallengeJsonProvider implements ChallengeProvider { private final Set unreadableFiles = new HashSet<>(); public ChallengeJsonProvider(File dataFolder) { + this(dataFolder, dataFolder.getParentFile()); + } + + /** + * @param dataFolder the folder the challenge files live in + * @param pluginDataFolder the plugin's data folder, where the backups are kept + */ + public ChallengeJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; + this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); } @@ -53,6 +68,7 @@ public Set getAllChallenges(@Nullable String tablePrefix) throws unreadableFiles.add(file.getName()); LoggingUtils.severe("The GuildChallenge file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, "the record has no id"); continue; } @@ -63,6 +79,7 @@ public Set getAllChallenges(@Nullable String tablePrefix) throws LoggingUtils.severe("There was an error loading a GuildChallenge from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this GuildChallenge has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, ex); } } diff --git a/src/main/java/me/glaremasters/guilds/database/cooldowns/CooldownAdapter.java b/src/main/java/me/glaremasters/guilds/database/cooldowns/CooldownAdapter.java index 0cee3e053..cc89cadc5 100644 --- a/src/main/java/me/glaremasters/guilds/database/cooldowns/CooldownAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/cooldowns/CooldownAdapter.java @@ -24,7 +24,7 @@ public CooldownAdapter(Guilds guilds, DatabaseAdapter adapter) { default: case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "cooldowns"); - provider = new CooldownJsonProvider(fileDataFolder); + provider = new CooldownJsonProvider(fileDataFolder, guilds.getDataFolder()); break; case MYSQL: case SQLITE: diff --git a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java index c00edd869..a7c8cae16 100644 --- a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java @@ -23,12 +23,27 @@ import java.util.UUID; public class CooldownJsonProvider implements CooldownProvider { + /** + * The kind of record the cooldown file holds, used as the name of the backup folder. + */ + private static final String DATA_TYPE = "cooldowns"; + private final File dataFolder; + private final File pluginDataFolder; private final File cooldownFile; private Gson gson; public CooldownJsonProvider(File dataFolder) { + this(dataFolder, dataFolder.getParentFile()); + } + + /** + * @param dataFolder the folder the cooldown file lives in + * @param pluginDataFolder the plugin's data folder, where the backups are kept + */ + public CooldownJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; + this.pluginDataFolder = pluginDataFolder; this.cooldownFile = new File(dataFolder, "cooldowns.json"); this.gson = Guilds.getGson(); } @@ -108,6 +123,8 @@ private CooldownFile readCooldownFile() throws IOException { try (Reader reader = Files.newBufferedReader(this.cooldownFile.toPath(), StandardCharsets.UTF_8)) { root = new JsonParser().parse(reader); } catch (RuntimeException ex) { + // The file cannot be read at all, so it is kept aside before the caller is told. + JsonFileUtils.backupCorruptFile(pluginDataFolder, this.cooldownFile, DATA_TYPE, ex); throw new IOException("Failed to parse cooldowns JSON file: " + this.cooldownFile.getAbsolutePath(), ex); } @@ -117,6 +134,7 @@ private CooldownFile readCooldownFile() throws IOException { } if (!root.isJsonArray()) { + JsonFileUtils.backupCorruptFile(pluginDataFolder, this.cooldownFile, DATA_TYPE, "the file does not hold a list of cooldowns"); throw new IOException("The cooldowns JSON file does not hold a list of cooldowns: " + this.cooldownFile.getAbsolutePath()); } @@ -133,7 +151,7 @@ private CooldownFile readCooldownFile() throws IOException { if (cooldown == null || cooldown.getCooldownType() == null || cooldown.getCooldownOwner() == null) { contents.unreadable.add(entry); LoggingUtils.severe("There was an error loading a Cooldown from the cooldowns file: " + this.cooldownFile.getAbsolutePath()); - LoggingUtils.severe("To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); + LoggingUtils.severe("The entry at position " + i + " could not be read. To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); continue; } @@ -141,10 +159,20 @@ private CooldownFile readCooldownFile() throws IOException { } catch (RuntimeException ex) { contents.unreadable.add(entry); LoggingUtils.severe("There was an error loading a Cooldown from the cooldowns file: " + this.cooldownFile.getAbsolutePath(), ex); - LoggingUtils.severe("To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); + LoggingUtils.severe("The entry at position " + i + " could not be read. To prevent data loss in the plugin, this Cooldown has been prevented from loading. The entry has been left in the file as it is."); } } + /* + * One entry that cannot be read costs every entry in the file, because the file is the + * only place the cooldowns are. The whole file is kept aside, and the entries that could + * be read are still loaded. + */ + if (!contents.unreadable.isEmpty()) { + JsonFileUtils.backupCorruptFile(pluginDataFolder, this.cooldownFile, DATA_TYPE, + contents.unreadable.size() + " of the " + entries.size() + " entries could not be read"); + } + return contents; } diff --git a/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java b/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java index f2836bf7b..8bcf05f25 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java @@ -23,7 +23,7 @@ public GuildAdapter(Guilds guilds, DatabaseAdapter adapter) { default: case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "data"); - provider = new GuildJsonProvider(fileDataFolder); + provider = new GuildJsonProvider(fileDataFolder, guilds.getDataFolder()); break; case MYSQL: case SQLITE: diff --git a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java index 3a0a22c28..32e1e15fe 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java @@ -23,7 +23,13 @@ * Time: 11:38 AM */ public class GuildJsonProvider implements GuildProvider { + /** + * The kind of record the guild files hold, used as the name of the backup folder. + */ + private static final String DATA_TYPE = "guilds"; + private final File dataFolder; + private final File pluginDataFolder; private Gson gson; /** @@ -37,7 +43,16 @@ public class GuildJsonProvider implements GuildProvider { private final Set unreadableFiles = new HashSet<>(); public GuildJsonProvider(File dataFolder) { + this(dataFolder, dataFolder.getParentFile()); + } + + /** + * @param dataFolder the folder the guild files live in + * @param pluginDataFolder the plugin's data folder, where the backups are kept + */ + public GuildJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; + this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); } @@ -78,6 +93,7 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException unreadableFiles.add(file.getName()); LoggingUtils.severe("The Guild file " + file.getAbsolutePath() + " has no id, so it has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, "the record has no id"); continue; } @@ -88,6 +104,7 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException LoggingUtils.severe("There was an error loading a Guild from the following file: " + file.getAbsolutePath(), ex); LoggingUtils.severe("To prevent data loss in the plugin, this Guild has been prevented from loading."); LoggingUtils.severe("The file has been left where it is, so it can be repaired by hand."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, ex); } } diff --git a/src/test/java/me/glaremasters/guilds/database/CorruptBackups.java b/src/test/java/me/glaremasters/guilds/database/CorruptBackups.java new file mode 100644 index 000000000..def63ba6f --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/CorruptBackups.java @@ -0,0 +1,70 @@ +package me.glaremasters.guilds.database; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Finds the backups the plugin writes for the data it cannot read. + * + *

The backups live under {@code plugins/Guilds/backups/corrupt///}, so + * the kind of record a backup belongs to is the name of the folder it sits in. + */ +public final class CorruptBackups { + + private CorruptBackups() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + } + + /** + * @param pluginFolder the plugin's data folder + * @return the folder every backup lives under + */ + public static Path backupRoot(Path pluginFolder) { + return pluginFolder.resolve("backups").resolve("corrupt"); + } + + /** + * @param pluginFolder the plugin's data folder + * @param dataType the kind of record + * @return every backup file of that kind of record + * @throws IOException if the backups cannot be listed + */ + public static List backupFiles(Path pluginFolder, String dataType) throws IOException { + final List files = new ArrayList<>(); + final Path root = backupRoot(pluginFolder); + + if (!Files.isDirectory(root)) { + return files; + } + + try (Stream paths = Files.walk(root)) { + paths.filter(Files::isRegularFile) + .filter(path -> path.getParent() != null && dataType.equals(path.getParent().getFileName().toString())) + .forEach(files::add); + } + + return files; + } + + /** + * @param pluginFolder the plugin's data folder + * @param dataType the kind of record + * @return the one backup of that kind of record + * @throws IOException if the backups cannot be listed + */ + public static Path onlyBackup(Path pluginFolder, String dataType) throws IOException { + final List files = backupFiles(pluginFolder, dataType); + + if (files.size() != 1) { + fail("expected one " + dataType + " backup but found " + files.size() + ": " + files); + } + + return files.get(0); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java b/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java new file mode 100644 index 000000000..0c0cf6728 --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java @@ -0,0 +1,144 @@ +package me.glaremasters.guilds.database; + +import me.glaremasters.guilds.database.arenas.provider.ArenaJsonProvider; +import me.glaremasters.guilds.database.challenges.provider.ChallengeJsonProvider; +import me.glaremasters.guilds.database.cooldowns.provider.CooldownJsonProvider; +import me.glaremasters.guilds.database.guild.provider.GuildJsonProvider; +import me.glaremasters.guilds.guild.GuildChallenge; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Covers the backups as a whole. + * + *

The backups live outside the four data folders, so a backup is never loaded as a record, + * however it is named. Two of them with the same name must not overwrite each other either. + */ +class JsonStorageBackupTest { + + private static final String GUILD_ID = "11111111-1111-1111-1111-111111111111"; + private static final String ARENA_ID = "33333333-3333-3333-3333-333333333333"; + private static final String CHALLENGE_ID = "44444444-4444-4444-4444-444444444444"; + + @TempDir + Path tempDir; + + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + + @BeforeEach + void setUp() throws IOException { + Files.createDirectories(dataFolder()); + Files.createDirectories(arenaFolder()); + Files.createDirectories(challengeFolder()); + Files.createDirectories(cooldownFolder()); + + Files.write(dataFolder().resolve(GUILD_ID + ".json"), ("{ \"id\": \"" + GUILD_ID + "\", \"name\": \"A guild\" }") + .getBytes(StandardCharsets.UTF_8)); + Files.write(arenaFolder().resolve(ARENA_ID + ".json"), ("{ \"id\": \"" + ARENA_ID + "\", \"name\": \"An arena\" }") + .getBytes(StandardCharsets.UTF_8)); + Files.write(challengeFolder().resolve(CHALLENGE_ID + ".json"), ("{ \"id\": \"" + CHALLENGE_ID + "\", \"isCompleted\": false }") + .getBytes(StandardCharsets.UTF_8)); + Files.write(cooldownFolder().resolve("cooldowns.json"), "[]".getBytes(StandardCharsets.UTF_8)); + } + + private Path pluginFolder() { + return tempDir; + } + + private Path dataFolder() { + return tempDir.resolve("data"); + } + + private Path arenaFolder() { + return tempDir.resolve("arenas"); + } + + private Path challengeFolder() { + return tempDir.resolve("challenges"); + } + + private Path cooldownFolder() { + return tempDir.resolve("cooldowns"); + } + + @Test + @DisplayName("a backup is never enumerated as a record by any of the four providers") + void aBackupIsNeverEnumeratedAsARecord() throws IOException { + // A backup, where the helper puts it, and a copy of one an operator left in the backup + // folder by hand. + final Path backup = tempDir.resolve("backups").resolve("corrupt").resolve("20200101-000000").resolve("guilds") + .resolve(GUILD_ID + ".json"); + Files.createDirectories(backup.getParent()); + Files.write(backup, ("{ \"id\": \"" + GUILD_ID + "\", \"name\": \"A guild\" }").getBytes(StandardCharsets.UTF_8)); + Files.write(tempDir.resolve("backups").resolve(GUILD_ID + ".json"), + ("{ \"id\": \"" + GUILD_ID + "\", \"name\": \"A guild\" }").getBytes(StandardCharsets.UTF_8)); + + final GuildJsonProvider guilds = new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + final ArenaJsonProvider arenas = new ArenaJsonProvider(arenaFolder().toFile(), pluginFolder().toFile()); + final ChallengeJsonProvider challenges = new ChallengeJsonProvider(challengeFolder().toFile(), pluginFolder().toFile()); + final CooldownJsonProvider cooldowns = new CooldownJsonProvider(cooldownFolder().toFile(), pluginFolder().toFile()); + + assertEquals(List.of(GUILD_ID), guilds.getAllGuildIds(null)); + assertEquals(List.of(ARENA_ID), arenas.getAllArenaIds(null)); + + final Set loaded = challenges.getAllChallenges(null); + assertEquals(1, loaded.size()); + assertEquals(CHALLENGE_ID, loaded.iterator().next().getId().toString()); + + assertTrue(cooldowns.getAllCooldowns(null).isEmpty()); + + // The backup is still where it was. + assertTrue(Files.exists(backup)); + } + + @Test + @DisplayName("the backup folder sits beside the data folders, not inside one") + void theBackupFolderSitsBesideTheDataFolders() throws IOException { + Files.write(dataFolder().resolve("broken.json"), "{ this is not json".getBytes(StandardCharsets.UTF_8)); + + new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()).getAllGuilds(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + assertFalse(backup.startsWith(dataFolder())); + assertFalse(backup.startsWith(arenaFolder())); + assertFalse(backup.startsWith(challengeFolder())); + assertFalse(backup.startsWith(cooldownFolder())); + } + + @Test + @DisplayName("two corrupt files with the same name in two storages do not overwrite each other") + void twoCorruptFilesWithTheSameNameInTwoStorages() throws IOException { + final String corrupt = "{ this is not json"; + Files.write(dataFolder().resolve("same-name.json"), corrupt.getBytes(StandardCharsets.UTF_8)); + Files.write(arenaFolder().resolve("same-name.json"), corrupt.getBytes(StandardCharsets.UTF_8)); + + new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()).getAllGuilds(null); + new ArenaJsonProvider(arenaFolder().toFile(), pluginFolder().toFile()).getAllArenas(null); + + final Path guildBackup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + final Path arenaBackup = CorruptBackups.onlyBackup(pluginFolder(), "arenas"); + + assertEquals("same-name.json", guildBackup.getFileName().toString()); + assertEquals("same-name.json", arenaBackup.getFileName().toString()); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(guildBackup)); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(arenaBackup)); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java index 156aee8f4..80d06de4b 100644 --- a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java @@ -1,6 +1,7 @@ package me.glaremasters.guilds.database.arenas.provider; import me.glaremasters.guilds.arena.Arena; +import me.glaremasters.guilds.database.CorruptBackups; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; import org.junit.jupiter.api.BeforeAll; @@ -13,8 +14,11 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.util.Arrays; import java.util.List; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -23,7 +27,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Covers which files the JSON arena storage treats as arenas. + * Covers which files the JSON arena storage treats as arenas, and what happens to the ones it + * cannot read. * *

The arena folder has the same problem the guild folder had: everything in it was taken to be * an arena. See {@code GuildJsonProviderTest} for the full story. @@ -44,19 +49,34 @@ static void installGson() throws ReflectiveOperationException { @BeforeEach void setUp() throws IOException { - provider = new ArenaJsonProvider(tempDir.toFile()); + Files.createDirectories(dataFolder()); + provider = new ArenaJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); write(ARENA_ID + ".json", arenaJson(ARENA_ID, "The Real Arena")); write(ARENA_ID + ".json" + "1234567890" + ".tmp", arenaJson(ARENA_ID, "The Real Arena")); write("notes.txt", "this is not an arena"); write("README", "nothing to see here"); - Files.createDirectory(tempDir.resolve("backup")); - write(tempDir.resolve("backup").resolve("old-arena.json"), arenaJson(ARENA_ID, "Old Arena")); + Files.createDirectory(dataFolder().resolve("backup")); + write(dataFolder().resolve("backup").resolve("old-arena.json"), arenaJson(ARENA_ID, "Old Arena")); + } + + /** + * @return the plugin's data folder, the one the backups are kept in + */ + private Path pluginFolder() { + return tempDir; + } + + /** + * @return the folder the arena files live in + */ + private Path dataFolder() { + return tempDir.resolve("arenas"); } private void write(String name, String content) throws IOException { - write(tempDir.resolve(name), content); + write(dataFolder().resolve(name), content); } private void write(Path path, String content) throws IOException { @@ -132,12 +152,12 @@ void ignoredFilesAreLeftAlone() throws IOException { provider.arenaExists(null, ARENA_ID); provider.getArena(null, ARENA_ID); - assertTrue(Files.exists(tempDir.resolve(ARENA_ID + ".json"))); - assertTrue(Files.exists(tempDir.resolve(ARENA_ID + ".json" + "1234567890" + ".tmp"))); - assertTrue(Files.exists(tempDir.resolve("notes.txt"))); - assertTrue(Files.exists(tempDir.resolve("README"))); - assertTrue(Files.isDirectory(tempDir.resolve("backup"))); - assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-arena.json"))); + assertTrue(Files.exists(dataFolder().resolve(ARENA_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(ARENA_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(dataFolder().resolve("notes.txt"))); + assertTrue(Files.exists(dataFolder().resolve("README"))); + assertTrue(Files.isDirectory(dataFolder().resolve("backup"))); + assertTrue(Files.exists(dataFolder().resolve("backup").resolve("old-arena.json"))); } @Test @@ -158,9 +178,9 @@ void aBrokenArenaFileIsLeftOnDisk() throws IOException { provider.getAllArenas(null); - assertTrue(Files.exists(tempDir.resolve("broken-arena.json"))); + assertTrue(Files.exists(dataFolder().resolve("broken-arena.json"))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve("broken-arena.json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve("broken-arena.json")), StandardCharsets.UTF_8)); } @Test @@ -181,7 +201,7 @@ void anArenaFileWithoutAnIdIsLeftOnDisk() throws IOException { provider.getAllArenas(null); - assertTrue(Files.exists(tempDir.resolve("no-id-arena.json"))); + assertTrue(Files.exists(dataFolder().resolve("no-id-arena.json"))); } @Test @@ -193,7 +213,7 @@ void anArenaFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { assertThrows(IOException.class, () -> provider.updateArena(null, "broken-arena", arenaJson(ARENA_ID, "Replaced"))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve("broken-arena.json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve("broken-arena.json")), StandardCharsets.UTF_8)); } @Test @@ -206,7 +226,7 @@ void anArenaFileThatCouldNotBeReadIsNotDeleted() throws IOException { // The arena storage deletes the files it has no record for, so a file it could not read // has to survive that pass. provider.deleteArena(null, "broken-arena"); - assertTrue(Files.exists(tempDir.resolve("broken-arena.json"))); + assertTrue(Files.exists(dataFolder().resolve("broken-arena.json"))); } @Test @@ -217,6 +237,141 @@ void anArenaFileWithoutAnIdIsNotDeleted() throws IOException { provider.getAllArenas(null); provider.deleteArena(null, "no-id-arena"); - assertTrue(Files.exists(tempDir.resolve("no-id-arena.json"))); + assertTrue(Files.exists(dataFolder().resolve("no-id-arena.json"))); + } + + @Test + @DisplayName("an arena file that cannot be read is backed up byte for byte") + void aBrokenArenaFileIsBackedUpByteForByte() throws IOException { + final String corrupt = "{ this is not json"; + write("broken-arena.json", corrupt); + + provider.getAllArenas(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "arenas"); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + + assertTrue(Files.exists(dataFolder().resolve("broken-arena.json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("broken-arena.json"))); + } + + @Test + @DisplayName("an arena file without an id is backed up byte for byte") + void anArenaFileWithoutAnIdIsBackedUpByteForByte() throws IOException { + final String unusable = "{ \"name\": \"No Id Here\" }"; + write("no-id-arena.json", unusable); + + provider.getAllArenas(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "arenas"); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("no-id-arena.json"))); + } + + @Test + @DisplayName("the arena backup lands outside the data folder and keeps the file name") + void theBackupLandsOutsideTheDataFolder() throws IOException { + write("broken-arena.json", "{ this is not json"); + + provider.getAllArenas(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "arenas"); + + assertEquals("broken-arena.json", backup.getFileName().toString()); + assertEquals("arenas", backup.getParent().getFileName().toString()); + assertTrue(backup.getParent().getParent().getFileName().toString().matches("\\d{8}-\\d{6}"), + "the backup folder is not named after a timestamp: " + backup); + assertEquals("corrupt", backup.getParent().getParent().getParent().getFileName().toString()); + assertEquals("backups", backup.getParent().getParent().getParent().getParent().getFileName().toString()); + assertEquals(pluginFolder(), backup.getParent().getParent().getParent().getParent().getParent()); + assertFalse(backup.startsWith(dataFolder())); + } + + @Test + @DisplayName("the log names the arena file, the reason and the backup") + void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { + write("broken-arena.json", "{ this is not json"); + + try (LogCapture log = new LogCapture()) { + provider.getAllArenas(null); + + assertTrue(log.contains("broken-arena.json"), "the log did not name the file"); + assertTrue(log.contains("could not be read"), "the log did not say what went wrong"); + assertTrue(log.contains("backups"), "the log did not say where the backup is"); + } + } + + @Test + @DisplayName("two corrupt arena files with the same name do not overwrite each other") + void twoCorruptArenaFilesWithTheSameNameDoNotOverwriteEachOther() throws IOException { + final Path file = dataFolder().resolve("broken-arena.json"); + final String first = "{ this is not json"; + write("broken-arena.json", first); + final long modified = Files.getLastModifiedTime(file).toMillis(); + + provider.getAllArenas(null); + + final String second = "{ this is not json either"; + write("broken-arena.json", second); + Files.setLastModifiedTime(file, FileTime.fromMillis(modified)); + + provider.getAllArenas(null); + + final List backups = CorruptBackups.backupFiles(pluginFolder(), "arenas"); + assertEquals(2, backups.size()); + assertEquals(2, backups.stream().map(path -> path.getFileName().toString()).distinct().count()); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(first.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the first backup was overwritten"); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(second.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the second backup was not written"); + } + + @Test + @DisplayName("an arena backup that cannot be written is reported and nothing is lost") + void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { + final String corrupt = "{ this is not json"; + write("broken-arena.json", corrupt); + + Files.createFile(pluginFolder().resolve("backups")); + + try (LogCapture log = new LogCapture()) { + provider.getAllArenas(null); + + assertTrue(log.contains("could not be written"), "the failure to back the file up was not reported"); + assertTrue(log.contains("broken-arena.json"), "the log did not name the file"); + } + + assertTrue(Files.exists(dataFolder().resolve("broken-arena.json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("broken-arena.json"))); + assertEquals(1, provider.getAllArenas(null).size()); + } + + @Test + @DisplayName("the same arena corruption is backed up once, not once per read") + void theSameCorruptionIsBackedUpOnce() throws IOException { + write("broken-arena.json", "{ this is not json"); + + provider.getAllArenas(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "arenas").size()); + + provider.getAllArenas(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "arenas").size()); + + final ArenaJsonProvider restarted = new ArenaJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + restarted.getAllArenas(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "arenas").size()); } } diff --git a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java index fa69479f9..eb147c1c8 100644 --- a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java @@ -1,5 +1,6 @@ package me.glaremasters.guilds.database.challenges.provider; +import me.glaremasters.guilds.database.CorruptBackups; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; import me.glaremasters.guilds.guild.GuildChallenge; @@ -13,8 +14,12 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.util.Arrays; +import java.util.List; import java.util.Set; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -23,7 +28,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Covers which files the JSON challenge storage treats as challenges. + * Covers which files the JSON challenge storage treats as challenges, and what happens to the ones + * it cannot read. * *

The challenge folder has the same problem the guild folder had: everything in it was taken to * be a challenge. See {@code GuildJsonProviderTest} for the full story. @@ -44,19 +50,34 @@ static void installGson() throws ReflectiveOperationException { @BeforeEach void setUp() throws IOException { - provider = new ChallengeJsonProvider(tempDir.toFile()); + Files.createDirectories(dataFolder()); + provider = new ChallengeJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); write(CHALLENGE_ID + ".json", challengeJson(CHALLENGE_ID)); write(CHALLENGE_ID + ".json" + "1234567890" + ".tmp", challengeJson(CHALLENGE_ID)); write("notes.txt", "this is not a challenge"); write("README", "nothing to see here"); - Files.createDirectory(tempDir.resolve("backup")); - write(tempDir.resolve("backup").resolve("old-challenge.json"), challengeJson(CHALLENGE_ID)); + Files.createDirectory(dataFolder().resolve("backup")); + write(dataFolder().resolve("backup").resolve("old-challenge.json"), challengeJson(CHALLENGE_ID)); + } + + /** + * @return the plugin's data folder, the one the backups are kept in + */ + private Path pluginFolder() { + return tempDir; + } + + /** + * @return the folder the challenge files live in + */ + private Path dataFolder() { + return tempDir.resolve("challenges"); } private void write(String name, String content) throws IOException { - write(tempDir.resolve(name), content); + write(dataFolder().resolve(name), content); } private void write(Path path, String content) throws IOException { @@ -124,12 +145,12 @@ void ignoredFilesAreLeftAlone() throws IOException { provider.challengeExists(null, CHALLENGE_ID); provider.getChallenge(null, CHALLENGE_ID); - assertTrue(Files.exists(tempDir.resolve(CHALLENGE_ID + ".json"))); - assertTrue(Files.exists(tempDir.resolve(CHALLENGE_ID + ".json" + "1234567890" + ".tmp"))); - assertTrue(Files.exists(tempDir.resolve("notes.txt"))); - assertTrue(Files.exists(tempDir.resolve("README"))); - assertTrue(Files.isDirectory(tempDir.resolve("backup"))); - assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-challenge.json"))); + assertTrue(Files.exists(dataFolder().resolve(CHALLENGE_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(CHALLENGE_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(dataFolder().resolve("notes.txt"))); + assertTrue(Files.exists(dataFolder().resolve("README"))); + assertTrue(Files.isDirectory(dataFolder().resolve("backup"))); + assertTrue(Files.exists(dataFolder().resolve("backup").resolve("old-challenge.json"))); } @Test @@ -150,9 +171,9 @@ void aBrokenChallengeFileIsLeftOnDisk() throws IOException { provider.getAllChallenges(null); - assertTrue(Files.exists(tempDir.resolve("broken-challenge.json"))); + assertTrue(Files.exists(dataFolder().resolve("broken-challenge.json"))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve("broken-challenge.json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve("broken-challenge.json")), StandardCharsets.UTF_8)); } @Test @@ -173,7 +194,7 @@ void aChallengeFileWithoutAnIdIsLeftOnDisk() throws IOException { provider.getAllChallenges(null); - assertTrue(Files.exists(tempDir.resolve("no-id-challenge.json"))); + assertTrue(Files.exists(dataFolder().resolve("no-id-challenge.json"))); } @Test @@ -185,7 +206,7 @@ void aChallengeFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { assertThrows(IOException.class, () -> provider.updateChallenge(null, "broken-challenge", challengeJson(CHALLENGE_ID))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve("broken-challenge.json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve("broken-challenge.json")), StandardCharsets.UTF_8)); } @Test @@ -196,6 +217,141 @@ void aChallengeFileThatCouldNotBeReadIsNotDeleted() throws IOException { provider.getAllChallenges(null); assertThrows(IOException.class, () -> provider.deleteChallenge(null, "broken-challenge")); - assertTrue(Files.exists(tempDir.resolve("broken-challenge.json"))); + assertTrue(Files.exists(dataFolder().resolve("broken-challenge.json"))); + } + + @Test + @DisplayName("a challenge file that cannot be read is backed up byte for byte") + void aBrokenChallengeFileIsBackedUpByteForByte() throws IOException { + final String corrupt = "{ this is not json"; + write("broken-challenge.json", corrupt); + + provider.getAllChallenges(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "challenges"); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + + assertTrue(Files.exists(dataFolder().resolve("broken-challenge.json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("broken-challenge.json"))); + } + + @Test + @DisplayName("a challenge file without an id is backed up byte for byte") + void aChallengeFileWithoutAnIdIsBackedUpByteForByte() throws IOException { + final String unusable = "{ \"isCompleted\": false }"; + write("no-id-challenge.json", unusable); + + provider.getAllChallenges(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "challenges"); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("no-id-challenge.json"))); + } + + @Test + @DisplayName("the challenge backup lands outside the data folder and keeps the file name") + void theBackupLandsOutsideTheDataFolder() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + provider.getAllChallenges(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "challenges"); + + assertEquals("broken-challenge.json", backup.getFileName().toString()); + assertEquals("challenges", backup.getParent().getFileName().toString()); + assertTrue(backup.getParent().getParent().getFileName().toString().matches("\\d{8}-\\d{6}"), + "the backup folder is not named after a timestamp: " + backup); + assertEquals("corrupt", backup.getParent().getParent().getParent().getFileName().toString()); + assertEquals("backups", backup.getParent().getParent().getParent().getParent().getFileName().toString()); + assertEquals(pluginFolder(), backup.getParent().getParent().getParent().getParent().getParent()); + assertFalse(backup.startsWith(dataFolder())); + } + + @Test + @DisplayName("the log names the challenge file, the reason and the backup") + void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + try (LogCapture log = new LogCapture()) { + provider.getAllChallenges(null); + + assertTrue(log.contains("broken-challenge.json"), "the log did not name the file"); + assertTrue(log.contains("could not be read"), "the log did not say what went wrong"); + assertTrue(log.contains("backups"), "the log did not say where the backup is"); + } + } + + @Test + @DisplayName("two corrupt challenge files with the same name do not overwrite each other") + void twoCorruptChallengeFilesWithTheSameNameDoNotOverwriteEachOther() throws IOException { + final Path file = dataFolder().resolve("broken-challenge.json"); + final String first = "{ this is not json"; + write("broken-challenge.json", first); + final long modified = Files.getLastModifiedTime(file).toMillis(); + + provider.getAllChallenges(null); + + final String second = "{ this is not json either"; + write("broken-challenge.json", second); + Files.setLastModifiedTime(file, FileTime.fromMillis(modified)); + + provider.getAllChallenges(null); + + final List backups = CorruptBackups.backupFiles(pluginFolder(), "challenges"); + assertEquals(2, backups.size()); + assertEquals(2, backups.stream().map(path -> path.getFileName().toString()).distinct().count()); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(first.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the first backup was overwritten"); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(second.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the second backup was not written"); + } + + @Test + @DisplayName("a challenge backup that cannot be written is reported and nothing is lost") + void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { + final String corrupt = "{ this is not json"; + write("broken-challenge.json", corrupt); + + Files.createFile(pluginFolder().resolve("backups")); + + try (LogCapture log = new LogCapture()) { + provider.getAllChallenges(null); + + assertTrue(log.contains("could not be written"), "the failure to back the file up was not reported"); + assertTrue(log.contains("broken-challenge.json"), "the log did not name the file"); + } + + assertTrue(Files.exists(dataFolder().resolve("broken-challenge.json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve("broken-challenge.json"))); + assertEquals(1, provider.getAllChallenges(null).size()); + } + + @Test + @DisplayName("the same challenge corruption is backed up once, not once per read") + void theSameCorruptionIsBackedUpOnce() throws IOException { + write("broken-challenge.json", "{ this is not json"); + + provider.getAllChallenges(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "challenges").size()); + + provider.getAllChallenges(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "challenges").size()); + + final ChallengeJsonProvider restarted = new ChallengeJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + restarted.getAllChallenges(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "challenges").size()); } } diff --git a/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java index 9aa46d442..1948d1f73 100644 --- a/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java @@ -3,6 +3,8 @@ import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; import me.glaremasters.guilds.cooldowns.Cooldown; +import me.glaremasters.guilds.database.CorruptBackups; +import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -20,11 +22,14 @@ import java.util.List; import java.util.UUID; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Covers what the JSON cooldown storage does with an entry it cannot read. + * Covers what the JSON cooldown storage does with an entry it cannot read, and with the file it + * cannot read at all. * *

The cooldown file is the one data file that holds a list rather than a single record, so it * is the one place where a single broken entry used to cost every other entry as well: the file @@ -48,12 +53,27 @@ static void installGson() throws ReflectiveOperationException { @BeforeEach void setUp() throws IOException { - provider = new CooldownJsonProvider(tempDir.toFile()); + Files.createDirectories(dataFolder()); + provider = new CooldownJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); provider.createContainer(null); } + /** + * @return the plugin's data folder, the one the backups are kept in + */ + private Path pluginFolder() { + return tempDir; + } + + /** + * @return the folder the cooldown file lives in + */ + private Path dataFolder() { + return tempDir.resolve("cooldowns"); + } + private Path cooldownFile() { - return tempDir.resolve("cooldowns.json"); + return dataFolder().resolve("cooldowns.json"); } private void writeCooldowns(String content) throws IOException { @@ -138,4 +158,133 @@ void theFileFormatIsUnchanged() throws IOException { void anEmptyFileHoldsNoCooldowns() throws IOException { assertTrue(provider.getAllCooldowns(null).isEmpty()); } + + @Test + @DisplayName("a cooldown file that cannot be read is backed up byte for byte") + void aBrokenCooldownFileIsBackedUpByteForByte() throws IOException { + final String corrupt = "{ this is not json"; + writeCooldowns(corrupt); + + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "cooldowns"); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + + // The original is still there, with the same bytes. + assertTrue(Files.exists(cooldownFile())); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + + @Test + @DisplayName("a cooldown file that does not hold a list is backed up") + void aCooldownFileThatIsNotAListIsBackedUp() throws IOException { + final String unusable = "{ \"not\": \"a list\" }"; + writeCooldowns(unusable); + + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "cooldowns"); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + + @Test + @DisplayName("a cooldown file with a broken entry is backed up") + void aCooldownFileWithABrokenEntryIsBackedUp() throws IOException { + final String broken = "[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n" + + "]\n"; + writeCooldowns(broken); + + provider.getAllCooldowns(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "cooldowns"); + assertArrayEquals(broken.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(broken.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + + @Test + @DisplayName("the cooldown backup lands outside the data folder and keeps the file name") + void theBackupLandsOutsideTheDataFolder() throws IOException { + writeCooldowns("{ this is not json"); + + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "cooldowns"); + + assertEquals("cooldowns.json", backup.getFileName().toString()); + assertEquals("cooldowns", backup.getParent().getFileName().toString()); + assertTrue(backup.getParent().getParent().getFileName().toString().matches("\\d{8}-\\d{6}"), + "the backup folder is not named after a timestamp: " + backup); + assertEquals("corrupt", backup.getParent().getParent().getParent().getFileName().toString()); + assertEquals("backups", backup.getParent().getParent().getParent().getParent().getFileName().toString()); + assertEquals(pluginFolder(), backup.getParent().getParent().getParent().getParent().getParent()); + assertTrue(backup.startsWith(pluginFolder())); + } + + @Test + @DisplayName("the log names the cooldown file, the reason and the backup") + void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { + writeCooldowns("{ this is not json"); + + try (LogCapture log = new LogCapture()) { + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + + assertTrue(log.contains("cooldowns.json"), "the log did not name the file"); + assertTrue(log.contains("could not be read"), "the log did not say what went wrong"); + assertTrue(log.contains("backups"), "the log did not say where the backup is"); + } + } + + @Test + @DisplayName("the log says which entry of the cooldown file could not be read") + void theLogSaysWhichEntryCouldNotBeRead() throws IOException { + writeCooldowns("[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n" + + "]\n"); + + try (LogCapture log = new LogCapture()) { + provider.getAllCooldowns(null); + + assertTrue(log.contains("position 0"), "the log did not say which entry was at fault"); + } + } + + @Test + @DisplayName("a cooldown backup that cannot be written is reported and nothing is lost") + void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { + final String corrupt = "{ this is not json"; + writeCooldowns(corrupt); + + Files.createFile(pluginFolder().resolve("backups")); + + try (LogCapture log = new LogCapture()) { + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + + assertTrue(log.contains("could not be written"), "the failure to back the file up was not reported"); + assertTrue(log.contains("cooldowns.json"), "the log did not name the file"); + } + + assertTrue(Files.exists(cooldownFile())); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + + @Test + @DisplayName("the same cooldown corruption is backed up once, not once per read") + void theSameCorruptionIsBackedUpOnce() throws IOException { + writeCooldowns("{ this is not json"); + + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "cooldowns").size()); + + assertThrows(IOException.class, () -> provider.getAllCooldowns(null)); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "cooldowns").size()); + + // A restart: a new provider over the same folder. + final CooldownJsonProvider restarted = new CooldownJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + assertThrows(IOException.class, () -> restarted.getAllCooldowns(null)); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "cooldowns").size()); + } } diff --git a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java index 495dd4a83..be37acb20 100644 --- a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java @@ -1,5 +1,6 @@ package me.glaremasters.guilds.database.guild.provider; +import me.glaremasters.guilds.database.CorruptBackups; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; import me.glaremasters.guilds.guild.Guild; @@ -13,8 +14,11 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.util.Arrays; import java.util.List; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -23,12 +27,16 @@ import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Covers which files the JSON guild storage treats as guilds. + * Covers which files the JSON guild storage treats as guilds, and what happens to the ones it + * cannot read. * *

The guild data folder is a folder an operator can see and touch. Anything can end up in it: * a leftover temporary file from a write that was interrupted, a note, a backup folder. The * provider used to take every entry it found and call it a guild, which let a stray file stand in * for a real guild and made the plugin report a guild that did not exist. + * + *

A file the plugin cannot read is a different matter. It is the only copy of that guild's + * data, so it is copied aside and left exactly where it was. */ class GuildJsonProviderTest { @@ -47,7 +55,8 @@ static void installGson() throws ReflectiveOperationException { @BeforeEach void setUp() throws IOException { - provider = new GuildJsonProvider(tempDir.toFile()); + Files.createDirectories(dataFolder()); + provider = new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); // The one real guild. write(GUILD_ID + ".json", guildJson(GUILD_ID, "The Real Guild")); @@ -61,12 +70,26 @@ void setUp() throws IOException { write("README", "nothing to see here"); // A folder, because people make backups that way. - Files.createDirectory(tempDir.resolve("backup")); - write(tempDir.resolve("backup").resolve("old-guild.json"), guildJson(OTHER_GUILD_ID, "Old Guild")); + Files.createDirectory(dataFolder().resolve("backup")); + write(dataFolder().resolve("backup").resolve("old-guild.json"), guildJson(OTHER_GUILD_ID, "Old Guild")); + } + + /** + * @return the plugin's data folder, the one the backups are kept in + */ + private Path pluginFolder() { + return tempDir; + } + + /** + * @return the folder the guild files live in + */ + private Path dataFolder() { + return tempDir.resolve("data"); } private void write(String name, String content) throws IOException { - write(tempDir.resolve(name), content); + write(dataFolder().resolve(name), content); } private void write(Path path, String content) throws IOException { @@ -152,12 +175,12 @@ void ignoredFilesAreLeftAlone() throws IOException { provider.guildExists(null, GUILD_ID); provider.getGuild(null, GUILD_ID); - assertTrue(Files.exists(tempDir.resolve(GUILD_ID + ".json"))); - assertTrue(Files.exists(tempDir.resolve(GUILD_ID + ".json" + "1234567890" + ".tmp"))); - assertTrue(Files.exists(tempDir.resolve("notes.txt"))); - assertTrue(Files.exists(tempDir.resolve("README"))); - assertTrue(Files.isDirectory(tempDir.resolve("backup"))); - assertTrue(Files.exists(tempDir.resolve("backup").resolve("old-guild.json"))); + assertTrue(Files.exists(dataFolder().resolve(GUILD_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(GUILD_ID + ".json" + "1234567890" + ".tmp"))); + assertTrue(Files.exists(dataFolder().resolve("notes.txt"))); + assertTrue(Files.exists(dataFolder().resolve("README"))); + assertTrue(Files.isDirectory(dataFolder().resolve("backup"))); + assertTrue(Files.exists(dataFolder().resolve("backup").resolve("old-guild.json"))); } @Test @@ -178,9 +201,9 @@ void aBrokenGuildFileIsLeftOnDisk() throws IOException { provider.getAllGuilds(null); - assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test @@ -201,9 +224,9 @@ void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { provider.getAllGuilds(null); - assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); assertEquals("{ \"name\": \"No Id Here\" }", new String( - Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test @@ -215,7 +238,7 @@ void aGuildFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); assertEquals("{ this is not json", new String( - Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test @@ -227,7 +250,7 @@ void aGuildFileWithoutAnIdIsNotWrittenOver() throws IOException { assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); assertEquals("{ \"name\": \"No Id Here\" }", new String( - Files.readAllBytes(tempDir.resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test @@ -238,7 +261,7 @@ void aGuildFileThatCouldNotBeReadIsNotDeleted() throws IOException { provider.getAllGuilds(null); assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); - assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); } @Test @@ -249,6 +272,147 @@ void aGuildFileWithoutAnIdIsNotDeleted() throws IOException { provider.getAllGuilds(null); assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); - assertTrue(Files.exists(tempDir.resolve(OTHER_GUILD_ID + ".json"))); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + } + + @Test + @DisplayName("a guild file that cannot be read is backed up byte for byte") + void aBrokenGuildFileIsBackedUpByteForByte() throws IOException { + final String corrupt = "{ this is not json"; + write(OTHER_GUILD_ID + ".json", corrupt); + + provider.getAllGuilds(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + + // The original is still there, with the same bytes. + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + } + + @Test + @DisplayName("a guild file without an id is backed up byte for byte") + void aGuildFileWithoutAnIdIsBackedUpByteForByte() throws IOException { + final String unusable = "{ \"name\": \"No Id Here\" }"; + write(OTHER_GUILD_ID + ".json", unusable); + + provider.getAllGuilds(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(unusable.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + } + + @Test + @DisplayName("the backup lands outside the data folder and keeps the file name") + void theBackupLandsOutsideTheDataFolder() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + + assertEquals(OTHER_GUILD_ID + ".json", backup.getFileName().toString()); + assertEquals("guilds", backup.getParent().getFileName().toString()); + assertTrue(backup.getParent().getParent().getFileName().toString().matches("\\d{8}-\\d{6}"), + "the backup folder is not named after a timestamp: " + backup); + assertEquals("corrupt", backup.getParent().getParent().getParent().getFileName().toString()); + assertEquals("backups", backup.getParent().getParent().getParent().getParent().getFileName().toString()); + assertEquals(pluginFolder(), backup.getParent().getParent().getParent().getParent().getParent()); + assertFalse(backup.startsWith(dataFolder())); + } + + @Test + @DisplayName("the log names the file, the reason and the backup") + void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + try (LogCapture log = new LogCapture()) { + provider.getAllGuilds(null); + + assertTrue(log.contains(OTHER_GUILD_ID + ".json"), "the log did not name the file"); + assertTrue(log.contains("could not be read"), "the log did not say what went wrong"); + assertTrue(log.contains("backups"), "the log did not say where the backup is"); + } + } + + @Test + @DisplayName("two corrupt guild files with the same name do not overwrite each other") + void twoCorruptGuildFilesWithTheSameNameDoNotOverwriteEachOther() throws IOException { + final Path file = dataFolder().resolve(OTHER_GUILD_ID + ".json"); + final String first = "{ this is not json"; + write(OTHER_GUILD_ID + ".json", first); + final long modified = Files.getLastModifiedTime(file).toMillis(); + + provider.getAllGuilds(null); + + // The same name, different content, and the same last modified time, so the second copy + // lands in the same backup folder and has to get a name of its own. + final String second = "{ this is not json either"; + write(OTHER_GUILD_ID + ".json", second); + Files.setLastModifiedTime(file, FileTime.fromMillis(modified)); + + provider.getAllGuilds(null); + + final List backups = CorruptBackups.backupFiles(pluginFolder(), "guilds"); + assertEquals(2, backups.size()); + assertEquals(2, backups.stream().map(path -> path.getFileName().toString()).distinct().count()); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(first.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the first backup was overwritten"); + assertTrue(backups.stream().anyMatch(path -> { + try { + return Arrays.equals(second.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(path)); + } catch (IOException ex) { + return false; + } + }), "the second backup was not written"); + } + + @Test + @DisplayName("a backup that cannot be written is reported and nothing is lost") + void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { + final String corrupt = "{ this is not json"; + write(OTHER_GUILD_ID + ".json", corrupt); + + // A file where the backup folder should be, so the backup cannot be written. + Files.createFile(pluginFolder().resolve("backups")); + + try (LogCapture log = new LogCapture()) { + provider.getAllGuilds(null); + + assertTrue(log.contains("could not be written"), "the failure to back the file up was not reported"); + assertTrue(log.contains(OTHER_GUILD_ID + ".json"), "the log did not name the file"); + } + + // The file itself is untouched, and the guild that reads is still loaded. + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertEquals(1, provider.getAllGuilds(null).size()); + } + + @Test + @DisplayName("the same corruption is backed up once, not once per read") + void theSameCorruptionIsBackedUpOnce() throws IOException { + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); + + provider.getAllGuilds(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); + + // A restart: a new provider over the same folder. + final GuildJsonProvider restarted = new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + restarted.getAllGuilds(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); } } From a17f6d8a90e2594f925825dbf06bb9419f9a3aa7 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:16:15 +0000 Subject: [PATCH 5/7] fix: report a delete that did not happen and catch a swapped folder --- .../guilds/database/JsonFileUtils.java | 39 +++++++++++++++ .../guilds/database/arenas/ArenaAdapter.java | 9 +++- .../arenas/provider/ArenaJsonProvider.java | 17 ++++++- .../provider/ChallengeJsonProvider.java | 15 +++++- .../provider/CooldownJsonProvider.java | 2 + .../guild/provider/GuildJsonProvider.java | 15 +++++- .../database/JsonStorageBackupTest.java | 30 ++++++++++++ .../guilds/database/UnwritableFolders.java | 39 +++++++++++++++ .../provider/ArenaJsonProviderTest.java | 49 +++++++++++++++++++ .../provider/ChallengeJsonProviderTest.java | 19 +++++++ .../guild/provider/GuildJsonProviderTest.java | 19 +++++++ 11 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 src/test/java/me/glaremasters/guilds/database/UnwritableFolders.java diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index 9cd05d0cb..cb6311de1 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -111,6 +111,45 @@ public static String getRecordId(File file) { return dotIndex == -1 ? name : name.substring(0, dotIndex); } + /** + * Warns when the plugin's data folder is not the parent of a data folder. + * + *

The data folders sit inside the plugin's own folder: {@code plugins/Guilds/data}, + * {@code plugins/Guilds/arenas}, {@code plugins/Guilds/challenges} and + * {@code plugins/Guilds/cooldowns}. The copies of the files the plugin cannot read go into + * {@code plugins/Guilds/backups/corrupt}, so they have to be taken from the plugin's folder + * rather than from the data folder. + * + *

Both are {@link File} arguments, so the two can be given the wrong way round without the + * compiler noticing. That would put the copies inside the folder the plugin enumerates, where + * they would be read as records. A loud message at startup is the right weight for it: the + * constructor is public, and another layout is a legitimate thing for a third-party caller to + * want. + * + * @param pluginDataFolder the plugin's data folder + * @param dataFolder the folder the records live in + * @param dataType the kind of record the folder holds + */ + public static void warnIfFoldersAreSwapped(File pluginDataFolder, File dataFolder, String dataType) { + if (pluginDataFolder == null || dataFolder == null) { + return; + } + + final File parent = dataFolder.getParentFile(); + + if (parent == null) { + return; + } + + if (parent.getAbsoluteFile().equals(pluginDataFolder.getAbsoluteFile())) { + return; + } + + LoggingUtils.severe("The " + dataType + " data folder (" + dataFolder.getAbsolutePath() + ") is not inside the plugin's data folder (" + + pluginDataFolder.getAbsolutePath() + "), so the copies of the files the plugin cannot read will not land where they should. " + + "The two folders may have been given the wrong way round."); + } + /** * Keeps a copy of a file the plugin could not read and says where it is. * diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java b/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java index a096fc9c3..fbc9ef3be 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/ArenaAdapter.java @@ -111,7 +111,14 @@ public void saveArenas(@NotNull Collection arenas) throws IOException { for (String arenaId : getAllArenaIds()) { boolean keep = savedIds.stream().anyMatch(id -> id.equals(arenaId)); if (!keep) { - deleteArena(arenaId); + try { + deleteArena(arenaId); + } catch (IOException ex) { + // The arena file could not be removed. The rest of the batch carries on: one + // file that will not delete must not cost the other arenas their cleanup. + LoggingUtils.warn("The arena " + arenaId + " could not be deleted: " + ex.getMessage() + + " The other arenas have still been cleaned up.", ex); + } } } diff --git a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java index c2090b21b..c88f3eb94 100644 --- a/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProvider.java @@ -49,6 +49,8 @@ public ArenaJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); + + JsonFileUtils.warnIfFoldersAreSwapped(pluginDataFolder, dataFolder, DATA_TYPE); } @Override @@ -166,13 +168,24 @@ public void deleteArena(@Nullable String tablePrefix, @NotNull String id) throws * and one unreadable file must not stop the rest of the batch from being cleaned up. The * load already said which files could not be read. * + *

A delete that did not happen is a different matter, and it is raised. The caller is told + * which file is still there, so it can carry on with the rest of the batch and say what it + * could not remove. + * * @param file the file to delete + * @throws IOException if the file could not be deleted */ - private void deleteArena(File file) { + private void deleteArena(File file) throws IOException { if (unreadableFiles.contains(file.getName())) { return; } - if (file.exists()) file.delete(); + if (!file.exists()) return; + + file.delete(); + + if (file.exists()) { + throw new IOException("The arena file " + file.getAbsolutePath() + " could not be deleted."); + } } } diff --git a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java index 7915240ec..f7ad8b7e5 100644 --- a/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProvider.java @@ -46,6 +46,8 @@ public ChallengeJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); + + JsonFileUtils.warnIfFoldersAreSwapped(pluginDataFolder, dataFolder, DATA_TYPE); } @Override @@ -146,6 +148,17 @@ public void deleteChallenge(@Nullable String tablePrefix, @NotNull String id) th private void deleteChallenge(File file) throws IOException { refuseToOverwriteUnreadableFile(file); - if (file.exists()) file.delete(); + if (!file.exists()) return; + + file.delete(); + + /* + * A delete that did not happen has to be reported. The challenge is gone from the plugin + * once the delete is asked for, so nothing would try again, and the next restart would + * load the challenge that was supposed to be gone. + */ + if (file.exists()) { + throw new IOException("The challenge file " + file.getAbsolutePath() + " could not be deleted."); + } } } diff --git a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java index a7c8cae16..2d958738e 100644 --- a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java @@ -46,6 +46,8 @@ public CooldownJsonProvider(File dataFolder, File pluginDataFolder) { this.pluginDataFolder = pluginDataFolder; this.cooldownFile = new File(dataFolder, "cooldowns.json"); this.gson = Guilds.getGson(); + + JsonFileUtils.warnIfFoldersAreSwapped(pluginDataFolder, dataFolder, DATA_TYPE); } @Override diff --git a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java index 32e1e15fe..4b7323450 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java @@ -54,6 +54,8 @@ public GuildJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); + + JsonFileUtils.warnIfFoldersAreSwapped(pluginDataFolder, dataFolder, DATA_TYPE); } @Override @@ -166,6 +168,17 @@ public void deleteGuild(@Nullable String tablePrefix, @NotNull String id) throws private void deleteGuild(File file) throws IOException { refuseToOverwriteUnreadableFile(file); - if (file.exists()) file.delete(); + if (!file.exists()) return; + + file.delete(); + + /* + * A delete that did not happen has to be reported. The caller has already taken the guild + * out of the plugin, so the autosave will not touch the file again, and the next restart + * would load the guild the player was told was deleted. + */ + if (file.exists()) { + throw new IOException("The guild file " + file.getAbsolutePath() + " could not be deleted."); + } } } diff --git a/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java b/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java index 0c0cf6728..cae0a3bd9 100644 --- a/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java +++ b/src/test/java/me/glaremasters/guilds/database/JsonStorageBackupTest.java @@ -141,4 +141,34 @@ void twoCorruptFilesWithTheSameNameInTwoStorages() throws IOException { assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(guildBackup)); assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(arenaBackup)); } + + @Test + @DisplayName("the four providers complain when the two folders are given the wrong way round") + void theProvidersComplainWhenTheFoldersAreSwapped() throws IOException { + try (LogCapture log = new LogCapture()) { + // The plugin's data folder handed over as the record folder, and the record folder as + // the plugin's folder. Both are File arguments, so the compiler does not notice. + new GuildJsonProvider(pluginFolder().toFile(), dataFolder().toFile()); + new ArenaJsonProvider(pluginFolder().toFile(), arenaFolder().toFile()); + new ChallengeJsonProvider(pluginFolder().toFile(), challengeFolder().toFile()); + new CooldownJsonProvider(pluginFolder().toFile(), cooldownFolder().toFile()); + + assertTrue(log.contains("wrong way round"), "the swap was not reported"); + assertTrue(log.contains(pluginFolder().toString()), "the message did not name the plugin's data folder"); + assertTrue(log.contains(dataFolder().toString()), "the message did not name the record folder"); + } + } + + @Test + @DisplayName("the four providers do not complain about the folders the plugin uses") + void theProvidersDoNotComplainAboutTheRightFolders() throws IOException { + try (LogCapture log = new LogCapture()) { + new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); + new ArenaJsonProvider(arenaFolder().toFile(), pluginFolder().toFile()); + new ChallengeJsonProvider(challengeFolder().toFile(), pluginFolder().toFile()); + new CooldownJsonProvider(cooldownFolder().toFile(), pluginFolder().toFile()); + + assertFalse(log.contains("wrong way round"), "the right folders were reported as the wrong way round"); + } + } } diff --git a/src/test/java/me/glaremasters/guilds/database/UnwritableFolders.java b/src/test/java/me/glaremasters/guilds/database/UnwritableFolders.java new file mode 100644 index 000000000..ef892122b --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/UnwritableFolders.java @@ -0,0 +1,39 @@ +package me.glaremasters.guilds.database; + +import java.io.IOException; +import java.nio.file.Path; + +/** + * Locks a folder so the files in it cannot be removed. + * + *

A file in a folder that cannot be written to cannot be unlinked, which is how a delete + * fails in the field: the folder was made read-only, or the file system ran out of space. + */ +public final class UnwritableFolders { + + private UnwritableFolders() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + } + + /** + * Takes the write permission off a folder. + * + * @param folder the folder to lock + * @return true when the folder really cannot be written to. A process that can write to it + * anyway, such as one running as root, can still remove the files in it. + * @throws IOException if the permissions cannot be changed + */ + public static boolean lock(Path folder) throws IOException { + folder.toFile().setWritable(false, false); + return !folder.toFile().canWrite(); + } + + /** + * Gives a folder its write permission back, so the folder can be cleaned up afterwards. + * + * @param folder the folder to unlock + */ + public static void unlock(Path folder) { + folder.toFile().setWritable(true, false); + } +} diff --git a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java index 80d06de4b..69f3a85f9 100644 --- a/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/arenas/provider/ArenaJsonProviderTest.java @@ -1,14 +1,20 @@ package me.glaremasters.guilds.database.arenas.provider; +import me.glaremasters.guilds.Guilds; import me.glaremasters.guilds.arena.Arena; import me.glaremasters.guilds.database.CorruptBackups; +import me.glaremasters.guilds.database.DatabaseAdapter; +import me.glaremasters.guilds.database.DatabaseBackend; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; +import me.glaremasters.guilds.database.UnwritableFolders; +import me.glaremasters.guilds.database.arenas.ArenaAdapter; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mockito; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -25,6 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; /** * Covers which files the JSON arena storage treats as arenas, and what happens to the ones it @@ -374,4 +381,46 @@ void theSameCorruptionIsBackedUpOnce() throws IOException { restarted.getAllArenas(null); assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "arenas").size()); } + + @Test + @DisplayName("an arena file that cannot be deleted is reported rather than swallowed") + void anArenaFileThatCannotBeDeletedIsReported() throws IOException { + write("stuck-arena.json", arenaJson(ARENA_ID, "Stuck")); + + assumeTrue(UnwritableFolders.lock(dataFolder()), "the test needs a folder that cannot be written to"); + + try { + final IOException failure = assertThrows(IOException.class, () -> provider.deleteArena(null, "stuck-arena")); + + assertTrue(failure.getMessage().contains("stuck-arena.json"), "the failure did not name the file"); + assertTrue(Files.exists(dataFolder().resolve("stuck-arena.json")), "the file was removed after all"); + } finally { + UnwritableFolders.unlock(dataFolder()); + } + } + + @Test + @DisplayName("one arena file that cannot be deleted does not stop the rest of the batch") + void oneUndeletableArenaFileDoesNotStopTheBatch() throws IOException { + write("stuck-arena.json", arenaJson(ARENA_ID, "Stuck")); + write("doomed-arena.json", arenaJson(ARENA_ID, "Doomed")); + + final Guilds guilds = Mockito.mock(Guilds.class); + Mockito.when(guilds.getDataFolder()).thenReturn(pluginFolder().toFile()); + final DatabaseAdapter adapter = Mockito.mock(DatabaseAdapter.class); + Mockito.when(adapter.getBackend()).thenReturn(DatabaseBackend.JSON); + final ArenaAdapter arenaAdapter = new ArenaAdapter(guilds, adapter); + + assumeTrue(UnwritableFolders.lock(dataFolder()), "the test needs a folder that cannot be written to"); + + try (LogCapture log = new LogCapture()) { + // Nothing to save, so every arena file is one the storage has no record for. + arenaAdapter.saveArenas(List.of()); + + assertTrue(log.contains("stuck-arena.json"), "the failure did not name the file"); + assertTrue(log.contains("doomed-arena.json"), "the batch stopped at the first file it could not delete"); + } finally { + UnwritableFolders.unlock(dataFolder()); + } + } } diff --git a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java index eb147c1c8..60199bbff 100644 --- a/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/challenges/provider/ChallengeJsonProviderTest.java @@ -3,6 +3,7 @@ import me.glaremasters.guilds.database.CorruptBackups; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; +import me.glaremasters.guilds.database.UnwritableFolders; import me.glaremasters.guilds.guild.GuildChallenge; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -26,6 +27,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; /** * Covers which files the JSON challenge storage treats as challenges, and what happens to the ones @@ -354,4 +356,21 @@ void theSameCorruptionIsBackedUpOnce() throws IOException { restarted.getAllChallenges(null); assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "challenges").size()); } + + @Test + @DisplayName("a challenge file that cannot be deleted is reported rather than swallowed") + void aChallengeFileThatCannotBeDeletedIsReported() throws IOException { + write("stuck-challenge.json", challengeJson(CHALLENGE_ID)); + + assumeTrue(UnwritableFolders.lock(dataFolder()), "the test needs a folder that cannot be written to"); + + try { + final IOException failure = assertThrows(IOException.class, () -> provider.deleteChallenge(null, "stuck-challenge")); + + assertTrue(failure.getMessage().contains("stuck-challenge.json"), "the failure did not name the file"); + assertTrue(Files.exists(dataFolder().resolve("stuck-challenge.json")), "the file was removed after all"); + } finally { + UnwritableFolders.unlock(dataFolder()); + } + } } diff --git a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java index be37acb20..0655dfa31 100644 --- a/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java @@ -3,6 +3,7 @@ import me.glaremasters.guilds.database.CorruptBackups; import me.glaremasters.guilds.database.LogCapture; import me.glaremasters.guilds.database.TestGson; +import me.glaremasters.guilds.database.UnwritableFolders; import me.glaremasters.guilds.guild.Guild; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -25,6 +26,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; /** * Covers which files the JSON guild storage treats as guilds, and what happens to the ones it @@ -415,4 +417,21 @@ void theSameCorruptionIsBackedUpOnce() throws IOException { restarted.getAllGuilds(null); assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); } + + @Test + @DisplayName("a guild file that cannot be deleted is reported rather than swallowed") + void aGuildFileThatCannotBeDeletedIsReported() throws IOException { + write(OTHER_GUILD_ID + ".json", guildJson(OTHER_GUILD_ID, "Another Guild")); + + assumeTrue(UnwritableFolders.lock(dataFolder()), "the test needs a folder that cannot be written to"); + + try { + final IOException failure = assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); + + assertTrue(failure.getMessage().contains(OTHER_GUILD_ID + ".json"), "the failure did not name the file"); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json")), "the file was removed after all"); + } finally { + UnwritableFolders.unlock(dataFolder()); + } + } } From 79ff94079e9cafb3568c246b62aeb35678048933 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:18:48 +0000 Subject: [PATCH 6/7] fix: word the folder warning so both branches can share it --- .../java/me/glaremasters/guilds/database/JsonFileUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index cb6311de1..bfc39f07c 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -145,9 +145,9 @@ public static void warnIfFoldersAreSwapped(File pluginDataFolder, File dataFolde return; } - LoggingUtils.severe("The " + dataType + " data folder (" + dataFolder.getAbsolutePath() + ") is not inside the plugin's data folder (" - + pluginDataFolder.getAbsolutePath() + "), so the copies of the files the plugin cannot read will not land where they should. " - + "The two folders may have been given the wrong way round."); + LoggingUtils.severe("The " + dataType + " data folder (" + dataFolder.getAbsolutePath() + ") is not a folder inside the " + + "plugin's data folder (" + pluginDataFolder.getAbsolutePath() + "), so the copies of the files the plugin cannot read " + + "will not land where they should. The two folders may have been given the wrong way round."); } /** From 99e0e86f1490368819ebc02b0d7683592eb28377 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 21:23:38 +0000 Subject: [PATCH 7/7] fix(storage): refuse cooldown writes without verified backup --- .../guilds/database/CorruptDataBackup.java | 12 +++++++ .../guilds/database/JsonFileUtils.java | 36 +++++++++++++++++++ .../provider/CooldownJsonProvider.java | 7 ++-- .../provider/CooldownJsonProviderTest.java | 31 ++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java b/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java index 9ac65dcc0..be64f11e9 100644 --- a/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java +++ b/src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java @@ -163,6 +163,18 @@ public static Result backup(File pluginDataFolder, File sourceFile, String dataT } } + /** + * Whether a backup file is a byte-for-byte copy of its source. + * + * @param sourceFile the original file + * @param backupFile the proposed backup + * @return true when both files exist and have the same contents + */ + public static boolean isVerifiedCopy(File sourceFile, File backupFile) { + return sourceFile != null && sourceFile.isFile() && backupFile != null && backupFile.isFile() + && sameContent(sourceFile, backupFile); + } + /** * The directory a backup of a file goes in. * diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java index bfc39f07c..e49c71eab 100644 --- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java +++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java @@ -202,6 +202,42 @@ public static void backupCorruptFile(File pluginDataFolder, File file, String da + "The file itself has been left where it is."); } + /** + * Keeps a copy of a corrupt file and reports whether the copy exists and matches its source. + * + * @param pluginDataFolder the plugin's data folder + * @param file the corrupt file + * @param dataType the kind of data the file holds + * @param reason why the file could not be read + * @return true when a byte-for-byte backup is available + */ + public static boolean backupCorruptFileAndVerify(File pluginDataFolder, File file, String dataType, String reason) { + final CorruptDataBackup.Result result = CorruptDataBackup.backup(pluginDataFolder, file, dataType); + + if (result.isFailed()) { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "), " + + "and its backup could not be written: " + result.getFailure() + ". The file has been left where it is."); + return false; + } + + if (!CorruptDataBackup.isVerifiedCopy(file, result.getBackupFile())) { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "), " + + "and its backup could not be verified. The file has been left where it is."); + return false; + } + + if (result.isSkipped()) { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "). " + + "A verified copy of it is already at " + result.getBackupFile().getAbsolutePath() + "."); + } else { + LoggingUtils.severe("The " + dataType + " file " + file.getAbsolutePath() + " could not be read (" + reason + "). " + + "A verified copy of it was written to " + result.getBackupFile().getAbsolutePath() + ". " + + "The file itself has been left where it is."); + } + + return true; + } + /** * Describes what went wrong, in a form that fits in a log line. * diff --git a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java index 2d958738e..b567f9120 100644 --- a/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java +++ b/src/main/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProvider.java @@ -171,8 +171,11 @@ private CooldownFile readCooldownFile() throws IOException { * be read are still loaded. */ if (!contents.unreadable.isEmpty()) { - JsonFileUtils.backupCorruptFile(pluginDataFolder, this.cooldownFile, DATA_TYPE, - contents.unreadable.size() + " of the " + entries.size() + " entries could not be read"); + final String reason = contents.unreadable.size() + " of the " + entries.size() + " entries could not be read"; + if (!JsonFileUtils.backupCorruptFileAndVerify(pluginDataFolder, this.cooldownFile, DATA_TYPE, reason)) { + throw new IOException("The cooldowns JSON file could not be backed up and verified: " + + this.cooldownFile.getAbsolutePath()); + } } return contents; diff --git a/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java index 1948d1f73..cca1cf1cb 100644 --- a/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java +++ b/src/test/java/me/glaremasters/guilds/database/cooldowns/provider/CooldownJsonProviderTest.java @@ -271,6 +271,37 @@ void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); } + @Test + @DisplayName("creating a cooldown is refused when a partial-corruption backup cannot be written") + void createIsRefusedWhenPartialCorruptionCannotBeBackedUp() throws IOException { + final String corrupt = "[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n" + + "]\n"; + writeCooldowns(corrupt); + Files.createFile(pluginFolder().resolve("backups")); + + assertThrows(IOException.class, () -> provider.createCooldown(null, UUID.randomUUID().toString(), + Cooldown.Type.Home.name(), OWNER_ID.toString(), new Timestamp(9999999999999L))); + + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + + @Test + @DisplayName("deleting a cooldown is refused when a partial-corruption backup cannot be written") + void deleteIsRefusedWhenPartialCorruptionCannotBeBackedUp() throws IOException { + final String corrupt = "[\n" + + " \"this is not a cooldown\",\n" + + cooldownEntry(COOLDOWN_ID, Cooldown.Type.Request, OWNER_ID, 9999999999999L) + "\n" + + "]\n"; + writeCooldowns(corrupt); + Files.createFile(pluginFolder().resolve("backups")); + + assertThrows(IOException.class, () -> provider.deleteCooldown(null, Cooldown.Type.Request.name(), OWNER_ID.toString())); + + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(cooldownFile())); + } + @Test @DisplayName("the same cooldown corruption is backed up once, not once per read") void theSameCorruptionIsBackedUpOnce() throws IOException {