From 11ba322b5f746c900dad01a9a4b4f1fc9c72aba1 Mon Sep 17 00:00:00 2001
From: Dan Mulloy <dmulloy2@live.com>
Date: Sat, 5 Mar 2016 23:28:48 -0500
Subject: [PATCH] Fix an issue with cloning metadata packets

Also improved some documentation and tests
---
 .../reflect/cloning/BukkitCloner.java         |  11 +
 .../protocol/utility/MinecraftReflection.java |   4 +
 .../protocol/wrappers/MinecraftKey.java       |  37 +++
 .../protocol/wrappers/WrappedDataWatcher.java | 241 ++++++++++--------
 .../wrappers/WrappedWatchableObject.java      |   5 +
 .../protocol/events/PacketContainerTest.java  |  18 +-
 6 files changed, 215 insertions(+), 101 deletions(-)

diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/BukkitCloner.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/BukkitCloner.java
index 4a7372e7..c6164f61 100644
--- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/BukkitCloner.java
+++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/BukkitCloner.java
@@ -24,6 +24,7 @@ import com.comphenix.protocol.utility.MinecraftReflection;
 import com.comphenix.protocol.wrappers.BlockPosition;
 import com.comphenix.protocol.wrappers.BukkitConverters;
 import com.comphenix.protocol.wrappers.ChunkPosition;
+import com.comphenix.protocol.wrappers.MinecraftKey;
 import com.comphenix.protocol.wrappers.WrappedDataWatcher;
 import com.comphenix.protocol.wrappers.WrappedServerPing;
 import com.google.common.collect.Maps;
@@ -55,6 +56,11 @@ public class BukkitCloner implements Cloner {
 		if (MinecraftReflection.isUsingNetty()) {
 			addClass(4, MinecraftReflection.getServerPingClass());
 		}
+
+		if (MinecraftReflection.dataWatcherItemExists()) {
+			addClass(5, MinecraftReflection.getDataWatcherSerializerClass());
+			addClass(6, MinecraftReflection.getMinecraftKeyClass());
+		}
 	}
 
 	private void addClass(int id, Class<?> clazz) {
@@ -102,6 +108,11 @@ public class BukkitCloner implements Cloner {
 			case 4:
 				EquivalentConverter<WrappedServerPing> serverConverter = BukkitConverters.getWrappedServerPingConverter();
 				return serverConverter.getGeneric(clonableClasses.get(4), serverConverter.getSpecific(source).deepClone());
+			case 5:
+				return source;
+			case 6:
+				EquivalentConverter<MinecraftKey> keyConverter = MinecraftKey.getConverter();
+				return keyConverter.getGeneric(clonableClasses.get(5), keyConverter.getSpecific(source));
 			default:
 				throw new IllegalArgumentException("Cannot clone objects of type " + source.getClass());
 		}
diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java b/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java
index b76b310d..74ef8809 100644
--- a/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java
+++ b/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java
@@ -2077,4 +2077,8 @@ public class MinecraftReflection {
 			throw new RuntimeException("Cannot construct packet serializer.", e);
 		}
 	}
+
+	public static Class<?> getMinecraftKeyClass() {
+		return getMinecraftClass("MinecraftKey");
+	}
 }
\ No newline at end of file
diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/MinecraftKey.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/MinecraftKey.java
index 79823572..e8679743 100644
--- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/MinecraftKey.java
+++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/MinecraftKey.java
@@ -16,7 +16,11 @@
  */
 package com.comphenix.protocol.wrappers;
 
+import java.lang.reflect.Constructor;
+
+import com.comphenix.protocol.reflect.EquivalentConverter;
 import com.comphenix.protocol.reflect.StructureModifier;
+import com.comphenix.protocol.utility.MinecraftReflection;
 
 /**
  * @author dmulloy2
@@ -59,4 +63,37 @@ public class MinecraftKey {
 	public String getEnumFormat() {
 		return key.toUpperCase().replace(".", "_");
 	}
+
+	private static Constructor<?> constructor = null;
+
+	public static EquivalentConverter<MinecraftKey> getConverter() {
+		return new EquivalentConverter<MinecraftKey>() {
+			@Override
+			public MinecraftKey getSpecific(Object generic) {
+				return MinecraftKey.fromHandle(generic);
+			}
+
+			@Override
+			public Object getGeneric(Class<?> genericType, MinecraftKey specific) {
+				if (constructor == null) {
+					try {
+						constructor = MinecraftReflection.getMinecraftKeyClass().getConstructor(String.class, String.class);
+					} catch (ReflectiveOperationException e) {
+						throw new RuntimeException("Failed to obtain MinecraftKey constructor", e);
+					}
+				}
+
+				try {
+					return constructor.newInstance(specific.getPrefix(), specific.getKey());
+				} catch (ReflectiveOperationException e) {
+					throw new RuntimeException("Failed to create new MinecraftKey", e);
+				}
+			}
+
+			@Override
+			public Class<MinecraftKey> getSpecificType() {
+				return MinecraftKey.class;
+			}
+		};
+	}
 }
\ No newline at end of file
diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java
index 398730b0..f3c08ed7 100644
--- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java
+++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java
@@ -47,6 +47,7 @@ import com.google.common.base.Optional;
 
 /**
  * Represents a DataWatcher in 1.9
+ * 
  * @author dmulloy2
  */
 public class WrappedDataWatcher extends AbstractWrapper implements Iterable<WrappedWatchableObject> {
@@ -173,6 +174,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * Gets the watchable object at a given index.
+	 * 
 	 * @param index Index
 	 * @return The watchable object, or null if none exists
 	 */
@@ -187,6 +189,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * Whether or not this DataWatcher has an object at a given index.
+	 * 
 	 * @param index Index
 	 * @return True if it does, false if not
 	 */
@@ -196,111 +199,126 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	// ---- Object Getters
 
-    /**
-     * Get a watched byte.
-     * @param index - index of the watched byte.
-     * @return The watched byte, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public Byte getByte(int index) throws FieldAccessException {
-    	return (Byte) getObject(index);
-    }
-
-    /**
-     * Get a watched short.
-     * @param index - index of the watched short.
-     * @return The watched short, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public Short getShort(int index) throws FieldAccessException {
-    	return (Short) getObject(index);
-    }
-
-    /**
-     * Get a watched integer.
-     * @param index - index of the watched integer.
-     * @return The watched integer, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public Integer getInteger(int index) throws FieldAccessException {
-    	return (Integer) getObject(index);
-    }
-
-    /**
-     * Get a watched float.
-     * @param index - index of the watched float.
-     * @return The watched float, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public Float getFloat(int index) throws FieldAccessException {
-    	return (Float) getObject(index);
-    }
-
-    /**
-     * Get a watched string.
-     * @param index - index of the watched string.
-     * @return The watched string, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public String getString(int index) throws FieldAccessException {
-    	return (String) getObject(index);
-    }
-
-    /**
-     * Get a watched string.
-     * @param index - index of the watched string.
-     * @return The watched string, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public ItemStack getItemStack(int index) throws FieldAccessException {
-    	return (ItemStack) getObject(index);
-    }
-
-    /**
-     * Get a watched string.
-     * @param index - index of the watched string.
-     * @return The watched string, or NULL if this value doesn't exist.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public WrappedChunkCoordinate getChunkCoordinate(int index) throws FieldAccessException {
-    	return (WrappedChunkCoordinate) getObject(index);
-    }
-
-    /**
-     * Retrieve a watchable object by index.
-     * @param index - index of the object to retrieve.
-     * @return The watched object.
-     * @throws FieldAccessException Cannot read underlying field.
-     */
-    public Object getObject(int index) throws FieldAccessException {
-    	return WrappedWatchableObject.getWrapped(getWatchedObject(index));
-    }
-
-	private Object getWatchedObject(int index) {
-		return getWatcherObject(new WrappedDataWatcherObject(index, null));
+	/**
+	 * Get a watched byte.
+	 * 
+	 * @param index - index of the watched byte.
+	 * @return The watched byte, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public Byte getByte(int index) throws FieldAccessException {
+		return (Byte) getObject(index);
 	}
-   
-    private Object getWatcherObject(WrappedDataWatcherObject watcherObject) {
+
+	/**
+	 * Get a watched short.
+	 * 
+	 * @param index - index of the watched short.
+	 * @return The watched short, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public Short getShort(int index) throws FieldAccessException {
+		return (Short) getObject(index);
+	}
+
+	/**
+	 * Get a watched integer.
+	 * 
+	 * @param index - index of the watched integer.
+	 * @return The watched integer, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public Integer getInteger(int index) throws FieldAccessException {
+		return (Integer) getObject(index);
+	}
+
+	/**
+	 * Get a watched float.
+	 * 
+	 * @param index - index of the watched float.
+	 * @return The watched float, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public Float getFloat(int index) throws FieldAccessException {
+		return (Float) getObject(index);
+	}
+
+	/**
+	 * Get a watched string.
+	 * 
+	 * @param index - index of the watched string.
+	 * @return The watched string, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public String getString(int index) throws FieldAccessException {
+		return (String) getObject(index);
+	}
+
+	/**
+	 * Get a watched string.
+	 * 
+	 * @param index - index of the watched string.
+	 * @return The watched string, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public ItemStack getItemStack(int index) throws FieldAccessException {
+		return (ItemStack) getObject(index);
+	}
+
+	/**
+	 * Get a watched string.
+	 * 
+	 * @param index - index of the watched string.
+	 * @return The watched string, or NULL if this value doesn't exist.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public WrappedChunkCoordinate getChunkCoordinate(int index) throws FieldAccessException {
+		return (WrappedChunkCoordinate) getObject(index);
+	}
+
+	/**
+	 * Retrieve a watchable object by index.
+	 * 
+	 * @param index - index of the object to retrieve.
+	 * @return The watched object.
+	 * @throws FieldAccessException Cannot read underlying field.
+	 */
+	public Object getObject(int index) throws FieldAccessException {
+		return getObject(new WrappedDataWatcherObject(index, null));
+	}
+
+	/**
+	 * Retrieve a watchable object by watcher object.
+	 * 
+	 * @param object The watcher object
+	 * @return The watched object
+	 */
+	public Object getObject(WrappedDataWatcherObject object) {
+		Validate.notNull(object, "Watcher object cannot be null!");
+
 		if (GETTER == null) {
-			GETTER = Accessors.getMethodAccessor(handleType, "get", watcherObject.getHandleType());
+			GETTER = Accessors.getMethodAccessor(handleType, "get", object.getHandleType());
 		}
 
-		return GETTER.invoke(handle, watcherObject.getHandle());
-    }
+		Object value = GETTER.invoke(handle, object.getHandle());
+		return WrappedWatchableObject.getWrapped(value);
+	}
 
 	// ---- Object Setters
 
-    /**
-     * Sets the DataWatcher Item at a given index to a new value.
-     * @param index Index
-     * @param value New value
-     */
+	/**
+	 * Sets the DataWatcher Item at a given index to a new value.
+	 * 
+	 * @param index Index
+	 * @param value New value
+	 */
 	public void setObject(int index, Object value) {
 		setObject(new WrappedDataWatcherObject(index, null), value);
 	}
 
 	/**
 	 * Sets the DataWatcher Item associated with a given watcher object to a new value.
+	 * 
 	 * @param object Associated watcher object
 	 * @param value New value
 	 */
@@ -319,8 +337,6 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 					SETTER = Accessors.getMethodAccessor(method);
 				} else if (method.getName().equals("register")) {
 					REGISTER = Accessors.getMethodAccessor(method);
-				} else {
-					System.out.println(method);
 				}
 			}
 		}
@@ -423,6 +439,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * No longer supported in 1.9 due to the removal of a consistent type <-> ID map.
+	 * 
 	 * @param clazz
 	 * @return Null
 	 */
@@ -433,6 +450,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * No longer supported in 1.9 due to the removal of a consistent type <-> ID map.
+	 * 
 	 * @param typeID
 	 * @return Null
 	 */
@@ -473,6 +491,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * Represents a DataWatcherObject in 1.9.
+	 * 
 	 * @author dmulloy2
 	 */
 	public static class WrappedDataWatcherObject extends AbstractWrapper {
@@ -484,6 +503,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 		/**
 		 * Creates a new watcher object from a NMS handle
+		 * 
 		 * @param handle NMS handle
 		 */
 		public WrappedDataWatcherObject(Object handle) {
@@ -495,6 +515,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 		/**
 		 * Creates a new watcher object from an index and serializer
+		 * 
 		 * @param index Index
 		 * @param serializer Serializer, see {@link Registry}
 		 */
@@ -512,17 +533,23 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 		}
 
 		/**
-		 * Gets this watcher object's index
+		 * Gets this watcher object's index.
+		 * 
 		 * @return The index
 		 */
 		public int getIndex() {
 			return (int) modifier.read(0);
 		}
 
+		/**
+		 * Gets this watcher object's serializer.
+		 * 
+		 * @return The serializer
+		 */
 		public Serializer getSerializer() {
 			if (getSerializer == null) {
-				getSerializer = Accessors.getMethodAccessor(FuzzyReflection.fromClass(HANDLE_TYPE, true).getMethodByParameters(
-						"getSerializer", MinecraftReflection.getDataWatcherSerializerClass(), new Class[0]));
+				getSerializer = Accessors.getMethodAccessor(FuzzyReflection.fromClass(HANDLE_TYPE, true)
+						.getMethodByParameters("getSerializer", MinecraftReflection.getDataWatcherSerializerClass(), new Class[0]));
 			}
 
 			Object serializer = getSerializer.invoke(handle);
@@ -534,10 +561,16 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 				return new Serializer(null, serializer, false);
 			}
 		}
+
+		@Override
+		public String toString() {
+			return "DataWatcherObject[index=" + getIndex() + ", serializer=" + getSerializer() + "]";
+		}
 	}
 
 	/**
 	 * Represents a DataWatcherSerializer in 1.9.
+	 * 
 	 * @author dmulloy2
 	 */
 	public static class Serializer extends AbstractWrapper {
@@ -548,6 +581,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 		/**
 		 * Constructs a new Serializer
+		 * 
 		 * @param type Type it serializes
 		 * @param handle NMS handle
 		 * @param optional Whether or not it's {@link Optional}
@@ -562,6 +596,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 		/**
 		 * Gets the type this serializer serializes.
+		 * 
 		 * @return The type
 		 */
 		public Class<?> getType() {
@@ -569,8 +604,8 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 		}
 
 		/**
-		 * Whether or not this serializer is optional, that is whether or not
-		 * the return type is wrapped in a {@link Optional}.
+		 * Whether or not this serializer is optional, that is whether or not the return type is wrapped in a {@link Optional}.
+		 * 
 		 * @return True if it is, false if not
 		 */
 		public boolean isOptional() {
@@ -585,6 +620,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 
 	/**
 	 * Represents a DataWatcherRegistry containing the supported {@link Serializer}s in 1.9.
+	 * 
 	 * @author dmulloy2
 	 */
 	public static class Registry {
@@ -603,6 +639,11 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 			return REGISTRY.get(clazz);
 		}
 
+		/**
+		 * Gets the serializer associated with a given NMS handle.
+		 * @param handle The NMS handle
+		 * @return The serializer, or null if none exists
+		 */
 		public static Serializer fromHandle(Object handle) {
 			initialize();
 
@@ -623,7 +664,7 @@ public class WrappedDataWatcher extends AbstractWrapper implements Iterable<Wrap
 			}
 
 			List<Field> candidates = FuzzyReflection.fromClass(MinecraftReflection.getMinecraftClass("DataWatcherRegistry"), true)
-				.getFieldListByType(MinecraftReflection.getDataWatcherSerializerClass());
+					.getFieldListByType(MinecraftReflection.getDataWatcherSerializerClass());
 			for (Field candidate : candidates) {
 				Type generic = candidate.getGenericType();
 				if (generic instanceof ParameterizedType) {
diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java
index 570bbae4..a5380737 100644
--- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java
+++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedWatchableObject.java
@@ -223,4 +223,9 @@ public class WrappedWatchableObject extends AbstractWrapper {
 
 		return false;
 	}
+
+	@Override
+	public String toString() {
+		return "DataWatcherItem[object=" + getWatcherObject() + ", value=" + getValue() + ", dirty=" + getDirtyState() + "]";
+	}
 }
\ No newline at end of file
diff --git a/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java b/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java
index c4b62a5d..d91eef49 100644
--- a/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java
+++ b/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java
@@ -36,6 +36,7 @@ import net.minecraft.server.v1_9_R1.PacketPlayOutUpdateAttributes;
 import net.minecraft.server.v1_9_R1.PacketPlayOutUpdateAttributes.AttributeSnapshot;
 
 import org.apache.commons.lang.SerializationUtils;
+import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.bukkit.ChatColor;
 import org.bukkit.DyeColor;
 import org.bukkit.Material;
@@ -59,6 +60,8 @@ import com.comphenix.protocol.wrappers.BukkitConverters;
 import com.comphenix.protocol.wrappers.WrappedBlockData;
 import com.comphenix.protocol.wrappers.WrappedChatComponent;
 import com.comphenix.protocol.wrappers.WrappedDataWatcher;
+import com.comphenix.protocol.wrappers.WrappedDataWatcher.Registry;
+import com.comphenix.protocol.wrappers.WrappedDataWatcher.WrappedDataWatcherObject;
 import com.comphenix.protocol.wrappers.WrappedGameProfile;
 import com.comphenix.protocol.wrappers.WrappedWatchableObject;
 import com.comphenix.protocol.wrappers.nbt.NbtCompound;
@@ -465,7 +468,7 @@ public class PacketContainerTest {
 
 	private static final List<PacketType> BLACKLISTED = Util.asList(
 			PacketType.Play.Client.CUSTOM_PAYLOAD, PacketType.Play.Server.CUSTOM_PAYLOAD,
-			PacketType.Play.Server.SET_COOLDOWN, PacketType.Play.Server.NAMED_SOUND_EFFECT
+			PacketType.Play.Server.SET_COOLDOWN
 	);
 
 	@Test
@@ -489,6 +492,15 @@ public class PacketContainerTest {
 				// Initialize default values
 				constructed.getModifier().writeDefaults();
 
+				// Make sure watchable collections can be cloned
+				if (type == PacketType.Play.Server.ENTITY_METADATA) {
+					constructed.getWatchableCollectionModifier().write(0, Util.asList(
+							new WrappedWatchableObject(new WrappedDataWatcherObject(0, Registry.get(Byte.class)), (byte) 1),
+							new WrappedWatchableObject(new WrappedDataWatcherObject(0, Registry.get(String.class)), "String"),
+							new WrappedWatchableObject(new WrappedDataWatcherObject(0, Registry.get(Float.class)), 1.0F)
+					));
+				}
+
 				// Clone the packet
 				PacketContainer cloned = constructed.deepClone();
 
@@ -549,6 +561,10 @@ public class PacketContainerTest {
 			}
 		}
 
+		if (EqualsBuilder.reflectionEquals(a, b)) {
+			return;
+		}
+
 		assertEquals(a, b);
 	}