From 3ef537242f4af86a08c92b6d13d14b241b466e85 Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Tue, 24 Jul 2018 17:05:30 -0400 Subject: [PATCH] Fix some issues with item equality Seems to be an internal Bukkit bug, hopefully nothing more serious --- .../comphenix/protocol/ProtocolLogger.java | 6 +- .../protocol/wrappers/EnumWrappers.java | 72 +++++++++-------- .../protocol/wrappers/WrappedDataWatcher.java | 81 +++++++++++++------ .../protocol/events/PacketContainerTest.java | 38 ++++----- .../utility/MinecraftReflectionTest.java | 3 +- .../utility/StreamSerializerTest.java | 5 +- .../comphenix/protocol/utility/TestUtils.java | 45 +++++++++++ .../wrappers/BukkitConvertersTest.java | 42 ++++++++++ 8 files changed, 201 insertions(+), 91 deletions(-) create mode 100644 modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/TestUtils.java create mode 100644 modules/ProtocolLib/src/test/java/com/comphenix/protocol/wrappers/BukkitConvertersTest.java diff --git a/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java b/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java index eea46d76..faf14a8a 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java +++ b/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java @@ -31,11 +31,11 @@ public class ProtocolLogger { ProtocolLogger.plugin = plugin; } - private static boolean isDebugEnabled() { + public static boolean isDebugEnabled() { try { return plugin.getConfig().getBoolean("global.debug", false); - } catch (Throwable ex) { // Maybe we're testing or something - return false; + } catch (Throwable ex) { // Enable in testing environments + return true; } } diff --git a/modules/API/src/main/java/com/comphenix/protocol/wrappers/EnumWrappers.java b/modules/API/src/main/java/com/comphenix/protocol/wrappers/EnumWrappers.java index 59fa8238..d8a5d320 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/wrappers/EnumWrappers.java +++ b/modules/API/src/main/java/com/comphenix/protocol/wrappers/EnumWrappers.java @@ -3,17 +3,17 @@ package com.comphenix.protocol.wrappers; import java.util.HashMap; import java.util.Locale; import java.util.Map; -import java.util.function.Function; - -import org.bukkit.GameMode; import com.comphenix.protocol.PacketType; import com.comphenix.protocol.PacketType.Protocol; +import com.comphenix.protocol.ProtocolLogger; import com.comphenix.protocol.reflect.EquivalentConverter; import com.comphenix.protocol.reflect.FuzzyReflection; import com.comphenix.protocol.utility.MinecraftReflection; import com.google.common.collect.Maps; +import org.bukkit.GameMode; + /** * Represents a generic enum converter. * @author Kristian @@ -22,26 +22,26 @@ public abstract class EnumWrappers { public enum ClientCommand { PERFORM_RESPAWN, REQUEST_STATS, - OPEN_INVENTORY_ACHIEVEMENT; + OPEN_INVENTORY_ACHIEVEMENT } public enum ChatVisibility { FULL, SYSTEM, - HIDDEN; + HIDDEN } public enum Difficulty { PEACEFUL, EASY, NORMAL, - HARD; + HARD } public enum EntityUseAction { INTERACT, ATTACK, - INTERACT_AT; + INTERACT_AT } /** @@ -111,7 +111,7 @@ public abstract class EnumWrappers { SUCCESSFULLY_LOADED, DECLINED, FAILED_DOWNLOAD, - ACCEPTED; + ACCEPTED } public enum PlayerInfoAction { @@ -119,15 +119,16 @@ public abstract class EnumWrappers { UPDATE_GAME_MODE, UPDATE_LATENCY, UPDATE_DISPLAY_NAME, - REMOVE_PLAYER; + REMOVE_PLAYER } public enum TitleAction { TITLE, SUBTITLE, + ACTIONBAR, TIMES, CLEAR, - RESET; + RESET } public enum WorldBorderAction { @@ -136,13 +137,13 @@ public abstract class EnumWrappers { SET_CENTER, INITIALIZE, SET_WARNING_TIME, - SET_WARNING_BLOCKS; + SET_WARNING_BLOCKS } public enum CombatEventType { ENTER_COMBAT, END_COMBAT, - ENTITY_DIED; + ENTITY_DIED } public enum PlayerDigType { @@ -152,7 +153,7 @@ public abstract class EnumWrappers { DROP_ALL_ITEMS, DROP_ITEM, RELEASE_USE_ITEM, - SWAP_HELD_ITEMS; + SWAP_HELD_ITEMS } public enum PlayerAction { @@ -164,12 +165,12 @@ public abstract class EnumWrappers { START_RIDING_JUMP, STOP_RIDING_JUMP, OPEN_INVENTORY, - START_FALL_FLYING; + START_FALL_FLYING } public enum ScoreboardAction { CHANGE, - REMOVE; + REMOVE } public enum Particle { @@ -227,8 +228,8 @@ public abstract class EnumWrappers { private static final Map BY_ID; static { - BY_ID = new HashMap(); - BY_NAME = new HashMap(); + BY_ID = new HashMap<>(); + BY_NAME = new HashMap<>(); for (Particle particle : values()) { BY_NAME.put(particle.getName().toLowerCase(Locale.ENGLISH), particle); @@ -241,11 +242,11 @@ public abstract class EnumWrappers { private final boolean longDistance; private final int dataLength; - private Particle(String name, int id, boolean longDistance) { + Particle(String name, int id, boolean longDistance) { this(name, id, longDistance, 0); } - private Particle(String name, int id, boolean longDistance, int dataLength) { + Particle(String name, int id, boolean longDistance, int dataLength) { this.name = name; this.id = id; this.longDistance = longDistance; @@ -298,7 +299,8 @@ public abstract class EnumWrappers { } private final String key; - private SoundCategory(String key) { + + SoundCategory(String key) { this.key = key; } @@ -317,12 +319,12 @@ public abstract class EnumWrappers { FEET, LEGS, CHEST, - HEAD; + HEAD } public enum Hand { MAIN_HAND, - OFF_HAND; + OFF_HAND } public enum Direction { @@ -331,22 +333,16 @@ public abstract class EnumWrappers { NORTH, SOUTH, WEST, - EAST; + EAST } public enum ChatType { - CHAT(0), - SYSTEM(1), - GAME_INFO(2); - - private byte id; - - ChatType(int id) { - this.id = (byte) id; - } - + CHAT, + SYSTEM, + GAME_INFO; + public byte getId() { - return id; + return (byte) ordinal(); } } @@ -435,6 +431,8 @@ public abstract class EnumWrappers { if (nativeClass != null) { FROM_NATIVE.put(nativeClass, converter); FROM_WRAPPER.put(wrapperClass, converter); + } else if (ProtocolLogger.isDebugEnabled()) { + new ClassNotFoundException(wrapperClass.getSimpleName()).printStackTrace(); } } @@ -448,7 +446,11 @@ public abstract class EnumWrappers { try { return FuzzyReflection.fromClass(clazz, true).getFieldListByType(Enum.class).get(index).getType(); } catch (Throwable ex) { - return null; // Unsupported in this version + if (ProtocolLogger.isDebugEnabled()) { + ex.printStackTrace(); + } + + return null; } } diff --git a/modules/API/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java b/modules/API/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java index 7851b6a4..8d228518 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java +++ b/modules/API/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java @@ -1,6 +1,6 @@ /** * ProtocolLib - Bukkit server library that allows access to the Minecraft protocol. - * Copyright (C) 2016 dmulloy2 + * Copyright (C) 2018 dmulloy2 * * This program is free software; you can redistribute it and/or modify it under the terms of the * GNU General Public License as published by the Free Software Foundation; either version 2 of @@ -39,8 +39,7 @@ import org.bukkit.entity.Entity; import org.bukkit.inventory.ItemStack; /** - * Represents a DataWatcher in 1.8 thru 1.10 - * + * Represents a DataWatcher * @author dmulloy2 */ public class WrappedDataWatcher extends AbstractWrapper implements Iterable { @@ -74,7 +73,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable getMap() { if (MAP_FIELD == null) { - FuzzyReflection fuzzy = FuzzyReflection.fromClass(handleType, true); - MAP_FIELD = Accessors.getFieldAccessor(fuzzy.getField(FuzzyFieldContract.newBuilder() - .banModifier(Modifier.STATIC) - .typeDerivedOf(Map.class) - .build())); - } - - if (MAP_FIELD == null) { - throw new FieldAccessException("Could not find index <-> Item map."); + try { + FuzzyReflection fuzzy = FuzzyReflection.fromClass(handleType, true); + MAP_FIELD = Accessors.getFieldAccessor(fuzzy.getField(FuzzyFieldContract + .newBuilder() + .banModifier(Modifier.STATIC) + .typeDerivedOf(Map.class) + .build())); + } catch (IllegalArgumentException ex) { + throw new FieldAccessException("Failed to find watchable object map"); + } } return (Map) MAP_FIELD.get(handle); @@ -371,7 +372,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable(HANDLE_TYPE).withTarget(handle); + this.modifier = new StructureModifier<>(HANDLE_TYPE).withTarget(handle); } /** @@ -692,18 +693,31 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable= 0, "index cannot be negative!"); + if (constructor == null) { constructor = Accessors.getConstructorAccessor(HANDLE_TYPE.getConstructors()[0]); } - Object handle = serializer != null ? serializer.getHandle() : null; + return constructor.invoke(index, null); + } + + private static Object newHandle(int index, Serializer serializer) { + Validate.isTrue(index >= 0, "index cannot be negative!"); + Validate.notNull(serializer, "serializer cannot be null!"); + + if (constructor == null) { + constructor = Accessors.getConstructorAccessor(HANDLE_TYPE.getConstructors()[0]); + } + + Object handle = serializer.getHandle(); return constructor.invoke(index, handle); } @@ -914,7 +928,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable clazz) { - Validate.notNull("Class cannot be null!"); + Validate.notNull(clazz,"Class cannot be null!"); initialize(); for (Serializer serializer : REGISTRY) { @@ -923,7 +937,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable" : clazz)); } /** @@ -956,7 +972,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable innerClass = null; + Class innerClass; boolean optional = false; if (arg instanceof Class) { @@ -1004,6 +1020,10 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterablemust be wrapped in an {@link Optional} + * @return The serializer + */ + public static Serializer getChatComponentSerializer(boolean optional) { + return get(MinecraftReflection.getIChatBaseComponentClass(), optional); } /** diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java index e442b953..a0f76f7b 100644 --- a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java @@ -18,10 +18,7 @@ package com.comphenix.protocol.events; import java.lang.reflect.Array; import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.List; -import java.util.Objects; -import java.util.UUID; +import java.util.*; import com.comphenix.protocol.BukkitInitialization; import com.comphenix.protocol.PacketType; @@ -57,6 +54,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.powermock.core.classloader.annotations.PowerMockIgnore; +import static com.comphenix.protocol.utility.TestUtils.*; import static org.junit.Assert.*; // Ensure that the CraftItemFactory is mockable @@ -218,17 +216,7 @@ public class PacketContainerTest { // Read back array List comparison = itemAccess.read(0); - assertEquals(items, comparison); - } - - private boolean equivalentItem(ItemStack first, ItemStack second) { - if (first == null) { - return second == null; - } else if (second == null) { - return false; - } else { - return first.getType().equals(second.getType()); - } + assertItemCollectionsEqual(items, comparison); } @Test @@ -270,14 +258,11 @@ public class PacketContainerTest { PacketContainer mobSpawnPacket = new PacketContainer(PacketType.Play.Server.SPAWN_ENTITY_LIVING); StructureModifier watcherAccessor = mobSpawnPacket.getDataWatcherModifier(); - Entity entity = new EntityEgg(null, 0, 0, 0); - DataWatcher watcher = entity.getDataWatcher(); - - WrappedDataWatcher dataWatcher = new WrappedDataWatcher(watcher); - dataWatcher.setObject(1, (byte) 1); - dataWatcher.setObject(2, 301); - dataWatcher.setObject(3, true); - dataWatcher.setObject(4, "Lorem"); + WrappedDataWatcher dataWatcher = new WrappedDataWatcher(); + dataWatcher.setObject(new WrappedDataWatcherObject(1, Registry.get(Byte.class)), (byte) 1); + dataWatcher.setObject(new WrappedDataWatcherObject(2, Registry.get(String.class)), "Lorem"); + dataWatcher.setObject(new WrappedDataWatcherObject(3, Registry.get(Boolean.class)), true); + dataWatcher.setObject(new WrappedDataWatcherObject(4, Registry.getUUIDSerializer(true)), Optional.of(UUID.randomUUID())); assertNull(watcherAccessor.read(0)); @@ -463,7 +448,7 @@ public class PacketContainerTest { PacketContainer container = new PacketContainer(PacketType.Play.Server.NAMED_SOUND_EFFECT); container.getSoundCategories().write(0, SoundCategory.PLAYERS); - assertEquals(container.getSoundCategories().read(0), SoundCategory.PLAYERS); + assertEquals(SoundCategory.PLAYERS, container.getSoundCategories().read(0)); } @Test @@ -580,6 +565,11 @@ public class PacketContainerTest { } } + if (a instanceof ItemStack || b instanceof ItemStack) { + assertItemsEqual((ItemStack) a, (ItemStack) b); + return; + } + if (a == null || b == null) { if (a == null && b == null) { return; diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/MinecraftReflectionTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/MinecraftReflectionTest.java index f7976d80..a07887ad 100644 --- a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/MinecraftReflectionTest.java +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/MinecraftReflectionTest.java @@ -1,5 +1,6 @@ package com.comphenix.protocol.utility; +import static com.comphenix.protocol.utility.TestUtils.assertItemsEqual; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.mockito.Mockito.mock; @@ -138,7 +139,7 @@ public class MinecraftReflectionTest { public void testItemStacks() { ItemStack stack = new ItemStack(Material.GOLDEN_SWORD); Object nmsStack = MinecraftReflection.getMinecraftItemStack(stack); - assertEquals(stack, MinecraftReflection.getBukkitItemStack(nmsStack)); + assertItemsEqual(stack, MinecraftReflection.getBukkitItemStack(nmsStack)); // The NMS handle for CraftItemStack is null with Material.AIR, make sure it is handled correctly assertNotNull(MinecraftReflection.getMinecraftItemStack(CraftItemStack.asCraftCopy(new ItemStack(Material.AIR)))); diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/StreamSerializerTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/StreamSerializerTest.java index fb738e87..b286c374 100644 --- a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/StreamSerializerTest.java +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/StreamSerializerTest.java @@ -17,6 +17,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.powermock.core.classloader.annotations.PowerMockIgnore; +import static com.comphenix.protocol.utility.TestUtils.assertItemsEqual; import static org.junit.Assert.assertEquals; @RunWith(org.powermock.modules.junit4.PowerMockRunner.class) @@ -75,7 +76,7 @@ public class StreamSerializerTest { String serialized = serializer.serializeItemStack(initial); ItemStack deserialized = serializer.deserializeItemStack(serialized); - assertEquals(initial, deserialized); + assertItemsEqual(initial, deserialized); } @Test @@ -90,6 +91,6 @@ public class StreamSerializerTest { String serialized = serializer.serializeItemStack(initial); ItemStack deserialized = serializer.deserializeItemStack(serialized); - assertEquals(initial, deserialized); + assertItemsEqual(initial, deserialized); } } diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/TestUtils.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/TestUtils.java new file mode 100644 index 00000000..4fa4a570 --- /dev/null +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/utility/TestUtils.java @@ -0,0 +1,45 @@ +package com.comphenix.protocol.utility; + +import java.util.List; + +import org.bukkit.Bukkit; +import org.bukkit.inventory.ItemStack; + +import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; + +public class TestUtils { + + public static void assertItemCollectionsEqual(List first, List second) { + assertEquals(first.size(), second.size()); + for (int i = 0; i < first.size(); i++) { + assertItemsEqual(first.get(i), second.get(i)); + } + } + + public static void assertItemsEqual(ItemStack first, ItemStack second) { + if (first == null) { + assertNull(second); + } else { + assertNotNull(first); + + // The legacy check in ItemStack#isSimilar causes a null pointer + assertEquals(first.getType(), second.getType()); + assertEquals(first.getDurability(), second.getDurability()); + assertEquals(first.hasItemMeta(), second.hasItemMeta()); + if (first.hasItemMeta()) { + assertTrue(Bukkit.getItemFactory().equals(first.getItemMeta(), second.getItemMeta())); + } + } + } + + public static boolean equivalentItem(ItemStack first, ItemStack second) { + if (first == null) { + return second == null; + } else if (second == null) { + return false; + } else { + return first.getType().equals(second.getType()); + } + } +} diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/wrappers/BukkitConvertersTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/wrappers/BukkitConvertersTest.java new file mode 100644 index 00000000..a52cc044 --- /dev/null +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/wrappers/BukkitConvertersTest.java @@ -0,0 +1,42 @@ +package com.comphenix.protocol.wrappers; + +import com.comphenix.protocol.BukkitInitialization; +import com.comphenix.protocol.reflect.EquivalentConverter; + +import org.bukkit.Bukkit; +import org.bukkit.ChatColor; +import org.bukkit.Material; +import org.bukkit.enchantments.Enchantment; +import org.bukkit.inventory.ItemStack; +import org.bukkit.inventory.meta.ItemMeta; +import org.junit.BeforeClass; +import org.junit.Test; + +import static com.comphenix.protocol.utility.TestUtils.assertItemsEqual; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class BukkitConvertersTest { + @BeforeClass + public static void beforeClass() { + BukkitInitialization.initializeItemMeta(); + } + + @Test + public void testItemStacks() { + ItemStack item = new ItemStack(Material.DIAMOND_SWORD, 16); + item.addEnchantment(Enchantment.DAMAGE_ALL, 4); + ItemMeta meta = item.getItemMeta(); + meta.setDisplayName(ChatColor.GREEN + "Diamond Sword"); + item.setItemMeta(meta); + + EquivalentConverter converter = BukkitConverters.getItemStackConverter(); + Object nmsStack = converter.getGeneric(item); + ItemStack back = converter.getSpecific(nmsStack); + + assertEquals(item.getType(), back.getType()); + assertEquals(item.getDurability(), back.getDurability()); + assertEquals(item.hasItemMeta(), back.hasItemMeta()); + assertTrue(Bukkit.getItemFactory().equals(item.getItemMeta(), back.getItemMeta())); + } +}