diff --git a/build.gradle.kts b/build.gradle.kts index 9539df2a0..2271454e0 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -612,6 +612,23 @@ plugwright { file("plugins/Guilds/config.yml", file("src/test/e2e/server/plugins/Guilds/config.yml")) file("plugins/Guilds/tiers.yml", file("src/test/e2e/server/plugins/Guilds/tiers.yml")) file("plugins/Guilds/buffs.yml", file("src/test/e2e/server/plugins/Guilds/buffs.yml")) + + /* + * Two guild files the plugin finds in its data folder when it starts. + * + * One of them is a guild that reads, and one of them is a file that does not parse. + * The second one is the case `startup-loading.spec.ts` is about: a single file the + * plugin cannot read must not stop it, must not cost it the guilds that did read, + * and must be left on disk untouched. + */ + file( + "plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json", + file("src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json"), + ) + file( + "plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json", + file("src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json"), + ) } plugins { diff --git a/docs/e2e-coverage.md b/docs/e2e-coverage.md index 10d87eacd..d72c5ed2a 100644 --- a/docs/e2e-coverage.md +++ b/docs/e2e-coverage.md @@ -3,7 +3,7 @@ Which Guilds feature is tested where, and what is deliberately left out. See `e2e-testing.md` for how to run any of it. -141 tests across 12 spec files, plus 3 preflight tests that run before every suite and fail the run +162 tests across 14 spec files, plus 3 preflight tests that run before every suite and fail the run if Guilds or any of its hard dependencies did not load. ## Preflight — `plugins/guilds-preflight.spec.ts` @@ -17,7 +17,7 @@ meaningful. | The economy and permission providers Vault needs are registered | Vault's `Economy` and `Permission` services resolve | | Guilds commands are reachable and its language is English | `/guild` answers, and does so in English | -## A. Guild lifecycle — `tests/lifecycle.spec.ts` (19) +## A. Guild lifecycle — `tests/lifecycle.spec.ts` (21) | Requirement | Test | | --- | --- | @@ -32,7 +32,7 @@ meaningful. | Status and information commands | toggles the guild between public and private / changes the guild prefix / sets, shows and clears the guild MOTD / reports guild membership through the guild list and rejects guild-scoped commands elsewhere | | Name blacklist | rejects a guild name that is a substring of a blacklisted word / accepts a guild name that contains a blacklisted word | -## B. Membership management — `tests/membership.spec.ts` (19) +## B. Membership management — `tests/membership.spec.ts` (21) | Requirement | Test | | --- | --- | @@ -51,7 +51,7 @@ meaningful. | Guild capacity | a full guild refuses another member | | Permissions after leaving or being removed | a player who leaves loses the permissions their guild granted them / a player who is kicked loses the permissions their guild granted them | -## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (7) +## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (8) | Requirement | Test | | --- | --- | @@ -128,7 +128,7 @@ meaningful. | Alliance chat between guilds | `chat.spec.ts`, ally chat reaches an allied guild and not the third party | | Cleanup on deletion | a deleted guild drops out of its allies | -## H. Invite codes — `tests/codes.spec.ts` (12, of which 5 are one `describe.serial` chain) +## H. Invite codes — `tests/codes.spec.ts` (18, of which 5 are one `describe.serial` chain) | Requirement | Test | | --- | --- | @@ -188,6 +188,22 @@ the server answering in another language for the rest of the run. ACF's `usePerI false)` is what makes that safe, and one player switching language leaves everyone else in English asserts it rather than assuming it. +## L. Startup and the guild data — `tests/startup-loading.spec.ts` (4) + +The environment stages two guild files into the plugin's data folder: one that reads and one that +does not parse. + +| Requirement | Test | +| --- | --- | +| One unreadable file does not stop the plugin | the plugin starts with a guild file it cannot read, and says which file | +| The guilds that could be read are available | the guilds that could be read are available | +| The unreadable file is left alone, and copied aside | the guild file that could not be read is left on disk, with a copy in the backup folder | +| The readable file is untouched | the guild file that could be read is still readable | + +## M. Smoke — `tests/smoke.spec.ts` (7) + +The rows for this section belong to the branch that adds the smoke suite. + ## Fixture self-check — `tests/bank.spec.ts` `STARTING_BALANCE is what the fixtures actually fund` is not a Guilds test. It checks that the diff --git a/src/main/java/me/glaremasters/guilds/Guilds.java b/src/main/java/me/glaremasters/guilds/Guilds.java index 50fdc2a22..00869063d 100644 --- a/src/main/java/me/glaremasters/guilds/Guilds.java +++ b/src/main/java/me/glaremasters/guilds/Guilds.java @@ -255,14 +255,32 @@ public void onEnable() { // Load the challenge handler challengeHandler = new ChallengeHandler(this); challengeHandler.loadChallenges(); - // Load guildhandler with provider - guildHandler = new GuildHandler(this, settingsHandler.getMainConf()); } catch (IOException e) { LoggingUtils.severe("An error occurred loading data! Stopping plugin..", e); Bukkit.getPluginManager().disablePlugin(this); return; } + /* + * Load the guild data. + * + * The guilds are the data the plugin cannot do without, and a load that failed leaves the + * plugin without them. This used to happen inside the guild handler, which caught the + * failure, logged it and carried on with an empty guild map: the server came up as if + * nothing was wrong, and the first save then wrote that empty map back over the guild data + * the plugin had failed to read. The failure stops the plugin instead, and the message + * names what could not be read. + */ + try { + guildHandler = new GuildHandler(this, settingsHandler.getMainConf()); + } catch (IOException | RuntimeException e) { + LoggingUtils.severe("Guilds could not load the guild data, so it will not start. " + + "The guild files in the data folder have not been changed. " + + "Fix the cause reported below and restart the server.", e); + Bukkit.getPluginManager().disablePlugin(this); + return; + } + // If they have placeholderapi, enable it. if (Bukkit.getPluginManager().isPluginEnabled("PlaceholderAPI")) { new PlaceholderAPI(guildHandler).register(); 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 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();
+ }
+}
diff --git a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java
index 63f161fa5..bfc39f07c 100644
--- a/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java
+++ b/src/main/java/me/glaremasters/guilds/database/JsonFileUtils.java
@@ -1,6 +1,8 @@
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;
import java.io.IOException;
@@ -11,16 +13,209 @@
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
+ * @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 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);
+ }
+
+ /**
+ * 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 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.");
+ }
+
+ /**
+ * 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 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 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) {
- deleteGuild(new File(dataFolder, id + ".json"));
+ 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) {
- if (file.exists()) file.delete();
+ private void deleteGuild(File file) throws IOException {
+ refuseToOverwriteUnreadableFile(file);
+
+ 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/main/java/me/glaremasters/guilds/guild/GuildHandler.java b/src/main/java/me/glaremasters/guilds/guild/GuildHandler.java
index eaa16e32e..945597e28 100644
--- a/src/main/java/me/glaremasters/guilds/guild/GuildHandler.java
+++ b/src/main/java/me/glaremasters/guilds/guild/GuildHandler.java
@@ -66,67 +66,131 @@ public class GuildHandler {
//as well as guild permissions from tiers using permission field and tiers list.
- public GuildHandler(Guilds guildsPlugin, SettingsManager settingsManager) {
+ /**
+ * Creates the handler and loads the guild data.
+ *
+ * A failure to read the guild data is reported to the caller rather than swallowed. This
+ * used to catch the {@link IOException}, log it and carry on with an empty guild map, which
+ * left the failure invisible to the plugin lifecycle: the server came up as if nothing was
+ * wrong, and the first save then wrote that empty map back over the very data the plugin had
+ * failed to read.
+ *
+ * @param guildsPlugin the plugin
+ * @param settingsManager the plugin's settings
+ * @throws IOException if the guild data cannot be read. The plugin must not start in that
+ * case, because it cannot know what its guilds are.
+ */
+ public GuildHandler(Guilds guildsPlugin, SettingsManager settingsManager) throws IOException {
this.guildsPlugin = guildsPlugin;
this.settingsManager = settingsManager;
loadRoles();
loadTiers();
- try {
- loadGuilds();
- } catch (IOException e) {
- LoggingUtils.severe("An error occurred while loading guild data.", e);
- }
+ loadGuilds();
}
/**
* Loads all guilds from the database and sets the necessary data for each guild and member.
*
+ * Reading the guild data as a whole is all or nothing. An {@link IOException} here means the
+ * plugin cannot know what its guilds are, so it is passed on and the plugin stops rather than
+ * running on an empty map.
+ *
+ * One guild that cannot be set up is a different matter, and is handled the way the tier
+ * fallback below handles a guild with an unknown tier: that guild is left out of the running
+ * plugin, with a message naming it, and every other guild carries on. Its file is not written
+ * to, because the save only writes the guilds that are in memory.
+ *
* @throws IOException if there is an error while loading guilds from the database.
*/
private void loadGuilds() throws IOException {
final List The guild is dropped from the map, so the save does not write it and its file stays as it
+ * is on disk. The other guilds are unaffected.
+ *
+ * @param guild the guild that could not be set up
+ * @param error what went wrong
+ */
+ private void leaveOutGuild(Guild guild, RuntimeException error) {
+ guilds.remove(guild.getId());
+
+ final List The backups live under {@code plugins/Guilds/backups/corrupt/ 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 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/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/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java
new file mode 100644
index 000000000..c5926832a
--- /dev/null
+++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java
@@ -0,0 +1,556 @@
+package me.glaremasters.guilds.database.guild.provider;
+
+import me.glaremasters.guilds.database.CorruptBackups;
+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.guild.GuildAdapter;
+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.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;
+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;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * 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 {
+
+ 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 {
+ Files.createDirectories(dataFolder());
+ provider = new GuildJsonProvider(dataFolder().toFile(), pluginFolder().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(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(dataFolder().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";
+ }
+
+ /**
+ * @return an adapter over the same folder the provider reads, so a save goes through the
+ * production path rather than through the provider directly
+ */
+ private GuildAdapter adapter() {
+ final me.glaremasters.guilds.Guilds plugin = mock(me.glaremasters.guilds.Guilds.class);
+ when(plugin.getDataFolder()).thenReturn(pluginFolder().toFile());
+
+ final me.glaremasters.guilds.database.DatabaseAdapter database =
+ mock(me.glaremasters.guilds.database.DatabaseAdapter.class);
+ when(database.getBackend()).thenReturn(DatabaseBackend.JSON);
+
+ return new GuildAdapter(plugin, database);
+ }
+
+ @Test
+ @DisplayName("only the guild data files are enumerated")
+ 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() throws IOException {
+ final List {@link GuildHandler} loads every guild during construction, and the load used to swallow an
+ * {@link IOException}: the handler logged it and carried on with an empty or half-filled guild
+ * map. The plugin lifecycle never heard about it, so the server came up as if nothing was wrong,
+ * and the first save then wrote that empty map back over the data the plugin had failed to read.
+ *
+ * These tests pin the other half of that contract down too: a load that works must still work,
+ * and a load that fails must be visible to whoever starts the plugin.
+ */
+class GuildHandlerLoadTest {
+
+ /** The id of the guild the fake database hands back. */
+ private static final String GUILD_ID = "11111111-1111-1111-1111-111111111111";
+
+ /** The id of the guild whose stored data cannot be used. */
+ private static final String BROKEN_ID = "22222222-2222-2222-2222-222222222222";
+
+ @TempDir
+ Path tempDir;
+
+ private Guilds plugin;
+ private GuildAdapter guildAdapter;
+
+ @BeforeEach
+ void setUp() throws IOException {
+ writeRoles();
+ writeTiers();
+
+ plugin = mock(Guilds.class);
+ when(plugin.getDataFolder()).thenReturn(tempDir.toFile());
+
+ final DatabaseAdapter database = mock(DatabaseAdapter.class);
+ guildAdapter = mock(GuildAdapter.class);
+ when(database.getGuildAdapter()).thenReturn(guildAdapter);
+ when(plugin.getDatabase()).thenReturn(database);
+ }
+
+ /**
+ * Writes the roles table the handler reads at startup.
+ *
+ * The plugin generates this file on the server; the handler reads it with the plain YAML
+ * reader rather than through the settings manager, so the test has to put one there.
+ *
+ * @throws IOException if the file cannot be written
+ */
+ private void writeRoles() throws IOException {
+ Files.write(tempDir.resolve("roles.yml"), String.join("\n",
+ "roles:",
+ " 0:",
+ " name: GuildMaster",
+ " permission-node: guilds.roles.master",
+ " 1:",
+ " name: Officer",
+ " permission-node: guilds.roles.officer",
+ ""
+ ).getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * Writes the tiers table the handler reads at startup.
+ *
+ * @throws IOException if the file cannot be written
+ */
+ private void writeTiers() throws IOException {
+ Files.write(tempDir.resolve("tiers.yml"), String.join("\n",
+ "tiers:",
+ " list:",
+ " 1:",
+ " level: 1",
+ " name: Bronze",
+ " cost: 0.0",
+ " max-members: 15",
+ " vault-amount: 1",
+ " mob-xp-multiplier: 1.0",
+ " damage-multiplier: 1.0",
+ " max-bank-balance: 10000.0",
+ " members-to-rankup: 0",
+ " max-allies: 5",
+ " use-buffs: true",
+ " permissions: []",
+ ""
+ ).getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * @return a guild as the fake database hands it over
+ */
+ private Guild guild() {
+ final Guild guild = new Guild(UUID.fromString(GUILD_ID));
+ guild.setName("GoodGuild");
+ guild.setTier(GuildTier.builder().level(1).name("Bronze").build());
+ guild.setMembers(new ArrayList<>());
+ return guild;
+ }
+
+ /**
+ * @return a guild whose stored data cannot be used, the way a hand-edited file cannot be
+ */
+ private Guild brokenGuild() {
+ final Guild guild = new Guild(UUID.fromString(BROKEN_ID));
+ guild.setName("BrokenGuild");
+ guild.setTier(GuildTier.builder().level(1).name("Bronze").build());
+
+ // Gson leaves a field null when the stored file does not have it, and the handler reads a
+ // member's role without a null check.
+ final GuildMember member = new GsonBuilder().create().fromJson(
+ "{\"uuid\": \"33333333-3333-3333-3333-333333333333\"}", GuildMember.class);
+ guild.setMembers(Collections.singletonList(member));
+
+ return guild;
+ }
+
+ @Test
+ @DisplayName("a guild data read failure is reported to whoever started the plugin")
+ void loadFailureIsNotSwallowed() throws IOException {
+ when(guildAdapter.getAllGuilds()).thenThrow(new IOException("the database is unreachable"));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+
+ assertThrows(IOException.class, () -> new GuildHandler(plugin, settings),
+ "a failed guild load has to reach the plugin lifecycle, or the plugin starts up "
+ + "with an empty guild map and overwrites the data it could not read");
+ }
+
+ @Test
+ @DisplayName("a guild data read failure is not swallowed even when nothing else failed")
+ void loadFailureIsNotSwallowedSilently() throws IOException {
+ when(guildAdapter.getAllGuilds()).thenThrow(new IOException("the database is unreachable"));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+
+ // The handler used to log the failure and return normally. Nothing was thrown, so the
+ // plugin started up and the scheduled save wrote the empty map back over the guild data.
+ final IOException thrown = assertThrows(IOException.class, () -> new GuildHandler(plugin, settings));
+ assertEquals("the database is unreachable", thrown.getMessage(),
+ "the original failure has to reach the plugin lifecycle unchanged");
+ }
+
+ @Test
+ @DisplayName("a load where every guild file is unreadable stops the plugin")
+ void aLoadWhereNothingCanBeReadStopsThePlugin() throws IOException {
+ // What the JSON provider does when it finds files and cannot read one of them.
+ when(guildAdapter.getAllGuilds()).thenThrow(new IOException(
+ "None of the 3 files in /plugins/Guilds/data could be read as a guild, so the guild data could not be loaded."));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+
+ assertThrows(IOException.class, () -> new GuildHandler(plugin, settings),
+ "a server that comes up with no guilds at all hides that the guild data could not be read");
+ }
+
+ @Test
+ @DisplayName("a guild data load that works still loads the guilds")
+ void successfulLoadStillWorks() throws IOException {
+ when(guildAdapter.getAllGuilds()).thenReturn(Collections.singletonList(guild()));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+ final GuildHandler handler = new GuildHandler(plugin, settings);
+
+ assertEquals(1, handler.getGuildsSize(), "a working load has to keep working");
+ }
+
+ @Test
+ @DisplayName("one guild that cannot be set up does not cost the server the other guilds")
+ void oneBrokenGuildDoesNotStopTheOthers() throws IOException {
+ when(guildAdapter.getAllGuilds()).thenReturn(Arrays.asList(guild(), brokenGuild()));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+ final GuildHandler handler = new GuildHandler(plugin, settings);
+
+ assertEquals(1, handler.getGuildsSize(), "the guild that reads has to stay available");
+ assertNotNull(handler.getGuild(UUID.fromString(GUILD_ID)));
+ assertNull(handler.getGuild(UUID.fromString(BROKEN_ID)),
+ "the guild that could not be read must not be in the running plugin");
+ }
+
+ @Test
+ @DisplayName("when no guild at all can be set up, the plugin refuses to start")
+ void noUsableGuildIsFatal() throws IOException {
+ when(guildAdapter.getAllGuilds()).thenReturn(Collections.singletonList(brokenGuild()));
+
+ final SettingsManager settings = mock(SettingsManager.class);
+
+ assertThrows(IOException.class, () -> new GuildHandler(plugin, settings),
+ "a plugin that came up with no guilds at all would write that back over the data");
+ }
+}