Fix ArrayIndexOutOfBounds for certain events due to spigot API bug

Fixes #4488

Changelog.txt

@@ -1,5 +1,6 @@
 Version 2.1.189
     FlatFileDB now stores the last login of users again (was completely non functional for a while)
+    mcMMO will once again purge old users if the config option is on (see notes)
     Newly created flat file databases (mcmmo.users file) will have a comment line at the top noting the date the database was created
     Fixed a bug where FlatFileDatabase users could have their names saved as "null" (names will be fixed the next time the player logs in)
     Rewrote how FlatFileDatabase verifies data integrity
@@ -24,7 +25,9 @@ Version 2.1.189
     (API) Some members of PrimarySkillType were removed and not deprecated (such as the field constants)
 
     NOTES:
-    I spent over 20 hours refactoring FlatFileDB and writing unit tests for it, this will ensure that any changes in the code that could break the database are caught
+    Regarding purging old users on the FlatFileDB, since this wasn't functioning for a while, the last login of users has been reset and if mcMMO hasn't seen that user since this update, it won't purge them as it has no way to know if they are truly an old user
+    I'm likely going to add SQLite DB as an option in the future, I spent time to fix up the FlatFileDB as some Unit Testing practice.
+    I spent over 26 hours refactoring FlatFileDB and writing unit tests for it, this will ensure that any changes in the code that could break the database are caught
     Ultra Permissions is SAFE to use with mcMMO
     After getting in contact with the UltraPermissions devs and exhaustive testing, I have concluded that using UltraPermissions is completely safe with mcMMO. The users who had an issue with performance currently have an unknown cause, potentially it is from a plugin using the UltraPermissions API I really can't say without more data. My apologies to the UltraPermissions team for reporting an issue between our two plugins directly, as that is not the case. I would have tested it myself sooner but UltraPermissions was closed source and premium so I wasn't particularly motivated to do so, however I have been given access to the binaries so now I can do all the testing I want if future issues ever arise which I have zero expectations that they will.
 

src/main/java/com/gmail/nossr50/listeners/BlockListener.java

@@ -20,6 +20,7 @@ import com.gmail.nossr50.skills.repair.Repair;
 import com.gmail.nossr50.skills.salvage.Salvage;
 import com.gmail.nossr50.skills.woodcutting.WoodcuttingManager;
 import com.gmail.nossr50.util.*;
+import com.gmail.nossr50.util.compat.layers.world.WorldCompatibilityLayer;
 import com.gmail.nossr50.util.player.UserManager;
 import com.gmail.nossr50.util.skills.SkillUtils;
 import com.gmail.nossr50.util.sounds.SoundManager;
@@ -147,13 +148,21 @@ public class BlockListener implements Listener {
         // Get opposite direction so we get correct block
         BlockFace direction = event.getDirection();
         Block movedBlock = event.getBlock().getRelative(direction);
-        if (movedBlock.getY() >= Misc.getWorldMinCompat(movedBlock.getWorld())) // Very weird that the event is giving us these, they shouldn't exist
+
+        WorldCompatibilityLayer worldCompatibilityLayer = mcMMO.getCompatibilityManager().getWorldCompatibilityLayer();
+
+        World world = movedBlock.getWorld();
+
+        //Spigot makes bad things happen in its API
+        if(event.getBlock().getY() < worldCompatibilityLayer.getMaxWorldHeight(world) || event.getBlock().getY() >= worldCompatibilityLayer.getMinWorldHeight(world)) {
             mcMMO.getPlaceStore().setTrue(movedBlock);
+        }
 
         for (Block block : event.getBlocks()) {
-            movedBlock = block.getRelative(direction);
+            if(block.getY() < worldCompatibilityLayer.getMaxWorldHeight(world) || block.getY() >= worldCompatibilityLayer.getMinWorldHeight(world)) {
-            if (movedBlock.getY() < Misc.getWorldMinCompat(movedBlock.getWorld())) // Very weird that the event is giving us these, they shouldn't exist
+                mcMMO.getPlaceStore().setTrue(block.getRelative(direction));
-                continue;
+            }
+
             mcMMO.getPlaceStore().setTrue(movedBlock);
         }
     }
@@ -185,13 +194,21 @@ public class BlockListener implements Listener {
     @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
     public void onBlockFormEvent(BlockFormEvent event)
     {
-        /* WORLD BLACKLIST CHECK */
+        World world = event.getBlock().getWorld();
-        if(WorldBlacklist.isWorldBlacklisted(event.getBlock().getWorld()))
+        /* WORLD BLACKLIST CHECK */ {
-            return;
+            if(WorldBlacklist.isWorldBlacklisted(world))
+                return;
+        }
 
         BlockState newState = event.getNewState();
 
         if(ExperienceConfig.getInstance().preventStoneLavaFarming()) {
+            WorldCompatibilityLayer worldCompatibilityLayer = mcMMO.getCompatibilityManager().getWorldCompatibilityLayer();
+
+            if(event.getBlock().getY() > worldCompatibilityLayer.getMaxWorldHeight(world) || event.getBlock().getY() < worldCompatibilityLayer.getMinWorldHeight(world)) {
+                return;
+            }
+
             if(newState.getType() != Material.OBSIDIAN
                     && ExperienceConfig.getInstance().doesBlockGiveSkillXP(PrimarySkillType.MINING, newState.getBlockData())) {
                 mcMMO.getPlaceStore().setTrue(newState);

src/main/java/com/gmail/nossr50/util/Misc.java

@@ -260,12 +260,6 @@ public final class Misc {
         }
     }
 
-    public static int getWorldMinCompat(World world)
-    {
-        // TODO this method should access the world min variable in a version safe manner so that we don't restrict usage to new versions of spigot only
-        return 0;
-    }
-
     public static void printProgress(int convertedUsers, int progressInterval, long startMillis) {
         if ((convertedUsers % progressInterval) == 0) {
             mcMMO.p.getLogger().info(String.format("Conversion progress: %d users at %.2f users/second", convertedUsers, convertedUsers / (double) ((System.currentTimeMillis() - startMillis) / TIME_CONVERSION_FACTOR)));

src/main/java/com/gmail/nossr50/util/blockmeta/BitSetChunkStore.java

@@ -1,5 +1,6 @@
 package com.gmail.nossr50.util.blockmeta;
 
+import com.gmail.nossr50.mcMMO;
 import com.gmail.nossr50.util.Misc;
 import org.bukkit.Bukkit;
 import org.bukkit.World;
@@ -25,7 +26,7 @@ public class BitSetChunkStore implements ChunkStore {
     private transient boolean dirty = false;
 
     public BitSetChunkStore(@NotNull World world, int cx, int cz) {
-        this(world.getUID(), Misc.getWorldMinCompat(world), world.getMaxHeight(), cx, cz);
+        this(world.getUID(), mcMMO.getCompatibilityManager().getWorldCompatibilityLayer().getMinWorldHeight(world), world.getMaxHeight(), cx, cz);
     }
 
     private BitSetChunkStore(@NotNull UUID worldUid, int worldMin, int worldMax, int cx, int cz) {
@@ -109,15 +110,14 @@ public class BitSetChunkStore implements ChunkStore {
         return (z * 16 + x) + (256 * (y + yOffset));
     }
 
-    private static int getWorldMin(@NotNull UUID worldUid, int storedWorldMin)
+    private static int getWorldMin(@NotNull UUID worldUid, int storedWorldMin) {
-    {
         World world = Bukkit.getWorld(worldUid);
 
         // Not sure how this case could come up, but might as well handle it gracefully.  Loading a chunkstore for an unloaded world?
         if (world == null)
             return storedWorldMin;
 
-        return Misc.getWorldMinCompat(world);
+        return mcMMO.getCompatibilityManager().getWorldCompatibilityLayer().getMinWorldHeight(world);
     }
 
     private static int getWorldMax(@NotNull UUID worldUid, int storedWorldMax)

src/main/java/com/gmail/nossr50/util/compat/CompatibilityLayer.java

@@ -8,5 +8,5 @@ public interface CompatibilityLayer {
      * Whether or not this CompatibilityLayer successfully initialized and in theory should be functional
      * @return true if this CompatibilityLayer is functional
      */
-    boolean noErrorsOnInitialize();
+    default boolean noErrorsOnInitialize() { return true; };
 }

src/main/java/com/gmail/nossr50/util/compat/CompatibilityManager.java

@@ -10,9 +10,12 @@ import com.gmail.nossr50.util.compat.layers.persistentdata.SpigotPersistentDataL
 import com.gmail.nossr50.util.compat.layers.persistentdata.SpigotPersistentDataLayer_1_14;
 import com.gmail.nossr50.util.compat.layers.skills.AbstractMasterAnglerCompatibility;
 import com.gmail.nossr50.util.compat.layers.skills.MasterAnglerCompatibilityLayer;
+import com.gmail.nossr50.util.compat.layers.world.WorldCompatibilityLayer;
+import com.gmail.nossr50.util.compat.layers.world.WorldCompatibilityLayer_1_16_4;
 import com.gmail.nossr50.util.nms.NMSVersion;
 import com.gmail.nossr50.util.platform.MinecraftGameVersion;
 import com.gmail.nossr50.util.text.StringUtils;
+import org.bukkit.World;
 import org.bukkit.command.CommandSender;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -37,6 +40,7 @@ public class CompatibilityManager {
     private AbstractPersistentDataLayer persistentDataLayer;
     private AbstractBungeeSerializerCompatibilityLayer bungeeSerializerCompatibilityLayer;
     private AbstractMasterAnglerCompatibility masterAnglerCompatibility;
+    private WorldCompatibilityLayer worldCompatibilityLayer;
 
     public CompatibilityManager(MinecraftGameVersion minecraftGameVersion) {
         mcMMO.p.getLogger().info("Loading compatibility layers...");
@@ -67,10 +71,31 @@ public class CompatibilityManager {
         initPersistentDataLayer();
         initBungeeSerializerLayer();
         initMasterAnglerLayer();
+        initWorldCompatibilityLayer();
 
         isFullyCompatibleServerSoftware = true;
     }
 
+    private void initWorldCompatibilityLayer() {
+        if(minecraftGameVersion.getMinorVersion().asInt() >= 16 && minecraftGameVersion.getPatchVersion().asInt() >= 4 || minecraftGameVersion.getMajorVersion().asInt() >= 2) {
+            if(hasNewWorldMinHeightAPI()) {
+                worldCompatibilityLayer = new WorldCompatibilityLayer_1_16_4();
+            }
+        } else {
+            worldCompatibilityLayer = new WorldCompatibilityLayer() {
+                @Override
+                public int getMinWorldHeight(@NotNull World world) {
+                    return WorldCompatibilityLayer.super.getMinWorldHeight(world);
+                }
+
+                @Override
+                public int getMaxWorldHeight(@NotNull World world) {
+                    return WorldCompatibilityLayer.super.getMaxWorldHeight(world);
+                }
+            };
+        }
+    }
+
     private void initMasterAnglerLayer() {
         if(minecraftGameVersion.getMinorVersion().asInt() >= 16 || minecraftGameVersion.getMajorVersion().asInt() >= 2) {
             if(hasNewFishingHookAPI()) {
@@ -81,6 +106,16 @@ public class CompatibilityManager {
         }
     }
 
+    private boolean hasNewWorldMinHeightAPI() {
+        try {
+            Class<?> checkForClass = Class.forName("org.bukkit.World");
+            checkForClass.getMethod("getMinHeight");
+            return true;
+        } catch (ClassNotFoundException | NoSuchMethodException e) {
+            return false;
+        }
+    }
+
     private boolean hasNewFishingHookAPI() {
         try {
             Class<?> checkForClass = Class.forName("org.bukkit.entity.FishHook");
@@ -182,4 +217,8 @@ public class CompatibilityManager {
     public @Nullable AbstractMasterAnglerCompatibility getMasterAnglerCompatibilityLayer() {
         return masterAnglerCompatibility;
     }
+
+    public @NotNull WorldCompatibilityLayer getWorldCompatibilityLayer() {
+        return worldCompatibilityLayer;
+    }
 }

src/main/java/com/gmail/nossr50/util/compat/layers/world/WorldCompatibilityLayer.java

@@ -0,0 +1,11 @@
+package com.gmail.nossr50.util.compat.layers.world;
+
+import com.gmail.nossr50.util.compat.CompatibilityLayer;
+import org.bukkit.World;
+import org.jetbrains.annotations.NotNull;
+
+public interface WorldCompatibilityLayer extends CompatibilityLayer {
+    default int getMinWorldHeight(@NotNull World world) { return 0; }
+
+    default int getMaxWorldHeight(@NotNull World world) { return 255; }
+}

src/main/java/com/gmail/nossr50/util/compat/layers/world/WorldCompatibilityLayer_1_16_4.java

@@ -0,0 +1,16 @@
+package com.gmail.nossr50.util.compat.layers.world;
+
+import org.bukkit.World;
+import org.jetbrains.annotations.NotNull;
+
+public class WorldCompatibilityLayer_1_16_4 implements WorldCompatibilityLayer {
+    @Override
+    public int getMinWorldHeight(@NotNull World world) {
+        return world.getMinHeight();
+    }
+
+    @Override
+    public int getMaxWorldHeight(@NotNull World world) {
+        return world.getMaxHeight();
+    }
+}

src/test/java/com/gmail/nossr50/util/blockmeta/ChunkStoreTest.java

@@ -1,20 +1,34 @@
 package com.gmail.nossr50.util.blockmeta;
 
 import com.gmail.nossr50.TestUtil;
-import com.gmail.nossr50.util.Misc;
+import com.gmail.nossr50.mcMMO;
+import com.gmail.nossr50.util.compat.CompatibilityManager;
+import com.gmail.nossr50.util.compat.layers.world.WorldCompatibilityLayer;
+import com.gmail.nossr50.util.platform.PlatformManager;
 import com.google.common.io.Files;
-import org.bukkit.Bukkit;
+import org.bukkit.*;
-import org.bukkit.World;
+import org.bukkit.block.*;
-import org.bukkit.block.Block;
+import org.bukkit.block.data.BlockData;
+import org.bukkit.entity.Entity;
+import org.bukkit.inventory.ItemStack;
+import org.bukkit.metadata.MetadataValue;
+import org.bukkit.plugin.Plugin;
+import org.bukkit.util.BoundingBox;
+import org.bukkit.util.RayTraceResult;
+import org.bukkit.util.Vector;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.junit.*;
 import org.junit.runner.RunWith;
 import org.mockito.Mockito;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
+import org.powermock.reflect.Whitebox;
 
 import java.io.*;
+import java.util.Collection;
+import java.util.List;
 import java.util.UUID;
 
 import static org.mockito.Mockito.mock;
@@ -23,7 +37,7 @@ import static org.mockito.Mockito.mock;
  * Could be a lot better.  But some tests are better than none!  Tests the major things, still kinda unit-testy.  Verifies that the serialization isn't completely broken.
  */
 @RunWith(PowerMockRunner.class)
-@PrepareForTest({ Bukkit.class, Misc.class })
+@PrepareForTest({ Bukkit.class, mcMMO.class})
 public class ChunkStoreTest {
     private static File tempDir;
     @BeforeClass
@@ -37,6 +51,10 @@ public class ChunkStoreTest {
     }
 
     private World mockWorld;
+    private CompatibilityManager compatibilityManager;
+    private WorldCompatibilityLayer worldCompatibilityLayer;
+    private PlatformManager platformManager;
+
     @Before
     public void setUpMock(){
         UUID worldUUID = UUID.randomUUID();
@@ -46,6 +64,39 @@ public class ChunkStoreTest {
         Mockito.when(mockWorld.getWorldFolder()).thenReturn(tempDir);
         PowerMockito.mockStatic(Bukkit.class);
         Mockito.when(Bukkit.getWorld(worldUUID)).thenReturn(mockWorld);
+
+        platformManager = mock(PlatformManager.class);
+        compatibilityManager = mock(CompatibilityManager.class);
+        worldCompatibilityLayer = mock(WorldCompatibilityLayer.class);
+
+        Whitebox.setInternalState(mcMMO.class, "platformManager", platformManager);
+        Mockito.when(mcMMO.getCompatibilityManager()).thenReturn(compatibilityManager);
+
+        Assert.assertNotNull(mcMMO.getCompatibilityManager());
+        Mockito.when(platformManager.getCompatibilityManager()).thenReturn(compatibilityManager);
+        Mockito.when(platformManager.getCompatibilityManager().getWorldCompatibilityLayer()).thenReturn(worldCompatibilityLayer);
+        Assert.assertNotNull(mcMMO.getCompatibilityManager().getWorldCompatibilityLayer());
+        Mockito.when(worldCompatibilityLayer.getMinWorldHeight(mockWorld)).thenReturn(0);
+        Mockito.when(worldCompatibilityLayer.getMaxWorldHeight(mockWorld)).thenReturn(255);
+    }
+
+    @Test
+    public void testSetTrue() {
+        Mockito.when(mcMMO.getCompatibilityManager().getWorldCompatibilityLayer().getMinWorldHeight(mockWorld)).thenReturn(-64);
+        HashChunkManager hashChunkManager = new HashChunkManager();
+        int radius = 2; //Could be anything but drastically changes test time
+
+        for(int x = -radius; x < radius; x++) {
+            for(int y = mockWorld.getMinHeight(); y < mockWorld.getMaxHeight(); y++) {
+                for(int z = -radius; z < radius; z++) {
+                    TestBlock testBlock = new TestBlock(x, y, z, mockWorld);
+                    hashChunkManager.setTrue(testBlock);
+                    Assert.assertTrue(hashChunkManager.isTrue(testBlock));
+                    hashChunkManager.setFalse(testBlock);
+                    Assert.assertFalse(hashChunkManager.isTrue(testBlock));
+                }
+            }
+        }
     }
 
     @Test
@@ -79,8 +130,7 @@ public class ChunkStoreTest {
 
     @Test
     public void testNegativeWorldMin() throws IOException {
-        PowerMockito.mockStatic(Misc.class);
+        Mockito.when(mcMMO.getCompatibilityManager().getWorldCompatibilityLayer().getMinWorldHeight(mockWorld)).thenReturn(-64);
-        Mockito.when(Misc.getWorldMinCompat(mockWorld)).thenReturn(-64);
 
         BitSetChunkStore original = new BitSetChunkStore(mockWorld, 1, 2);
         original.setTrue(14, -32, 12);
@@ -99,8 +149,7 @@ public class ChunkStoreTest {
         original.setTrue(13, 3, 12);
         byte[] serializedBytes = serializeChunkstore(original);
 
-        PowerMockito.mockStatic(Misc.class);
+        Mockito.when(mcMMO.getCompatibilityManager().getWorldCompatibilityLayer().getMinWorldHeight(mockWorld)).thenReturn(-64);
-        Mockito.when(Misc.getWorldMinCompat(mockWorld)).thenReturn(-64);
         ChunkStore deserialized = BitSetChunkStore.Serialization.readChunkStore(new DataInputStream(new ByteArrayInputStream(serializedBytes)));
         assertEqualIgnoreMinMax(original, deserialized);
     }
@@ -360,4 +409,276 @@ public class ChunkStoreTest {
             super.writeUTF(str);
         }
     }
+
+    private class TestBlock implements Block {
+
+        private final int x, y, z;
+        private final @NotNull World world;
+
+        private TestBlock(int x, int y, int z, World world) {
+            this.x = x;
+            this.y = y;
+            this.z = z;
+            this.world = world;
+        }
+
+        @Override
+        public byte getData() {
+            return 0;
+        }
+
+        @NotNull
+        @Override
+        public BlockData getBlockData() {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Block getRelative(int modX, int modY, int modZ) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Block getRelative(@NotNull BlockFace face) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Block getRelative(@NotNull BlockFace face, int distance) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Material getType() {
+            return null;
+        }
+
+        @Override
+        public byte getLightLevel() {
+            return 0;
+        }
+
+        @Override
+        public byte getLightFromSky() {
+            return 0;
+        }
+
+        @Override
+        public byte getLightFromBlocks() {
+            return 0;
+        }
+
+        @NotNull
+        @Override
+        public World getWorld() {
+            return world;
+        }
+
+        @Override
+        public int getX() {
+            return x;
+        }
+
+        @Override
+        public int getY() {
+            return y;
+        }
+
+        @Override
+        public int getZ() {
+            return z;
+        }
+
+        @NotNull
+        @Override
+        public Location getLocation() {
+            return null;
+        }
+
+        @Nullable
+        @Override
+        public Location getLocation(@Nullable Location loc) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Chunk getChunk() {
+            return null;
+        }
+
+        @Override
+        public void setBlockData(@NotNull BlockData data) {
+
+        }
+
+        @Override
+        public void setBlockData(@NotNull BlockData data, boolean applyPhysics) {
+
+        }
+
+        @Override
+        public void setType(@NotNull Material type) {
+
+        }
+
+        @Override
+        public void setType(@NotNull Material type, boolean applyPhysics) {
+
+        }
+
+        @Nullable
+        @Override
+        public BlockFace getFace(@NotNull Block block) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public BlockState getState() {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Biome getBiome() {
+            return null;
+        }
+
+        @Override
+        public void setBiome(@NotNull Biome bio) {
+
+        }
+
+        @Override
+        public boolean isBlockPowered() {
+            return false;
+        }
+
+        @Override
+        public boolean isBlockIndirectlyPowered() {
+            return false;
+        }
+
+        @Override
+        public boolean isBlockFacePowered(@NotNull BlockFace face) {
+            return false;
+        }
+
+        @Override
+        public boolean isBlockFaceIndirectlyPowered(@NotNull BlockFace face) {
+            return false;
+        }
+
+        @Override
+        public int getBlockPower(@NotNull BlockFace face) {
+            return 0;
+        }
+
+        @Override
+        public int getBlockPower() {
+            return 0;
+        }
+
+        @Override
+        public boolean isEmpty() {
+            return false;
+        }
+
+        @Override
+        public boolean isLiquid() {
+            return false;
+        }
+
+        @Override
+        public double getTemperature() {
+            return 0;
+        }
+
+        @Override
+        public double getHumidity() {
+            return 0;
+        }
+
+        @NotNull
+        @Override
+        public PistonMoveReaction getPistonMoveReaction() {
+            return null;
+        }
+
+        @Override
+        public boolean breakNaturally() {
+            return false;
+        }
+
+        @Override
+        public boolean breakNaturally(@Nullable ItemStack tool) {
+            return false;
+        }
+
+        @Override
+        public boolean applyBoneMeal(@NotNull BlockFace face) {
+            return false;
+        }
+
+        @NotNull
+        @Override
+        public Collection<ItemStack> getDrops() {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Collection<ItemStack> getDrops(@Nullable ItemStack tool) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public Collection<ItemStack> getDrops(@NotNull ItemStack tool, @Nullable Entity entity) {
+            return null;
+        }
+
+        @Override
+        public boolean isPassable() {
+            return false;
+        }
+
+        @Nullable
+        @Override
+        public RayTraceResult rayTrace(@NotNull Location start, @NotNull Vector direction, double maxDistance, @NotNull FluidCollisionMode fluidCollisionMode) {
+            return null;
+        }
+
+        @NotNull
+        @Override
+        public BoundingBox getBoundingBox() {
+            return null;
+        }
+
+        @Override
+        public void setMetadata(@NotNull String metadataKey, @NotNull MetadataValue newMetadataValue) {
+
+        }
+
+        @NotNull
+        @Override
+        public List<MetadataValue> getMetadata(@NotNull String metadataKey) {
+            return null;
+        }
+
+        @Override
+        public boolean hasMetadata(@NotNull String metadataKey) {
+            return false;
+        }
+
+        @Override
+        public void removeMetadata(@NotNull String metadataKey, @NotNull Plugin owningPlugin) {
+
+        }
+    }
 }