Change item id to int, fix valid string being considered as invalid, tests

common/src/main/java/us/myles/ViaVersion/api/minecraft/item/Item.java

@@ -8,9 +8,28 @@ import lombok.*;
 @NoArgsConstructor
 @AllArgsConstructor
 @ToString
+@EqualsAndHashCode
 public class Item {
-    private short id;
+    private int identifier;
     private byte amount;
     private short data;
     private CompoundTag tag;
+
+    @Deprecated
+    public short getId() {
+        return (short) identifier;
+    }
+
+    @Deprecated
+    public void setId(short id) {
+        identifier = id;
+    }
+
+    @Deprecated
+    public Item(short id, byte amount, short data, CompoundTag tag) {
+        this.identifier = id;
+        this.amount = amount;
+        this.data = data;
+        this.tag = tag;
+    }
 }

common/src/main/java/us/myles/ViaVersion/api/type/types/StringType.java

@@ -6,6 +6,10 @@ import io.netty.buffer.ByteBuf;
 import us.myles.ViaVersion.api.type.Type;
 
 public class StringType extends Type<String> {
+    // String#length() (used to limit the string in Minecraft source code) uses char[]#length
+    private static final int maxJavaCharUtf8Length = Character.toString(Character.MAX_VALUE)
+            .getBytes(Charsets.UTF_8).length;
+
     public StringType() {
         super(String.class);
     }
@@ -13,12 +17,17 @@ public class StringType extends Type<String> {
     @Override
     public String read(ByteBuf buffer) throws Exception {
         int len = Type.VAR_INT.read(buffer);
-        Preconditions.checkArgument(len <= Short.MAX_VALUE, "Cannot receive string longer than Short.MAX_VALUE (got %s characters)", len);
+
+        Preconditions.checkArgument(len <= Short.MAX_VALUE * maxJavaCharUtf8Length,
+                "Cannot receive string longer than Short.MAX_VALUE * "  + maxJavaCharUtf8Length + " bytes (got %s bytes)", len);
 
         byte[] b = new byte[len];
         buffer.readBytes(b);
+        String string = new String(b, Charsets.UTF_8);
+        Preconditions.checkArgument(string.length() <= Short.MAX_VALUE,
+                "Cannot receive string longer than Short.MAX_VALUE characters (got %s bytes)", string.length());
 
-        return new String(b, Charsets.UTF_8);
+        return string;
     }
 
     @Override

common/src/main/java/us/myles/ViaVersion/api/type/types/minecraft/FlatItemType.java

@@ -16,7 +16,7 @@ public class FlatItemType extends BaseItemType {
             return null;
         } else {
             Item item = new Item();
-            item.setId(id);
+            item.setIdentifier(id);
             item.setAmount(buffer.readByte());
             item.setTag(Type.NBT.read(buffer));
             return item;
@@ -28,7 +28,7 @@ public class FlatItemType extends BaseItemType {
         if (object == null) {
             buffer.writeShort(-1);
         } else {
-            buffer.writeShort(object.getId());
+            buffer.writeShort(object.getIdentifier());
             buffer.writeByte(object.getAmount());
             Type.NBT.write(buffer, object.getTag());
         }

common/src/main/java/us/myles/ViaVersion/api/type/types/minecraft/FlatVarIntItemType.java

@@ -16,7 +16,7 @@ public class FlatVarIntItemType extends BaseItemType {
             return null;
         } else {
             Item item = new Item();
-            item.setId(Type.VAR_INT.read(buffer).shortValue());  //TODO Maybe we should consider changing id field type to int
+            item.setIdentifier(Type.VAR_INT.read(buffer));
             item.setAmount(buffer.readByte());
             item.setTag(Type.NBT.read(buffer));
             return item;
@@ -29,7 +29,7 @@ public class FlatVarIntItemType extends BaseItemType {
             buffer.writeBoolean(false);
         } else {
             buffer.writeBoolean(true);
-            Type.VAR_INT.write(buffer, (int) object.getId());
+            Type.VAR_INT.write(buffer, object.getIdentifier());
             buffer.writeByte(object.getAmount());
             Type.NBT.write(buffer, object.getTag());
         }

common/src/main/java/us/myles/ViaVersion/api/type/types/minecraft/ItemType.java

@@ -16,7 +16,7 @@ public class ItemType extends BaseItemType {
             return null;
         } else {
             Item item = new Item();
-            item.setId(id);
+            item.setIdentifier(id);
             item.setAmount(buffer.readByte());
             item.setData(buffer.readShort());
             item.setTag(Type.NBT.read(buffer));
@@ -29,7 +29,7 @@ public class ItemType extends BaseItemType {
         if (object == null) {
             buffer.writeShort(-1);
         } else {
-            buffer.writeShort(object.getId());
+            buffer.writeShort(object.getIdentifier());
             buffer.writeByte(object.getAmount());
             buffer.writeShort(object.getData());
             Type.NBT.write(buffer, object.getTag());

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_11to1_10/EntityIdRewriter.java

@@ -127,7 +127,7 @@ public class EntityIdRewriter {
     }
 
     private static boolean hasEntityTag(Item item) {
-        if (item != null && item.getId() == 383) { // Monster Egg
+        if (item != null && item.getIdentifier() == 383) { // Monster Egg
             CompoundTag tag = item.getTag();
             if (tag != null && tag.contains("EntityTag") && tag.get("EntityTag") instanceof CompoundTag) {
                 if (((CompoundTag) tag.get("EntityTag")).get("id") instanceof StringTag) {

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_12to1_11_1/BedRewriter.java

@@ -6,14 +6,14 @@ public class BedRewriter {
 
     public static void toClientItem(Item item) {
         if (item == null) return;
-        if (item.getId() == 355 && item.getData() == 0) {
+        if (item.getIdentifier() == 355 && item.getData() == 0) {
             item.setData((short) 14);
         }
     }
 
     public static void toServerItem(Item item) {
         if (item == null) return;
-        if (item.getId() == 355 && item.getData() == 14) {
+        if (item.getIdentifier() == 355 && item.getData() == 14) {
             item.setData((short) 0);
         }
     }

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_13_1to1_13/packets/InventoryPackets.java

@@ -202,7 +202,7 @@ public class InventoryPackets {
 
     public static void toClient(Item item) {
         if (item == null) return;
-        item.setId((short) getNewItemId(item.getId()));
+        item.setIdentifier(getNewItemId(item.getIdentifier()));
     }
 
     public static int getNewItemId(int itemId) {
@@ -214,7 +214,7 @@ public class InventoryPackets {
 
     public static void toServer(Item item) {
         if (item == null) return;
-        item.setId((short) getOldItemId(item.getId()));
+        item.setIdentifier(getOldItemId(item.getIdentifier()));
     }
 
     public static int getOldItemId(int newId) {

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_13to1_12_2/packets/InventoryPackets.java

@@ -285,16 +285,16 @@ public class InventoryPackets {
         CompoundTag tag = item.getTag();
 
         // Save original id
-        int originalId = (item.getId() << 16 | item.getData() & 0xFFFF);
+        int originalId = (item.getIdentifier() << 16 | item.getData() & 0xFFFF);
 
-        int rawId = (item.getId() << 4 | item.getData() & 0xF);
+        int rawId = (item.getIdentifier() << 4 | item.getData() & 0xF);
 
         // NBT Additions
-        if (isDamageable(item.getId())) {
+        if (isDamageable(item.getIdentifier())) {
             if (tag == null) item.setTag(tag = new CompoundTag("tag"));
             tag.put(new IntTag("Damage", item.getData()));
         }
-        if (item.getId() == 358) { // map
+        if (item.getIdentifier() == 358) { // map
             if (tag == null) item.setTag(tag = new CompoundTag("tag"));
             tag.put(new IntTag("map", item.getData()));
         }
@@ -302,7 +302,7 @@ public class InventoryPackets {
         // NBT Changes
         if (tag != null) {
             // Invert shield color id
-            if (item.getId() == 442 || item.getId() == 425) {
+            if (item.getIdentifier() == 442 || item.getIdentifier() == 425) {
                 if (tag.get("BlockEntityTag") instanceof CompoundTag) {
                     CompoundTag blockEntityTag = tag.get("BlockEntityTag");
                     if (blockEntityTag.get("Base") instanceof IntTag) {
@@ -374,7 +374,7 @@ public class InventoryPackets {
                 tag.put(newStoredEnch);
             }
             // Handle SpawnEggs
-            if (item.getId() == 383) {
+            if (item.getIdentifier() == 383) {
                 if (tag.get("EntityTag") instanceof CompoundTag) {
                     CompoundTag entityTag = tag.get("EntityTag");
                     if (entityTag.get("id") instanceof StringTag) {
@@ -402,23 +402,23 @@ public class InventoryPackets {
         }
 
         if (!MappingData.oldToNewItems.containsKey(rawId)) {
-            if (!isDamageable(item.getId()) && item.getId() != 358) { // Map
+            if (!isDamageable(item.getIdentifier()) && item.getIdentifier() != 358) { // Map
                 if (tag == null) item.setTag(tag = new CompoundTag("tag"));
                 tag.put(new IntTag(NBT_TAG_NAME, originalId)); // Data will be lost, saving original id
             }
-            if (item.getId() == 31 && item.getData() == 0) { // Shrub was removed
+            if (item.getIdentifier() == 31 && item.getData() == 0) { // Shrub was removed
                 rawId = 32 << 4; // Dead Bush
             } else if (MappingData.oldToNewItems.containsKey(rawId & ~0xF)) {
                 rawId &= ~0xF; // Remove data
             } else {
                 if (!Via.getConfig().isSuppress1_13ConversionErrors() || Via.getManager().isDebug()) {
-                    Via.getPlatform().getLogger().warning("Failed to get 1.13 item for " + item.getId());
+                    Via.getPlatform().getLogger().warning("Failed to get 1.13 item for " + item.getIdentifier());
                 }
                 rawId = 16; // Stone
             }
         }
 
-        item.setId(MappingData.oldToNewItems.get(rawId).shortValue());
+        item.setIdentifier(MappingData.oldToNewItems.get(rawId).shortValue());
         item.setData((short) 0);
     }
 
@@ -474,7 +474,7 @@ public class InventoryPackets {
         }
 
         if (rawId == null) {
-            Integer oldId = MappingData.oldToNewItems.inverse().get((int) item.getId());
+            Integer oldId = MappingData.oldToNewItems.inverse().get(item.getIdentifier());
             if (oldId != null) {
                 // Handle spawn eggs
                 Optional<String> eggEntityId = SpawnEggRewriter.getEntityId(oldId);
@@ -495,17 +495,17 @@ public class InventoryPackets {
 
         if (rawId == null) {
             if (!Via.getConfig().isSuppress1_13ConversionErrors() || Via.getManager().isDebug()) {
-                Via.getPlatform().getLogger().warning("Failed to get 1.12 item for " + item.getId());
+                Via.getPlatform().getLogger().warning("Failed to get 1.12 item for " + item.getIdentifier());
             }
             rawId = 0x10000; // Stone
         }
 
-        item.setId((short) (rawId >> 16));
+        item.setIdentifier((short) (rawId >> 16));
         item.setData((short) (rawId & 0xFFFF));
 
         // NBT changes
         if (tag != null) {
-            if (isDamageable(item.getId())) {
+            if (isDamageable(item.getIdentifier())) {
                 if (tag.get("Damage") instanceof IntTag) {
                     if (!gotRawIdFromTag)
                         item.setData((short) (int) tag.get("Damage").getValue());
@@ -513,7 +513,7 @@ public class InventoryPackets {
                 }
             }
 
-            if (item.getId() == 358) { // map
+            if (item.getIdentifier() == 358) { // map
                 if (tag.get("map") instanceof IntTag) {
                     if (!gotRawIdFromTag)
                         item.setData((short) (int) tag.get("map").getValue());
@@ -521,7 +521,7 @@ public class InventoryPackets {
                 }
             }
 
-            if (item.getId() == 442 || item.getId() == 425) { // shield / banner
+            if (item.getIdentifier() == 442 || item.getIdentifier() == 425) { // shield / banner
                 if (tag.get("BlockEntityTag") instanceof CompoundTag) {
                     CompoundTag blockEntityTag = tag.get("BlockEntityTag");
                     if (blockEntityTag.get("Base") instanceof IntTag) {

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_14to1_13_2/packets/InventoryPackets.java

@@ -197,7 +197,7 @@ public class InventoryPackets {
 
     public static void toClient(Item item) {
         if (item == null) return;
-        item.setId((short) getNewItemId(item.getId()));
+        item.setIdentifier(getNewItemId(item.getIdentifier()));
     }
 
     public static int getNewItemId(int id) {
@@ -215,7 +215,7 @@ public class InventoryPackets {
 
     public static void toServer(Item item) {
         if (item == null) return;
-        item.setId((short) getOldItemId(item.getId()));
+        item.setIdentifier(getOldItemId(item.getIdentifier()));
     }
 
     public static int getOldItemId(int id) {

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_9to1_8/ItemRewriter.java

@@ -139,7 +139,7 @@ public class ItemRewriter {
 
     public static void toServer(Item item) {
         if (item != null) {
-            if (item.getId() == 383 && item.getData() == 0) { // Monster Egg
+            if (item.getIdentifier() == 383 && item.getData() == 0) { // Monster Egg
                 CompoundTag tag = item.getTag();
                 int data = 0;
                 if (tag != null && tag.get("EntityTag") instanceof CompoundTag) {
@@ -154,7 +154,7 @@ public class ItemRewriter {
                 item.setTag(tag);
                 item.setData((short) data);
             }
-            if (item.getId() == 373) { // Potion
+            if (item.getIdentifier() == 373) { // Potion
                 CompoundTag tag = item.getTag();
                 int data = 0;
                 if (tag != null && tag.get("Potion") instanceof StringTag) {
@@ -169,10 +169,10 @@ public class ItemRewriter {
                 item.setData((short) data);
             }
             // Splash potion
-            if (item.getId() == 438) {
+            if (item.getIdentifier() == 438) {
                 CompoundTag tag = item.getTag();
                 int data = 0;
-                item.setId((short) 373); // Potion
+                item.setIdentifier(373); // Potion
                 if (tag != null && tag.get("Potion") instanceof StringTag) {
                     StringTag potion = tag.get("Potion");
                     String potionName = potion.getValue().replace("minecraft:", "");
@@ -188,7 +188,7 @@ public class ItemRewriter {
     }
 
     public static void rewriteBookToServer(Item item) {
-        short id = item.getId();
+        int id = item.getIdentifier();
         if (id != 387) {
             return;
         }
@@ -224,7 +224,7 @@ public class ItemRewriter {
 
     public static void toClient(Item item) {
         if (item != null) {
-            if (item.getId() == 383 && item.getData() != 0) { // Monster Egg
+            if (item.getIdentifier() == 383 && item.getData() != 0) { // Monster Egg
                 CompoundTag tag = item.getTag();
                 if (tag == null) {
                     tag = new CompoundTag("tag");
@@ -239,13 +239,13 @@ public class ItemRewriter {
                 item.setTag(tag);
                 item.setData((short) 0);
             }
-            if (item.getId() == 373) { // Potion
+            if (item.getIdentifier() == 373) { // Potion
                 CompoundTag tag = item.getTag();
                 if (tag == null) {
                     tag = new CompoundTag("tag");
                 }
                 if (item.getData() >= 16384) {
-                    item.setId((short) 438); // splash id
+                    item.setIdentifier(438); // splash id
                     item.setData((short) (item.getData() - 8192));
                 }
                 String name = potionNameFromDamage(item.getData());
@@ -254,7 +254,7 @@ public class ItemRewriter {
                 item.setTag(tag);
                 item.setData((short) 0);
             }
-            if (item.getId() == 387) { // WRITTEN_BOOK
+            if (item.getIdentifier() == 387) { // WRITTEN_BOOK
                 CompoundTag tag = item.getTag();
                 if (tag == null) {
                     tag = new CompoundTag("tag");

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_9to1_8/packets/EntityPackets.java

@@ -162,7 +162,7 @@ public class EntityPackets {
                         Item stack = wrapper.get(Type.ITEM, 0);
 
                         if (stack != null) {
-                            if (Protocol1_9TO1_8.isSword(stack.getId())) {
+                            if (Protocol1_9TO1_8.isSword(stack.getIdentifier())) {
                                 entityTracker.getValidBlocking().add(entityID);
                                 return;
                             }

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_9to1_8/packets/PlayerPackets.java

@@ -501,7 +501,7 @@ public class PlayerPackets {
                         if (name.equalsIgnoreCase("MC|BSign")) {
                             Item item = wrapper.passthrough(Type.ITEM);
                             if (item != null) {
-                                item.setId((short) 387); // Written Book
+                                item.setIdentifier(387); // Written Book
                                 ItemRewriter.rewriteBookToServer(item);
                             }
                         }

common/src/main/java/us/myles/ViaVersion/protocols/protocol1_9to1_8/packets/WorldPackets.java

@@ -308,7 +308,7 @@ public class WorldPackets {
                         if (Via.getConfig().isShieldBlocking()) {
                             EntityTracker tracker = wrapper.user().get(EntityTracker.class);
 
-                            if (item != null && Protocol1_9TO1_8.isSword(item.getId())) {
+                            if (item != null && Protocol1_9TO1_8.isSword(item.getIdentifier())) {
                                 if (hand == 0) {
                                     if (!tracker.isBlocking()) {
                                         tracker.setBlocking(true);

common/src/test/java/us/myles/ViaVersion/common/test/type/ItemTypeTest.java

@@ -0,0 +1,84 @@
+package us.myles.ViaVersion.common.test.type;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import us.myles.ViaVersion.api.minecraft.item.Item;
+import us.myles.ViaVersion.api.type.Type;
+
+public class ItemTypeTest {
+    @Test
+    public void test() throws Exception {
+        ByteBuf buf = Unpooled.buffer();
+
+        // Test empty item read
+        Assertions.assertNull(Type.ITEM.read(Unpooled.wrappedBuffer(new byte[]{-1, -1})));
+        Assertions.assertNull(Type.FLAT_ITEM.read(Unpooled.wrappedBuffer(new byte[]{-1, -1})));
+        Assertions.assertNull(Type.FLAT_VAR_INT_ITEM.read(Unpooled.wrappedBuffer(new byte[]{0})));
+
+        // Test item read
+        Assertions.assertEquals(
+                new Item((int) Short.MAX_VALUE, (byte) -128, (short) 257, null),
+                Type.ITEM.read(Unpooled.wrappedBuffer(new byte[]{
+                        127, -1,
+                        -128,
+                        1, 1,
+                        0
+                }))
+        );
+        Assertions.assertEquals(
+                new Item(420, (byte) 53, (short) 0, null),
+                Type.FLAT_ITEM.read(Unpooled.wrappedBuffer(new byte[]{
+                        1, (byte) 164,
+                        53,
+                        0
+                }))
+        );
+        Assertions.assertEquals(
+                new Item(268435456, (byte) 127, (short) 0, null),
+                Type.FLAT_VAR_INT_ITEM.read(Unpooled.wrappedBuffer(new byte[]{
+                        1,
+                        -128, -128, -128, -128, 1,
+                        127,
+                        0
+                }))
+        );
+
+        // Test item empty write
+        Type.ITEM.write(buf, null);
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{-1, -1});
+        Type.FLAT_ITEM.write(buf, null);
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{-1, -1});
+        Type.FLAT_VAR_INT_ITEM.write(buf, null);
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{0});
+
+        // Test item write
+        Type.ITEM.write(buf, new Item((int) Short.MAX_VALUE, (byte) -128, (short) 257, null));
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{
+                127, -1,
+                -128,
+                1, 1,
+                0
+        });
+        Type.FLAT_ITEM.write(buf, new Item(420, (byte) 53, (short) 0, null));
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{
+                1, (byte) 164,
+                53,
+                0
+        });
+        Type.FLAT_VAR_INT_ITEM.write(buf, new Item(268435456, (byte) 127, (short) 0, null));
+        Assertions.assertArrayEquals(toBytes(buf), new byte[]{
+                1,
+                -128, -128, -128, -128, 1,
+                127,
+                0
+        });
+    }
+
+    private byte[] toBytes(ByteBuf byteBuf) {
+        byte[] bytes = new byte[byteBuf.readableBytes()];
+        byteBuf.readBytes(bytes);
+        return bytes;
+    }
+}

common/src/test/java/us/myles/ViaVersion/common/test/type/StringTypeTest.java

@@ -0,0 +1,57 @@
+package us.myles.ViaVersion.common.test.type;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
+import io.netty.buffer.Unpooled;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+import us.myles.ViaVersion.api.type.Type;
+
+public class StringTypeTest {
+    @Test
+    public void test() throws Exception {
+        // Write
+        final ByteBuf buf = Unpooled.buffer();
+        Type.STRING.write(buf, "\uD83E\uDDFD"); // Sponge Emoji
+        Assertions.assertEquals(ByteBufUtil.hexDump(buf), "04f09fa7bd");
+        buf.clear();
+
+        // Read Write
+        Type.STRING.write(buf, new String(new char[Short.MAX_VALUE]));
+        Assertions.assertEquals(Type.STRING.read(buf), new String(new char[Short.MAX_VALUE]));
+
+        Type.STRING.write(buf, new String(new char[Short.MAX_VALUE]).replace("\0", "ç"));
+        Assertions.assertEquals(Type.STRING.read(buf), new String(new char[Short.MAX_VALUE]).replace("\0", "ç"));
+
+        Type.STRING.write(buf, new String(new char[Short.MAX_VALUE / 2]).replace("\0", "\uD83E\uDDFD"));
+        Assertions.assertEquals(Type.STRING.read(buf), new String(new char[Short.MAX_VALUE / 2]).replace("\0", "\uD83E\uDDFD"));
+
+        // Read exception
+        Type.VAR_INT.write(buf, (Short.MAX_VALUE + 1) * 4);
+        for (int i = 0; i < Short.MAX_VALUE / 2 + 1; i++) {
+            buf.writeBytes(new byte[]{0x04, (byte) 0xf0, (byte) 0x9f, (byte) 0xa7, (byte) 0xbd}); // Sponge emoji
+        }
+        Assertions.assertThrows(IllegalArgumentException.class, new Executable() {
+            @Override
+            public void execute() throws Throwable {
+                Type.STRING.read(buf);
+            }
+        });
+
+        // Write exceptions
+        Assertions.assertThrows(IllegalArgumentException.class, new Executable() {
+            @Override
+            public void execute() throws Throwable {
+                Type.STRING.write(buf, new String(new char[Short.MAX_VALUE / 2 + 1]).replace("\0", "\uD83E\uDDFD"));
+            }
+        });
+
+        Assertions.assertThrows(IllegalArgumentException.class, new Executable() {
+            @Override
+            public void execute() throws Throwable {
+                Type.STRING.write(buf, new String(new char[Short.MAX_VALUE + 1]));
+            }
+        });
+    }
+}

pom.xml

@@ -117,6 +117,14 @@
             <version>17.0</version>
             <scope>provided</scope>
         </dependency>
+
+        <!-- JUnit -->
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <version>5.3.1</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
 </project>