From b133887b851c976b0e426784c6e5ebd68326aff9 Mon Sep 17 00:00:00 2001 From: CraftBukkit/Spigot Date: Sat, 31 Aug 2024 08:40:57 +1000 Subject: [PATCH] SPIGOT-7882, #1467: Fix conversion of name in Profile Component to empty if it is missing By: Doc --- .../bukkit/craftbukkit/block/CraftSkull.java | 30 ++++----- .../craftbukkit/inventory/CraftMetaSkull.java | 62 ++++++++----------- .../profile/CraftPlayerProfile.java | 53 +++++++++++----- .../profile/PlayerProfileTest.java | 6 +- 4 files changed, 85 insertions(+), 66 deletions(-) diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/block/CraftSkull.java b/paper-server/src/main/java/org/bukkit/craftbukkit/block/CraftSkull.java index 9a94707f58..650e25c17e 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/block/CraftSkull.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/block/CraftSkull.java @@ -27,7 +27,7 @@ import org.jetbrains.annotations.Nullable; public class CraftSkull extends CraftBlockEntityState implements Skull { private static final int MAX_OWNER_LENGTH = 16; - private GameProfile profile; + private ResolvableProfile profile; public CraftSkull(World world, TileEntitySkull tileEntity) { super(world, tileEntity); @@ -43,7 +43,7 @@ public class CraftSkull extends CraftBlockEntityState implement ResolvableProfile owner = skull.getOwnerProfile(); if (owner != null) { - profile = owner.gameProfile(); + profile = owner; } } @@ -54,7 +54,7 @@ public class CraftSkull extends CraftBlockEntityState implement @Override public String getOwner() { - return hasOwner() ? profile.getName() : null; + return hasOwner() ? profile.name().orElse(null) : null; } @Override @@ -68,19 +68,19 @@ public class CraftSkull extends CraftBlockEntityState implement return false; } - this.profile = profile; + this.profile = new ResolvableProfile(profile); return true; } @Override public OfflinePlayer getOwningPlayer() { - if (profile != null) { - if (!profile.getId().equals(SystemUtils.NIL_UUID)) { - return Bukkit.getOfflinePlayer(profile.getId()); + if (hasOwner()) { + if (profile.id().filter(u -> !u.equals(SystemUtils.NIL_UUID)).isPresent()) { + return Bukkit.getOfflinePlayer(profile.id().get()); } - if (!profile.getName().isEmpty()) { - return Bukkit.getOfflinePlayer(profile.getName()); + if (profile.name().filter(s -> !s.isEmpty()).isPresent()) { + return Bukkit.getOfflinePlayer(profile.name().get()); } } @@ -91,10 +91,10 @@ public class CraftSkull extends CraftBlockEntityState implement public void setOwningPlayer(OfflinePlayer player) { Preconditions.checkNotNull(player, "player"); - if (player instanceof CraftPlayer) { - this.profile = ((CraftPlayer) player).getProfile(); + if (player instanceof CraftPlayer craftPlayer) { + this.profile = new ResolvableProfile(craftPlayer.getProfile()); } else { - this.profile = new GameProfile(player.getUniqueId(), player.getName()); + this.profile = new ResolvableProfile(new GameProfile(player.getUniqueId(), (player.getName() == null) ? "" : player.getName())); } } @@ -112,7 +112,7 @@ public class CraftSkull extends CraftBlockEntityState implement if (profile == null) { this.profile = null; } else { - this.profile = CraftPlayerProfile.validateSkullProfile(((CraftPlayerProfile) profile).buildGameProfile()); + this.profile = new ResolvableProfile(CraftPlayerProfile.validateSkullProfile(((CraftPlayerProfile) profile).buildGameProfile())); } } @@ -134,7 +134,7 @@ public class CraftSkull extends CraftBlockEntityState implement @Override public BlockFace getRotation() { BlockData blockData = getBlockData(); - return (blockData instanceof Rotatable) ? ((Rotatable) blockData).getRotation() : ((Directional) blockData).getFacing(); + return (blockData instanceof Rotatable rotatable) ? rotatable.getRotation() : ((Directional) blockData).getFacing(); } @Override @@ -187,7 +187,7 @@ public class CraftSkull extends CraftBlockEntityState implement super.applyTo(skull); if (getSkullType() == SkullType.PLAYER) { - skull.setOwner((profile != null) ? new ResolvableProfile(profile) : null); + skull.setOwner(hasOwner() ? profile : null); } } diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java b/paper-server/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java index 557be08f12..65bb9730c8 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaSkull.java @@ -36,15 +36,14 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { static final ItemMetaKeyType NOTE_BLOCK_SOUND = new ItemMetaKeyType<>(DataComponents.NOTE_BLOCK_SOUND, "note_block_sound"); static final int MAX_OWNER_LENGTH = 16; - private GameProfile profile; + private ResolvableProfile profile; private MinecraftKey noteBlockSound; CraftMetaSkull(CraftMetaItem meta) { super(meta); - if (!(meta instanceof CraftMetaSkull)) { + if (!(meta instanceof CraftMetaSkull skullMeta)) { return; } - CraftMetaSkull skullMeta = (CraftMetaSkull) meta; this.setProfile(skullMeta.profile); this.noteBlockSound = skullMeta.noteBlockSound; } @@ -52,21 +51,17 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { CraftMetaSkull(DataComponentPatch tag) { super(tag); - getOrEmpty(tag, SKULL_PROFILE).ifPresent((resolvableProfile) -> { - this.setProfile(resolvableProfile.gameProfile()); - }); + getOrEmpty(tag, SKULL_PROFILE).ifPresent(this::setProfile); - getOrEmpty(tag, NOTE_BLOCK_SOUND).ifPresent((minecraftKey) -> { - this.noteBlockSound = minecraftKey; - }); + getOrEmpty(tag, NOTE_BLOCK_SOUND).ifPresent((minecraftKey) -> this.noteBlockSound = minecraftKey); } CraftMetaSkull(Map map) { super(map); if (profile == null) { Object object = map.get(SKULL_OWNER.BUKKIT); - if (object instanceof PlayerProfile) { - setOwnerProfile((PlayerProfile) object); + if (object instanceof PlayerProfile playerProfile) { + setOwnerProfile(playerProfile); } else { setOwner(SerializableMeta.getString(map, SKULL_OWNER.BUKKIT, true)); } @@ -92,7 +87,7 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { skullTag.putUUID("Id", uuid); } - this.setProfile(ResolvableProfile.CODEC.parse(DynamicOpsNBT.INSTANCE, skullTag).result().get().gameProfile()); + ResolvableProfile.CODEC.parse(DynamicOpsNBT.INSTANCE, skullTag).result().ifPresent(this::setProfile); } if (tag.contains(BLOCK_ENTITY_TAG.NBT, CraftMagicNumbers.NBT.TAG_COMPOUND)) { @@ -103,7 +98,7 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { } } - private void setProfile(GameProfile profile) { + private void setProfile(ResolvableProfile profile) { this.profile = profile; } @@ -111,15 +106,15 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { void applyToItem(CraftMetaItem.Applicator tag) { super.applyToItem(tag); - if (profile != null) { + if (hasOwner()) { // SPIGOT-6558: Set initial textures - tag.put(SKULL_PROFILE, new ResolvableProfile(profile)); + tag.put(SKULL_PROFILE, profile); // Fill in textures PlayerProfile ownerProfile = new CraftPlayerProfile(profile); // getOwnerProfile may return null if (ownerProfile.getTextures().isEmpty()) { ownerProfile.update().thenAccept((filledProfile) -> { setOwnerProfile(filledProfile); - tag.put(SKULL_PROFILE, new ResolvableProfile(profile)); + tag.put(SKULL_PROFILE, profile); }); } } @@ -145,23 +140,23 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { @Override public boolean hasOwner() { - return profile != null && !profile.getName().isEmpty(); + return profile != null; } @Override public String getOwner() { - return hasOwner() ? profile.getName() : null; + return hasOwner() ? profile.name().orElse(null) : null; } @Override public OfflinePlayer getOwningPlayer() { if (hasOwner()) { - if (!profile.getId().equals(SystemUtils.NIL_UUID)) { - return Bukkit.getOfflinePlayer(profile.getId()); + if (profile.id().filter(u -> !u.equals(SystemUtils.NIL_UUID)).isPresent()) { + return Bukkit.getOfflinePlayer(profile.id().get()); } - if (!profile.getName().isEmpty()) { - return Bukkit.getOfflinePlayer(profile.getName()); + if (profile.name().filter(s -> !s.isEmpty()).isPresent()) { + return Bukkit.getOfflinePlayer(profile.name().get()); } } @@ -177,7 +172,7 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { if (name == null) { setProfile(null); } else { - setProfile(new GameProfile(SystemUtils.NIL_UUID, name)); + setProfile(new ResolvableProfile(new GameProfile(SystemUtils.NIL_UUID, name))); } return true; @@ -187,10 +182,10 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { public boolean setOwningPlayer(OfflinePlayer owner) { if (owner == null) { setProfile(null); - } else if (owner instanceof CraftPlayer) { - setProfile(((CraftPlayer) owner).getProfile()); + } else if (owner instanceof CraftPlayer craftPlayer) { + setProfile(new ResolvableProfile(craftPlayer.getProfile())); } else { - setProfile(new GameProfile(owner.getUniqueId(), owner.getName())); + setProfile(new ResolvableProfile(new GameProfile(owner.getUniqueId(), (owner.getName() == null) ? "" : owner.getName()))); } return true; @@ -207,10 +202,10 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { @Override public void setOwnerProfile(PlayerProfile profile) { - if (profile == null) { - setProfile(null); + if (profile instanceof CraftPlayerProfile craftPlayerProfile) { + setProfile(CraftPlayerProfile.validateSkullProfile(craftPlayerProfile.buildResolvableProfile())); } else { - setProfile(CraftPlayerProfile.validateSkullProfile(((CraftPlayerProfile) profile).buildGameProfile())); + setProfile(null); } } @@ -246,11 +241,8 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { if (!super.equalsCommon(meta)) { return false; } - if (meta instanceof CraftMetaSkull) { - CraftMetaSkull that = (CraftMetaSkull) meta; - - // SPIGOT-5403: equals does not check properties - return (this.profile != null ? that.profile != null && this.profile.equals(that.profile) && this.profile.getProperties().equals(that.profile.getProperties()) : that.profile == null) && Objects.equals(this.noteBlockSound, that.noteBlockSound); + if (meta instanceof CraftMetaSkull that) { + return Objects.equals(this.profile, that.profile) && Objects.equals(this.noteBlockSound, that.noteBlockSound); } return true; } @@ -264,7 +256,7 @@ class CraftMetaSkull extends CraftMetaItem implements SkullMeta { Builder serialize(Builder builder) { super.serialize(builder); - if (this.profile != null) { + if (this.hasOwner()) { builder.put(SKULL_OWNER.BUKKIT, new CraftPlayerProfile(this.profile)); } diff --git a/paper-server/src/main/java/org/bukkit/craftbukkit/profile/CraftPlayerProfile.java b/paper-server/src/main/java/org/bukkit/craftbukkit/profile/CraftPlayerProfile.java index 4aeb974e59..f382688596 100644 --- a/paper-server/src/main/java/org/bukkit/craftbukkit/profile/CraftPlayerProfile.java +++ b/paper-server/src/main/java/org/bukkit/craftbukkit/profile/CraftPlayerProfile.java @@ -12,6 +12,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; @@ -19,6 +20,7 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; import net.minecraft.SystemUtils; import net.minecraft.server.dedicated.DedicatedServer; +import net.minecraft.world.item.component.ResolvableProfile; import org.apache.commons.lang.StringUtils; import org.bukkit.Bukkit; import org.bukkit.configuration.serialization.SerializableAs; @@ -26,6 +28,7 @@ import org.bukkit.craftbukkit.CraftServer; import org.bukkit.craftbukkit.configuration.ConfigSerializationUtil; import org.bukkit.profile.PlayerProfile; import org.bukkit.profile.PlayerTextures; +import org.jetbrains.annotations.ApiStatus; @SerializableAs("PlayerProfile") public final class CraftPlayerProfile implements PlayerProfile { @@ -40,6 +43,15 @@ public final class CraftPlayerProfile implements PlayerProfile { return gameProfile; } + @Nonnull + public static ResolvableProfile validateSkullProfile(@Nonnull ResolvableProfile resolvableProfile) { + // The ResolvableProfile needs to contain either both a uuid and textures, or a name. + boolean isValidSkullProfile = (resolvableProfile.name().isPresent()) + || (resolvableProfile.id().isPresent() && resolvableProfile.properties().containsKey(CraftPlayerTextures.PROPERTY_NAME)); + Preconditions.checkArgument(isValidSkullProfile, "The skull profile is missing a name or textures!"); + return resolvableProfile; + } + @Nullable public static Property getProperty(@Nonnull GameProfile profile, String propertyName) { return Iterables.getFirst(profile.getProperties().get(propertyName), null); @@ -53,15 +65,21 @@ public final class CraftPlayerProfile implements PlayerProfile { public CraftPlayerProfile(UUID uniqueId, String name) { Preconditions.checkArgument((uniqueId != null) || !StringUtils.isBlank(name), "uniqueId is null or name is blank"); - this.uniqueId = (uniqueId == null) ? SystemUtils.NIL_UUID : uniqueId; - this.name = (name == null) ? "" : name; + this.uniqueId = uniqueId; + this.name = name; + } + + @ApiStatus.Internal + public CraftPlayerProfile(@Nonnull ResolvableProfile resolvableProfile) { + this(resolvableProfile.id().orElse(null), resolvableProfile.name().orElse(null)); + this.properties.putAll(resolvableProfile.properties()); } // The Map of properties of the given GameProfile is not immutable. This captures a snapshot of the properties of // the given GameProfile at the time this CraftPlayerProfile is created. public CraftPlayerProfile(@Nonnull GameProfile gameProfile) { this(gameProfile.getId(), gameProfile.getName()); - properties.putAll(gameProfile.getProperties()); + this.properties.putAll(gameProfile.getProperties()); } private CraftPlayerProfile(@Nonnull CraftPlayerProfile other) { @@ -72,12 +90,12 @@ public final class CraftPlayerProfile implements PlayerProfile { @Override public UUID getUniqueId() { - return (uniqueId.equals(SystemUtils.NIL_UUID)) ? null : uniqueId; + return (Objects.equals(uniqueId, SystemUtils.NIL_UUID)) ? null : uniqueId; } @Override public String getName() { - return (name.isEmpty()) ? null : name; + return (StringUtils.isBlank(name)) ? null : name; } @Nullable @@ -145,6 +163,14 @@ public final class CraftPlayerProfile implements PlayerProfile { return new CraftPlayerProfile(profile); } + // This always returns a new GameProfile instance to ensure that property changes to the original or previously + // built ResolvableProfile don't affect the use of this profile in other contexts. + @Nonnull + public ResolvableProfile buildResolvableProfile() { + rebuildDirtyProperties(); + return new ResolvableProfile(Optional.ofNullable(this.name), Optional.ofNullable(this.uniqueId), this.properties); + } + // This always returns a new GameProfile instance to ensure that property changes to the original or previously // built GameProfiles don't affect the use of this profile in other contexts. @Nonnull @@ -184,8 +210,7 @@ public final class CraftPlayerProfile implements PlayerProfile { @Override public boolean equals(Object obj) { if (this == obj) return true; - if (!(obj instanceof CraftPlayerProfile)) return false; - CraftPlayerProfile other = (CraftPlayerProfile) obj; + if (!(obj instanceof CraftPlayerProfile other)) return false; if (!Objects.equals(uniqueId, other.uniqueId)) return false; if (!Objects.equals(name, other.name)) return false; @@ -238,18 +263,16 @@ public final class CraftPlayerProfile implements PlayerProfile { @Override public Map serialize() { Map map = new LinkedHashMap<>(); - if (getUniqueId() != null) { - map.put("uniqueId", getUniqueId().toString()); + if (this.uniqueId != null) { + map.put("uniqueId", this.uniqueId.toString()); } - if (getName() != null) { + if (this.name != null) { map.put("name", getName()); } rebuildDirtyProperties(); - if (!properties.isEmpty()) { + if (!this.properties.isEmpty()) { List propertiesData = new ArrayList<>(); - properties.forEach((propertyName, property) -> { - propertiesData.add(CraftProfileProperty.serialize(property)); - }); + this.properties.forEach((propertyName, property) -> propertiesData.add(CraftProfileProperty.serialize(property))); map.put("properties", propertiesData); } return map; @@ -264,7 +287,7 @@ public final class CraftPlayerProfile implements PlayerProfile { if (map.containsKey("properties")) { for (Object propertyData : (List) map.get("properties")) { - Preconditions.checkArgument(propertyData instanceof Map, "Propertu data (%s) is not a valid Map", propertyData); + Preconditions.checkArgument(propertyData instanceof Map, "Property data (%s) is not a valid Map", propertyData); Property property = CraftProfileProperty.deserialize((Map) propertyData); profile.properties.put(property.name(), property); } diff --git a/paper-server/src/test/java/org/bukkit/craftbukkit/profile/PlayerProfileTest.java b/paper-server/src/test/java/org/bukkit/craftbukkit/profile/PlayerProfileTest.java index aee4ff1fd7..1043a49b33 100644 --- a/paper-server/src/test/java/org/bukkit/craftbukkit/profile/PlayerProfileTest.java +++ b/paper-server/src/test/java/org/bukkit/craftbukkit/profile/PlayerProfileTest.java @@ -7,6 +7,7 @@ import java.net.MalformedURLException; import java.net.URL; import java.util.UUID; import net.minecraft.SystemUtils; +import net.minecraft.world.item.component.ResolvableProfile; import org.bukkit.configuration.InvalidConfigurationException; import org.bukkit.configuration.file.YamlConfiguration; import org.bukkit.configuration.serialization.ConfigurationSerialization; @@ -90,7 +91,10 @@ public class PlayerProfileTest { public void testGameProfileWrapping() { // Invalid profiles: assertThrows(NullPointerException.class, () -> { - new CraftPlayerProfile(null); + new CraftPlayerProfile((GameProfile) null); + }); + assertThrows(NullPointerException.class, () -> { + new CraftPlayerProfile((ResolvableProfile) null); }); // Valid profiles: