fix: NMS exceptions being thrown when applying modifiers

Spigot's validation for this is like my hoover: it sucks.

bukkit/build.gradle

@@ -10,7 +10,7 @@ dependencies {
     implementation 'net.kyori:adventure-platform-bukkit:4.3.3'
     implementation 'dev.triumphteam:triumph-gui:3.1.10'
     implementation 'space.arim.morepaperlib:morepaperlib:0.4.4'
-    implementation 'de.tr7zw:item-nbt-api:2.13.1-SNAPSHOT'
+    implementation 'de.tr7zw:item-nbt-api:2.13.1'
 
     compileOnly 'org.spigotmc:spigot-api:1.17.1-R0.1-SNAPSHOT'
     compileOnly 'com.github.retrooper.packetevents:spigot:2.3.0'

bukkit/src/main/java/net/william278/husksync/data/BukkitData.java

@@ -25,6 +25,7 @@ import com.google.gson.annotations.SerializedName;
 import de.tr7zw.changeme.nbtapi.NBTCompound;
 import de.tr7zw.changeme.nbtapi.NBTPersistentDataContainer;
 import lombok.*;
+import net.kyori.adventure.util.TriState;
 import net.william278.desertwell.util.ThrowingConsumer;
 import net.william278.desertwell.util.Version;
 import net.william278.husksync.BukkitHuskSync;
@@ -35,6 +36,7 @@ import org.bukkit.Bukkit;
 import org.bukkit.Material;
 import org.bukkit.Registry;
 import org.bukkit.Statistic;
+import org.bukkit.NamespacedKey;
 import org.bukkit.advancement.AdvancementProgress;
 import org.bukkit.attribute.AttributeInstance;
 import org.bukkit.attribute.AttributeModifier;
@@ -565,6 +567,11 @@ public abstract class BukkitData implements Data {
     @NoArgsConstructor(access = AccessLevel.PRIVATE)
     public static class Attributes extends BukkitData implements Data.Attributes, Adaptable {
 
+        private static final String EQUIPMENT_SLOT_GROUP = "org.bukkit.inventory.EquipmentSlotGroup";
+        private static final String EQUIPMENT_SLOT_GROUP$ANY = "ANY";
+        private static final String EQUIPMENT_SLOT$getGroup = "getGroup";
+        private static TriState USE_KEYED_MODIFIERS = TriState.NOT_SET;
+
         private List<Attribute> attributes;
 
         @NotNull
@@ -572,12 +579,12 @@ public abstract class BukkitData implements Data {
             final List<Attribute> attributes = Lists.newArrayList();
             Registry.ATTRIBUTE.forEach(id -> {
                 final AttributeInstance instance = player.getAttribute(id);
-                if (instance == null || instance.getValue() == instance.getDefaultValue() || plugin
+                if (instance == null || Double.compare(instance.getValue(), instance.getDefaultValue()) == 0
-                        .getSettings().getSynchronization().isIgnoredAttribute(id.getKey().toString())) {
+                        || plugin.getSettings().getSynchronization().isIgnoredAttribute(id.getKey().toString())) {
                     // We don't sync unmodified or disabled attributes
                     return;
                 }
-                attributes.add(adapt(instance, plugin.getMinecraftVersion()));
+                attributes.add(adapt(instance));
             });
             return new BukkitData.Attributes(attributes);
         }
@@ -596,18 +603,18 @@ public abstract class BukkitData implements Data {
         }
 
         @NotNull
-        private static Attribute adapt(@NotNull AttributeInstance instance, @NotNull Version version) {
+        private static Attribute adapt(@NotNull AttributeInstance instance) {
             return new Attribute(
                     instance.getAttribute().getKey().toString(),
                     instance.getBaseValue(),
-                    instance.getModifiers().stream().map(m -> adapt(m, version)).collect(Collectors.toSet())
+                    instance.getModifiers().stream().map(BukkitData.Attributes::adapt).collect(Collectors.toSet())
             );
         }
 
         @NotNull
-        private static Modifier adapt(@NotNull AttributeModifier modifier, @NotNull Version version) {
+        private static Modifier adapt(@NotNull AttributeModifier modifier) {
             return new Modifier(
-                    version.compareTo(Version.fromString("1.21")) >= 0 ? null : modifier.getUniqueId(),
+                    getModifierId(modifier),
                     modifier.getName(),
                     modifier.getAmount(),
                     modifier.getOperation().ordinal(),
@@ -615,28 +622,71 @@ public abstract class BukkitData implements Data {
             );
         }
 
-        @Override
+        @Nullable
-        public void apply(@NotNull BukkitUser user, @NotNull BukkitHuskSync plugin) throws IllegalStateException {
+        private static UUID getModifierId(@NotNull AttributeModifier modifier) {
-            Registry.ATTRIBUTE.forEach(id -> applyAttribute(user.getPlayer().getAttribute(id), getAttribute(id).orElse(null)));
+            try {
+                return modifier.getUniqueId();
+            } catch (Throwable e) {
+                return null;
+            }
         }
 
-        private static void applyAttribute(@Nullable AttributeInstance instance, @Nullable Attribute attribute) {
+        private static void applyAttribute(@Nullable AttributeInstance instance, @Nullable Attribute attribute,
+                                           @NotNull HuskSync plugin) {
             if (instance == null) {
                 return;
             }
             instance.setBaseValue(attribute == null ? instance.getDefaultValue() : attribute.baseValue());
             instance.getModifiers().forEach(instance::removeModifier);
             if (attribute != null) {
-                attribute.modifiers().forEach(modifier -> instance.addModifier(new AttributeModifier(
+                attribute.modifiers().forEach(modifier -> instance.addModifier(adapt(modifier, plugin)));
-                        modifier.uuid(),
-                        modifier.name(),
-                        modifier.amount(),
-                        AttributeModifier.Operation.values()[modifier.operationType()],
-                        modifier.equipmentSlot() != -1 ? EquipmentSlot.values()[modifier.equipmentSlot()] : null
-                )));
             }
         }
 
+        @SuppressWarnings("JavaReflectionMemberAccess")
+        @NotNull
+        private static AttributeModifier adapt(@NotNull Modifier modifier, @NotNull HuskSync plugin) {
+            final int slotId = modifier.equipmentSlot();
+            if (USE_KEYED_MODIFIERS == TriState.NOT_SET) {
+                USE_KEYED_MODIFIERS = TriState.byBoolean(plugin.getMinecraftVersion()
+                        .compareTo(Version.fromString("1.21")) >= 0);
+            }
+            if (USE_KEYED_MODIFIERS == TriState.TRUE) {
+                try {
+                    // Reflexively create a modern keyed attribute modifier instance. Remove in favor of API long-term.
+                    final EquipmentSlot slot = slotId != -1 ? EquipmentSlot.values()[slotId] : null;
+                    final Class<?> slotGroup = Class.forName(EQUIPMENT_SLOT_GROUP);
+                    final String modifierName = modifier.name() == null ? modifier.uuid().toString() : modifier.name();
+                    return AttributeModifier.class.getDeclaredConstructor(
+                            NamespacedKey.class, double.class, AttributeModifier.Operation.class, slotGroup
+                    ).newInstance(
+                            NamespacedKey.fromString(modifierName),
+                            modifier.amount(),
+                            AttributeModifier.Operation.values()[modifier.operationType()],
+                            slot == null ? slotGroup.getField(EQUIPMENT_SLOT_GROUP$ANY).get(null)
+                                    : EquipmentSlot.class.getDeclaredMethod(EQUIPMENT_SLOT$getGroup).invoke(slot)
+                    );
+                } catch (Throwable e) {
+                    plugin.log(Level.WARNING, "Error reflectively creating keyed attribute modifier", e);
+                    USE_KEYED_MODIFIERS = TriState.FALSE;
+                }
+            }
+            return new AttributeModifier(
+                    modifier.uuid(),
+                    modifier.name(),
+                    modifier.amount(),
+                    AttributeModifier.Operation.values()[modifier.operationType()],
+                    slotId != -1 ? EquipmentSlot.values()[slotId] : null
+            );
+        }
+
+        @Override
+        public void apply(@NotNull BukkitUser user, @NotNull BukkitHuskSync plugin) throws IllegalStateException {
+            Registry.ATTRIBUTE.forEach(id -> applyAttribute(
+                    user.getPlayer().getAttribute(id), getAttribute(id).orElse(null), plugin
+            ));
+        }
+
     }
 
     @Getter
@@ -696,11 +746,12 @@ public abstract class BukkitData implements Data {
             }
 
             // Set health scale
+            double scale = healthScale <= 0 ? player.getMaxHealth() : healthScale;
             try {
-                player.setHealthScale(healthScale);
+                player.setHealthScale(scale);
                 player.setHealthScaled(isHealthScaled);
             } catch (Throwable e) {
-                plugin.log(Level.WARNING, "Error setting %s's health scale to %s".formatted(player.getName(), healthScale), e);
+                plugin.log(Level.WARNING, "Error setting %s's health scale to %s".formatted(player.getName(), scale), e);
             }
         }
 

bukkit/src/main/java/net/william278/husksync/data/BukkitSerializer.java

@@ -165,6 +165,7 @@ public class BukkitSerializer {
                 case "1.20", "1.20.1", "1.20.2" -> DataFixerUtil.VERSION1_20_2;
                 case "1.20.3", "1.20.4" -> DataFixerUtil.VERSION1_20_4;
                 case "1.20.5", "1.20.6" -> DataFixerUtil.VERSION1_20_5;
+                case "1.21" -> DataFixerUtil.VERSION1_21;
                 default -> DataFixerUtil.getCurrentVersion();
             };
         }

common/src/main/java/net/william278/husksync/data/Data.java

@@ -155,14 +155,14 @@ public interface Data {
      */
     interface Advancements extends Data {
 
+        String RECIPE_ADVANCEMENT = "minecraft:recipe";
+
         @NotNull
         List<Advancement> getCompleted();
 
         @NotNull
         default List<Advancement> getCompletedExcludingRecipes() {
-            return getCompleted().stream()
+            return getCompleted().stream().filter(adv -> !adv.getKey().startsWith(RECIPE_ADVANCEMENT)).toList();
-                .filter(advancement -> !advancement.getKey().startsWith("minecraft:recipe"))
-                .collect(Collectors.toList());
         }
 
         void setCompleted(@NotNull List<Advancement> completed);
@@ -191,13 +191,13 @@ public interface Data {
             @NotNull
             private static Map<String, Long> adaptDateMap(@NotNull Map<String, Date> dateMap) {
                 return dateMap.entrySet().stream()
-                    .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getTime()));
+                        .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getTime()));
             }
 
             @NotNull
             private static Map<String, Date> adaptLongMap(@NotNull Map<String, Long> dateMap) {
                 return dateMap.entrySet().stream()
-                    .collect(Collectors.toMap(Map.Entry::getKey, e -> new Date(e.getValue())));
+                        .collect(Collectors.toMap(Map.Entry::getKey, e -> new Date(e.getValue())));
             }
 
             @NotNull
@@ -250,9 +250,9 @@ public interface Data {
         void setWorld(@NotNull World world);
 
         record World(
-            @SerializedName("name") @NotNull String name,
+                @SerializedName("name") @NotNull String name,
-            @SerializedName("uuid") @NotNull UUID uuid,
+                @SerializedName("uuid") @NotNull UUID uuid,
-            @SerializedName("environment") @NotNull String environment
+                @SerializedName("environment") @NotNull String environment
         ) {
         }
     }
@@ -324,9 +324,9 @@ public interface Data {
         List<Attribute> getAttributes();
 
         record Attribute(
-            @NotNull String name,
+                @NotNull String name,
-            double baseValue,
+                double baseValue,
-            @NotNull Set<Modifier> modifiers
+                @NotNull Set<Modifier> modifiers
         ) {
 
             public double getValue() {
@@ -387,8 +387,8 @@ public interface Data {
 
         default Optional<Attribute> getAttribute(@NotNull Key key) {
             return getAttributes().stream()
-                .filter(attribute -> attribute.name().equals(key.asString()))
+                    .filter(attribute -> attribute.name().equals(key.asString()))
-                .findFirst();
+                    .findFirst();
         }
 
         default void removeAttribute(@NotNull Key key) {
@@ -397,8 +397,8 @@ public interface Data {
 
         default double getMaxHealth() {
             return getAttribute(MAX_HEALTH_KEY)
-                .map(Attribute::getValue)
+                    .map(Attribute::getValue)
-                .orElse(20.0);
+                    .orElse(20.0);
         }
 
         default void setMaxHealth(double maxHealth) {

common/src/main/java/net/william278/husksync/data/DataSnapshot.java

@@ -395,7 +395,7 @@ public class DataSnapshot {
                     .map(entry -> Map.entry(plugin.getIdentifier(entry.getKey()).orElseThrow(), entry.getValue()))
                     .collect(Collectors.toMap(
                             Map.Entry::getKey,
-                            entry -> plugin.deserializeData(entry.getKey(), entry.getValue()),
+                            entry -> plugin.deserializeData(entry.getKey(), entry.getValue(), getMinecraftVersion()),
                             (a, b) -> b, () -> Maps.newTreeMap(SerializerRegistry.DEPENDENCY_ORDER_COMPARATOR)
                     ));
         }

common/src/main/java/net/william278/husksync/data/SerializerRegistry.java

@@ -19,6 +19,7 @@
 
 package net.william278.husksync.data;
 
+import net.william278.desertwell.util.Version;
 import net.william278.husksync.HuskSync;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
@@ -119,19 +120,36 @@ public interface SerializerRegistry {
     }
 
     /**
-     * Deserialize data for the given {@link Identifier}
+     * Deserialize data of a given {@link Version Minecraft version} for the given {@link Identifier data identifier}
+     *
+     * @param identifier    the {@link Identifier} to deserialize data for
+     * @param data          the data to deserialize
+     * @param dataMcVersion the Minecraft version of the data
+     * @return the deserialized data
+     * @throws IllegalStateException if no serializer is found for the given {@link Identifier}
+     * @since 3.6.4
+     */
+    @NotNull
+    default Data deserializeData(@NotNull Identifier identifier, @NotNull String data,
+                                 @NotNull Version dataMcVersion) throws IllegalStateException {
+        return getSerializer(identifier).map(serializer -> serializer.deserialize(data, dataMcVersion)).orElseThrow(
+                () -> new IllegalStateException("No serializer found for %s".formatted(identifier))
+        );
+    }
+
+    /**
+     * Deserialize data for the given {@link Identifier data identifier}
      *
      * @param identifier the {@link Identifier} to deserialize data for
      * @param data       the data to deserialize
      * @return the deserialized data
-     * @throws IllegalStateException if no serializer is found for the given {@link Identifier}
      * @since 3.5.4
+     * @deprecated Use {@link #deserializeData(Identifier, String, Version)} instead
      */
     @NotNull
-    default Data deserializeData(@NotNull Identifier identifier, @NotNull String data) throws IllegalStateException {
+    @Deprecated(since = "3.6.5")
-        return getSerializer(identifier).map(serializer -> serializer.deserialize(data)).orElseThrow(
+    default Data deserializeData(@NotNull Identifier identifier, @NotNull String data) {
-                () -> new IllegalStateException("No serializer found for %s".formatted(identifier))
+        return deserializeData(identifier, data, getPlugin().getMinecraftVersion());
-        );
     }
 
     /**

gradle.properties

@@ -3,7 +3,7 @@ org.gradle.jvmargs='-Dfile.encoding=UTF-8'
 org.gradle.daemon=true
 javaVersion=17
 
-plugin_version=3.6.3
+plugin_version=3.6.5
 plugin_archive=husksync
 plugin_description=A modern, cross-server player data synchronization system
 

paper/build.gradle

@@ -50,7 +50,7 @@ shadowJar {
 
 tasks {
     runServer {
-        minecraftVersion('1.20.4')
+        minecraftVersion('1.21')
 
         downloadPlugins {
             url('https://download.luckperms.net/1549/bukkit/loader/LuckPerms-Bukkit-5.4.134.jar')