Don't wrap attribute in holder < 1.20.5

Addresses #2998

Author: dmulloy2

src/main/java/com/comphenix/protocol/wrappers/WrappedAttribute.java

@@ -1,5 +1,14 @@
 package com.comphenix.protocol.wrappers;
 
+import java.lang.reflect.Constructor;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
 import com.comphenix.protocol.PacketType;
 import com.comphenix.protocol.events.PacketContainer;
 import com.comphenix.protocol.reflect.FuzzyReflection;
@@ -9,37 +18,33 @@
 import com.comphenix.protocol.utility.MinecraftVersion;
 import com.comphenix.protocol.wrappers.collection.CachedSet;
 import com.comphenix.protocol.wrappers.collection.ConvertedSet;
+
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Sets;
 
-import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
-import java.lang.reflect.Constructor;
-import java.util.*;
-
 /**
  * Represents a single attribute sent in packet 44.
  *
  * @author Kristian
  */
-public class WrappedAttribute extends AbstractWrapper {
-    public static boolean KEY_WRAPPED = MinecraftVersion.NETHER_UPDATE.atOrAbove();
-    public static boolean IS_STATIC = MinecraftVersion.CAVES_CLIFFS_1.atOrAbove();
-    public static boolean IS_IN_HOLDER = MinecraftVersion.v1_20_5.atOrAbove();
+public final class WrappedAttribute extends AbstractWrapper {
+    static final boolean KEY_WRAPPED = MinecraftVersion.NETHER_UPDATE.atOrAbove();
+    static final boolean IS_STATIC = MinecraftVersion.CAVES_CLIFFS_1.atOrAbove();
+    static final boolean IS_IN_HOLDER = MinecraftVersion.v1_20_5.atOrAbove();
 
     // Shared structure modifier
-    private static StructureModifier<Object> ATTRIBUTE_MODIFIER;
+    static StructureModifier<Object> ATTRIBUTE_MODIFIER;
 
     // The one constructor
-    private static Constructor<?> ATTRIBUTE_CONSTRUCTOR;
+    static Constructor<?> ATTRIBUTE_CONSTRUCTOR;
 
     /**
      * Remaps attributes if needed.
      *
      * @return the remapped attribute or the attribute itself if no remapping was necessary.
      */
-    private static String remap(String string) {
+    static String remap(String string) {
         switch (string) {
             case "generic.maxHealth": return "generic.max_health";
             case "generic.followRange": return "generic.follow_range";
@@ -55,11 +60,7 @@ private static String remap(String string) {
         }
     }
 
-    /**
-     * Reference to the underlying attribute snapshot.
-     */
-    protected Object handle;
-    protected StructureModifier<Object> modifier;
+    StructureModifier<Object> modifier;
 
     // Cached computed value
     private double computedValue = Double.NaN;
@@ -72,7 +73,7 @@ private static String remap(String string) {
      *
      * @param handle - the NMS instance.
      */
-    private WrappedAttribute(@Nonnull Object handle) {
+    WrappedAttribute(@Nonnull Object handle) {
         super(MinecraftReflection.getAttributeSnapshotClass());
         setHandle(handle);
 
@@ -83,7 +84,6 @@ private WrappedAttribute(@Nonnull Object handle) {
         this.modifier = ATTRIBUTE_MODIFIER.withTarget(handle);
     }
 
-
     /**
      * Construct a new wrapped attribute around a specific NMS instance.
      *
@@ -205,7 +205,7 @@ public PacketContainer getParentPacket() {
      * @return TRUE if it does, FALSE otherwise.
      */
     public boolean hasModifier(UUID id) {
-        return getModifiers().contains(WrappedAttributeModifier.newBuilder(id).build());
+        return getModifierByUUID(id) != null;
     }
 
     /**
@@ -214,14 +214,14 @@ public boolean hasModifier(UUID id) {
      * @param id - the id to look for.
      * @return The single attribute modifier with the given ID.
      */
+    @Nullable
     public WrappedAttributeModifier getModifierByUUID(UUID id) {
-        if (hasModifier(id)) {
-            for (WrappedAttributeModifier modifier : getModifiers()) {
-                if (Objects.equal(modifier.getUUID(), id)) {
-                    return modifier;
-                }
+        for (WrappedAttributeModifier modifier : getModifiers()) {
+            if (Objects.equal(modifier.getUUID(), id)) {
+                return modifier;
             }
         }
+
         return null;
     }
 
@@ -283,7 +283,7 @@ public boolean equals(Object obj) {
     public int hashCode() {
         if (attributeModifiers == null)
             getModifiers();
-        return Objects.hashCode(getAttributeKey(), getBaseValue(), attributeModifiers);
+        return Objects.hashCode(getAttributeKey(), getBaseValue(), attributeModifiers.hashCode());
     }
 
     /**
@@ -365,7 +365,7 @@ public static class Builder {
         private PacketContainer packet;
         private Collection<WrappedAttributeModifier> modifiers = Collections.emptyList();
 
-        private Builder(WrappedAttribute template) {
+        Builder(WrappedAttribute template) {
             if (template != null) {
                 baseValue = template.getBaseValue();
                 attributeKey = template.getAttributeKey();
@@ -431,7 +431,7 @@ public Builder packet(PacketContainer packet) {
          * @return Unwrapped modifiers.
          */
         private Set<Object> getUnwrappedModifiers() {
-            final Set<Object> output = new HashSet<>();
+            Set<Object> output = new HashSet<>();
 
             for (WrappedAttributeModifier modifier : modifiers) {
                 output.add(modifier.getHandle());
@@ -476,7 +476,9 @@ public WrappedAttribute build() {
                     throw new IllegalArgumentException("Invalid attribute name: " + this.attributeKey);
                 }
 
-                attributeKey = registry.getHolder(attributeKey);
+                if (IS_IN_HOLDER) {
+                    attributeKey = registry.getHolder(attributeKey);
+                }
             } else {
                 attributeKey = this.attributeKey;
             }

src/test/java/com/comphenix/protocol/wrappers/WrappedAttributeTest.java

@@ -7,6 +7,7 @@
 import com.comphenix.protocol.PacketType;
 import com.comphenix.protocol.events.PacketContainer;
 import com.comphenix.protocol.wrappers.WrappedAttributeModifier.Operation;
+
 import com.google.common.collect.Lists;
 import net.minecraft.core.Holder;
 import net.minecraft.core.IRegistry;
@@ -17,12 +18,9 @@
 import net.minecraft.world.entity.ai.attributes.AttributeModifier;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotSame;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;
 
 public class WrappedAttributeTest {
 
@@ -71,7 +69,6 @@ public void testEquality() {
     }
 
     @Test
-    @Disabled // TODO -- modifiers are missing (or the hasModifier check is wrong)
     public void testAttribute() {
         assertEquals(this.attribute, WrappedAttribute.fromHandle(this.getAttributeCopy(this.attribute)));