From 1a40f66732e562ce751cf777d63c93246a679fa4 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 05:53:55 +0000 Subject: [PATCH 1/8] fix: refuse to start when the guild data cannot be read GuildHandler caught the IOException from its own guild load, logged it and returned with an empty guild map, so the catch in Guilds.onEnable() never fired. The server came up as if the data had loaded and the scheduled save then wrote that empty map back over the guilds the plugin had failed to read. The constructor now lets the failure out, and onEnable loads the guild data in its own try/catch that stops the plugin with a message naming what could not be read. The handler stays null in that case, so nothing is saved. A single guild that cannot be set up is handled per record, the way the tier fallback already handles a guild with an unknown tier: that guild is left out of the running plugin and the other guilds carry on. Its file is not written, because the save only writes the guilds that are in memory. GuildJsonProvider remembers the files it could not read and refuses to write to them or delete them, since such a file is the only copy of its guild. A data folder that cannot be listed now raises an IOException naming the folder instead of a NullPointerException that named nothing. --- build.gradle.kts | 17 ++ docs/e2e-coverage.md | 14 +- .../java/me/glaremasters/guilds/Guilds.java | 22 +- .../guild/provider/GuildJsonProvider.java | 85 +++++- .../guilds/guild/GuildHandler.java | 144 ++++++--- .../11111111-1111-1111-1111-111111111111.json | 36 +++ .../22222222-2222-2222-2222-222222222222.json | 1 + src/test/e2e/tests/startup-loading.spec.ts | 91 ++++++ .../guild/provider/GuildJsonProviderTest.java | 289 ++++++++++++++++++ .../guilds/guild/GuildHandlerLoadTest.java | 204 +++++++++++++ 10 files changed, 849 insertions(+), 54 deletions(-) create mode 100644 src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json create mode 100644 src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json create mode 100644 src/test/e2e/tests/startup-loading.spec.ts create mode 100644 src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java create mode 100644 src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java 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..cc39f026d 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 +145 tests across 13 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` @@ -188,6 +188,18 @@ 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 | the guild file that could not be read is left on disk | +| The readable file is untouched | the guild file that could be read is still readable | + ## 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/guild/provider/GuildJsonProvider.java b/src/main/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProvider.java index 13db0d238..e55b18869 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 @@ -14,8 +14,9 @@ import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; -import java.util.Objects; +import java.util.Set; /** * Created by GlareMasters @@ -26,6 +27,15 @@ 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 data the plugin could not parse, which makes it the only copy of that + * guild. 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(); @@ -37,44 +47,75 @@ 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())) + public boolean guildExists(@Nullable String tablePrefix, @NotNull String id) throws IOException { + return Arrays.stream(listGuildFiles()) .map(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName())) .anyMatch(n -> n.equals(id)); } @Override - public List getAllGuildIds(@Nullable String tablePrefix) { + public List getAllGuildIds(@Nullable String tablePrefix) throws IOException { List loadedGuildIds = new ArrayList<>(); - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { + for (File file : listGuildFiles()) { loadedGuildIds.add(com.google.common.io.Files.getNameWithoutExtension(file.getName())); } return loadedGuildIds; } + /** + * Reads every guild file in the data folder. + * + *

A folder that cannot be listed is a problem with the storage, not with one guild, and it + * used to come out of here as a {@link NullPointerException} that named nothing. It is an + * {@link IOException} naming the folder instead, so the plugin can say what went wrong. + * + * @return the files in the guild data folder + * @throws IOException if the folder cannot be read + */ + private File[] listGuildFiles() throws IOException { + final File[] files = dataFolder.listFiles(); + + if (files == null) { + throw new IOException("The guild data folder could not be read: " + dataFolder.getAbsolutePath()); + } + + return files; + } + @Override - public List getAllGuilds(@Nullable String tablePrefix) { + public List getAllGuilds(@Nullable String tablePrefix) throws IOException { List loadedGuilds = new ArrayList<>(); + int unreadable = 0; - for (File file : Objects.requireNonNull(dataFolder.listFiles())) { + for (File file : listGuildFiles()) { try { Guild guild = JsonFileUtils.readJson(file, gson, Guild.class, "Guild"); guild.getId(); loadedGuilds.add(guild); } catch (Exception ex) { + unreadable++; + // 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("To prevent data loss in the plugin, this Guild has been prevented from loading. " + + "The file has been left untouched and will not be written to. " + + "Fix the file, or move it out of the data folder, and restart the server."); } } + if (unreadable > 0) { + LoggingUtils.severe(unreadable + " of " + (loadedGuilds.size() + unreadable) + " guild files could not be read. " + + "Those guilds are not available until the files are fixed."); + } + return loadedGuilds; } @Override public Guild getGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { - File data = Arrays.stream(Objects.requireNonNull(dataFolder.listFiles())) + File data = Arrays.stream(listGuildFiles()) .filter(f -> com.google.common.io.Files.getNameWithoutExtension(f.getName()).equals(id)) .findFirst() .orElse(null); @@ -96,15 +137,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 + ".json")); } - private void deleteGuild(File file) { + private void deleteGuild(File file) throws IOException { + refuseToOverwriteUnreadableFile(file); + if (file.exists()) file.delete(); } } 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 loaded = guildsPlugin.getDatabase().getGuildAdapter().getAllGuilds(); loaded.forEach(guild -> guilds.put(guild.getId(), guild)); + int usable = 0; + // Loop through each guild and set the data needed - for (Guild guild : guilds.values()) { - // Create the vault cache - createVaultCache(guild); - // A guild whose tier could not be resolved when it was last saved has a null tier. - // Gson omits null fields, so the tier key is missing from the stored JSON entirely. - // Reading through it here would throw and take the whole plugin down at startup, so - // put the guild back on the lowest tier and say so. - if (guild.getTier() == null) { - guild.setTier(getLowestGuildTier()); - LoggingUtils.severe("The guild (" + guild.getName() + ") had no tier saved. To prevent issues, they've been automatically set to the lowest tier level on the server."); + for (Guild guild : new ArrayList<>(guilds.values())) { + try { + setUpGuild(guild); + usable++; + } catch (RuntimeException e) { + leaveOutGuild(guild, e); } - // Create a temp tier object for the guild - GuildTier tier = getGuildTier(guild.getTier().getLevel()); - if (tier != null) { - // Set the tier object - guild.setTier(tier); + } + + // Every guild failing to set up is not one broken guild, it is a broken setup: the plugin + // would come up with no guilds at all and quietly write that back. Stop instead. + if (usable == 0 && !loaded.isEmpty()) { + throw new IOException("None of the " + loaded.size() + " guilds in the database could be read. " + + "The guild files have not been changed."); + } + } + + /** + * Sets up the data a loaded guild needs before it can be used: its vault cache, its tier and + * the role of each of its members. + * + * @param guild the guild to set up + */ + private void setUpGuild(Guild guild) { + // Create the vault cache + createVaultCache(guild); + // A guild whose tier could not be resolved when it was last saved has a null tier. + // Gson omits null fields, so the tier key is missing from the stored JSON entirely. + // Reading through it here would throw and take the whole plugin down at startup, so + // put the guild back on the lowest tier and say so. + if (guild.getTier() == null) { + guild.setTier(getLowestGuildTier()); + LoggingUtils.severe("The guild (" + guild.getName() + ") had no tier saved. To prevent issues, they've been automatically set to the lowest tier level on the server."); + } + // Create a temp tier object for the guild + GuildTier tier = getGuildTier(guild.getTier().getLevel()); + if (tier != null) { + // Set the tier object + guild.setTier(tier); + } else { + guild.setTier(getLowestGuildTier()); + LoggingUtils.severe("The guild (" + guild.getName() + ") had a tier level that doesn't exist on the server anymore. To prevent issues, they've been automatically set to the lowest tier level on the server."); + } + // Check creation date + if (guild.getCreationDate() == 0) { + guild.setCreationDate(System.currentTimeMillis()); + } + // Loop through each member. + for (GuildMember member : guild.getMembers()) { + // Create a temp role + GuildRole role = getGuildRole(member.getRole().getLevel()); + if (role != null) { + // Set each member to their role + member.setRole(role); } else { - guild.setTier(getLowestGuildTier()); - LoggingUtils.severe("The guild (" + guild.getName() + ") had a tier level that doesn't exist on the server anymore. To prevent issues, they've been automatically set the the lowest tier level on the server."); - } - // Check creation date - if (guild.getCreationDate() == 0) { - guild.setCreationDate(System.currentTimeMillis()); - } - // Loop through each member. - for (GuildMember member : guild.getMembers()) { - // Create a temp role - GuildRole role = getGuildRole(member.getRole().getLevel()); - if (role != null) { - // Set each member to their role - member.setRole(role); - } else { - member.setRole(getLowestGuildRole()); - LoggingUtils.severe("The player (" + member.getName() + ") had a role level that doesn't exist on the server anymore. To prevent issues, they've been automatically set the the lowest role level on the server."); - } - addToMemberCache(member.getUuid(), guild.getId()); + member.setRole(getLowestGuildRole()); + LoggingUtils.severe("The player (" + member.getName() + ") had a role level that doesn't exist on the server anymore. To prevent issues, they've been automatically set to the lowest role level on the server."); } + addToMemberCache(member.getUuid(), guild.getId()); + } + } + + /** + * Leaves a guild that could not be set up out of the running plugin. + * + *

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 members = guild.getMembers(); + if (members != null) { + members.forEach(member -> removeFromMemberCache(member.getUuid())); } + + LoggingUtils.severe("The guild (" + guild.getName() + ") could not be read and has been left out of the plugin. " + + "Its data has not been changed. Fix the guild's file and restart the server.", error); } diff --git a/src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json b/src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json new file mode 100644 index 000000000..f3380557e --- /dev/null +++ b/src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json @@ -0,0 +1,36 @@ +{ + "id": "11111111-1111-1111-1111-111111111111", + "name": "E2EStaged", + "prefix": "Staged", + "motd": "A guild that was on the server before it started.", + "guildMaster": { + "uuid": "99999999-9999-9999-9999-999999999999", + "role": { + "level": 0 + }, + "joinDate": 1700000000000, + "lastLogin": 1700000000000 + }, + "status": "Public", + "tier": { + "level": 1 + }, + "balance": 0.0, + "members": [ + { + "uuid": "99999999-9999-9999-9999-999999999999", + "role": { + "level": 0 + }, + "joinDate": 1700000000000, + "lastLogin": 1700000000000 + } + ], + "invitedMembers": [], + "allies": [], + "pendingAllies": [], + "codes": [], + "vaults": [], + "lastDefended": 0, + "creationDate": 1700000000000 +} diff --git a/src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json b/src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json new file mode 100644 index 000000000..643d56b6c --- /dev/null +++ b/src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json @@ -0,0 +1 @@ +{ "id": "22222222-2222-2222-2222-222222222222", "name": "E2EBroken", "members": [ diff --git a/src/test/e2e/tests/startup-loading.spec.ts b/src/test/e2e/tests/startup-loading.spec.ts new file mode 100644 index 000000000..3dd5a1748 --- /dev/null +++ b/src/test/e2e/tests/startup-loading.spec.ts @@ -0,0 +1,91 @@ +import { expect, test } from '@plugwright/runner'; +import { existsSync, readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { guildList } from '../plugins/support/state.js'; + +/** + * What the plugin does when one of its guild files cannot be read. + * + * The server starts with two guild files in the plugin's data folder: one that reads and one that + * does not parse. The second one is the case this file is about. + * + * A guild file that does not parse is the only copy of that guild, so the plugin may not write + * over it. One unreadable file is a problem with one guild rather than with the whole database, + * so the plugin may not refuse to start either, and it may not cost the server the guilds that + * did read. + */ + +/** The id of the staged guild file that reads. */ +const READABLE_GUILD_ID = '11111111-1111-1111-1111-111111111111'; + +/** The id of the staged guild file that does not parse. */ +const UNREADABLE_GUILD_ID = '22222222-2222-2222-2222-222222222222'; + +/** The name of the guild in the file that reads. */ +const READABLE_GUILD_NAME = 'E2EStaged'; + +/** The name the unreadable file claims for the guild it holds. */ +const UNREADABLE_GUILD_NAME = 'E2EBroken'; + +/** + * The plugin's guild data folder inside the throwaway server directory. + * + * The folder is looked up rather than assumed: the runner's working directory depends on how it + * was started, and a spec that hard-codes a path fails for a reason that has nothing to do with + * what it is testing. + * + * @returns the path of the guild data folder + */ +function dataFolder(): string { + const data = join('plugins', 'Guilds', 'data'); + let dir = process.cwd(); + + for (let depth = 0; depth < 8; depth++) { + const candidates = [ + join(dir, 'generated', 'local', 'run', data), + join(dir, 'src', 'test', 'e2e', 'generated', 'local', 'run', data), + ]; + + for (const candidate of candidates) { + if (existsSync(candidate)) return candidate; + } + + dir = dirname(dir); + } + + throw new Error('could not find the server data folder'); +} + +test('the plugin starts with a guild file it cannot read, and says which file', async ({ server }) => { + const log = server.session.consoleLog.slice().join('\n'); + + // The plugin disables itself when it cannot start, so the log is where an operator finds out + // whether it did. + expect(log).toContain('Enabling Guilds'); + expect(log).not.toContain('Disabling Guilds'); + + // The operator is told which file is the problem, rather than only that something was wrong. + expect(log).toContain(`${UNREADABLE_GUILD_ID}.json`); +}); + +test('the guilds that could be read are available', async ({ player }) => { + const listed = await guildList(player); + + expect(listed.some(entry => entry.name === READABLE_GUILD_NAME)).toBe(true); + // The guild whose file does not parse is not there, and pretending otherwise would be worse. + expect(listed.some(entry => entry.name === UNREADABLE_GUILD_NAME)).toBe(false); +}); + +test('the guild file that could not be read is left on disk', async () => { + const file = join(dataFolder(), `${UNREADABLE_GUILD_ID}.json`); + + expect(existsSync(file)).toBe(true); + expect(readFileSync(file, 'utf8')).toContain(UNREADABLE_GUILD_NAME); +}); + +test('the guild file that could be read is still readable', async () => { + const file = join(dataFolder(), `${READABLE_GUILD_ID}.json`); + + expect(existsSync(file)).toBe(true); + expect(readFileSync(file, 'utf8')).toContain(READABLE_GUILD_NAME); +}); 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..452a97dae --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java @@ -0,0 +1,289 @@ +package me.glaremasters.guilds.database.guild.provider; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import me.glaremasters.guilds.Guilds; +import me.glaremasters.guilds.database.DatabaseBackend; +import me.glaremasters.guilds.database.guild.GuildAdapter; +import me.glaremasters.guilds.guild.Guild; +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.File; +import java.io.IOException; +import java.lang.reflect.Field; +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.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Covers what the JSON backend does with a guild file it cannot parse. + * + *

The plugin stores one JSON file per guild, named after the guild's id. A file that does not + * parse is a file the plugin cannot read, and the plugin must not respond by writing over it: the + * only copy of that guild's data is in that file. + * + *

Three properties are asserted here, and they are the ones a server owner cares about: + * + *

+ * + *

The third one is the reason the corrupt file has to be protected explicitly. The save writes + * every guild it has in memory, one file at a time, so a file the plugin could not parse is a file + * the plugin does not know about. Whether that is safe depends entirely on the save path refusing + * to write over a file it failed to read. + */ +class GuildJsonProviderTest { + + /** The id of the guild whose file parses. */ + private static final String GOOD_ID = "11111111-1111-1111-1111-111111111111"; + + /** The id of the guild whose file does not parse. */ + private static final String CORRUPT_ID = "22222222-2222-2222-2222-222222222222"; + + /** A guild file that is not valid JSON at all. */ + private static final String CORRUPT_CONTENT = "{ this is not json"; + + @TempDir + Path tempDir; + + private File dataFolder; + private GuildJsonProvider provider; + + @BeforeEach + void setUp() throws Exception { + setPluginGson(new GsonBuilder().setPrettyPrinting().create()); + + dataFolder = tempDir.resolve("data").toFile(); + assertTrue(dataFolder.mkdirs(), "the data folder should have been created"); + + provider = new GuildJsonProvider(dataFolder); + } + + /** + * Puts a Gson instance into the plugin's static field. + * + *

{@link Guilds#getGson()} is what every provider uses to read and write, and the plugin + * fills the field in during startup. A test that does not set it gets a null Gson and a + * NullPointerException that has nothing to do with what it is testing. + * + * @param gson the Gson instance the plugin should use + * @throws Exception if the field cannot be reached + */ + private static void setPluginGson(Gson gson) throws Exception { + final Field field = Guilds.class.getDeclaredField("gson"); + field.setAccessible(true); + field.set(null, gson); + } + + /** + * @param id the guild's id + * @param name the guild's name + * @return a guild file as the plugin writes it + */ + private static String guildFile(String id, String name) { + return "{\n" + + " \"id\": \"" + id + "\",\n" + + " \"name\": \"" + name + "\",\n" + + " \"prefix\": \"Pfx\",\n" + + " \"motd\": \"A guild.\",\n" + + " \"guildMaster\": {\n" + + " \"uuid\": \"33333333-3333-3333-3333-333333333333\",\n" + + " \"role\": {\n" + + " \"level\": 0\n" + + " },\n" + + " \"joinDate\": 0,\n" + + " \"lastLogin\": 0\n" + + " },\n" + + " \"status\": \"Public\",\n" + + " \"tier\": {\n" + + " \"level\": 1\n" + + " },\n" + + " \"balance\": 0.0,\n" + + " \"members\": [\n" + + " {\n" + + " \"uuid\": \"33333333-3333-3333-3333-333333333333\",\n" + + " \"role\": {\n" + + " \"level\": 0\n" + + " },\n" + + " \"joinDate\": 0,\n" + + " \"lastLogin\": 0\n" + + " }\n" + + " ],\n" + + " \"invitedMembers\": [],\n" + + " \"allies\": [],\n" + + " \"pendingAllies\": [],\n" + + " \"codes\": [],\n" + + " \"vaults\": [],\n" + + " \"lastDefended\": 0,\n" + + " \"creationDate\": 1700000000000\n" + + "}"; + } + + /** + * @param id the guild's id + * @param name the guild's name + * @param content the file's contents + * @return the path of the guild file + * @throws IOException if the file cannot be written + */ + private Path writeGuildFile(String id, String name, String content) throws IOException { + final Path file = dataFolder.toPath().resolve(id + ".json"); + Files.write(file, content.getBytes(StandardCharsets.UTF_8)); + return file; + } + + /** + * @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 Guilds plugin = mock(Guilds.class); + when(plugin.getDataFolder()).thenReturn(tempDir.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("a guild file that cannot be parsed is not loaded") + void corruptFileIsNotLoaded() throws IOException { + writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + + final List loaded = provider.getAllGuilds(null); + + assertEquals(1, loaded.size(), "only the guild that parsed may be loaded"); + assertEquals(GOOD_ID, loaded.get(0).getId().toString()); + } + + @Test + @DisplayName("a guild file that cannot be parsed is left exactly as it was") + void corruptFileIsLeftUntouched() throws IOException { + writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final byte[] before = Files.readAllBytes(corrupt); + + provider.getAllGuilds(null); + + assertTrue(Files.exists(corrupt), "the file that could not be parsed has to stay on disk"); + assertArrayEquals(before, Files.readAllBytes(corrupt), "the file must not be rewritten"); + } + + @Test + @DisplayName("the guilds that did load are not rewritten by the load") + void otherFilesAreNotRewrittenByTheLoad() throws IOException { + final Path good = writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final byte[] before = Files.readAllBytes(good); + + provider.getAllGuilds(null); + + assertArrayEquals(before, Files.readAllBytes(good), "loading must not write anything"); + } + + @Test + @DisplayName("the save after a partial load does not overwrite the file that could not be parsed") + void saveDoesNotOverwriteTheCorruptFile() throws IOException { + writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final byte[] before = Files.readAllBytes(corrupt); + + final GuildAdapter guildAdapter = adapter(); + final List loaded = guildAdapter.getAllGuilds(); + + // This is what the scheduled save task does every minute, and what onDisable does. + guildAdapter.saveGuilds(loaded); + + assertTrue(Files.exists(corrupt), "the file that could not be parsed has to stay on disk"); + assertArrayEquals(before, Files.readAllBytes(corrupt), + "the save must not write over a file the plugin could not read"); + } + + @Test + @DisplayName("the save after a partial load leaves the guilds that did load alone") + void saveKeepsTheLoadedGuilds() throws IOException { + final Path good = writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + + final GuildAdapter guildAdapter = adapter(); + final List loaded = guildAdapter.getAllGuilds(); + guildAdapter.saveGuilds(loaded); + + final List reloaded = guildAdapter.getAllGuilds(); + assertEquals(1, reloaded.size()); + assertEquals(GOOD_ID, reloaded.get(0).getId().toString()); + assertTrue(Files.exists(good), "the guild that loaded has to stay on disk"); + } + + @Test + @DisplayName("a guild file that cannot be parsed is not written over, even by its own id") + void corruptFileIsProtectedFromEveryWrite() throws IOException { + final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final byte[] before = Files.readAllBytes(corrupt); + + provider.getAllGuilds(null); + + // The id of a file the plugin could not read is known: it is the file's name. Writing to + // that id is the one way the corrupt file could be destroyed, so it has to be refused. + provider.createGuild(null, CORRUPT_ID, guildFile(CORRUPT_ID, "BadGuild")); + assertThrows(IOException.class, + () -> provider.updateGuild(null, CORRUPT_ID, guildFile(CORRUPT_ID, "BadGuild")), + "a file the plugin could not read must not be written over"); + + assertArrayEquals(before, Files.readAllBytes(corrupt), + "a file that could not be parsed must never be written over"); + } + + @Test + @DisplayName("a guild file that could not be parsed is not deleted either") + void corruptFileIsProtectedFromDeletion() throws IOException { + final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.deleteGuild(null, CORRUPT_ID), + "a file the plugin could not read must not be deleted"); + + assertTrue(Files.exists(corrupt), "the file has to stay on disk"); + } + + @Test + @DisplayName("a data folder that cannot be read is reported as an I/O failure") + void unreadableDataFolderIsReported() throws IOException { + final GuildJsonProvider missing = new GuildJsonProvider(tempDir.resolve("gone").toFile()); + + final IOException failure = assertThrows(IOException.class, () -> missing.getAllGuilds(null)); + + assertTrue(failure.getMessage().contains("gone"), + "the message has to name the folder that could not be read: " + failure.getMessage()); + } + + @Test + @DisplayName("a guild file that cannot be parsed does not stop the other guilds from loading") + void otherGuildsStayAvailable() throws IOException { + writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); + writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + + final List loaded = provider.getAllGuilds(null); + + assertFalse(loaded.isEmpty(), "one broken file must not cost the server every guild"); + } +} diff --git a/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java b/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java new file mode 100644 index 000000000..f8bd4b664 --- /dev/null +++ b/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java @@ -0,0 +1,204 @@ +package me.glaremasters.guilds.guild; + +import ch.jalu.configme.SettingsManager; +import com.google.gson.GsonBuilder; +import me.glaremasters.guilds.Guilds; +import me.glaremasters.guilds.database.DatabaseAdapter; +import me.glaremasters.guilds.database.guild.GuildAdapter; +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.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.assertEquals; +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.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Covers what the plugin does when the guild data cannot be read. + * + *

{@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 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"); + } +} From ee5612995b0780ca366cff14600815f7c1ff60c7 Mon Sep 17 00:00:00 2001 From: Chief of Staff Date: Thu, 22 Oct 2026 12:04:09 +0000 Subject: [PATCH 2/8] 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 e3f5670861030dbc2e8df1c0224236ba9762750f Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 12:27:28 +0000 Subject: [PATCH 3/8] fix: back up the guild files the plugin cannot read A guild file that does not parse is the only copy of that guild. The plugin now copies it into the backup folder before it carries on without it, so that there is a second copy somewhere the plugin will never write to. The original stays exactly where the operator put it, and it keeps being protected from writes and deletions. A folder where no guild file can be read is not an empty guild database. It is a guild database the plugin failed to read, and a server that comes up as if it had no guilds hides that. The load fails there now, the way it already failed for a folder that cannot be listed, and the plugin stops with a message that says what could not be read. --- docs/e2e-coverage.md | 8 +- .../guilds/database/guild/GuildAdapter.java | 3 +- .../guild/provider/GuildJsonProvider.java | 66 ++++- src/test/e2e/tests/startup-loading.spec.ts | 64 ++++- .../guild/provider/GuildJsonProviderTest.java | 268 ++++++++++++++++-- .../guilds/guild/GuildHandlerLoadTest.java | 13 + 6 files changed, 377 insertions(+), 45 deletions(-) diff --git a/docs/e2e-coverage.md b/docs/e2e-coverage.md index cc39f026d..715187463 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. -145 tests across 13 spec files, plus 3 preflight tests that run before every suite and fail the run +147 tests across 13 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` @@ -188,7 +188,7 @@ 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) +## L. Startup and the guild data — `tests/startup-loading.spec.ts` (6) The environment stages two guild files into the plugin's data folder: one that reads and one that does not parse. @@ -197,8 +197,10 @@ does not parse. | --- | --- | | 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 | the guild file that could not be read is left on disk | +| The unreadable file is left alone | the guild file that could not be read is left on disk, unchanged | | The readable file is untouched | the guild file that could be read is still readable | +| The unreadable file is copied aside | the guild file that could not be read was copied to the backup folder | +| The copy is outside the data folder | the backup of the guild file is outside the data folder | ## Fixture self-check — `tests/bank.spec.ts` 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..dc6673c0b 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,8 @@ public GuildAdapter(Guilds guilds, DatabaseAdapter adapter) { default: case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "data"); - provider = new GuildJsonProvider(fileDataFolder); + // The plugin's own data folder is where the backups of unreadable guild files go. + provider = new GuildJsonProvider(guilds.getDataFolder(), fileDataFolder); 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 e55b18869..ad2d73ee6 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 @@ -2,6 +2,7 @@ import com.google.gson.Gson; import me.glaremasters.guilds.Guilds; +import me.glaremasters.guilds.database.CorruptDataBackup; import me.glaremasters.guilds.database.JsonFileUtils; import me.glaremasters.guilds.database.guild.GuildProvider; import me.glaremasters.guilds.guild.Guild; @@ -24,6 +25,13 @@ * Time: 11:38 AM */ public class GuildJsonProvider implements GuildProvider { + + /** + * The name the unreadable guild files are copied under, inside the backup folder. + */ + private static final String BACKUP_DATA_TYPE = "guilds"; + + private final File pluginDataFolder; private final File dataFolder; private Gson gson; @@ -36,7 +44,27 @@ public class GuildJsonProvider implements GuildProvider { */ private final Set unreadableFiles = new HashSet<>(); + /** + * Creates a provider over a guild data folder. + * + *

The plugin's own data folder is the parent of the guild data folder, which is where the + * backups of unreadable files go. + * + * @param dataFolder the folder the guild files live in + */ public GuildJsonProvider(File dataFolder) { + this(dataFolder == null ? null : dataFolder.getParentFile(), dataFolder); + } + + /** + * Creates a provider over a guild data folder. + * + * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds}. The backups of + * unreadable guild files are written below it. + * @param dataFolder the folder the guild files live in + */ + public GuildJsonProvider(File pluginDataFolder, File dataFolder) { + this.pluginDataFolder = pluginDataFolder; this.dataFolder = dataFolder; this.gson = Guilds.getGson(); } @@ -87,9 +115,10 @@ private File[] listGuildFiles() throws IOException { @Override public List getAllGuilds(@Nullable String tablePrefix) throws IOException { List loadedGuilds = new ArrayList<>(); + final File[] files = listGuildFiles(); int unreadable = 0; - for (File file : listGuildFiles()) { + for (File file : files) { try { Guild guild = JsonFileUtils.readJson(file, gson, Guild.class, "Guild"); guild.getId(); @@ -102,6 +131,7 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException LoggingUtils.severe("To prevent data loss in the plugin, this Guild has been prevented from loading. " + "The file has been left untouched and will not be written to. " + "Fix the file, or move it out of the data folder, and restart the server."); + backUp(file); } } @@ -110,9 +140,43 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException + "Those guilds are not available until the files are fixed."); } + /* + * A folder full of files the plugin cannot read is not an empty guild database. It is a + * guild database the plugin failed to read, and a server that comes up as if it had no + * guilds at all hides that: the guilds are still on disk, and the first save would write + * over them. The load fails instead, the way a folder that cannot be listed does. + */ + if (loadedGuilds.isEmpty() && files.length > 0) { + throw new IOException("None of the " + files.length + " files in " + dataFolder.getAbsolutePath() + + " could be read as a guild, so the guild data could not be loaded. " + + "The files have been left where they are. " + + "Fix them, or move them out of the data folder, and restart the server."); + } + return loadedGuilds; } + /** + * Copies a guild file the plugin could not read into the backup folder. + * + *

The copy is a safeguard, not a repair. The original stays exactly where the operator put + * it, and the plugin keeps refusing to write to it. + * + * @param file the file that could not be read + */ + private void backUp(File file) { + final CorruptDataBackup.Result backup = CorruptDataBackup.backup(pluginDataFolder, file, BACKUP_DATA_TYPE); + + if (backup.isCreated()) { + LoggingUtils.severe("A copy of the file was taken to " + backup.getBackupFile().getAbsolutePath() + "."); + } else if (backup.isSkipped()) { + LoggingUtils.severe("A copy of the file was already taken to " + backup.getBackupFile().getAbsolutePath() + "."); + } else { + LoggingUtils.severe("No copy of the file could be taken: " + backup.getFailure() + ". " + + "The file itself is still where it was."); + } + } + @Override public Guild getGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { File data = Arrays.stream(listGuildFiles()) diff --git a/src/test/e2e/tests/startup-loading.spec.ts b/src/test/e2e/tests/startup-loading.spec.ts index 3dd5a1748..72e50de19 100644 --- a/src/test/e2e/tests/startup-loading.spec.ts +++ b/src/test/e2e/tests/startup-loading.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@plugwright/runner'; -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync, readdirSync, readFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { guildList } from '../plugins/support/state.js'; @@ -28,22 +28,21 @@ const READABLE_GUILD_NAME = 'E2EStaged'; const UNREADABLE_GUILD_NAME = 'E2EBroken'; /** - * The plugin's guild data folder inside the throwaway server directory. + * The plugin's data folder inside the throwaway server directory. * * The folder is looked up rather than assumed: the runner's working directory depends on how it * was started, and a spec that hard-codes a path fails for a reason that has nothing to do with * what it is testing. * - * @returns the path of the guild data folder + * @returns the path of the plugin's data folder */ -function dataFolder(): string { - const data = join('plugins', 'Guilds', 'data'); +function pluginFolder(): string { let dir = process.cwd(); for (let depth = 0; depth < 8; depth++) { const candidates = [ - join(dir, 'generated', 'local', 'run', data), - join(dir, 'src', 'test', 'e2e', 'generated', 'local', 'run', data), + join(dir, 'generated', 'local', 'run', 'plugins', 'Guilds'), + join(dir, 'src', 'test', 'e2e', 'generated', 'local', 'run', 'plugins', 'Guilds'), ]; for (const candidate of candidates) { @@ -53,7 +52,34 @@ function dataFolder(): string { dir = dirname(dir); } - throw new Error('could not find the server data folder'); + throw new Error('could not find the plugin data folder'); +} + +/** + * @returns the folder the plugin reads its guild files from + */ +function dataFolder(): string { + return join(pluginFolder(), 'data'); +} + +/** + * @param name the file's name + * @returns every copy of that file in the backup folder + */ +function backupsOf(name: string): string[] { + const root = join(pluginFolder(), 'backups', 'corrupt'); + if (!existsSync(root)) return []; + + const copies: string[] = []; + for (const stamp of readdirSync(root)) { + const folder = join(root, stamp, 'guilds'); + if (!existsSync(folder)) continue; + for (const file of readdirSync(folder)) { + copies.push(join(folder, file)); + } + } + + return copies.filter(copy => copy.endsWith(name)); } test('the plugin starts with a guild file it cannot read, and says which file', async ({ server }) => { @@ -76,11 +102,15 @@ test('the guilds that could be read are available', async ({ player }) => { expect(listed.some(entry => entry.name === UNREADABLE_GUILD_NAME)).toBe(false); }); -test('the guild file that could not be read is left on disk', async () => { +test('the guild file that could not be read is left on disk, unchanged', async () => { const file = join(dataFolder(), `${UNREADABLE_GUILD_ID}.json`); + const copies = backupsOf(`${UNREADABLE_GUILD_ID}.json`); expect(existsSync(file)).toBe(true); - expect(readFileSync(file, 'utf8')).toContain(UNREADABLE_GUILD_NAME); + expect(copies.length).toBe(1); + // The backup was taken when the plugin read the file, so the file on disk and the backup are + // the same bytes: nothing has written over it since. + expect(readFileSync(file, 'utf8')).toBe(readFileSync(copies[0], 'utf8')); }); test('the guild file that could be read is still readable', async () => { @@ -89,3 +119,17 @@ test('the guild file that could be read is still readable', async () => { expect(existsSync(file)).toBe(true); expect(readFileSync(file, 'utf8')).toContain(READABLE_GUILD_NAME); }); + +test('the guild file that could not be read was copied to the backup folder', async () => { + const original = join(dataFolder(), `${UNREADABLE_GUILD_ID}.json`); + const copies = backupsOf(`${UNREADABLE_GUILD_ID}.json`); + + expect(copies.length).toBe(1); + expect(readFileSync(copies[0], 'utf8')).toBe(readFileSync(original, 'utf8')); +}); + +test('the backup of the guild file is outside the data folder', async () => { + for (const copy of backupsOf(`${UNREADABLE_GUILD_ID}.json`)) { + expect(copy.startsWith(dataFolder())).toBe(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 index 452a97dae..9c3978cfd 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 @@ -11,13 +11,17 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.io.File; import java.io.IOException; import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.List; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -28,7 +32,7 @@ import static org.mockito.Mockito.when; /** - * Covers what the JSON backend does with a guild file it cannot parse. + * Covers what the JSON backend does with a guild file it cannot read. * *

The plugin stores one JSON file per guild, named after the guild's id. A file that does not * parse is a file the plugin cannot read, and the plugin must not respond by writing over it: the @@ -46,11 +50,19 @@ * every guild it has in memory, one file at a time, so a file the plugin could not parse is a file * the plugin does not know about. Whether that is safe depends entirely on the save path refusing * to write over a file it failed to read. + * + *

A file that cannot be read is also copied into the backup folder, so that there is a second + * copy of it somewhere the plugin will never write to. + * + *

The JSON storage workstream (#846) adds a file of its own at this path. Its cases about the + * enumeration rule, that only {@code .json} files are records and that a record without an id is + * skipped, describe rules that land with that branch and are therefore not repeated here. The + * cases about a file that cannot be parsed are covered above. */ class GuildJsonProviderTest { /** The id of the guild whose file parses. */ - private static final String GOOD_ID = "11111111-1111-1111-1111-111111111111"; + private static final String GUILD_ID = "11111111-1111-1111-1111-111111111111"; /** The id of the guild whose file does not parse. */ private static final String CORRUPT_ID = "22222222-2222-2222-2222-222222222222"; @@ -58,20 +70,27 @@ class GuildJsonProviderTest { /** A guild file that is not valid JSON at all. */ private static final String CORRUPT_CONTENT = "{ this is not json"; + /** A guild file that parses but carries no id. */ + private static final String NO_ID_CONTENT = "{ \"name\": \"No Id Here\" }"; + @TempDir Path tempDir; - private File dataFolder; + private Path dataFolder; private GuildJsonProvider provider; @BeforeEach void setUp() throws Exception { setPluginGson(new GsonBuilder().setPrettyPrinting().create()); - dataFolder = tempDir.resolve("data").toFile(); - assertTrue(dataFolder.mkdirs(), "the data folder should have been created"); + dataFolder = tempDir.resolve("data"); + Files.createDirectories(dataFolder); - provider = new GuildJsonProvider(dataFolder); + // The plugin's data folder is the parent of the guild data folder, and that is where the + // backups of unreadable files go. + provider = new GuildJsonProvider(tempDir.toFile(), dataFolder.toFile()); + + write(GUILD_ID + ".json", guildJson(GUILD_ID, "The Real Guild")); } /** @@ -95,7 +114,7 @@ private static void setPluginGson(Gson gson) throws Exception { * @param name the guild's name * @return a guild file as the plugin writes it */ - private static String guildFile(String id, String name) { + private static String guildJson(String id, String name) { return "{\n" + " \"id\": \"" + id + "\",\n" + " \"name\": \"" + name + "\",\n" @@ -136,13 +155,24 @@ private static String guildFile(String id, String name) { /** * @param id the guild's id - * @param name the guild's name * @param content the file's contents * @return the path of the guild file * @throws IOException if the file cannot be written */ - private Path writeGuildFile(String id, String name, String content) throws IOException { - final Path file = dataFolder.toPath().resolve(id + ".json"); + private Path writeGuildFile(String id, String content) throws IOException { + final Path file = dataFolder.resolve(id + ".json"); + Files.write(file, content.getBytes(StandardCharsets.UTF_8)); + return file; + } + + /** + * @param name the file's name + * @param content the file's contents + * @return the path of the file + * @throws IOException if the file cannot be written + */ + private Path write(String name, String content) throws IOException { + final Path file = dataFolder.resolve(name); Files.write(file, content.getBytes(StandardCharsets.UTF_8)); return file; } @@ -162,23 +192,45 @@ private GuildAdapter adapter() { return new GuildAdapter(plugin, database); } + /** + * @param name the file's name + * @return every copy of that file in the backup folder + */ + private List backupsOf(String name) throws IOException { + final Path backupRoot = tempDir.resolve("backups").resolve("corrupt"); + final List backups = new ArrayList<>(); + + if (!Files.isDirectory(backupRoot)) { + return backups; + } + + try (Stream stamps = Files.list(backupRoot)) { + for (Path stamp : stamps.toArray(Path[]::new)) { + final Path copy = stamp.resolve("guilds").resolve(name); + if (Files.exists(copy)) { + backups.add(copy); + } + } + } + + return backups; + } + @Test @DisplayName("a guild file that cannot be parsed is not loaded") void corruptFileIsNotLoaded() throws IOException { - writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final List loaded = provider.getAllGuilds(null); assertEquals(1, loaded.size(), "only the guild that parsed may be loaded"); - assertEquals(GOOD_ID, loaded.get(0).getId().toString()); + assertEquals(GUILD_ID, loaded.get(0).getId().toString()); } @Test @DisplayName("a guild file that cannot be parsed is left exactly as it was") void corruptFileIsLeftUntouched() throws IOException { - writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final byte[] before = Files.readAllBytes(corrupt); provider.getAllGuilds(null); @@ -190,8 +242,8 @@ void corruptFileIsLeftUntouched() throws IOException { @Test @DisplayName("the guilds that did load are not rewritten by the load") void otherFilesAreNotRewrittenByTheLoad() throws IOException { - final Path good = writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path good = dataFolder.resolve(GUILD_ID + ".json"); + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final byte[] before = Files.readAllBytes(good); provider.getAllGuilds(null); @@ -202,8 +254,7 @@ void otherFilesAreNotRewrittenByTheLoad() throws IOException { @Test @DisplayName("the save after a partial load does not overwrite the file that could not be parsed") void saveDoesNotOverwriteTheCorruptFile() throws IOException { - writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final byte[] before = Files.readAllBytes(corrupt); final GuildAdapter guildAdapter = adapter(); @@ -220,8 +271,8 @@ void saveDoesNotOverwriteTheCorruptFile() throws IOException { @Test @DisplayName("the save after a partial load leaves the guilds that did load alone") void saveKeepsTheLoadedGuilds() throws IOException { - final Path good = writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path good = dataFolder.resolve(GUILD_ID + ".json"); + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final GuildAdapter guildAdapter = adapter(); final List loaded = guildAdapter.getAllGuilds(); @@ -229,23 +280,48 @@ void saveKeepsTheLoadedGuilds() throws IOException { final List reloaded = guildAdapter.getAllGuilds(); assertEquals(1, reloaded.size()); - assertEquals(GOOD_ID, reloaded.get(0).getId().toString()); + assertEquals(GUILD_ID, reloaded.get(0).getId().toString()); assertTrue(Files.exists(good), "the guild that loaded has to stay on disk"); } + @Test + @DisplayName("the guilds that could be read are still there after a save and a restart") + void theHealthyGuildsSurviveASaveAndARestart() throws IOException { + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + final byte[] corruptBefore = Files.readAllBytes(corrupt); + + // The server starts. + final GuildAdapter guildAdapter = adapter(); + final List loaded = guildAdapter.getAllGuilds(); + assertEquals(1, loaded.size()); + + // The autosave runs, and the server shuts down, which saves again. + guildAdapter.saveGuilds(loaded); + guildAdapter.saveGuilds(loaded); + + // The server starts again, from the same folder. + final GuildAdapter afterRestart = adapter(); + final List reloaded = afterRestart.getAllGuilds(); + + assertEquals(1, reloaded.size(), "the guild that could be read has to be there after a restart"); + assertEquals(GUILD_ID, reloaded.get(0).getId().toString()); + assertArrayEquals(corruptBefore, Files.readAllBytes(corrupt), + "a restart must not change the file the plugin could not read"); + } + @Test @DisplayName("a guild file that cannot be parsed is not written over, even by its own id") void corruptFileIsProtectedFromEveryWrite() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final byte[] before = Files.readAllBytes(corrupt); provider.getAllGuilds(null); // The id of a file the plugin could not read is known: it is the file's name. Writing to // that id is the one way the corrupt file could be destroyed, so it has to be refused. - provider.createGuild(null, CORRUPT_ID, guildFile(CORRUPT_ID, "BadGuild")); + provider.createGuild(null, CORRUPT_ID, guildJson(CORRUPT_ID, "BadGuild")); assertThrows(IOException.class, - () -> provider.updateGuild(null, CORRUPT_ID, guildFile(CORRUPT_ID, "BadGuild")), + () -> provider.updateGuild(null, CORRUPT_ID, guildJson(CORRUPT_ID, "BadGuild")), "a file the plugin could not read must not be written over"); assertArrayEquals(before, Files.readAllBytes(corrupt), @@ -253,9 +329,9 @@ void corruptFileIsProtectedFromEveryWrite() throws IOException { } @Test - @DisplayName("a guild file that could not be parsed is not deleted either") + @DisplayName("a guild file that cannot be parsed is not deleted either") void corruptFileIsProtectedFromDeletion() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); provider.getAllGuilds(null); @@ -265,6 +341,17 @@ void corruptFileIsProtectedFromDeletion() throws IOException { assertTrue(Files.exists(corrupt), "the file has to stay on disk"); } + @Test + @DisplayName("a guild file without an id is left on disk") + void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { + final Path file = writeGuildFile(CORRUPT_ID, NO_ID_CONTENT); + + provider.getAllGuilds(null); + + assertTrue(Files.exists(file), "the file has to stay on disk"); + assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(file)); + } + @Test @DisplayName("a data folder that cannot be read is reported as an I/O failure") void unreadableDataFolderIsReported() throws IOException { @@ -279,11 +366,132 @@ void unreadableDataFolderIsReported() throws IOException { @Test @DisplayName("a guild file that cannot be parsed does not stop the other guilds from loading") void otherGuildsStayAvailable() throws IOException { - writeGuildFile(GOOD_ID, "GoodGuild", guildFile(GOOD_ID, "GoodGuild")); - writeGuildFile(CORRUPT_ID, "BadGuild", CORRUPT_CONTENT); + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); final List loaded = provider.getAllGuilds(null); assertFalse(loaded.isEmpty(), "one broken file must not cost the server every guild"); } + + @Test + @DisplayName("a folder where no guild file can be read is a load failure") + void aFolderWhereNothingCanBeReadIsALoadFailure() throws IOException { + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + Files.delete(dataFolder.resolve(GUILD_ID + ".json")); + + final IOException failure = assertThrows(IOException.class, () -> provider.getAllGuilds(null)); + + assertTrue(failure.getMessage().contains("the guild data could not be loaded"), + "the message has to say that the guild data could not be loaded: " + failure.getMessage()); + } + + @Test + @DisplayName("a folder with no guild files at all is not a load failure") + void anEmptyFolderIsNotALoadFailure() throws IOException { + Files.delete(dataFolder.resolve(GUILD_ID + ".json")); + + assertTrue(provider.getAllGuilds(null).isEmpty(), "a server with no guilds has to start"); + } + + @Test + @DisplayName("a guild file that cannot be parsed is copied into the backup folder") + void theCorruptFileIsBackedUp() throws IOException { + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + + provider.getAllGuilds(null); + + final List backups = backupsOf(CORRUPT_ID + ".json"); + assertEquals(1, backups.size(), "the file the plugin could not read has to be copied somewhere"); + assertArrayEquals(Files.readAllBytes(corrupt), Files.readAllBytes(backups.get(0)), + "the copy has to be byte for byte the file the plugin could not read"); + } + + @Test + @DisplayName("the backup of a guild file is outside the data folder") + void theBackupIsOutsideTheDataFolder() throws IOException { + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + + provider.getAllGuilds(null); + + for (Path backup : backupsOf(CORRUPT_ID + ".json")) { + assertFalse(backup.startsWith(dataFolder), + "a backup the plugin enumerates would be read as a guild: " + backup); + } + } + + @Test + @DisplayName("the same corruption read twice does not produce two backups") + void theSameCorruptionIsNotBackedUpTwice() throws IOException { + writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + + provider.getAllGuilds(null); + provider.getAllGuilds(null); + + assertEquals(1, backupsOf(CORRUPT_ID + ".json").size(), + "reading the same corruption again has to reuse the copy that is already there"); + } + + @Test + @DisplayName("a backup that cannot be written is reported and the original file is left alone") + void aBackupThatCannotBeWrittenIsReported() throws IOException { + final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + final byte[] before = Files.readAllBytes(corrupt); + + // A file where the backup folder has to be makes every backup fail. + Files.write(tempDir.resolve("backups"), "not a folder".getBytes(StandardCharsets.UTF_8)); + + try (LogCapture log = new LogCapture()) { + final List loaded = provider.getAllGuilds(null); + + assertEquals(1, loaded.size(), "a backup that cannot be written must not stop the load"); + assertTrue(log.contains("No copy of the file could be taken"), + "the operator has to be told that the backup failed"); + } + + assertArrayEquals(before, Files.readAllBytes(corrupt), + "a failed backup must not change the original file"); + assertTrue(Files.exists(corrupt)); + } + + /** + * Captures what the plugin logs while a test runs. + */ + private static final class LogCapture implements AutoCloseable { + + private final List messages = new ArrayList<>(); + + private final Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + if (record != null && record.getMessage() != null) { + messages.add(record.getMessage()); + } + } + + @Override + public void flush() { + } + + @Override + public void close() { + } + }; + + LogCapture() { + Logger.getLogger("Guilds").addHandler(handler); + } + + /** + * @param fragment the text to look for + * @return true when the plugin logged a line carrying that text + */ + boolean contains(String fragment) { + return messages.stream().anyMatch(message -> message.contains(fragment)); + } + + @Override + public void close() { + Logger.getLogger("Guilds").removeHandler(handler); + } + } } diff --git a/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java b/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java index f8bd4b664..a31959bb7 100644 --- a/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java +++ b/src/test/java/me/glaremasters/guilds/guild/GuildHandlerLoadTest.java @@ -166,6 +166,19 @@ void loadFailureIsNotSwallowedSilently() throws IOException { "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 { From 42909305fb29cd028331a38423520237b7717590 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 12:41:37 +0000 Subject: [PATCH 4/8] docs: bring the end-to-end coverage totals in line with the merged suite The totals were contended between the open branches. The header now says what the merged suite will have, the invite code section carries the rows for the tests the other branches add, and the startup section carries the count of the tests in this branch's spec. --- docs/e2e-coverage.md | 18 +++++++++--------- src/test/e2e/tests/startup-loading.spec.ts | 18 +++--------------- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/docs/e2e-coverage.md b/docs/e2e-coverage.md index 715187463..05d4413cc 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. -147 tests across 13 spec files, plus 3 preflight tests that run before every suite and fail the run +160 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` @@ -128,17 +128,19 @@ 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 | | --- | --- | | Creating invite codes | a code can be created, listed and deleted | | Viewing and listing codes | a code can be created, listed and deleted / *and it is listed for the guild master* | -| Redeeming a code | a code can be redeemed and puts the player in the guild | +| Redeeming a code | a code can be redeemed and puts the player in the guild / a code with two uses can be redeemed twice | | Deleting codes | a code can be created, listed and deleted | -| Invalid codes | an invalid code is refused | +| Invalid codes | an invalid code is refused / a code with no uses is refused and nothing is stored / a code with a negative number of uses is refused and nothing is stored | | Reusing codes where restricted | a code runs out of uses and then says so | -| Permission restrictions | code information is restricted by role / *a member can be added and cannot manage codes* | +| Refused codes | a refused code does not use up the guild allowance of codes | +| Permission restrictions | code information is restricted by role / *a member can be added and cannot manage codes* / a player who joins by code gets the permissions their guild grants them | +| Redemption cooldowns | a code cannot be redeemed while the join cooldown is running | | Full lifecycle | *one code through its whole life* — a `describe.serial` chain: create → listed → redeem → deleted, asserting membership at each step rather than the command's reply | ## I. Inventory and GUI functionality — `tests/gui.spec.ts` (18) @@ -188,7 +190,7 @@ 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` (6) +## 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. @@ -197,10 +199,8 @@ does not parse. | --- | --- | | 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 | the guild file that could not be read is left on disk, unchanged | +| 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 | -| The unreadable file is copied aside | the guild file that could not be read was copied to the backup folder | -| The copy is outside the data folder | the backup of the guild file is outside the data folder | ## Fixture self-check — `tests/bank.spec.ts` diff --git a/src/test/e2e/tests/startup-loading.spec.ts b/src/test/e2e/tests/startup-loading.spec.ts index 72e50de19..f3c90af08 100644 --- a/src/test/e2e/tests/startup-loading.spec.ts +++ b/src/test/e2e/tests/startup-loading.spec.ts @@ -102,7 +102,7 @@ test('the guilds that could be read are available', async ({ player }) => { expect(listed.some(entry => entry.name === UNREADABLE_GUILD_NAME)).toBe(false); }); -test('the guild file that could not be read is left on disk, unchanged', async () => { +test('the guild file that could not be read is left on disk, with a copy in the backup folder', async () => { const file = join(dataFolder(), `${UNREADABLE_GUILD_ID}.json`); const copies = backupsOf(`${UNREADABLE_GUILD_ID}.json`); @@ -111,6 +111,8 @@ test('the guild file that could not be read is left on disk, unchanged', async ( // The backup was taken when the plugin read the file, so the file on disk and the backup are // the same bytes: nothing has written over it since. expect(readFileSync(file, 'utf8')).toBe(readFileSync(copies[0], 'utf8')); + // The copy sits outside the folder the plugin reads its guilds from, so it is never a guild. + expect(copies[0].startsWith(dataFolder())).toBe(false); }); test('the guild file that could be read is still readable', async () => { @@ -119,17 +121,3 @@ test('the guild file that could be read is still readable', async () => { expect(existsSync(file)).toBe(true); expect(readFileSync(file, 'utf8')).toContain(READABLE_GUILD_NAME); }); - -test('the guild file that could not be read was copied to the backup folder', async () => { - const original = join(dataFolder(), `${UNREADABLE_GUILD_ID}.json`); - const copies = backupsOf(`${UNREADABLE_GUILD_ID}.json`); - - expect(copies.length).toBe(1); - expect(readFileSync(copies[0], 'utf8')).toBe(readFileSync(original, 'utf8')); -}); - -test('the backup of the guild file is outside the data folder', async () => { - for (const copy of backupsOf(`${UNREADABLE_GUILD_ID}.json`)) { - expect(copy.startsWith(dataFolder())).toBe(false); - } -}); From edbac9b06aa5f8ede29957f5b28a9f131c97267c Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:09:54 +0000 Subject: [PATCH 5/8] refactor: use the shared JSON helpers so the guild storage matches the others The guild provider now takes its folders in the order the other three providers use, and keeps them in the fields in the same order, so a merge cannot leave the two folders the wrong way round. Both of them are files, and the compiler says nothing about a swap. The provider also uses the shared helpers for listing the record files and for backing a file up, so all four storages call the backup in the same place and the merged tree has one way of doing it. The constructor warns when the plugin's data folder is not the parent of the record folder, naming both, because that is the one mistake the compiler cannot catch. The test file is now the superset of this branch's cases and the JSON storage branch's, so either side of that conflict keeps every case. --- .../guilds/database/JsonFileUtils.java | 156 +++++ .../guilds/database/guild/GuildAdapter.java | 2 +- .../guild/provider/GuildJsonProvider.java | 147 ++-- .../guilds/database/CorruptBackups.java | 70 ++ .../guilds/database/LogCapture.java | 70 ++ .../guilds/database/TestGson.java | 40 ++ .../guild/provider/GuildJsonProviderTest.java | 646 +++++++++--------- 7 files changed, 750 insertions(+), 381 deletions(-) create mode 100644 src/test/java/me/glaremasters/guilds/database/CorruptBackups.java create mode 100644 src/test/java/me/glaremasters/guilds/database/LogCapture.java create mode 100644 src/test/java/me/glaremasters/guilds/database/TestGson.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..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,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,170 @@ 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 listDataFiles(@Nullable File folder) throws IOException { + final List dataFiles = new ArrayList<>(); + + if (folder == null) { + return dataFiles; + } + + final File[] files = folder.listFiles(); + + if (files == null) { + throw new IOException("The data folder could not be read: " + folder.getAbsolutePath()); + } + + 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); + } + + /** + * 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/guild/GuildAdapter.java b/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java index dc6673c0b..c231bc6f9 100644 --- a/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java +++ b/src/main/java/me/glaremasters/guilds/database/guild/GuildAdapter.java @@ -24,7 +24,7 @@ public GuildAdapter(Guilds guilds, DatabaseAdapter adapter) { case JSON: File fileDataFolder = new File(guilds.getDataFolder(), "data"); // The plugin's own data folder is where the backups of unreadable guild files go. - provider = new GuildJsonProvider(guilds.getDataFolder(), 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 ad2d73ee6..99c22547d 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 @@ -2,7 +2,6 @@ import com.google.gson.Gson; import me.glaremasters.guilds.Guilds; -import me.glaremasters.guilds.database.CorruptDataBackup; import me.glaremasters.guilds.database.JsonFileUtils; import me.glaremasters.guilds.database.guild.GuildProvider; import me.glaremasters.guilds.guild.Guild; @@ -14,7 +13,6 @@ import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -27,20 +25,21 @@ public class GuildJsonProvider implements GuildProvider { /** - * The name the unreadable guild files are copied under, inside the backup folder. + * The kind of record the guild files hold, used as the name of the backup folder. */ - private static final String BACKUP_DATA_TYPE = "guilds"; + private static final String DATA_TYPE = "guilds"; - private final File pluginDataFolder; private final File dataFolder; + private final File pluginDataFolder; private Gson gson; /** * The guild files that could not be read when the guilds were loaded. * - *

A file in here holds data the plugin could not parse, which makes it the only copy of that - * guild. 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. + *

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<>(); @@ -53,20 +52,53 @@ public class GuildJsonProvider implements GuildProvider { * @param dataFolder the folder the guild files live in */ public GuildJsonProvider(File dataFolder) { - this(dataFolder == null ? null : dataFolder.getParentFile(), dataFolder); + this(dataFolder, dataFolder == null ? null : dataFolder.getParentFile()); } /** * Creates a provider over a guild data folder. * - * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds}. The backups of - * unreadable guild files are written below it. * @param dataFolder the folder the guild files live in + * @param pluginDataFolder the plugin's data folder, where the backups are kept */ - public GuildJsonProvider(File pluginDataFolder, File dataFolder) { - this.pluginDataFolder = pluginDataFolder; + public GuildJsonProvider(File dataFolder, File pluginDataFolder) { this.dataFolder = dataFolder; + this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); + + warnAboutAnUnexpectedLayout(dataFolder, pluginDataFolder); + } + + /** + * Warns when the plugin's data folder is not the parent of the guild data folder. + * + *

Every record folder the plugin reads is a direct child of the plugin's data folder, and + * the backups of the files it cannot read are written next to them. The two folders are both + * plain files, so a caller that passes them the wrong way round gets no complaint from the + * compiler and ends up with the backups inside the folder the plugin reads its records from. + * + *

This is a warning rather than an exception. The constructor is public, and a caller with + * another layout is allowed to use it; it just has to be told that the backups will not land + * where they are expected to. + * + * @param dataFolder the folder the guild files live in + * @param pluginDataFolder the plugin's data folder + */ + private static void warnAboutAnUnexpectedLayout(File dataFolder, File pluginDataFolder) { + if (dataFolder == null || pluginDataFolder == null) { + return; + } + + final File parent = dataFolder.getAbsoluteFile().getParentFile(); + + if (parent != null && parent.equals(pluginDataFolder.getAbsoluteFile())) { + return; + } + + LoggingUtils.severe("The guild data folder (" + dataFolder.getAbsolutePath() + ") is not a folder inside the " + + "plugin's data folder (" + pluginDataFolder.getAbsolutePath() + "), so the backup of a guild file " + + "the plugin cannot read will not land where it should. Every guild folder the plugin uses is a " + + "direct child of the plugin's data folder."); } @Override @@ -76,67 +108,57 @@ public void createContainer(@Nullable String tablePrefix) throws IOException { @Override public boolean guildExists(@Nullable String tablePrefix, @NotNull String id) throws IOException { - return Arrays.stream(listGuildFiles()) - .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) throws IOException { List loadedGuildIds = new ArrayList<>(); - for (File file : listGuildFiles()) { - loadedGuildIds.add(com.google.common.io.Files.getNameWithoutExtension(file.getName())); + for (File file : JsonFileUtils.listDataFiles(this.dataFolder)) { + loadedGuildIds.add(JsonFileUtils.getRecordId(file)); } return loadedGuildIds; } - /** - * Reads every guild file in the data folder. - * - *

A folder that cannot be listed is a problem with the storage, not with one guild, and it - * used to come out of here as a {@link NullPointerException} that named nothing. It is an - * {@link IOException} naming the folder instead, so the plugin can say what went wrong. - * - * @return the files in the guild data folder - * @throws IOException if the folder cannot be read - */ - private File[] listGuildFiles() throws IOException { - final File[] files = dataFolder.listFiles(); - - if (files == null) { - throw new IOException("The guild data folder could not be read: " + dataFolder.getAbsolutePath()); - } - - return files; - } - @Override public List getAllGuilds(@Nullable String tablePrefix) throws IOException { List loadedGuilds = new ArrayList<>(); - final File[] files = listGuildFiles(); + final List files = JsonFileUtils.listDataFiles(this.dataFolder); int unreadable = 0; for (File file : files) { 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) { + unreadable++; + // 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."); + JsonFileUtils.backupCorruptFile(pluginDataFolder, file, DATA_TYPE, "the record has no id"); + continue; + } + loadedGuilds.add(guild); } catch (Exception ex) { unreadable++; // 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. " - + "The file has been left untouched and will not be written to. " - + "Fix the file, or move it out of the data folder, and restart the server."); - backUp(file); + 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); } } if (unreadable > 0) { - LoggingUtils.severe(unreadable + " of " + (loadedGuilds.size() + unreadable) + " guild files could not be read. " + LoggingUtils.severe(unreadable + " of " + files.size() + " guild files could not be read. " + "Those guilds are not available until the files are fixed."); } @@ -146,8 +168,8 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException * guilds at all hides that: the guilds are still on disk, and the first save would write * over them. The load fails instead, the way a folder that cannot be listed does. */ - if (loadedGuilds.isEmpty() && files.length > 0) { - throw new IOException("None of the " + files.length + " files in " + dataFolder.getAbsolutePath() + if (loadedGuilds.isEmpty() && !files.isEmpty()) { + throw new IOException("None of the " + files.size() + " guild files in " + dataFolder.getAbsolutePath() + " could be read as a guild, so the guild data could not be loaded. " + "The files have been left where they are. " + "Fix them, or move them out of the data folder, and restart the server."); @@ -156,31 +178,10 @@ public List getAllGuilds(@Nullable String tablePrefix) throws IOException return loadedGuilds; } - /** - * Copies a guild file the plugin could not read into the backup folder. - * - *

The copy is a safeguard, not a repair. The original stays exactly where the operator put - * it, and the plugin keeps refusing to write to it. - * - * @param file the file that could not be read - */ - private void backUp(File file) { - final CorruptDataBackup.Result backup = CorruptDataBackup.backup(pluginDataFolder, file, BACKUP_DATA_TYPE); - - if (backup.isCreated()) { - LoggingUtils.severe("A copy of the file was taken to " + backup.getBackupFile().getAbsolutePath() + "."); - } else if (backup.isSkipped()) { - LoggingUtils.severe("A copy of the file was already taken to " + backup.getBackupFile().getAbsolutePath() + "."); - } else { - LoggingUtils.severe("No copy of the file could be taken: " + backup.getFailure() + ". " - + "The file itself is still where it was."); - } - } - @Override public Guild getGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { - File data = Arrays.stream(listGuildFiles()) - .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); @@ -192,12 +193,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 { @@ -226,7 +227,7 @@ private void refuseToOverwriteUnreadableFile(File file) throws IOException { @Override public void deleteGuild(@Nullable String tablePrefix, @NotNull String id) throws IOException { - deleteGuild(new File(dataFolder, id + ".json")); + deleteGuild(new File(dataFolder, id + JsonFileUtils.DATA_FILE_EXTENSION)); } private void deleteGuild(File file) throws IOException { 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/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/guild/provider/GuildJsonProviderTest.java b/src/test/java/me/glaremasters/guilds/database/guild/provider/GuildJsonProviderTest.java index 9c3978cfd..45daa8228 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,74 +1,58 @@ package me.glaremasters.guilds.database.guild.provider; -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; -import me.glaremasters.guilds.Guilds; +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.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.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.util.ArrayList; +import java.nio.file.attribute.FileTime; +import java.util.Arrays; import java.util.List; -import java.util.logging.Handler; -import java.util.logging.LogRecord; -import java.util.logging.Logger; -import java.util.stream.Stream; 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.mockito.Mockito.mock; import static org.mockito.Mockito.when; /** - * Covers what the JSON backend does with a guild file it cannot read. + * Covers which files the JSON guild storage treats as guilds, and what happens to the ones it + * cannot read. * - *

The plugin stores one JSON file per guild, named after the guild's id. A file that does not - * parse is a file the plugin cannot read, and the plugin must not respond by writing over it: the - * only copy of that guild's data is in that file. + *

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. * - *

Three properties are asserted here, and they are the ones a server owner cares about: + *

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. * - *

- * - *

The third one is the reason the corrupt file has to be protected explicitly. The save writes - * every guild it has in memory, one file at a time, so a file the plugin could not parse is a file - * the plugin does not know about. Whether that is safe depends entirely on the save path refusing - * to write over a file it failed to read. - * - *

A file that cannot be read is also copied into the backup folder, so that there is a second - * copy of it somewhere the plugin will never write to. - * - *

The JSON storage workstream (#846) adds a file of its own at this path. Its cases about the - * enumeration rule, that only {@code .json} files are records and that a record without an id is - * skipped, describe rules that land with that branch and are therefore not repeated here. The - * cases about a file that cannot be parsed are covered above. + *

And a folder where nothing at all can be read is not an empty guild database. It is a guild + * database the plugin failed to read, and a server that comes up as if it had no guilds would let + * the first save write over them. */ class GuildJsonProviderTest { - /** The id of the guild whose file parses. */ private static final String GUILD_ID = "11111111-1111-1111-1111-111111111111"; + private static final String OTHER_GUILD_ID = "22222222-2222-2222-2222-222222222222"; - /** The id of the guild whose file does not parse. */ - private static final String CORRUPT_ID = "22222222-2222-2222-2222-222222222222"; - - /** A guild file that is not valid JSON at all. */ - private static final String CORRUPT_CONTENT = "{ this is not json"; + /** A guild file that does not parse. */ + private static final String BROKEN_CONTENT = "{ this is not json"; /** A guild file that parses but carries no id. */ private static final String NO_ID_CONTENT = "{ \"name\": \"No Id Here\" }"; @@ -76,105 +60,73 @@ class GuildJsonProviderTest { @TempDir Path tempDir; - private Path dataFolder; private GuildJsonProvider provider; + @BeforeAll + static void installGson() throws ReflectiveOperationException { + TestGson.install(); + } + @BeforeEach - void setUp() throws Exception { - setPluginGson(new GsonBuilder().setPrettyPrinting().create()); + void setUp() throws IOException { + Files.createDirectories(dataFolder()); + provider = new GuildJsonProvider(dataFolder().toFile(), pluginFolder().toFile()); - dataFolder = tempDir.resolve("data"); - Files.createDirectories(dataFolder); + // The one real guild. + write(GUILD_ID + ".json", guildJson(GUILD_ID, "The Real Guild")); - // The plugin's data folder is the parent of the guild data folder, and that is where the - // backups of unreadable files go. - provider = new GuildJsonProvider(tempDir.toFile(), dataFolder.toFile()); + // 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")); - write(GUILD_ID + ".json", 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")); } /** - * Puts a Gson instance into the plugin's static field. - * - *

{@link Guilds#getGson()} is what every provider uses to read and write, and the plugin - * fills the field in during startup. A test that does not set it gets a null Gson and a - * NullPointerException that has nothing to do with what it is testing. - * - * @param gson the Gson instance the plugin should use - * @throws Exception if the field cannot be reached + * @return the plugin's data folder, the one the backups are kept in */ - private static void setPluginGson(Gson gson) throws Exception { - final Field field = Guilds.class.getDeclaredField("gson"); - field.setAccessible(true); - field.set(null, gson); + private Path pluginFolder() { + return tempDir; } /** - * @param id the guild's id - * @param name the guild's name - * @return a guild file as the plugin writes it + * @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\": \"Pfx\",\n" - + " \"motd\": \"A guild.\",\n" - + " \"guildMaster\": {\n" - + " \"uuid\": \"33333333-3333-3333-3333-333333333333\",\n" - + " \"role\": {\n" - + " \"level\": 0\n" - + " },\n" - + " \"joinDate\": 0,\n" - + " \"lastLogin\": 0\n" - + " },\n" + + " \"prefix\": \"TG\",\n" + + " \"motd\": \"A guild\",\n" + " \"status\": \"Public\",\n" - + " \"tier\": {\n" - + " \"level\": 1\n" - + " },\n" + " \"balance\": 0.0,\n" - + " \"members\": [\n" - + " {\n" - + " \"uuid\": \"33333333-3333-3333-3333-333333333333\",\n" - + " \"role\": {\n" - + " \"level\": 0\n" - + " },\n" - + " \"joinDate\": 0,\n" - + " \"lastLogin\": 0\n" - + " }\n" - + " ],\n" + + " \"members\": [],\n" + " \"invitedMembers\": [],\n" + " \"allies\": [],\n" + " \"pendingAllies\": [],\n" + " \"codes\": [],\n" + " \"vaults\": [],\n" + " \"lastDefended\": 0,\n" - + " \"creationDate\": 1700000000000\n" - + "}"; - } - - /** - * @param id the guild's id - * @param content the file's contents - * @return the path of the guild file - * @throws IOException if the file cannot be written - */ - private Path writeGuildFile(String id, String content) throws IOException { - final Path file = dataFolder.resolve(id + ".json"); - Files.write(file, content.getBytes(StandardCharsets.UTF_8)); - return file; - } - - /** - * @param name the file's name - * @param content the file's contents - * @return the path of the file - * @throws IOException if the file cannot be written - */ - private Path write(String name, String content) throws IOException { - final Path file = dataFolder.resolve(name); - Files.write(file, content.getBytes(StandardCharsets.UTF_8)); - return file; + + " \"creationDate\": 0\n" + + "}\n"; } /** @@ -182,8 +134,8 @@ private Path write(String name, String content) throws IOException { * production path rather than through the provider directly */ private GuildAdapter adapter() { - final Guilds plugin = mock(Guilds.class); - when(plugin.getDataFolder()).thenReturn(tempDir.toFile()); + 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); @@ -192,306 +144,386 @@ private GuildAdapter adapter() { return new GuildAdapter(plugin, database); } - /** - * @param name the file's name - * @return every copy of that file in the backup folder - */ - private List backupsOf(String name) throws IOException { - final Path backupRoot = tempDir.resolve("backups").resolve("corrupt"); - final List backups = new ArrayList<>(); + @Test + @DisplayName("only the guild data files are enumerated") + void onlyDataFilesAreEnumerated() throws IOException { + assertEquals(List.of(GUILD_ID), provider.getAllGuildIds(null)); + } - if (!Files.isDirectory(backupRoot)) { - return backups; - } + @Test + @DisplayName("only the guild data files are loaded as guilds") + void onlyDataFilesAreLoaded() throws IOException { + final List guilds = provider.getAllGuilds(null); - try (Stream stamps = Files.list(backupRoot)) { - for (Path stamp : stamps.toArray(Path[]::new)) { - final Path copy = stamp.resolve("guilds").resolve(name); - if (Files.exists(copy)) { - backups.add(copy); - } - } + 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() throws IOException { + 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"); } + } - return backups; + @Test + @DisplayName("only the guild data files count as an existing guild") + void onlyDataFilesExist() throws IOException { + 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 file that cannot be parsed is not loaded") - void corruptFileIsNotLoaded() throws IOException { - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("a guild is only read from a guild data file") + void onlyDataFilesAreRead() throws IOException { + final Guild guild = provider.getGuild(null, GUILD_ID); - final List loaded = provider.getAllGuilds(null); + assertNotNull(guild); + assertEquals(GUILD_ID, guild.getId().toString()); + assertEquals("The Real Guild", guild.getName()); - assertEquals(1, loaded.size(), "only the guild that parsed may be loaded"); - assertEquals(GUILD_ID, loaded.get(0).getId().toString()); + assertNull(provider.getGuild(null, "notes")); + assertNull(provider.getGuild(null, "README")); + assertNull(provider.getGuild(null, "old-guild")); } @Test - @DisplayName("a guild file that cannot be parsed is left exactly as it was") - void corruptFileIsLeftUntouched() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] before = Files.readAllBytes(corrupt); - + @DisplayName("the files the plugin ignores are left on disk") + void ignoredFilesAreLeftAlone() throws IOException { provider.getAllGuilds(null); - - assertTrue(Files.exists(corrupt), "the file that could not be parsed has to stay on disk"); - assertArrayEquals(before, Files.readAllBytes(corrupt), "the file must not be rewritten"); + provider.getAllGuildIds(null); + provider.guildExists(null, GUILD_ID); + provider.getGuild(null, GUILD_ID); + + 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 - @DisplayName("the guilds that did load are not rewritten by the load") - void otherFilesAreNotRewrittenByTheLoad() throws IOException { - final Path good = dataFolder.resolve(GUILD_ID + ".json"); - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] before = Files.readAllBytes(good); + @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", BROKEN_CONTENT); - provider.getAllGuilds(null); + final List guilds = provider.getAllGuilds(null); - assertArrayEquals(before, Files.readAllBytes(good), "loading must not write anything"); + assertEquals(1, guilds.size()); + assertEquals(GUILD_ID, guilds.get(0).getId().toString()); } @Test - @DisplayName("the save after a partial load does not overwrite the file that could not be parsed") - void saveDoesNotOverwriteTheCorruptFile() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] before = Files.readAllBytes(corrupt); - - final GuildAdapter guildAdapter = adapter(); - final List loaded = guildAdapter.getAllGuilds(); + @DisplayName("a guild file that cannot be parsed is left on disk") + void aBrokenGuildFileIsLeftOnDisk() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - // This is what the scheduled save task does every minute, and what onDisable does. - guildAdapter.saveGuilds(loaded); + provider.getAllGuilds(null); - assertTrue(Files.exists(corrupt), "the file that could not be parsed has to stay on disk"); - assertArrayEquals(before, Files.readAllBytes(corrupt), - "the save must not write over a file the plugin could not read"); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertEquals(BROKEN_CONTENT, new String( + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test - @DisplayName("the save after a partial load leaves the guilds that did load alone") - void saveKeepsTheLoadedGuilds() throws IOException { - final Path good = dataFolder.resolve(GUILD_ID + ".json"); - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("a guild file without an id does not take the other guilds with it") + void aGuildFileWithoutAnIdDoesNotTakeTheOthersWithIt() throws IOException { + write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); - final GuildAdapter guildAdapter = adapter(); - final List loaded = guildAdapter.getAllGuilds(); - guildAdapter.saveGuilds(loaded); + final List guilds = provider.getAllGuilds(null); - final List reloaded = guildAdapter.getAllGuilds(); - assertEquals(1, reloaded.size()); - assertEquals(GUILD_ID, reloaded.get(0).getId().toString()); - assertTrue(Files.exists(good), "the guild that loaded has to stay on disk"); + assertEquals(1, guilds.size()); + assertEquals(GUILD_ID, guilds.get(0).getId().toString()); } @Test - @DisplayName("the guilds that could be read are still there after a save and a restart") - void theHealthyGuildsSurviveASaveAndARestart() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] corruptBefore = Files.readAllBytes(corrupt); + @DisplayName("a guild file without an id is left on disk") + void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { + write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); - // The server starts. - final GuildAdapter guildAdapter = adapter(); - final List loaded = guildAdapter.getAllGuilds(); - assertEquals(1, loaded.size()); + provider.getAllGuilds(null); - // The autosave runs, and the server shuts down, which saves again. - guildAdapter.saveGuilds(loaded); - guildAdapter.saveGuilds(loaded); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertEquals(NO_ID_CONTENT, new String( + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); + } - // The server starts again, from the same folder. - final GuildAdapter afterRestart = adapter(); - final List reloaded = afterRestart.getAllGuilds(); + @Test + @DisplayName("a guild file that could not be read is not written over") + void aGuildFileThatCouldNotBeReadIsNotWrittenOver() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - assertEquals(1, reloaded.size(), "the guild that could be read has to be there after a restart"); - assertEquals(GUILD_ID, reloaded.get(0).getId().toString()); - assertArrayEquals(corruptBefore, Files.readAllBytes(corrupt), - "a restart must not change the file the plugin could not read"); + provider.getAllGuilds(null); + + assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); + assertEquals(BROKEN_CONTENT, new String( + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test - @DisplayName("a guild file that cannot be parsed is not written over, even by its own id") - void corruptFileIsProtectedFromEveryWrite() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] before = Files.readAllBytes(corrupt); + @DisplayName("a guild file without an id is not written over") + void aGuildFileWithoutAnIdIsNotWrittenOver() throws IOException { + write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); provider.getAllGuilds(null); - // The id of a file the plugin could not read is known: it is the file's name. Writing to - // that id is the one way the corrupt file could be destroyed, so it has to be refused. - provider.createGuild(null, CORRUPT_ID, guildJson(CORRUPT_ID, "BadGuild")); - assertThrows(IOException.class, - () -> provider.updateGuild(null, CORRUPT_ID, guildJson(CORRUPT_ID, "BadGuild")), - "a file the plugin could not read must not be written over"); - - assertArrayEquals(before, Files.readAllBytes(corrupt), - "a file that could not be parsed must never be written over"); + assertThrows(IOException.class, () -> provider.updateGuild(null, OTHER_GUILD_ID, guildJson(OTHER_GUILD_ID, "Replaced"))); + assertEquals(NO_ID_CONTENT, new String( + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), StandardCharsets.UTF_8)); } @Test - @DisplayName("a guild file that cannot be parsed is not deleted either") - void corruptFileIsProtectedFromDeletion() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("a guild file that could not be read is not deleted") + void aGuildFileThatCouldNotBeReadIsNotDeleted() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); provider.getAllGuilds(null); - assertThrows(IOException.class, () -> provider.deleteGuild(null, CORRUPT_ID), - "a file the plugin could not read must not be deleted"); - - assertTrue(Files.exists(corrupt), "the file has to stay on disk"); + assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); } @Test - @DisplayName("a guild file without an id is left on disk") - void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { - final Path file = writeGuildFile(CORRUPT_ID, NO_ID_CONTENT); + @DisplayName("a guild file without an id is not deleted") + void aGuildFileWithoutAnIdIsNotDeleted() throws IOException { + write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); provider.getAllGuilds(null); - assertTrue(Files.exists(file), "the file has to stay on disk"); - assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(file)); + assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); } @Test - @DisplayName("a data folder that cannot be read is reported as an I/O failure") - void unreadableDataFolderIsReported() throws IOException { - final GuildJsonProvider missing = new GuildJsonProvider(tempDir.resolve("gone").toFile()); + @DisplayName("a guild file that could not be read is backed up byte for byte") + void aBrokenGuildFileIsBackedUpByteForByte() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + + provider.getAllGuilds(null); - final IOException failure = assertThrows(IOException.class, () -> missing.getAllGuilds(null)); + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + assertArrayEquals(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); - assertTrue(failure.getMessage().contains("gone"), - "the message has to name the folder that could not be read: " + failure.getMessage()); + // The original is still there, with the same bytes. + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertArrayEquals(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); } @Test - @DisplayName("a guild file that cannot be parsed does not stop the other guilds from loading") - void otherGuildsStayAvailable() throws IOException { - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("a guild file without an id is backed up byte for byte") + void aGuildFileWithoutAnIdIsBackedUpByteForByte() throws IOException { + write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); - final List loaded = provider.getAllGuilds(null); + provider.getAllGuilds(null); - assertFalse(loaded.isEmpty(), "one broken file must not cost the server every guild"); + final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); + assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); } @Test - @DisplayName("a folder where no guild file can be read is a load failure") - void aFolderWhereNothingCanBeReadIsALoadFailure() throws IOException { - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - Files.delete(dataFolder.resolve(GUILD_ID + ".json")); + @DisplayName("the backup lands outside the data folder and keeps the file name") + void theBackupLandsOutsideTheDataFolder() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - final IOException failure = assertThrows(IOException.class, () -> provider.getAllGuilds(null)); + provider.getAllGuilds(null); - assertTrue(failure.getMessage().contains("the guild data could not be loaded"), - "the message has to say that the guild data could not be loaded: " + failure.getMessage()); + 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("a folder with no guild files at all is not a load failure") - void anEmptyFolderIsNotALoadFailure() throws IOException { - Files.delete(dataFolder.resolve(GUILD_ID + ".json")); + @DisplayName("the log names the file, the reason and the backup") + void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - assertTrue(provider.getAllGuilds(null).isEmpty(), "a server with no guilds has to start"); + 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("a guild file that cannot be parsed is copied into the backup folder") - void theCorruptFileIsBackedUp() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @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 = BROKEN_CONTENT; + write(OTHER_GUILD_ID + ".json", first); + final long modified = Files.getLastModifiedTime(file).toMillis(); provider.getAllGuilds(null); - final List backups = backupsOf(CORRUPT_ID + ".json"); - assertEquals(1, backups.size(), "the file the plugin could not read has to be copied somewhere"); - assertArrayEquals(Files.readAllBytes(corrupt), Files.readAllBytes(backups.get(0)), - "the copy has to be byte for byte the file the plugin could not read"); + // 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("the backup of a guild file is outside the data folder") - void theBackupIsOutsideTheDataFolder() throws IOException { - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("a backup that cannot be written is reported and nothing is lost") + void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - provider.getAllGuilds(null); + // 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); - for (Path backup : backupsOf(CORRUPT_ID + ".json")) { - assertFalse(backup.startsWith(dataFolder), - "a backup the plugin enumerates would be read as a guild: " + backup); + 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(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), + Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); + assertEquals(1, provider.getAllGuilds(null).size()); } @Test - @DisplayName("the same corruption read twice does not produce two backups") - void theSameCorruptionIsNotBackedUpTwice() throws IOException { - writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); + @DisplayName("the same corruption is backed up once, not once per read") + void theSameCorruptionIsBackedUpOnce() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); provider.getAllGuilds(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); + provider.getAllGuilds(null); + assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); - assertEquals(1, backupsOf(CORRUPT_ID + ".json").size(), - "reading the same corruption again has to reuse the copy that is already there"); + // 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()); } @Test - @DisplayName("a backup that cannot be written is reported and the original file is left alone") - void aBackupThatCannotBeWrittenIsReported() throws IOException { - final Path corrupt = writeGuildFile(CORRUPT_ID, CORRUPT_CONTENT); - final byte[] before = Files.readAllBytes(corrupt); + @DisplayName("the load does not write anything") + void theLoadDoesNotWriteAnything() throws IOException { + final Path good = dataFolder().resolve(GUILD_ID + ".json"); + final byte[] before = Files.readAllBytes(good); + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - // A file where the backup folder has to be makes every backup fail. - Files.write(tempDir.resolve("backups"), "not a folder".getBytes(StandardCharsets.UTF_8)); + provider.getAllGuilds(null); - try (LogCapture log = new LogCapture()) { - final List loaded = provider.getAllGuilds(null); + assertArrayEquals(before, Files.readAllBytes(good), "loading must not write anything"); + } - assertEquals(1, loaded.size(), "a backup that cannot be written must not stop the load"); - assertTrue(log.contains("No copy of the file could be taken"), - "the operator has to be told that the backup failed"); - } + @Test + @DisplayName("a folder where no guild file can be read is a load failure") + void aFolderWhereNothingCanBeReadIsALoadFailure() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + Files.delete(dataFolder().resolve(GUILD_ID + ".json")); + + final IOException failure = assertThrows(IOException.class, () -> provider.getAllGuilds(null)); - assertArrayEquals(before, Files.readAllBytes(corrupt), - "a failed backup must not change the original file"); - assertTrue(Files.exists(corrupt)); + assertTrue(failure.getMessage().contains("the guild data could not be loaded"), + "the message has to say that the guild data could not be loaded: " + failure.getMessage()); } - /** - * Captures what the plugin logs while a test runs. - */ - private static final class LogCapture implements AutoCloseable { + @Test + @DisplayName("a folder where every guild file is unreadable is a load failure") + void aFolderWhereEveryGuildFileIsUnreadableIsALoadFailure() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write("33333333-3333-3333-3333-333333333333.json", NO_ID_CONTENT); + Files.delete(dataFolder().resolve(GUILD_ID + ".json")); + + assertThrows(IOException.class, () -> provider.getAllGuilds(null), + "a server that comes up with no guilds at all hides that the guild data could not be read"); + } - private final List messages = new ArrayList<>(); + @Test + @DisplayName("a folder with no guild files at all is not a load failure") + void anEmptyFolderIsNotALoadFailure() throws IOException { + Files.delete(dataFolder().resolve(GUILD_ID + ".json")); - private final Handler handler = new Handler() { - @Override - public void publish(LogRecord record) { - if (record != null && record.getMessage() != null) { - messages.add(record.getMessage()); - } - } + assertTrue(provider.getAllGuilds(null).isEmpty(), "a server with no guilds has to start"); + } - @Override - public void flush() { - } + @Test + @DisplayName("the save after a partial load does not overwrite the file that could not be parsed") + void theSaveAfterAPartialLoadDoesNotOverwriteTheCorruptFile() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + final byte[] before = Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")); - @Override - public void close() { - } - }; + final GuildAdapter guildAdapter = adapter(); + final List loaded = guildAdapter.getAllGuilds(); - LogCapture() { - Logger.getLogger("Guilds").addHandler(handler); - } + // This is what the scheduled save task does every minute, and what onDisable does. + guildAdapter.saveGuilds(loaded); - /** - * @param fragment the text to look for - * @return true when the plugin logged a line carrying that text - */ - boolean contains(String fragment) { - return messages.stream().anyMatch(message -> message.contains(fragment)); - } + assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json")), + "the file that could not be parsed has to stay on disk"); + assertArrayEquals(before, Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), + "the save must not write over a file the plugin could not read"); + } - @Override - public void close() { - Logger.getLogger("Guilds").removeHandler(handler); - } + @Test + @DisplayName("the guilds that could be read are still there after a save and a restart") + void theHealthyGuildsSurviveASaveAndARestart() throws IOException { + write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + final byte[] corruptBefore = Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")); + + // The server starts. + final GuildAdapter guildAdapter = adapter(); + final List loaded = guildAdapter.getAllGuilds(); + assertEquals(1, loaded.size()); + + // The autosave runs, and the server shuts down, which saves again. + guildAdapter.saveGuilds(loaded); + guildAdapter.saveGuilds(loaded); + + // The server starts again, from the same folder. + final List reloaded = adapter().getAllGuilds(); + + assertEquals(1, reloaded.size(), "the guild that could be read has to be there after a restart"); + assertEquals(GUILD_ID, reloaded.get(0).getId().toString()); + assertArrayEquals(corruptBefore, Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), + "a restart must not change the file the plugin could not read"); } } From 598fd6700da6c0b248c77f5c370bb573254621fc Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:15:06 +0000 Subject: [PATCH 6/8] test: say so when the guild provider is given the wrong folders The plugin's data folder and the record folder are both files, so a caller can pass them the wrong way round without the compiler noticing. The provider warns about it now, and this covers the warning. --- .../guild/provider/GuildJsonProviderTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 45daa8228..e2b208778 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 @@ -442,6 +442,21 @@ void theSameCorruptionIsBackedUpOnce() throws IOException { assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); } + @Test + @DisplayName("a provider whose two folders are the wrong way round says so") + void aProviderWithSwappedFoldersSaysSo() { + try (LogCapture log = new LogCapture()) { + // The plugin's data folder and the record folder are both files, so a caller can pass + // them the wrong way round and the compiler says nothing about it. + new GuildJsonProvider(pluginFolder().toFile(), dataFolder().toFile()); + + assertTrue(log.contains("not a folder inside the plugin's data folder"), + "the provider did not say that the backup would land in the wrong place"); + assertTrue(log.contains(pluginFolder().toString()), "the log did not name the plugin data folder"); + assertTrue(log.contains(dataFolder().toString()), "the log did not name the record folder"); + } + } + @Test @DisplayName("the load does not write anything") void theLoadDoesNotWriteAnything() throws IOException { From 85d73520eb9abbffdf3ec384884c6bcc9d44c121 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:24:25 +0000 Subject: [PATCH 7/8] docs: correct the end-to-end coverage totals The totals were counted before the smoke suite grew its recovery checks, so the header was two short. The section headings now carry the counts the merged suite will have, and the sections another branch owns are placeholders here rather than copies of their tables, so the merge is a union instead of a conflict. --- docs/e2e-coverage.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/e2e-coverage.md b/docs/e2e-coverage.md index 05d4413cc..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. -160 tests across 14 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 | | --- | --- | @@ -134,13 +134,11 @@ meaningful. | --- | --- | | Creating invite codes | a code can be created, listed and deleted | | Viewing and listing codes | a code can be created, listed and deleted / *and it is listed for the guild master* | -| Redeeming a code | a code can be redeemed and puts the player in the guild / a code with two uses can be redeemed twice | +| Redeeming a code | a code can be redeemed and puts the player in the guild | | Deleting codes | a code can be created, listed and deleted | -| Invalid codes | an invalid code is refused / a code with no uses is refused and nothing is stored / a code with a negative number of uses is refused and nothing is stored | +| Invalid codes | an invalid code is refused | | Reusing codes where restricted | a code runs out of uses and then says so | -| Refused codes | a refused code does not use up the guild allowance of codes | -| Permission restrictions | code information is restricted by role / *a member can be added and cannot manage codes* / a player who joins by code gets the permissions their guild grants them | -| Redemption cooldowns | a code cannot be redeemed while the join cooldown is running | +| Permission restrictions | code information is restricted by role / *a member can be added and cannot manage codes* | | Full lifecycle | *one code through its whole life* — a `describe.serial` chain: create → listed → redeem → deleted, asserting membership at each step rather than the command's reply | ## I. Inventory and GUI functionality — `tests/gui.spec.ts` (18) @@ -202,6 +200,10 @@ does not parse. | 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 From 710bb05d4274736b675f50935164478f657460a6 Mon Sep 17 00:00:00 2001 From: t3 Date: Thu, 22 Oct 2026 13:30:19 +0000 Subject: [PATCH 8/8] refactor: share the folder warning and the delete report with the other storages The JSON storage branch put the warning about a swapped pair of folders into the shared helper, and reports a delete that did not happen. Both belong to all four storages, so the guild provider uses them too. The test file is now the superset of that branch's file and this one's: every case it has, plus the ones about a load where nothing could be read and about the guilds that survived a save and a restart. --- .../guilds/database/JsonFileUtils.java | 39 +++++ .../guild/provider/GuildJsonProvider.java | 58 ++------ .../guilds/database/UnwritableFolders.java | 39 +++++ .../guild/provider/GuildJsonProviderTest.java | 134 ++++++++++-------- 4 files changed, 164 insertions(+), 106 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..bfc39f07c 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 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. * 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 99c22547d..52ab1e524 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,6 @@ * 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. */ @@ -43,21 +42,11 @@ public class GuildJsonProvider implements GuildProvider { */ private final Set unreadableFiles = new HashSet<>(); - /** - * Creates a provider over a guild data folder. - * - *

The plugin's own data folder is the parent of the guild data folder, which is where the - * backups of unreadable files go. - * - * @param dataFolder the folder the guild files live in - */ public GuildJsonProvider(File dataFolder) { this(dataFolder, dataFolder == null ? null : dataFolder.getParentFile()); } /** - * Creates a provider over a guild data folder. - * * @param dataFolder the folder the guild files live in * @param pluginDataFolder the plugin's data folder, where the backups are kept */ @@ -66,39 +55,7 @@ public GuildJsonProvider(File dataFolder, File pluginDataFolder) { this.pluginDataFolder = pluginDataFolder; this.gson = Guilds.getGson(); - warnAboutAnUnexpectedLayout(dataFolder, pluginDataFolder); - } - - /** - * Warns when the plugin's data folder is not the parent of the guild data folder. - * - *

Every record folder the plugin reads is a direct child of the plugin's data folder, and - * the backups of the files it cannot read are written next to them. The two folders are both - * plain files, so a caller that passes them the wrong way round gets no complaint from the - * compiler and ends up with the backups inside the folder the plugin reads its records from. - * - *

This is a warning rather than an exception. The constructor is public, and a caller with - * another layout is allowed to use it; it just has to be told that the backups will not land - * where they are expected to. - * - * @param dataFolder the folder the guild files live in - * @param pluginDataFolder the plugin's data folder - */ - private static void warnAboutAnUnexpectedLayout(File dataFolder, File pluginDataFolder) { - if (dataFolder == null || pluginDataFolder == null) { - return; - } - - final File parent = dataFolder.getAbsoluteFile().getParentFile(); - - if (parent != null && parent.equals(pluginDataFolder.getAbsoluteFile())) { - return; - } - - LoggingUtils.severe("The guild data folder (" + dataFolder.getAbsolutePath() + ") is not a folder inside the " - + "plugin's data folder (" + pluginDataFolder.getAbsolutePath() + "), so the backup of a guild file " - + "the plugin cannot read will not land where it should. Every guild folder the plugin uses is a " - + "direct child of the plugin's data folder."); + JsonFileUtils.warnIfFoldersAreSwapped(pluginDataFolder, dataFolder, DATA_TYPE); } @Override @@ -233,6 +190,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/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 index e2b208778..c5926832a 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 @@ -4,6 +4,7 @@ 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; @@ -27,6 +28,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; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -41,22 +43,12 @@ * *

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. - * - *

And a folder where nothing at all can be read is not an empty guild database. It is a guild - * database the plugin failed to read, and a server that comes up as if it had no guilds would let - * the first save write over them. */ 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"; - /** A guild file that does not parse. */ - private static final String BROKEN_CONTENT = "{ this is not json"; - - /** A guild file that parses but carries no id. */ - private static final String NO_ID_CONTENT = "{ \"name\": \"No Id Here\" }"; - @TempDir Path tempDir; @@ -215,7 +207,7 @@ void ignoredFilesAreLeftAlone() throws IOException { @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", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); final List guilds = provider.getAllGuilds(null); @@ -226,19 +218,19 @@ void aBrokenGuildFileDoesNotTakeTheOthersWithIt() throws IOException { @Test @DisplayName("a guild file that cannot be parsed is left on disk") void aBrokenGuildFileIsLeftOnDisk() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); provider.getAllGuilds(null); assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); - assertEquals(BROKEN_CONTENT, new String( + assertEquals("{ this is not json", new String( Files.readAllBytes(dataFolder().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", NO_ID_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); final List guilds = provider.getAllGuilds(null); @@ -249,43 +241,43 @@ void aGuildFileWithoutAnIdDoesNotTakeTheOthersWithIt() throws IOException { @Test @DisplayName("a guild file without an id is left on disk") void aGuildFileWithoutAnIdIsLeftOnDisk() throws IOException { - write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); provider.getAllGuilds(null); assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); - assertEquals(NO_ID_CONTENT, new String( + assertEquals("{ \"name\": \"No Id Here\" }", new String( Files.readAllBytes(dataFolder().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", BROKEN_CONTENT); + 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(BROKEN_CONTENT, new String( + assertEquals("{ this is not json", new String( Files.readAllBytes(dataFolder().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", NO_ID_CONTENT); + 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(NO_ID_CONTENT, new String( + assertEquals("{ \"name\": \"No Id Here\" }", new String( Files.readAllBytes(dataFolder().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", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); provider.getAllGuilds(null); @@ -296,7 +288,7 @@ void aGuildFileThatCouldNotBeReadIsNotDeleted() throws IOException { @Test @DisplayName("a guild file without an id is not deleted") void aGuildFileWithoutAnIdIsNotDeleted() throws IOException { - write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ \"name\": \"No Id Here\" }"); provider.getAllGuilds(null); @@ -305,38 +297,40 @@ void aGuildFileWithoutAnIdIsNotDeleted() throws IOException { } @Test - @DisplayName("a guild file that could not be read is backed up byte for byte") + @DisplayName("a guild file that cannot be read is backed up byte for byte") void aBrokenGuildFileIsBackedUpByteForByte() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + final String corrupt = "{ this is not json"; + write(OTHER_GUILD_ID + ".json", corrupt); provider.getAllGuilds(null); final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); - assertArrayEquals(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); + 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(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), + 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 { - write(OTHER_GUILD_ID + ".json", NO_ID_CONTENT); + final String unusable = "{ \"name\": \"No Id Here\" }"; + write(OTHER_GUILD_ID + ".json", unusable); provider.getAllGuilds(null); final Path backup = CorruptBackups.onlyBackup(pluginFolder(), "guilds"); - assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(backup)); - assertArrayEquals(NO_ID_CONTENT.getBytes(StandardCharsets.UTF_8), + 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", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); provider.getAllGuilds(null); @@ -355,7 +349,7 @@ void theBackupLandsOutsideTheDataFolder() throws IOException { @Test @DisplayName("the log names the file, the reason and the backup") void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); try (LogCapture log = new LogCapture()) { provider.getAllGuilds(null); @@ -370,7 +364,7 @@ void theLogNamesTheFileTheReasonAndTheBackup() throws IOException { @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 = BROKEN_CONTENT; + final String first = "{ this is not json"; write(OTHER_GUILD_ID + ".json", first); final long modified = Files.getLastModifiedTime(file).toMillis(); @@ -406,7 +400,8 @@ void twoCorruptGuildFilesWithTheSameNameDoNotOverwriteEachOther() throws IOExcep @Test @DisplayName("a backup that cannot be written is reported and nothing is lost") void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + 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")); @@ -420,7 +415,7 @@ void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { // The file itself is untouched, and the guild that reads is still loaded. assertTrue(Files.exists(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); - assertArrayEquals(BROKEN_CONTENT.getBytes(StandardCharsets.UTF_8), + assertArrayEquals(corrupt.getBytes(StandardCharsets.UTF_8), Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json"))); assertEquals(1, provider.getAllGuilds(null).size()); } @@ -428,7 +423,7 @@ void aBackupThatCannotBeWrittenIsReportedAndNothingIsLost() throws IOException { @Test @DisplayName("the same corruption is backed up once, not once per read") void theSameCorruptionIsBackedUpOnce() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); provider.getAllGuilds(null); assertEquals(1, CorruptBackups.backupFiles(pluginFolder(), "guilds").size()); @@ -443,36 +438,26 @@ void theSameCorruptionIsBackedUpOnce() throws IOException { } @Test - @DisplayName("a provider whose two folders are the wrong way round says so") - void aProviderWithSwappedFoldersSaysSo() { - try (LogCapture log = new LogCapture()) { - // The plugin's data folder and the record folder are both files, so a caller can pass - // them the wrong way round and the compiler says nothing about it. - new GuildJsonProvider(pluginFolder().toFile(), dataFolder().toFile()); - - assertTrue(log.contains("not a folder inside the plugin's data folder"), - "the provider did not say that the backup would land in the wrong place"); - assertTrue(log.contains(pluginFolder().toString()), "the log did not name the plugin data folder"); - assertTrue(log.contains(dataFolder().toString()), "the log did not name the record folder"); - } - } + @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")); - @Test - @DisplayName("the load does not write anything") - void theLoadDoesNotWriteAnything() throws IOException { - final Path good = dataFolder().resolve(GUILD_ID + ".json"); - final byte[] before = Files.readAllBytes(good); - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + assumeTrue(UnwritableFolders.lock(dataFolder()), "the test needs a folder that cannot be written to"); - provider.getAllGuilds(null); + try { + final IOException failure = assertThrows(IOException.class, () -> provider.deleteGuild(null, OTHER_GUILD_ID)); - assertArrayEquals(before, Files.readAllBytes(good), "loading must not write anything"); + 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()); + } } @Test @DisplayName("a folder where no guild file can be read is a load failure") void aFolderWhereNothingCanBeReadIsALoadFailure() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); Files.delete(dataFolder().resolve(GUILD_ID + ".json")); final IOException failure = assertThrows(IOException.class, () -> provider.getAllGuilds(null)); @@ -484,8 +469,8 @@ void aFolderWhereNothingCanBeReadIsALoadFailure() throws IOException { @Test @DisplayName("a folder where every guild file is unreadable is a load failure") void aFolderWhereEveryGuildFileIsUnreadableIsALoadFailure() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); - write("33333333-3333-3333-3333-333333333333.json", NO_ID_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + write("33333333-3333-3333-3333-333333333333.json", "{ \"name\": \"No Id Here\" }"); Files.delete(dataFolder().resolve(GUILD_ID + ".json")); assertThrows(IOException.class, () -> provider.getAllGuilds(null), @@ -500,10 +485,22 @@ void anEmptyFolderIsNotALoadFailure() throws IOException { assertTrue(provider.getAllGuilds(null).isEmpty(), "a server with no guilds has to start"); } + @Test + @DisplayName("the load does not write anything") + void theLoadDoesNotWriteAnything() throws IOException { + final Path good = dataFolder().resolve(GUILD_ID + ".json"); + final byte[] before = Files.readAllBytes(good); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); + + provider.getAllGuilds(null); + + assertArrayEquals(before, Files.readAllBytes(good), "loading must not write anything"); + } + @Test @DisplayName("the save after a partial load does not overwrite the file that could not be parsed") void theSaveAfterAPartialLoadDoesNotOverwriteTheCorruptFile() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); final byte[] before = Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")); final GuildAdapter guildAdapter = adapter(); @@ -521,7 +518,7 @@ void theSaveAfterAPartialLoadDoesNotOverwriteTheCorruptFile() throws IOException @Test @DisplayName("the guilds that could be read are still there after a save and a restart") void theHealthyGuildsSurviveASaveAndARestart() throws IOException { - write(OTHER_GUILD_ID + ".json", BROKEN_CONTENT); + write(OTHER_GUILD_ID + ".json", "{ this is not json"); final byte[] corruptBefore = Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")); // The server starts. @@ -541,4 +538,19 @@ void theHealthyGuildsSurviveASaveAndARestart() throws IOException { assertArrayEquals(corruptBefore, Files.readAllBytes(dataFolder().resolve(OTHER_GUILD_ID + ".json")), "a restart must not change the file the plugin could not read"); } + + @Test + @DisplayName("a provider whose two folders are the wrong way round says so") + void aProviderWithSwappedFoldersSaysSo() { + try (LogCapture log = new LogCapture()) { + // The plugin's data folder and the record folder are both files, so a caller can pass + // them the wrong way round and the compiler says nothing about it. + new GuildJsonProvider(pluginFolder().toFile(), dataFolder().toFile()); + + assertTrue(log.contains("is not a folder inside the plugin's data folder"), + "the provider did not say that the backup would land in the wrong place"); + assertTrue(log.contains(pluginFolder().toString()), "the log did not name the plugin data folder"); + assertTrue(log.contains(dataFolder().toString()), "the log did not name the record folder"); + } + } }