Sort compounds in NBT lists for hashing (#1391)

projects/common/src/main/java/dan200/computercraft/shared/util/NBTUtil.java

@@ -4,6 +4,7 @@
 
 package dan200.computercraft.shared.util;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.io.BaseEncoding;
 import dan200.computercraft.core.util.Nullability;
 import net.minecraft.nbt.*;
@@ -23,7 +24,8 @@ import java.util.Map;
 
 public final class NBTUtil {
     private static final Logger LOG = LoggerFactory.getLogger(NBTUtil.class);
-    private static final BaseEncoding ENCODING = BaseEncoding.base16().lowerCase();
+    @VisibleForTesting
+    static final BaseEncoding ENCODING = BaseEncoding.base16().lowerCase();
 
     private NBTUtil() {
     }
@@ -156,7 +158,7 @@ public final class NBTUtil {
         try {
             var digest = MessageDigest.getInstance("MD5");
             DataOutput output = new DataOutputStream(new DigestOutputStream(digest));
-            writeTag(output, "", tag);
+            writeNamedTag(output, "", tag);
             var hash = digest.digest();
             return ENCODING.encode(hash);
         } catch (NoSuchAlgorithmException | IOException e) {
@@ -175,24 +177,33 @@ public final class NBTUtil {
      * @throws IOException If the underlying stream throws.
      * @see NbtIo#write(CompoundTag, DataOutput)
      * @see CompoundTag#write(DataOutput)
+     * @see ListTag#write(DataOutput)
      */
-    private static void writeTag(DataOutput output, String name, Tag tag) throws IOException {
+    private static void writeNamedTag(DataOutput output, String name, Tag tag) throws IOException {
         output.writeByte(tag.getId());
         if (tag.getId() == 0) return;
         output.writeUTF(name);
+        writeTag(output, tag);
+    }
 
+    private static void writeTag(DataOutput output, Tag tag) throws IOException {
         if (tag instanceof CompoundTag compound) {
             var keys = compound.getAllKeys().toArray(new String[0]);
             Arrays.sort(keys);
-            for (var key : keys) writeTag(output, key, Nullability.assertNonNull(compound.get(key)));
+            for (var key : keys) writeNamedTag(output, key, Nullability.assertNonNull(compound.get(key)));
 
             output.writeByte(0);
+        } else if (tag instanceof ListTag list) {
+            output.writeByte(list.isEmpty() ? 0 : list.get(0).getId());
+            output.writeInt(list.size());
+            for (var value : list) writeTag(output, value);
         } else {
             tag.write(output);
         }
     }
 
-    private static final class DigestOutputStream extends OutputStream {
+    @VisibleForTesting
+    static final class DigestOutputStream extends OutputStream {
         private final MessageDigest digest;
 
         DigestOutputStream(MessageDigest digest) {

projects/common/src/test/java/dan200/computercraft/shared/util/NBTUtilTest.java

@@ -0,0 +1,119 @@
+// SPDX-FileCopyrightText: 2022 The CC: Tweaked Developers
+//
+// SPDX-License-Identifier: MPL-2.0
+
+package dan200.computercraft.shared.util;
+
+import dan200.computercraft.test.shared.WithMinecraft;
+import net.minecraft.Util;
+import net.minecraft.nbt.CompoundTag;
+import net.minecraft.nbt.ListTag;
+import net.minecraft.nbt.NbtIo;
+import org.junit.jupiter.api.Test;
+
+import java.io.DataOutput;
+import java.io.DataOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.List;
+
+import static dan200.computercraft.shared.util.NBTUtil.getNBTHash;
+import static org.junit.jupiter.api.Assertions.*;
+
+@WithMinecraft
+public class NBTUtilTest {
+    @Test
+    public void testCompoundTagSorting() {
+        var nbt1 = makeCompoundTag(false);
+        var hash1 = getNBTHash(nbt1);
+
+        var nbt2 = makeCompoundTag(true);
+        var hash2 = getNBTHash(nbt2);
+
+        assertNotNull(hash1, "NBT hash should not be null");
+        assertNotNull(hash2, "NBT hash should not be null");
+        assertEquals(hash1, hash2, "NBT hashes should be equal");
+    }
+
+    @Test
+    public void testListTagSorting() {
+        var nbt1 = new CompoundTag();
+        nbt1.put("Items", makeListTag(false));
+        var hash1 = getNBTHash(nbt1);
+
+        var nbt2 = new CompoundTag();
+        nbt2.put("Items", makeListTag(true));
+        var hash2 = getNBTHash(nbt2);
+
+        assertNotNull(hash1, "NBT hash should not be null");
+        assertNotNull(hash2, "NBT hash should not be null");
+        assertEquals(hash1, hash2, "NBT hashes should be equal");
+    }
+
+    @Test
+    public void testTagsHaveDifferentOrders() {
+        var nbt1 = makeCompoundTag(false);
+        var nbt2 = makeCompoundTag(true);
+        assertNotEquals(
+            List.copyOf(nbt1.getAllKeys()), List.copyOf(nbt2.getAllKeys()),
+            "Expected makeCompoundTag to return keys with different orders."
+        );
+    }
+
+    @Test
+    public void testHashEquivalentForBasicValues() {
+        var nbt = new CompoundTag();
+        nbt.put("list", Util.make(new ListTag(), list -> {
+            list.add(Util.make(new CompoundTag(), t -> t.putBoolean("key", true)));
+            list.add(Util.make(new CompoundTag(), t -> t.putInt("key", 23)));
+        }));
+
+        assertEquals(getNBTHash(nbt), getNBTHashDefault(nbt));
+    }
+
+    private static CompoundTag makeCompoundTag(boolean grow) {
+        var nbt = new CompoundTag();
+
+        nbt.putString("Slot", "Slot 1");
+        nbt.putString("Count", "64");
+        nbt.putString("id", "123");
+
+        // Grow the map to cause a rehash and (hopefully) change the order around a little bit.
+        if (grow) {
+            for (var i = 0; i < 64; i++) nbt.putBoolean("x" + i, true);
+            for (var i = 0; i < 64; i++) nbt.remove("x" + i);
+        }
+
+        return nbt;
+    }
+
+    private static ListTag makeListTag(boolean reverse) {
+        var list = new ListTag();
+
+        for (var i = 0; i < 3; i++) {
+            list.add(makeCompoundTag(reverse));
+        }
+
+        return list;
+    }
+
+    /**
+     * Equivalent to {@link NBTUtil#getNBTHash(CompoundTag)}, but using the default {@link NbtIo#write(CompoundTag, File)} method.
+     *
+     * @param tag The tag to hash.
+     * @return The resulting hash.
+     */
+    private static String getNBTHashDefault(CompoundTag tag) {
+        try {
+            var digest = MessageDigest.getInstance("MD5");
+            DataOutput output = new DataOutputStream(new NBTUtil.DigestOutputStream(digest));
+            NbtIo.write(tag, output);
+            var hash = digest.digest();
+            return NBTUtil.ENCODING.encode(hash);
+        } catch (NoSuchAlgorithmException | IOException e) {
+            throw new IllegalStateException(e);
+        }
+    }
+}