diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index fbbf938..01057d1 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -18,7 +18,7 @@ on: jobs: publish: - uses: bentoboxworld/.github/.github/workflows/publish-platforms.yml@fe4b1f03c19f4fd4212020a06a07a7097923adec # master + uses: bentoboxworld/.github/.github/workflows/publish-platforms.yml@1f91a0edf72e8c86d671b3b8fdd3121ac6fb88e1 # master with: use_release_asset: "true" # publish the jar attached to the release; do not rebuild hangar_slug: "InvSwitcher" # blank = skip Hangar diff --git a/README.md b/README.md index 311ee1c..71f8160 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,55 @@ The following are switched per-world: 3. Done! 4. (Optional) If you would prefer to have achievements not broadcasted in your server chat when players change worlds, run the command `/gamerule announceAdvancements false` in-game, or in your server console by removing the "/" symbol. +## Do not run two inventory managers + +InvSwitcher must be the only plugin managing per-world inventories. Running it alongside +Multiverse-Inventories, PerWorldInventory, MultiInv or similar makes both plugins save and restore +the player on every world change, and they overwrite each other's data. + +The symptom is disappearing items: pick up an item on your island, go to the lobby, come back, and +the island inventory is empty. Nothing appears in the console, because neither plugin is failing — +each is faithfully saving a player state the other has already rewritten. The InvSwitcher version +makes no difference, so if you are seeing this, look for a second inventory plugin first. + +### Multiverse-Inventories + +Two things commonly mislead admins: + +* **Leaving the BentoBox worlds out of every inventory group does not help.** Groups control which + worlds *share* an inventory, not which worlds Multiverse-Inventories handles. It still writes a + per-world profile for a world that is in no group. +* **`/mv remove ` does not help either.** Multiverse-Core re-registers BentoBox worlds as + they are created, so the removal is undone on the next restart. `auto-import-3rd-party-worlds: + false` does not prevent it — that only suppresses the import sweep run when Multiverse-Core + starts, which is before BentoBox has created its worlds. + +Multiverse-Inventories has no config option to ignore a world, but it does have a bypass +permission. Enable it in the Multiverse-Inventories `config.yml` (it ships as `false`): + +```yml +share-handling: + enable-bypass-permissions: true +``` + +Then grant `mvinv.bypass.world.` for each BentoBox world — including its nether and end — to +every player. With LuckPerms: + +``` +lp group default permission set mvinv.bypass.world.bskyblock_world true +lp group default permission set mvinv.bypass.world.bskyblock_world_nether true +lp group default permission set mvinv.bypass.world.bskyblock_world_the_end true +``` + +Setting the nodes on a group every player inherits (such as `default`) covers new players +automatically. Repeat for each game mode world you run. + +**Operators do not get this permission automatically** — it has to be granted explicitly. Testing +as an op without it looks exactly like the fix not working. + +Use one node per world. Avoid `mvinv.bypass.world.*`, which switches Multiverse-Inventories off for +every world, including the ones you still want it to manage. + ## Config.yml The config allows to define which worlds that InvSwitcher should operate, and what aspects should be kept separate. diff --git a/pom.xml b/pom.xml index 4767d14..0204a31 100644 --- a/pom.xml +++ b/pom.xml @@ -65,7 +65,7 @@ -LOCAL - 1.19.1 + 1.19.2 BentoBoxWorld_addon-invSwitcher bentobox-world @@ -323,7 +323,7 @@ org.jacoco jacoco-maven-plugin - 0.8.12 + 0.8.15 true diff --git a/src/main/java/com/wasteofplastic/invswitcher/Store.java b/src/main/java/com/wasteofplastic/invswitcher/Store.java index a5c69f1..52b41c8 100644 --- a/src/main/java/com/wasteofplastic/invswitcher/Store.java +++ b/src/main/java/com/wasteofplastic/invswitcher/Store.java @@ -474,10 +474,36 @@ public void storeAndSave(Player player, World world, boolean shutdown) { } if (settings.isStatistics()) { String k = settings.isIslandsStatistics() ? islandKey : worldKey; - saveStats(store, player, k, shutdown).thenAccept(database::saveObjectAsync); + // On shutdown saveStats() gathers synchronously and returns an already-completed + // future, so thenAccept runs on this thread and persist() writes before we return. + saveStats(store, player, k, shutdown).thenAccept(s -> persist(s, shutdown)); return; } - database.saveObjectAsync(store); + persist(store, shutdown); + } + + /** + * Writes the store to the database, synchronously when the server is shutting down. + *

+ * Saves are normally asynchronous, but a shutdown save must not be. BentoBox closes its + * database immediately after addons are disabled, and players are only kicked afterwards, so an + * asynchronous write issued from {@link #saveOnShutdown()} loses the race and is silently + * dropped — and the {@code PlayerQuitEvent} that would otherwise save them fires after the + * database is already closed. + *

+ * The effect was that everything a player did since their last world change went unsaved when + * the server stopped. Because {@code PlayerListener.onPlayerJoin} re-applies the stored + * inventory on login, the stale snapshot then overwrote the player's real inventory and they + * were rolled back to their last world change. + * @param store - the store to write + * @param shutdown - true if this is a shutdown save, which must be synchronous + */ + private void persist(InventoryStorage store, boolean shutdown) { + if (shutdown) { + database.saveObject(store); + } else { + database.saveObjectAsync(store); + } } private CompletableFuture saveStats(InventoryStorage store, Player player, String worldName, diff --git a/src/main/resources/addon.yml b/src/main/resources/addon.yml index 5cfe563..e681a54 100755 --- a/src/main/resources/addon.yml +++ b/src/main/resources/addon.yml @@ -5,4 +5,30 @@ api-version: 3.17.0 authors: tastybento -softdepend: AcidIsland, BSkyBlock, SkyGrid, CaveBock, AOneBlock \ No newline at end of file +softdepend: AcidIsland, BSkyBlock, SkyGrid, CaveBock, AOneBlock + +# Economy command permissions. These are only usable if options.money is enabled and Vault is +# installed - InvSwitcher does not register the commands otherwise. The [gamemode] placeholder is +# expanded by BentoBox into every game mode's permission prefix, e.g. bskyblock.invswitcher.balance +permissions: + '[gamemode].invswitcher.balance': + description: Player can use the balance command + default: true + '[gamemode].invswitcher.pay': + description: Player can use the pay command + default: true + '[gamemode].invswitcher.admin.eco': + description: Player can use the admin eco command + default: op + '[gamemode].invswitcher.admin.eco.balance': + description: Player can use the admin eco balance command + default: op + '[gamemode].invswitcher.admin.eco.give': + description: Player can use the admin eco give command + default: op + '[gamemode].invswitcher.admin.eco.take': + description: Player can use the admin eco take command + default: op + '[gamemode].invswitcher.admin.eco.set': + description: Player can use the admin eco set command + default: op diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index e04efdd..ec4a626 100755 --- a/src/main/resources/config.yml +++ b/src/main/resources/config.yml @@ -9,6 +9,11 @@ worlds: - caveblock-world - poseidon_world - stranger_world +- skygrid-world +- raft_world +- brix_world +- parkour_world +- tradewinds_world options: # # Per-world settings. Gamemode means Survivial, Creative, etc. diff --git a/src/test/java/com/wasteofplastic/invswitcher/StoreTest.java b/src/test/java/com/wasteofplastic/invswitcher/StoreTest.java index b46eea3..de7b57b 100644 --- a/src/test/java/com/wasteofplastic/invswitcher/StoreTest.java +++ b/src/test/java/com/wasteofplastic/invswitcher/StoreTest.java @@ -18,6 +18,7 @@ import static org.mockito.Mockito.when; import java.io.File; +import java.lang.reflect.Field; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -54,6 +55,7 @@ import org.mockito.quality.Strictness; import world.bentobox.bentobox.BentoBox; +import world.bentobox.bentobox.database.Database; import world.bentobox.bentobox.database.DatabaseSetup.DatabaseType; import com.wasteofplastic.invswitcher.dataobjects.InventoryStorage; @@ -905,4 +907,74 @@ void testGetStorageKeyForEventMultipleIslandsNotOwner() { } } + // --- Shutdown save tests --- + + /** + * Replaces the Store's database with a mock so the save path can be observed. + */ + @SuppressWarnings("unchecked") + private Database injectMockDatabase() throws Exception { + Database db = mock(Database.class); + Field field = Store.class.getDeclaredField("database"); + field.setAccessible(true); + field.set(s, db); + return db; + } + + /** + * A shutdown save must be synchronous. BentoBox closes its database immediately after addons + * are disabled, and players are only kicked afterwards, so an asynchronous write issued from + * saveOnShutdown() loses the race and is silently dropped. Everything the player did since + * their last world change was then lost, and because onPlayerJoin re-applies the stored + * inventory on login, the stale snapshot overwrote their real inventory on the next restart. + */ + @Test + void testShutdownSaveIsSynchronous() throws Exception { + sets.setStatistics(false); + sets.setAdvancements(false); + Database db = injectMockDatabase(); + + try (MockedStatic mockedBukkit = mockStatic(Bukkit.class, Mockito.RETURNS_MOCKS)) { + s.storeAndSave(player, world, true); + } + + verify(db).saveObject(any(InventoryStorage.class)); + verify(db, never()).saveObjectAsync(any(InventoryStorage.class)); + } + + /** + * The statistics branch returns early, so it needs its own check that a shutdown save is + * written synchronously. + */ + @Test + void testShutdownSaveIsSynchronousWithStatistics() throws Exception { + sets.setStatistics(true); + sets.setAdvancements(false); + Database db = injectMockDatabase(); + + try (MockedStatic mockedBukkit = mockStatic(Bukkit.class, Mockito.RETURNS_MOCKS)) { + s.storeAndSave(player, world, true); + } + + verify(db).saveObject(any(InventoryStorage.class)); + verify(db, never()).saveObjectAsync(any(InventoryStorage.class)); + } + + /** + * Normal (non-shutdown) saves must stay asynchronous so they do not block the main thread. + */ + @Test + void testNormalSaveStaysAsynchronous() throws Exception { + sets.setStatistics(false); + sets.setAdvancements(false); + Database db = injectMockDatabase(); + + try (MockedStatic mockedBukkit = mockStatic(Bukkit.class, Mockito.RETURNS_MOCKS)) { + s.storeAndSave(player, world, false); + } + + verify(db).saveObjectAsync(any(InventoryStorage.class)); + verify(db, never()).saveObject(any(InventoryStorage.class)); + } + }