Change NBT.modify calls on (Block-)Entities to be atomic

item-nbt-api/pom.xml

@@ -5,7 +5,7 @@
   <parent>
     <groupId>de.tr7zw</groupId>
     <artifactId>item-nbt-parent</artifactId>
-    <version>2.11.3</version>
+    <version>2.12.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>item-nbt-api</artifactId>

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBT.java

@@ -36,7 +36,7 @@ public class NBT {
      * @return The function is being returned.
      */
     public static <T> T get(ItemStack item, Function<ReadableNBT, T> getter) {
-        return getter.apply(new NBTItem(item));
+        return getter.apply(new NBTItem(item, false, true));
     }
 
     /**
@@ -48,7 +48,7 @@ public class NBT {
      * @return The NBTEntity class is being returned.
      */
     public static <T> T get(Entity entity, Function<ReadableNBT, T> getter) {
-        return getter.apply(new NBTEntity(entity));
+        return getter.apply(new NBTEntity(entity, true));
     }
 
     /**
@@ -62,7 +62,7 @@ public class NBT {
      * @return The return type is the same as the type of the getter function.
      */
     public static <T> T get(BlockState blockState, Function<ReadableNBT, T> getter) {
-        return getter.apply(new NBTTileEntity(blockState));
+        return getter.apply(new NBTTileEntity(blockState, true));
     }
 
     /**
@@ -130,7 +130,11 @@ public class NBT {
      * @return The return type is the same as the return type of the function.
      */
     public static <T> T modify(Entity entity, Function<ReadWriteNBT, T> function) {
-        return function.apply(new NBTEntity(entity));
+        NBTEntity nbtEnt = new NBTEntity(entity);
+        NBTContainer cont = new NBTContainer(nbtEnt.getCompound());
+        T ret = function.apply(cont);
+        nbtEnt.setCompound(cont.getCompound());
+        return ret;
     }
 
     /**
@@ -141,7 +145,10 @@ public class NBT {
      * @param consumer The consumer that will be called with the NBTEntity.
      */
     public static void modify(Entity entity, Consumer<ReadWriteNBT> consumer) {
-        consumer.accept(new NBTEntity(entity));
+        NBTEntity nbtEnt = new NBTEntity(entity);
+        NBTContainer cont = new NBTContainer(nbtEnt.getCompound());
+        consumer.accept(cont);
+        nbtEnt.setCompound(cont.getCompound());
     }
 
     /**
@@ -177,7 +184,11 @@ public class NBT {
      * @return The return type is the same as the return type of the function.
      */
     public static <T> T modify(BlockState blockState, Function<ReadWriteNBT, T> function) {
-        return function.apply(new NBTTileEntity(blockState));
+        NBTTileEntity blockEnt = new NBTTileEntity(blockState);
+        NBTContainer cont = new NBTContainer(blockEnt.getCompound());
+        T ret = function.apply(cont);
+        blockEnt.setCompound(cont);
+        return ret;
     }
 
     /**
@@ -190,7 +201,10 @@ public class NBT {
      *                   takes a ReadWriteNBT and does something with it.
      */
     public static void modify(BlockState blockState, Consumer<ReadWriteNBT> consumer) {
-        consumer.accept(new NBTTileEntity(blockState));
+        NBTTileEntity blockEnt = new NBTTileEntity(blockState);
+        NBTContainer cont = new NBTContainer(blockEnt.getCompound());
+        consumer.accept(cont);
+        blockEnt.setCompound(cont);
     }
 
     /**

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTEntity.java

@@ -19,6 +19,26 @@ import de.tr7zw.changeme.nbtapi.utils.annotations.CheckUtil;
 public class NBTEntity extends NBTCompound {
 
     private final Entity ent;
+    private final boolean readonly;
+    private final Object compound;
+
+    /**
+     * @param entity   Any valid Bukkit Entity
+     * @param readonly Readonly makes a copy at init, only reading from that copy
+     */
+    protected NBTEntity(Entity entity, boolean readonly) {
+        super(null, null);
+        if (entity == null) {
+            throw new NullPointerException("Entity can't be null!");
+        }
+        this.readonly = readonly;
+        ent = entity;
+        if (readonly) {
+            this.compound = getCompound();
+        } else {
+            this.compound = null;
+        }
+    }
 
     /**
      * @param entity Any valid Bukkit Entity
@@ -28,11 +48,17 @@ public class NBTEntity extends NBTCompound {
         if (entity == null) {
             throw new NullPointerException("Entity can't be null!");
         }
+        this.readonly = false;
+        this.compound = null;
         ent = entity;
     }
 
     @Override
     public Object getCompound() {
+        // this runs before async check, since it's just a copy
+        if (readonly && compound != null) {
+            return compound;
+        }
         if (!Bukkit.isPrimaryThread())
             throw new NbtApiException("Entity NBT needs to be accessed sync!");
         return NBTReflectionUtil.getEntityNBTTagCompound(NBTReflectionUtil.getNMSEntity(ent));
@@ -40,6 +66,9 @@ public class NBTEntity extends NBTCompound {
 
     @Override
     protected void setCompound(Object compound) {
+        if (readonly) {
+            throw new NbtApiException("Tried setting data in read only mode!");
+        }
         if (!Bukkit.isPrimaryThread())
             throw new NbtApiException("Entity NBT needs to be accessed sync!");
         NBTReflectionUtil.setEntityNBTTag(compound, NBTReflectionUtil.getNMSEntity(ent));

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTItem.java

@@ -9,6 +9,7 @@ import org.bukkit.inventory.meta.ItemMeta;
 import de.tr7zw.changeme.nbtapi.iface.ReadWriteItemNBT;
 import de.tr7zw.changeme.nbtapi.iface.ReadWriteNBT;
 import de.tr7zw.changeme.nbtapi.iface.ReadableNBT;
+import de.tr7zw.changeme.nbtapi.utils.nmsmappings.ClassWrapper;
 import de.tr7zw.changeme.nbtapi.utils.nmsmappings.ReflectionMethod;
 
 /**
@@ -22,6 +23,7 @@ public class NBTItem extends NBTCompound implements ReadWriteItemNBT {
 
     private ItemStack bukkitItem;
     private boolean directApply;
+    private boolean readOnly;
     private ItemStack originalSrcStack = null;
 
     /**
@@ -33,6 +35,30 @@ public class NBTItem extends NBTCompound implements ReadWriteItemNBT {
         this(item, false);
     }
 
+    /**
+     * @param item
+     * @param directApply
+     * @param readOnly    When turned on, no copy of the source item is created.
+     *                    Modifying the stack in that case is not valid! Also
+     *                    overwrites directApply
+     */
+    protected NBTItem(ItemStack item, boolean directApply, boolean readOnly) {
+        super(null, null);
+        if (item == null || item.getType() == Material.AIR || item.getAmount() <= 0) {
+            throw new NullPointerException("ItemStack can't be null/air/amount of 0! This is not a NBTAPI bug!");
+        }
+        if (readOnly) {
+            bukkitItem = item;
+            this.readOnly = true;
+        } else {
+            this.directApply = directApply;
+            bukkitItem = item.clone();
+            if (directApply) {
+                this.originalSrcStack = item;
+            }
+        }
+    }
+
     /**
      * Constructor for NBTItems. The ItemStack will be cloned! If directApply is
      * true, all changed will be mapped to the original item. Changes to the NBTItem
@@ -55,11 +81,17 @@ public class NBTItem extends NBTCompound implements ReadWriteItemNBT {
 
     @Override
     public Object getCompound() {
+        if (readOnly && ClassWrapper.CRAFT_ITEMSTACK.getClazz().isAssignableFrom(bukkitItem.getClass())) {
+            return NBTReflectionUtil.getItemRootNBTTagCompound(NBTReflectionUtil.getCraftItemHandle(bukkitItem));
+        }
         return NBTReflectionUtil.getItemRootNBTTagCompound(ReflectionMethod.ITEMSTACK_NMSCOPY.run(null, bukkitItem));
     }
 
     @Override
     protected void setCompound(Object compound) {
+        if (readOnly) {
+            throw new NbtApiException("Tried setting data in read only mode!");
+        }
         Object stack = ReflectionMethod.ITEMSTACK_NMSCOPY.run(null, bukkitItem);
         ReflectionMethod.ITEMSTACK_SET_TAG.run(stack, compound);
         bukkitItem = (ItemStack) ReflectionMethod.ITEMSTACK_BUKKITMIRROR.run(null, stack);

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTReflectionUtil.java

@@ -13,6 +13,7 @@ import java.util.UUID;
 
 import org.bukkit.block.BlockState;
 import org.bukkit.entity.Entity;
+import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.meta.ItemMeta;
 
 import de.tr7zw.changeme.nbtapi.utils.GsonWrapper;
@@ -32,6 +33,7 @@ import de.tr7zw.changeme.nbtapi.utils.nmsmappings.ReflectionMethod;
 public class NBTReflectionUtil {
 
     private static Field field_unhandledTags = null;
+    private static Field field_handle = null;
 
     static {
         try {
@@ -39,6 +41,12 @@ public class NBTReflectionUtil {
             field_unhandledTags.setAccessible(true);
         } catch (NoSuchFieldException e) {
 
+        }
+        try {
+            field_handle = ClassWrapper.CRAFT_ITEMSTACK.getClazz().getDeclaredField("handle");
+            field_handle.setAccessible(true);
+        } catch (NoSuchFieldException e) {
+
         }
     }
 
@@ -96,6 +104,21 @@ public class NBTReflectionUtil {
         }
     }
 
+    /**
+     * Gets the nms handle ItemStack from a CraftItemStack. Passing Spigot
+     * ItemStacks will cause an error!
+     * 
+     * @param item
+     * @return
+     */
+    public static Object getCraftItemHandle(ItemStack item) {
+        try {
+            return field_handle.get(item);
+        } catch (IllegalArgumentException | IllegalAccessException e) {
+            throw new NbtApiException("Error getting handle from " + item.getClass(), e);
+        }
+    }
+
     /**
      * Writes a Compound to an OutputStream
      * 

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/NBTTileEntity.java

@@ -20,6 +20,26 @@ import de.tr7zw.changeme.nbtapi.utils.annotations.CheckUtil;
 public class NBTTileEntity extends NBTCompound {
 
     private final BlockState tile;
+    private final boolean readonly;
+    private final Object compound;
+
+    /**
+     * @param tile     BlockState from any TileEntity
+     * @param readonly Readonly makes a copy at init, only reading from that copy
+     */
+    public NBTTileEntity(BlockState tile, boolean readonly) {
+        super(null, null);
+        if (tile == null || (MinecraftVersion.isAtLeastVersion(MinecraftVersion.MC1_8_R3) && !tile.isPlaced())) {
+            throw new NullPointerException("Tile can't be null/not placed!");
+        }
+        this.tile = tile;
+        this.readonly = readonly;
+        if (readonly) {
+            this.compound = getCompound();
+        } else {
+            this.compound = null;
+        }
+    }
 
     /**
      * @param tile BlockState from any TileEntity
@@ -29,11 +49,17 @@ public class NBTTileEntity extends NBTCompound {
         if (tile == null || (MinecraftVersion.isAtLeastVersion(MinecraftVersion.MC1_8_R3) && !tile.isPlaced())) {
             throw new NullPointerException("Tile can't be null/not placed!");
         }
+        this.readonly = false;
+        this.compound = null;
         this.tile = tile;
     }
 
     @Override
     public Object getCompound() {
+        // this runs before async check, since it's just a copy
+        if (readonly && compound != null) {
+            return compound;
+        }
         if (!Bukkit.isPrimaryThread())
             throw new NbtApiException("BlockEntity NBT needs to be accessed sync!");
         return NBTReflectionUtil.getTileEntityNBTTagCompound(tile);
@@ -41,6 +67,9 @@ public class NBTTileEntity extends NBTCompound {
 
     @Override
     protected void setCompound(Object compound) {
+        if (readonly) {
+            throw new NbtApiException("Tried setting data in read only mode!");
+        }
         if (!Bukkit.isPrimaryThread())
             throw new NbtApiException("BlockEntity NBT needs to be accessed sync!");
         NBTReflectionUtil.setTileEntityNBTTagCompound(tile, compound);

item-nbt-api/src/main/java/de/tr7zw/changeme/nbtapi/utils/MinecraftVersion.java

@@ -33,7 +33,7 @@ public enum MinecraftVersion {
     private static Logger logger = Logger.getLogger("NBTAPI");
 
     // NBT-API Version
-    protected static final String VERSION = "2.11.3";
+    protected static final String VERSION = "2.12.0-SNAPSHOT";
 
     private final int versionId;
     private final boolean mojangMapping;

item-nbt-plugin/pom.xml

@@ -4,7 +4,7 @@
   <parent>
     <groupId>de.tr7zw</groupId>
     <artifactId>item-nbt-parent</artifactId>
-    <version>2.11.3</version>
+    <version>2.12.0-SNAPSHOT</version>
   </parent>
   <artifactId>item-nbt-api-plugin</artifactId>
   <packaging>jar</packaging>
@@ -12,19 +12,19 @@
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>item-nbt-api</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>nbt-injector</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>nbt-data-api</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
   </dependencies>

item-nbt-plugin/src/main/java/de/tr7zw/nbtapi/plugin/tests/items/DirectApplyTest.java

@@ -23,6 +23,8 @@ public class DirectApplyTest implements Test {
             nbt.setString("SomeKey", "SomeValue");
             return nbt.getString("SomeKey");
         });
+        baseItem = NBT.itemStackFromNBT(NBT.itemStackToNBT(baseItem)); // trick to force the item to be "real", not a
+                                                                       // Spigot only item
         String outside = NBT.get(baseItem, nbt -> nbt.getString("SomeKey"));
         if (!new NBTItem(baseItem).hasTag("SomeKey")) {
             throw new NbtApiException("The data was not applied!");

mappings-parser/pom.xml

@@ -4,7 +4,7 @@
   <parent>
     <groupId>de.tr7zw</groupId>
     <artifactId>item-nbt-parent</artifactId>
-    <version>2.11.3</version>
+    <version>2.12.0-SNAPSHOT</version>
   </parent>
   <artifactId>mappings-parser</artifactId>
   <packaging>jar</packaging>
@@ -12,7 +12,7 @@
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>item-nbt-api</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
   </dependencies>

nbt-data-api/pom.xml

@@ -5,7 +5,7 @@
   <parent>
     <groupId>de.tr7zw</groupId>
     <artifactId>item-nbt-parent</artifactId>
-    <version>2.11.3</version>
+    <version>2.12.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>nbt-data-api</artifactId>
@@ -14,7 +14,7 @@
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>item-nbt-api</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
   </dependencies>

nbt-injector/pom.xml

@@ -4,7 +4,7 @@
   <parent>
     <groupId>de.tr7zw</groupId>
     <artifactId>item-nbt-parent</artifactId>
-    <version>2.11.3</version>
+    <version>2.12.0-SNAPSHOT</version>
   </parent>
   <artifactId>nbt-injector</artifactId>
   <packaging>jar</packaging>
@@ -12,7 +12,7 @@
     <dependency>
       <groupId>de.tr7zw</groupId>
       <artifactId>item-nbt-api</artifactId>
-      <version>2.11.3</version>
+      <version>2.12.0-SNAPSHOT</version>
       <scope>compile</scope>
     </dependency>
     <dependency>

pom.xml

@@ -3,7 +3,7 @@
   <modelVersion>4.0.0</modelVersion>
   <groupId>de.tr7zw</groupId>
   <artifactId>item-nbt-parent</artifactId>
-  <version>2.11.3</version>
+  <version>2.12.0-SNAPSHOT</version>
   <packaging>pom</packaging>
   <inceptionYear>2015</inceptionYear>
   <organization>

spigot.bb

@@ -112,7 +112,7 @@ Just use the Plugin as Maven dependency and remember to add "NBTAPI" as dependen
 <dependency>
   <groupId>de.tr7zw</groupId>
   <artifactId>item-nbt-api-plugin</artifactId>
-  <version>2.11.3</version>
+  <version>2.12.0-SNAPSHOT</version>
 </dependency>
 [/CODE]
 [/SPOILER]
@@ -122,7 +122,7 @@ Using this method the API gets copied into your plugin. Server owners don't have
 <dependency>
   <groupId>de.tr7zw</groupId>
   <artifactId>item-nbt-api</artifactId>
-  <version>2.11.3</version>
+  <version>2.12.0-SNAPSHOT</version>
 </dependency>
 ...
     <plugins>