Improve cloning tests (#1822)

src/main/java/com/comphenix/protocol/injector/StructureCache.java

@@ -17,6 +17,11 @@
 
 package com.comphenix.protocol.injector;
 
+import java.util.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
+
 import com.comphenix.protocol.PacketType;
 import com.comphenix.protocol.injector.packet.PacketRegistry;
 import com.comphenix.protocol.reflect.StructureModifier;
@@ -26,13 +31,11 @@ import com.comphenix.protocol.reflect.instances.DefaultInstances;
 import com.comphenix.protocol.utility.ByteBuddyFactory;
 import com.comphenix.protocol.utility.MinecraftMethods;
 import com.comphenix.protocol.utility.MinecraftReflection;
+import com.comphenix.protocol.utility.MinecraftVersion;
 import com.comphenix.protocol.utility.ZeroBuffer;
 import com.comphenix.protocol.wrappers.WrappedChatComponent;
 import com.google.common.base.Preconditions;
 import io.netty.buffer.ByteBuf;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Supplier;
 import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
 import net.bytebuddy.implementation.FixedValue;
 import net.bytebuddy.matcher.ElementMatchers;
@@ -52,33 +55,34 @@ public class StructureCache {
 	private static boolean TRICK_TRIED = false;
 	private static ConstructorAccessor TRICKED_DATA_SERIALIZER;
 
-	public static Object newPacket(Class<?> clazz) {
+	public static Object newPacket(Class<?> packetClass) {
-		Object result = DefaultInstances.DEFAULT.create(clazz);
+		Supplier<Object> packetConstructor = PACKET_INSTANCE_CREATORS.computeIfAbsent(packetClass, clazz -> {
-
+			// prefer construction via PacketDataSerializer constructor on 1.17 and above
-		if (result == null) {
+			if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) {
-			return PACKET_INSTANCE_CREATORS.computeIfAbsent(clazz, $ -> {
+				ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull(
-				ConstructorAccessor accessor = Accessors.getConstructorAccessorOrNull(clazz,
+						clazz,
 						MinecraftReflection.getPacketDataSerializerClass());
-				if (accessor != null) {
+				if (serializerAccessor != null) {
 					return () -> {
-						try {
+						// the tricked serializer should always be present... If not try using a normal buffer
-							return accessor.invoke(MinecraftReflection.getPacketDataSerializer(new ZeroBuffer()));
+						Object dataSerializer = getTrickDataSerializerOrNull();
-						} catch (Exception exception) {
+						if (dataSerializer == null) {
-							// try trick nms around as they want a non-null compound in the map_chunk packet constructor
+							dataSerializer = MinecraftReflection.getPacketDataSerializer(new ZeroBuffer());
-							Object trickyDataSerializer = getTrickDataSerializerOrNull();
-							if (trickyDataSerializer != null) {
-								return accessor.invoke(trickyDataSerializer);
-							}
-							// the tricks are over
-							throw new IllegalArgumentException("Unable to create packet " + clazz, exception);
 						}
+
+						return serializerAccessor.invoke(dataSerializer);
 					};
 				}
-				throw new IllegalArgumentException("No matching constructor to create packet in class " + clazz);
+			}
-			}).get();
-		}
 
-		return result;
+			// try via DefaultInstances as fallback
+			return () -> {
+				Object packetInstance = DefaultInstances.DEFAULT.create(clazz);
+				Objects.requireNonNull(packetInstance, "Unable to create packet instance for class " + clazz);
+				return packetInstance;
+			};
+		});
+		return packetConstructor.get();
 	}
 
 	/**
@@ -113,17 +117,15 @@ public class StructureCache {
 	public static StructureModifier<Object> getStructure(final PacketType packetType) {
 		Preconditions.checkNotNull(packetType, "type cannot be null");
 
-		StructureModifier<Object> modifier = STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> {
+		return STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> {
 			Class<?> packetClass = PacketRegistry.getPacketClassFromType(type);
 			return new StructureModifier<>(packetClass, MinecraftReflection.getPacketClass(), true);
 		});
-
-		return modifier;
 	}
 
 	/**
-	 * Creates a packet data serializer sub-class if needed to allow the fixed read of a NbtTagCompound because of a null
+	 * Creates a packet data serializer sub-class if needed to allow the fixed read of a NbtTagCompound because of a
-	 * check in the MapChunk packet constructor.
+	 * null check in the MapChunk packet constructor.
 	 *
 	 * @return an accessor to a constructor which creates a data serializer.
 	 */

src/main/java/com/comphenix/protocol/reflect/StructureModifier.java

@@ -269,13 +269,8 @@ public class StructureModifier<T> {
 			return null;
 		}
 
-		// don't try to convert null, just return null
+		// get the field value and convert it if needed
 		Object fieldValue = accessor.get(this.target);
-		if (fieldValue == null) {
-			return null;
-		}
-
-		// check if we need to convert the field value and do so if needed
 		return this.needConversion() ? this.converter.getSpecific(fieldValue) : (T) fieldValue;
 	}
 
@@ -332,14 +327,10 @@ public class StructureModifier<T> {
 			return this;
 		}
 
-		// just write the value if the specific given one is null or no conversion is needed
-		if (value == null || !this.needConversion()) {
-			accessor.set(this.target, value);
-			return this;
-		}
-
 		// convert and write
-		accessor.set(this.target, this.converter.getGeneric(value));
+		Object fieldValue = this.needConversion() ? this.converter.getGeneric(value) : value;
+		accessor.set(this.target, fieldValue);
+
 		return this;
 	}
 

src/main/java/com/comphenix/protocol/reflect/cloning/ImmutableDetector.java

@@ -76,6 +76,9 @@ public class ImmutableDetector implements Cloner {
 			add("world.entity.npc.VillagerProfession", "VillagerProfession");
 		}
 
+		add("world.entity.animal.CatVariant");
+		add("world.entity.animal.FrogVariant");
+
 		// TODO automatically detect the technically-not-an-enum enums that Mojang is so fond of
 		// Would also probably go in tandem with having the FieldCloner use this
 

src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java

@@ -2,55 +2,64 @@
  *  ProtocolLib - Bukkit server library that allows access to the Minecraft protocol.
  *  Copyright (C) 2012 Kristian S. Stangeland
  *
- *  This program is free software; you can redistribute it and/or modify it under the terms of the 
+ *  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 
+ *  GNU General Public License as published by the Free Software Foundation; either version 2 of
  *  the License, or (at your option) any later version.
  *
- *  This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; 
+ *  This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
- *  without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. 
+ *  without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
  *  See the GNU General Public License for more details.
  *
- *  You should have received a copy of the GNU General Public License along with this program; 
+ *  You should have received a copy of the GNU General Public License along with this program;
- *  if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 
+ *  if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
  *  02111-1307 USA
  */
 
 package com.comphenix.protocol.reflect.instances;
 
 import java.lang.reflect.Array;
-
+import java.util.Optional;
-import javax.annotation.Nullable;
 
 import com.google.common.base.Defaults;
 import com.google.common.primitives.Primitives;
+import javax.annotation.Nullable;
 
 /**
- * Provides constructors for primtive types, wrappers, arrays and strings.
+ * Provides constructors for primitive java types and wrappers.
+ *
  * @author Kristian
  */
 public class PrimitiveGenerator implements InstanceProvider {
-	
+
 	/**
 	 * Default value for Strings.
 	 */
+	@Deprecated
 	public static final String STRING_DEFAULT = "";
-	
+
 	/**
 	 * Shared instance of this generator.
 	 */
-	public static PrimitiveGenerator INSTANCE = new PrimitiveGenerator(STRING_DEFAULT);
+	public static PrimitiveGenerator INSTANCE = new PrimitiveGenerator();
 
 	// Our default string value
-	private String stringDefault;
+	private final String stringDefault;
-	
+
+	public PrimitiveGenerator() {
+		this.stringDefault = "";
+	}
+
+	@Deprecated
 	public PrimitiveGenerator(String stringDefault) {
 		this.stringDefault = stringDefault;
 	}
-	
+
 	/**
 	 * Retrieve the string default.
+	 *
 	 * @return Default instance of a string.
 	 */
+	@Deprecated
 	public String getStringDefault() {
 		return stringDefault;
 	}
@@ -68,13 +77,16 @@ public class PrimitiveGenerator implements InstanceProvider {
 			return Array.newInstance(arrayType, 0);
 		} else if (type.isEnum()) {
 			Object[] values = type.getEnumConstants();
-			if (values != null && values.length > 0)
+			if (values != null && values.length > 0) {
 				return values[0];
+			}
 		} else if (type.equals(String.class)) {
-			return stringDefault;
+			return this.stringDefault;
-		} 
+		} else if (type.equals(Optional.class)) {
-		
+			return Optional.empty();
+		}
+
 		// Cannot handle this type
 		return null;
-	}	
+	}
 }

src/main/java/com/comphenix/protocol/utility/ZeroBuffer.java

@@ -366,7 +366,7 @@ public class ZeroBuffer extends ByteBuf {
 
 	@Override
 	public short readUnsignedByte() {
-		return 0;
+		return 255;
 	}
 
 	@Override

src/test/java/com/comphenix/protocol/events/PacketContainerTest.java

@@ -35,6 +35,7 @@ import com.comphenix.protocol.reflect.StructureModifier;
 import com.comphenix.protocol.reflect.accessors.Accessors;
 import com.comphenix.protocol.reflect.accessors.FieldAccessor;
 import com.comphenix.protocol.reflect.cloning.SerializableCloner;
+import com.comphenix.protocol.utility.MinecraftMethods;
 import com.comphenix.protocol.utility.MinecraftReflection;
 import com.comphenix.protocol.wrappers.BlockPosition;
 import com.comphenix.protocol.wrappers.BukkitConverters;
@@ -67,6 +68,7 @@ import net.md_5.bungee.api.chat.ComponentBuilder;
 import net.md_5.bungee.api.chat.HoverEvent;
 import net.md_5.bungee.api.chat.hover.content.Text;
 import net.minecraft.core.IRegistry;
+import net.minecraft.network.protocol.game.PacketPlayOutGameStateChange;
 import net.minecraft.network.protocol.game.PacketPlayOutUpdateAttributes;
 import net.minecraft.network.protocol.game.PacketPlayOutUpdateAttributes.AttributeSnapshot;
 import net.minecraft.resources.MinecraftKey;
@@ -74,9 +76,8 @@ import net.minecraft.world.effect.MobEffect;
 import net.minecraft.world.effect.MobEffectList;
 import net.minecraft.world.entity.ai.attributes.AttributeBase;
 import net.minecraft.world.entity.ai.attributes.AttributeModifier;
-import net.minecraft.world.entity.npc.VillagerData;
+import net.minecraft.world.entity.animal.CatVariant;
-import net.minecraft.world.entity.npc.VillagerProfession;
+import net.minecraft.world.entity.animal.FrogVariant;
-import net.minecraft.world.entity.npc.VillagerType;
 import org.apache.commons.lang.SerializationUtils;
 import org.bukkit.ChatColor;
 import org.bukkit.Material;
@@ -794,7 +795,7 @@ public class PacketContainerTest {
 		assertArrayEquals(signature, read.getSignature());
 	}*/
 
-	private void assertPacketsEqual(PacketContainer constructed, PacketContainer cloned) {
+	private void assertPacketsEqualAndSerializable(PacketContainer constructed, PacketContainer cloned) {
 		StructureModifier<Object> firstMod = constructed.getModifier(), secondMod = cloned.getModifier();
 		assertEquals(firstMod.size(), secondMod.size());
 
@@ -810,6 +811,9 @@ public class PacketContainerTest {
 				}
 			}
 		}
+
+		Object buffer = MinecraftReflection.createPacketDataSerializer(0);
+		MinecraftMethods.getPacketWriteByteBufMethod().invoke(cloned.getHandle(), buffer);
 	}
 
 	@Test
@@ -833,32 +837,59 @@ public class PacketContainerTest {
 							new WrappedWatchableObject(
 									new WrappedDataWatcherObject(0, Registry.get(Byte.class)),
 									(byte) 1),
+							new WrappedWatchableObject(
+									new WrappedDataWatcherObject(0, Registry.get(Integer.class)),
+									1),
+							new WrappedWatchableObject(
+									new WrappedDataWatcherObject(0, Registry.get(Float.class)),
+									1F),
 							new WrappedWatchableObject(
 									new WrappedDataWatcherObject(0, Registry.get(String.class)),
 									"String"),
 							new WrappedWatchableObject(
-									new WrappedDataWatcherObject(0, Registry.get(Float.class)),
+									new WrappedDataWatcherObject(0, Registry.get(Boolean.class)),
-									1.0F),
+									true),
 							new WrappedWatchableObject(
 									new WrappedDataWatcherObject(0, Registry.getChatComponentSerializer(true)),
 									Optional.of(ComponentConverter.fromBaseComponent(TEST_COMPONENT).getHandle())),
 							new WrappedWatchableObject(
-									new WrappedDataWatcherObject(0, Registry.get(VillagerData.class)),
+									new WrappedDataWatcherObject(0, Registry.getItemStackSerializer(false)),
-									new VillagerData(VillagerType.b, VillagerProfession.c, 69))
+									BukkitConverters.getItemStackConverter().getGeneric(new ItemStack(Material.WOODEN_AXE))),
+							new WrappedWatchableObject(
+									new WrappedDataWatcherObject(0, Registry.get(CatVariant.class)),
+									CatVariant.a),
+							new WrappedWatchableObject(
+									new WrappedDataWatcherObject(0, Registry.get(FrogVariant.class)),
+									FrogVariant.c)
 					));
 				} else if (type == PacketType.Play.Server.CHAT) {
 					constructed.getChatComponents().write(0, ComponentConverter.fromBaseComponent(TEST_COMPONENT));
+				} else if (type == PacketType.Play.Server.REMOVE_ENTITY_EFFECT || type == PacketType.Play.Server.ENTITY_EFFECT) {
+					constructed.getEffectTypes().write(0, PotionEffectType.GLOWING);
+				} else if (type == PacketType.Play.Server.GAME_STATE_CHANGE) {
+					constructed.getStructures().write(
+							0,
+							InternalStructure.getConverter().getSpecific(PacketPlayOutGameStateChange.a));
+				} else if (type == PacketType.Play.Client.USE_ITEM || type == PacketType.Play.Client.BLOCK_PLACE) {
+					constructed.getLongs().write(0, 0L); // timestamp of the packet, not sent over the network
 				}
 
 				// gives some indication which cloning process fails as the checks itself are happening outside this method
 				System.out.println("Cloning " + type);
 
-				// Clone the packet both ways
+				// Clone the packet all three ways
 				PacketContainer shallowCloned = constructed.shallowClone();
-				this.assertPacketsEqual(constructed, shallowCloned);
+				this.assertPacketsEqualAndSerializable(constructed, shallowCloned);
 
 				PacketContainer deepCloned = constructed.deepClone();
-				this.assertPacketsEqual(constructed, deepCloned);
+				this.assertPacketsEqualAndSerializable(constructed, deepCloned);
+
+				PacketContainer serializedCloned = SerializableCloner.clone(constructed);
+				if (type == PacketType.Play.Client.USE_ITEM || type == PacketType.Play.Client.BLOCK_PLACE) {
+					// shit fix - but what are we supposed to do :/
+					serializedCloned.getLongs().write(0, 0L);
+				}
+				this.assertPacketsEqualAndSerializable(constructed, serializedCloned);
 			} catch (Exception ex) {
 				Assertions.fail("Unable to clone " + type, ex);
 			}