Fixes #4123 - Coal Generator will no longer be locked after researching (#4124)

Due to a logic bug in the Legacy storage backend if there was a duplicate ID it would mark it as researched for the first, then see it researched already and remove it on the second. This was happening for the Coal Generator and Bio Reactor here. Both shared he same research ID 173.

We're just doing this fix for now until we can move away from the legacy backend (work in progress).

pom.xml

@@ -406,8 +406,21 @@
                     <groupId>org.jetbrains</groupId>
                     <artifactId>annotations</artifactId>
                 </exclusion>
+                <exclusion>
+                    <groupId>io.papermc.paper</groupId>
+                    <artifactId>paper-api</artifactId>
+                </exclusion>
             </exclusions>
         </dependency>
+        <!-- Override MockBukkit's Paper to a pinned slightly older version -->
+        <!-- This is because MockBukkit currently fails after this PR: -->
+        <!-- https://github.com/PaperMC/Paper/pull/9629 -->
+        <dependency>
+            <groupId>io.papermc.paper</groupId>
+            <artifactId>paper-api</artifactId>
+            <version>1.20.4-R0.1-20240205.114523-90</version>
+            <scope>test</scope>
+        </dependency>
 
         <!-- Third party plugin integrations / soft dependencies -->
         <dependency>

src/main/java/io/github/thebusybiscuit/slimefun4/storage/backend/legacy/LegacyStorage.java

@@ -91,7 +91,17 @@ public class LegacyStorage implements Storage {
                 playerFile.setValue("researches." + research.getID(), true);
 
             // Remove the research if it's no longer researched
-            } else if (playerFile.contains("researches." + research.getID())) {
+            // ----
+            // We have a duplicate ID (173) used for both Coal Gen and Bio Reactor
+            // If you researched the Goal Gen we would remove it on save if you didn't also have the Bio Reactor
+            // Due to the fact we would set it as researched (true in the branch above) on Coal Gen
+            // but then go into this branch and remove it if you didn't have Bio Reactor
+            // Sooooo we're gonna hack this for now while we move away from the Legacy Storage
+            // Let's make sure the user doesn't have _any_ research with this ID and _then_ remove it
+            } else if (
+                playerFile.contains("researches." + research.getID())
+                && !data.getResearches().stream().anyMatch((r) -> r.getID() == research.getID())
+            ) {
                 playerFile.setValue("researches." + research.getID(), null);
             }
         }

src/test/java/io/github/thebusybiscuit/slimefun4/storage/backend/TestLegacyBackend.java

@@ -17,6 +17,7 @@ import org.bukkit.configuration.serialization.ConfigurationSerialization;
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.meta.ItemMeta;
 import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -52,8 +53,6 @@ class TestLegacyBackend {
         // within the class isn't being fired (where ItemStack and other classes are registered)
         ConfigurationSerialization.registerClass(ItemStack.class);
         ConfigurationSerialization.registerClass(ItemMeta.class);
-
-        setupResearches();
     }
 
     @AfterAll
@@ -62,9 +61,16 @@ class TestLegacyBackend {
         FileUtils.deleteDirectory(new File("data-storage"));
     }
 
+    @AfterEach
+    public void cleanup() {
+        Slimefun.getRegistry().getResearches().clear();
+    }
+
     // Test simple loading and saving of player data
     @Test
     void testLoadingResearches() throws IOException {
+        setupResearches();
+
         // Create a player file which we can load
         UUID uuid = UUID.randomUUID();
         File playerFile = new File("data-storage/Slimefun/Players/" + uuid + ".yml");
@@ -184,6 +190,8 @@ class TestLegacyBackend {
 
     @Test
     void testSavingResearches() throws InterruptedException {
+        setupResearches();
+
         // Create a player file which we can load
         UUID uuid = UUID.randomUUID();
         File playerFile = new File("data-storage/Slimefun/Players/" + uuid + ".yml");
@@ -279,6 +287,8 @@ class TestLegacyBackend {
     // Test realistic situations
     @Test
     void testResearchChanges() throws InterruptedException {
+        setupResearches();
+
         UUID uuid = UUID.randomUUID();
         File playerFile = new File("data-storage/Slimefun/Players/" + uuid + ".yml");
 
@@ -372,6 +382,41 @@ class TestLegacyBackend {
         Assertions.assertEquals(1, assertion.getWaypoints().size());
     }
 
+    @Test
+    void testDuplicateResearchesDontGetUnResearched() throws InterruptedException {
+        // Create a player file which we can load
+        UUID uuid = UUID.randomUUID();
+        File playerFile = new File("data-storage/Slimefun/Players/" + uuid + ".yml");
+
+        OfflinePlayer player = Bukkit.getOfflinePlayer(uuid);
+        PlayerProfile profile = TestUtilities.awaitProfile(player);
+
+        // Setup initial research
+        NamespacedKey initialKey = new NamespacedKey(plugin, "test_1");
+        Research initialResearch = new Research(initialKey, 1, "Test 1", 100);
+        initialResearch.register();
+
+        // Setup duplicate research
+        // Keep the ID as 1 but change name and key
+        NamespacedKey duplicateKey = new NamespacedKey(plugin, "test_2");
+        Research duplicateResearch = new Research(duplicateKey, 1, "Test 2", 100);
+        duplicateResearch.register();
+
+        profile.setResearched(initialResearch, true);
+
+        // Save the player data
+        LegacyStorage storage = new LegacyStorage();
+        storage.savePlayerData(uuid, profile.getPlayerData());
+
+        // Assert the file exists and data is correct
+        Assertions.assertTrue(playerFile.exists());
+        PlayerData assertion = storage.loadPlayerData(uuid);
+        // Will have both the initial and duplicate research
+        Assertions.assertEquals(2, assertion.getResearches().size());
+        Assertions.assertTrue(assertion.getResearches().contains(initialResearch));
+        Assertions.assertTrue(assertion.getResearches().contains(duplicateResearch));
+    }
+    
     // Utils
     private static void setupResearches() {
         for (int i = 0; i < 10; i++) {