[darbyjack]

A guild vault is stored in the guild file as a JSON document. A vault that cannot be read used to disappear from the in-memory cache, and that had two consequences.

The cache is positional and getGuildVault reads it by position. With one unreadable vault in a guild that had three, the cache held two entries, so the player who opened vault 2 was served the contents of vault 3, and vault 3 was out of range.

The save compared the length of the cache with the length of the stored list and gave up when they differed. One unreadable vault meant they always differed, so nothing was written for that guild again, on any autosave and on shutdown. Everything a player put into any of that guild's vaults was lost on the next restart. The same check meant a vault a player opened for the first time was never stored either.

Reading a vault had the same problem one level down. deserializeInventory parsed the slot key with Integer.parseInt and set the item without a bounds check, and it cast the stored value to an ItemStack without checking what it was. A slot key that was not a number threw a NumberFormatException, a slot outside the inventory threw an ArrayIndexOutOfBoundsException, and a value that was not an item threw a ClassCastException. Only InvalidConfigurationException was caught, so all three escaped the deserializer and out of the guild load.

The hole in reading a vault one item at a time

Reading a vault one item at a time has a second half to it. An item the running server cannot make sense of, a material that only exists on 1.8.8 for instance, costs that item and not the vault, and the inventory that comes back is missing it. That inventory was what the save wrote, so the item was gone from the guild file for good. The player was left with a vault that had a hole in it, and the next save wrote that hole over the data that was still in the file.

The read now reports the slots it could not reconstruct, and a vault that was not read in full keeps its stored data. Such a vault is not opened: an inventory without the items that were left out looks complete to the player who is handed it, and the stored data is the only copy of those items. The player is told that the vault stays closed and why.

The file a guild is stored in is copied aside when one of its vaults could not be read, so the data that could not be read is in more than one place. The file itself stays where it was. A piece of damage is copied once: the identity of the damage is the stored data of the vaults that could not be read, so a guild file that a save rewrote because something else in the guild changed is not copied again, while a vault that holds a different unreadable item is a different piece of damage and gets its own copy.

What changed

• The cache has an entry for every stored vault. An unreadable one gets an empty vault in its place, so vault N is vault N. The positions of the unreadable vaults are remembered in memory only, so nothing is added to the stored format.
• The save writes the cache back position by position. A vault that could not be read in full keeps the data it has, a stored vault the cache does not have is kept, and a vault the player opened for the first time is added.
• Reading a vault is per item now. A slot key that is not a number, a slot outside the inventory, and a value that is not an item are skipped and the rest of the vault is read. An item the running server version cannot make sense of, such as a material that only exists on 1.8.8, is skipped the same way, and one item no longer costs the other 53 slots.
• Serialization.readInventory reports what the document held: the inventory that could be read and the slots that were left out. deserializeInventory keeps its old contract and returns nothing unless the whole document could be read, so no caller can be handed an inventory with a hole in it.
• A vault that could not be read in full is not opened. getGuildVault returns nothing for it, and the new overload that takes the player tells the player why the vault stays closed.
• The guild file is copied into plugins/Guilds/backups/corrupt/<timestamp>/guilds/ when one of its vaults could not be read, with the console saying where the copy is. The file is copied byte for byte and is left where it was.

The stored format does not change. A vault is a size and an items section keyed by the slot, the shape every guild file on disk already has, and serializeInventory is untouched. A test pins that shape.

The new message

vaults.unreadable is a new key, and it is in en-US.yml only. The other locale files are Crowdin-managed, so they pick the key up on the next Crowdin sync. Until then ACF resolves the key against the manager's default locale, which the plugin takes from settings.messagesLanguage and which defaults to en-US, so the message is never shown as a raw key.

Tests added

• src/test/java/me/glaremasters/guilds/utils/SerializationTest.java: the round trip, the stored shape, a slot key that is not a number, a slot outside the inventory, a value that is not an item, an item with an unknown material, an item with meta that cannot be read, and two documents that are not a document. The four cases that lose an item also check that the read reports the slot it could not reconstruct and that deserializeInventory hands out nothing.
• src/test/java/me/glaremasters/guilds/guild/GuildVaultTest.java: a corrupt vault in the middle keeps the other vaults at their positions, the vault a player asks for is the vault they get, the data of an unreadable vault survives a save, the other vaults of a guild are saved anyway, a vault opened for the first time is stored, a stored vault the cache does not have is not dropped, an item that cannot be read stays in the stored data, the stored data survives a save, a reload and a second save, a vault with an unreadable item is not opened, the player is told why, the vault GUI refuses it, the other vaults of the same guild still open and still save, the guild file is copied byte for byte, the same damage is not copied again when the file is rewritten, a different piece of damage is copied again, a vault number outside the guild's vaults still fails the way it always did, and a guild whose vaults are all healthy is not affected at all.

The tests were run against the code before the fix first, and the failures are the ones described above: the cache held 2 entries where 3 were stored, vault 2 served vault 3's items, the item a player put into vault 1 was gone after the save, a vault opened for the first time was never stored, the item that could not be read was gone from the stored vault after the next save, and a guild file that was rewritten while the damage stayed the same was copied a second time.

Test results

./gradlew test testJava11 spotlessCheck   BUILD SUCCESSFUL
./gradlew check                           BUILD SUCCESSFUL   (2 Material constants checked against the 1.8.8 API)
./gradlew plugwrightTest                  144 passed, 0 failed, 0 skipped

The unit suite covers the 19 vault tests and the 9 serialization tests, and the whole existing suite still passes on Java 11 and on the current JVM.

Compatibility

• The on-disk format is unchanged, so guild files written by any version still load.
• No new Bukkit API is used. net.minidev:json-smart is the parser the bundled configuration library already used, and it is now declared in the version catalog instead of coming in transitively, so the vault code can read a document with it directly. It is bundled in the plugin jar, so it does not depend on the server version.
• The material of an item is never named in the code, so the 1.13 flattening and the 1.8.8 material list are untouched. The legacy Material probe in check passes.
• Java 11 bytecode, as before.
• getGuildVault(guild, vault) returns nothing for a vault that could not be read in full. A vault number that is not one of the guild's vaults still throws, exactly as it did before, and a guild that has no vault cache still fails the way it did.
• GuildsAPI.getGuildVault hands back what the handler hands back, so it hands back nothing for a vault that could not be read in full. Its Kotlin signature still says Inventory, so a Kotlin caller of the API has to be ready for that.

The path of the guild file

GuildHandler.guildDataFile rebuilds the path of the file a guild is stored in, <plugin folder>/data/<guild id>.json, because the backup needs the file and the storage provider does not hand it back. The duplication stands until the provider grows a method that returns the file for a guild, and the handler should use that method instead of rebuilding the path when it does.

Limitations

• A vault that could not be read in full opens for nobody, including the guild master and the admins. Recovering the items in it needs a server version that can read them, or a hand edit of the guild file.
• When the guilds are stored in a database rather than in files there is no file to copy, and the console says that the copy could not be made. The stored vault data is still left untouched.
• The end-to-end suite could not cover the persistence part of this. It runs against a live server that keeps the guild in memory, and it cannot restart the server or write a corrupt guild file, so the behaviour that was broken here is covered by the unit tests only.