fix: keep item meta on chest items (1.26.2)

pom.xml

@@ -67,7 +67,7 @@
         <!-- Do not change unless you want different name for local builds. -->
         <build.number>-LOCAL</build.number>
         <!-- This allows to change between versions. -->
-        <build.version>1.26.1</build.version>
+        <build.version>1.26.2</build.version>
         <!-- SonarCloud -->
         <sonar.projectKey>BentoBoxWorld_AOneBlock</sonar.projectKey>
         <sonar.organization>bentobox-world</sonar.organization>

src/main/java/world/bentobox/aoneblock/oneblocks/OneBlocksManager.java

@@ -39,6 +39,7 @@
 import org.bukkit.configuration.ConfigurationSection;
 import org.bukkit.configuration.InvalidConfigurationException;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.serialization.ConfigurationSerialization;
 import org.bukkit.entity.EntityType;
 import org.bukkit.inventory.ItemStack;
 import org.eclipse.jdt.annotation.NonNull;
@@ -844,20 +845,68 @@
     private @Nullable ItemStack chestItem(Map<?, ?> raw, String fileName) {
         String id = Objects.toString(raw.get("id"), Objects.toString(raw.get("type"), null));
         if (id != null && Material.matchMaterial(id) == null) {
             addon.log("Skipping item " + id + " in " + fileName + ": it does not exist on this server version.");
             return null;
         }
         try {
-            Map<String, Object> map = new LinkedHashMap<>();
-            raw.forEach((k, v) -> map.put(Objects.toString(k), v));
-            map.remove("==");
-            return ItemStack.deserialize(map);
+            Object deserialized = deserializeRaw(raw, fileName);
+            if (deserialized instanceof ItemStack stack) {
+                return stack;
+            }
+            // No type key, so the map is a bare item definition. Its meta, if any, is
+            // already a real ItemMeta after the walk above.
+            if (deserialized instanceof Map<?, ?> map) {
+                Map<String, Object> args = new LinkedHashMap<>();
+                map.forEach((k, v) -> args.put(Objects.toString(k), v));
+                return ItemStack.deserialize(args);
+            }
+            addon.log("Skipping item " + id + " in " + fileName + ": it is not an item definition.");
+            return null;
         } catch (Exception e) {
             addon.log("Skipping item " + id + " in " + fileName + ": " + e.getMessage());
             return null;
         }
     }
 
+    /**
+     * Walks a value parsed by plain SnakeYAML and turns any map carrying a
+     * serialized-type key into the object it describes, depth first.
+     * <p>
+     * Chest files are read with SnakeYAML rather than YamlConfiguration so that an
+     * item unknown to this server version can be skipped individually. The cost is
+     * that nothing is deserialized on the way in: an item's {@code meta} section
+     * arrives as a plain map, and {@link ItemStack#deserialize(Map)} silently
+     * ignores meta that is not already an {@link org.bukkit.inventory.meta.ItemMeta}.
+     * That is how stored enchantments, potion effects and custom names were being
+     * dropped. Deserializing here restores them while keeping per-item failures
+     * contained. A part that cannot be deserialized is left as a map and logged, so
+     * a bad meta section costs its meta rather than the whole item.
+     *
+     * @param value    raw value from the YAML parse
+     * @param fileName file the value came from, for log messages
+     * @return the deserialized value, or the value itself if there is nothing to do
+     */
+    private Object deserializeRaw(Object value, String fileName) {
+        if (value instanceof Map<?, ?> map) {
+            Map<String, Object> converted = new LinkedHashMap<>();
+            map.forEach((k, v) -> converted.put(Objects.toString(k), deserializeRaw(v, fileName)));
+            if (converted.get(ConfigurationSerialization.SERIALIZED_TYPE_KEY) instanceof String type) {
+                try {
+                    return ConfigurationSerialization.deserializeObject(converted);
+                } catch (Exception e) {
+                    addon.log("Could not read " + type + " in " + fileName + ": " + e.getMessage());
+                }
+            }
+            return converted;
+        }
+        if (value instanceof List<?> list) {
+            List<Object> converted = new ArrayList<>(list.size());
+            list.forEach(v -> converted.add(deserializeRaw(v, fileName)));
+            return converted;
+        }
+        return value;
+    }
+
     private int phaseLength(PhaseIndexEntry entry) {
         if (entry.getLength() > 0) {
             return entry.getLength();

src/test/java/world/bentobox/aoneblock/oneblocks/OneBlocksManagerTest3.java

@@ -5,6 +5,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
@@ -16,6 +17,7 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.util.List;
+import java.util.Map;
 import java.util.NavigableMap;
 import java.util.concurrent.CompletableFuture;
 import java.util.jar.JarEntry;
@@ -25,6 +27,11 @@
 import org.bukkit.block.Biome;
 import org.bukkit.configuration.InvalidConfigurationException;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.serialization.ConfigurationSerializable;
+import org.bukkit.configuration.serialization.ConfigurationSerialization;
+import org.bukkit.inventory.ItemStack;
+import org.bukkit.inventory.meta.EnchantmentStorageMeta;
+import org.bukkit.inventory.meta.ItemMeta;
 import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.Nullable;
 import org.junit.jupiter.api.AfterAll;
@@ -732,6 +739,98 @@
 		verify(plugin, never()).logError(anyString());
 	}
 
+	/**
+	 * Item meta in a chest file survives loading. Chest files are parsed with plain
+	 * SnakeYAML, so nested meta has to be deserialized before the item is built -
+	 * ItemStack.deserialize ignores a meta section that is still a plain map, which
+	 * is how enchanted books arrived with nothing stored on them.
+	 * <p>
+	 * The server's own "ItemMeta" serialization alias does not exist under
+	 * MockBukkit, so a stand-in is registered that records what it is given and
+	 * returns a mock. The YAML is a copy of what the shipped phase files hold.
+	 */
+	@Test
+	void testLoadPhasesChestItemKeepsMeta() throws IOException {
+		PHASES_DIR.mkdirs();
+		java.nio.file.Files.writeString(new File(PHASES_DIR, "alpha.yml").toPath(), """
+                '0':
+                  name: Alpha
+                  biome: PLAINS
+                  blocks:
+                    GRASS_BLOCK: 100
+                """);
+		java.nio.file.Files.writeString(new File(PHASES_DIR, "alpha_chests.yml").toPath(), """
+                '0':
+                  chests:
+                    '1':
+                      contents:
+                        0:
+                          ==: org.bukkit.inventory.ItemStack
+                          v: 2230
+                          type: ENCHANTED_BOOK
+                          meta:
+                            ==: ItemMeta
+                            meta-type: ENCHANTED
+                            stored-enchants:
+                              PROTECTION_FALL: 1
+                      rarity: COMMON
+                """);
+		java.nio.file.Files.writeString(INDEX_FILE.toPath(), """
+                phases:
+                  - file: alpha
+                    section: '0'
+                    name: Alpha
+                    length: 100
+                """);
+		// The item factory has to accept the meta for ItemStack to keep it
+		when(itemFactory.isApplicable(any(ItemMeta.class), any(Material.class))).thenReturn(true);
+		when(itemFactory.asMetaFor(any(ItemMeta.class), any(Material.class)))
+				.thenAnswer(invocation -> invocation.getArgument(0));
+		StandInItemMeta.lastArgs = null;
+		ConfigurationSerialization.registerClass(StandInItemMeta.class, "ItemMeta");
+		try {
+			obm.loadPhases();
+		} finally {
+			ConfigurationSerialization.unregisterClass("ItemMeta");
+		}
+		OneBlockPhase phase = obm.getPhase(0);
+		assertNotNull(phase);
+		assertEquals(1, phase.getChests().size());
+		ItemStack loaded = phase.getChests().iterator().next().getChest().get(0);
+		assertNotNull(loaded, "Chest should contain the book");
+		// The meta section reached the meta deserializer intact, so a real ItemMeta -
+		// not a plain map - is what ItemStack.deserialize is given. Before the fix
+		// the deserializer was never called at all. Whether the stack then keeps the
+		// meta cannot be checked here: ItemStack delegates meta to a craftDelegate,
+		// which is a mock while Bukkit is statically mocked.
+		assertNotNull(StandInItemMeta.lastArgs, "Meta should have been deserialized");
+		assertEquals("ENCHANTED", StandInItemMeta.lastArgs.get("meta-type"));
+		assertEquals(Map.of("PROTECTION_FALL", 1), StandInItemMeta.lastArgs.get("stored-enchants"));
+		verify(plugin, never()).logError(anyString());
+	}
+
+	/**
+	 * Stands in for the server's ItemMeta deserializer. Records the map it is
+	 * handed and returns a mock, so a test can check what reached it.
+	 */
+	public static class StandInItemMeta implements ConfigurationSerializable {
+
+		static Map<String, Object> lastArgs;
+
+		public static EnchantmentStorageMeta deserialize(Map<String, Object> args) {
+			lastArgs = args;
+			EnchantmentStorageMeta meta = Mockito.mock(EnchantmentStorageMeta.class);
+			// ItemStack hands back a clone of its meta
+			when(meta.clone()).thenReturn(meta);
+			return meta;
+		}
+
+		@Override
+		public Map<String, Object> serialize() {
+			return Map.of();
+		}
+	}
+
 	/**
 	 * Object-form block entries can carry a required Minecraft version; gated
 	 * entries are skipped with a log line, satisfied ones load with their weight.