Fix versioned bundles of equal contents being omitted from workspace tree

If you had 4 different versioned bundles with a basic module-info (same class bytes in each path) only one would show. This was due to the default behavior of PathNode#hasEqualOrChildValue which is now has an override in BundlePathNode.

Author: Col-E

recaf-core/src/main/java/software/coley/recaf/path/BundlePathNode.java

@@ -115,19 +115,36 @@ public Set<String> directParentTypeIds() {
 		return Set.of(ResourcePathNode.TYPE_ID);
 	}
 
+	@Override
+	public boolean hasEqualOrChildValue(@Nonnull PathNode<?> other) {
+		// Bundle equality checks are abysmally slow, but also potentially problematic for path comparisons.
+		// Two bundles may have the same contents, but they are not the same bundle.
+		// We kill two birds with one stone by doing a reference check here.
+		// If they are the same bundle, then they are the same path.
+		if (other instanceof BundlePathNode otherBundlePath)
+			return getValue() == otherBundlePath.getValue();
+		return super.hasEqualOrChildValue(other);
+	}
+
 	@Override
 	public int localCompare(PathNode<?> o) {
 		if (this == o)
 			return 0;
 
 		if (o instanceof BundlePathNode bundlePathNode) {
+			// Quick check for bundle reference equality. If they are the same bundle, then they are the same path.
+			Bundle bundle = getValue();
+			Bundle otherBundle = bundlePathNode.getValue();
+			if (bundle == otherBundle)
+				return 0;
+
+			// Order bundles by type.
+			// This is a bitmask that encodes the bundle type which also doubles as order preference.
 			int cmp = Integer.compare(bundleMask(), bundlePathNode.bundleMask());
 			if (cmp != 0)
 				return cmp;
 
 			// Order dex class bundles to be in alphabetical order.
-			Bundle bundle = getValue();
-			Object otherBundle = o.getValue();
 			if (getParent() != null &&
 					bundle instanceof AndroidClassBundle &&
 					otherBundle instanceof AndroidClassBundle) {
@@ -145,6 +162,11 @@ public int localCompare(PathNode<?> o) {
 						.orElse(null);
 				return Named.STRING_PATH_COMPARATOR.compare(dexName, otherDexName);
 			}
+			// Order versioned JVM class bundles by version number.
+			else if (bundle instanceof VersionedJvmClassBundle versionedBundle &&
+					otherBundle instanceof VersionedJvmClassBundle otherVersionedBundle) {
+				return Integer.compare(versionedBundle.version(), otherVersionedBundle.version());
+			}
 		}
 		return 0;
 	}

recaf-core/src/main/java/software/coley/recaf/workspace/model/bundle/BasicBundle.java

@@ -303,6 +303,11 @@ private void resetHash() {
 		hash = 0;
 	}
 
+	@Override
+	public String toString() {
+		return getClass().getSimpleName() + "[" + backing.size() + " items]";
+	}
+
 	@Override
 	public boolean equals(Object o) {
 		if (this == o)

recaf-core/src/main/java/software/coley/recaf/workspace/model/bundle/BasicVersionedJvmClassBundle.java

@@ -42,4 +42,9 @@ public void onUpdateItem(@Nonnull String key, @Nonnull JvmClassInfo oldValue, @N
 	public void onRemoveItem(@Nonnull String key, @Nonnull JvmClassInfo value) {
 		// no-op
 	}
+
+	@Override
+	public String toString() {
+		return super.toString() + " - version=" + version;
+	}
 }

recaf-core/src/test/java/software/coley/recaf/path/PathNodeTest.java

@@ -8,6 +8,7 @@
 import software.coley.recaf.info.ClassInfo;
 import software.coley.recaf.info.FileInfo;
 import software.coley.recaf.info.JvmClassInfo;
+import software.coley.recaf.info.StubClassInfo;
 import software.coley.recaf.info.annotation.Annotated;
 import software.coley.recaf.info.builder.AndroidClassInfoBuilder;
 import software.coley.recaf.info.builder.TextFileInfoBuilder;
@@ -18,6 +19,7 @@
 import software.coley.recaf.workspace.model.bundle.AndroidClassBundle;
 import software.coley.recaf.workspace.model.bundle.BasicAndroidClassBundle;
 import software.coley.recaf.workspace.model.bundle.BasicFileBundle;
+import software.coley.recaf.workspace.model.bundle.BasicVersionedJvmClassBundle;
 import software.coley.recaf.workspace.model.bundle.FileBundle;
 import software.coley.recaf.workspace.model.bundle.JvmClassBundle;
 import software.coley.recaf.workspace.model.resource.WorkspaceResource;
@@ -27,7 +29,9 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.NavigableMap;
 import java.util.Objects;
+import java.util.TreeMap;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatComparable;
@@ -317,6 +321,49 @@ void directoryNodesCanValidateParentChildRelationsFromPathValues() {
 			assertThat(classB.isDescendantOf(classAAA)).isFalse();
 			assertThat(classB.isDescendantOf(classB)).isTrue();
 		}
+
+		/**
+		 * When classes are in different versioned bundles, they should not be considered descendants of each other,
+		 * even if they have the same package and class name. In this scenario all versioned bundles will also have
+		 * map equality which is another problem case that this test is designed to catch (with how paths handle
+		 * value equality checks by default, and how bundle paths override that behavior to address this problem).
+		 */
+		@Test
+		void descendantOfDifferentVersionedClasses() {
+			int min = 9;
+			int max = 16;
+			NavigableMap<Integer, ClassPathNode> versionedClasses = new TreeMap<>();
+			NavigableMap<Integer, BundlePathNode> versionedBundles = new TreeMap<>();
+			for (int i = min; i < max; i++) {
+				BundlePathNode bundlePath = s4.child(new BasicVersionedJvmClassBundle(i));
+				ClassPathNode classPath = bundlePath.child(null).child(new StubClassInfo("Foo"));
+				versionedClasses.put(i, classPath);
+				versionedBundles.put(i, bundlePath);
+			}
+
+			// Validate that only classes from the same version are considered descendants of each other.
+			Integer previousVersion = null;
+			for (int a = min; a < max; a++) {
+				for (int b = min; b < max; b++) {
+					ClassPathNode classA = versionedClasses.get(a);
+					ClassPathNode classB = versionedClasses.get(b);
+					BundlePathNode bundleA = versionedBundles.get(a);
+					BundlePathNode bundleB = versionedBundles.get(b);
+
+					if (a == b) {
+						// Same version
+						assertThat(classA.isDescendantOf(classB)).isTrue();
+						assertThat(classA.isDescendantOf(bundleB)).isTrue();
+						assertThat(bundleA.isDescendantOf(bundleB)).isTrue();
+					} else {
+						// Different version
+						assertThat(classA.isDescendantOf(classB)).isFalse();
+						assertThat(classA.isDescendantOf(bundleB)).isFalse();
+						assertThat(bundleA.isDescendantOf(bundleB)).isFalse();
+					}
+				}
+			}
+		}
 	}
 
 	@Nested

recaf-ui/src/test/java/software/coley/recaf/ui/control/tree/WorkspaceTreeNodeTest.java

@@ -9,6 +9,7 @@
 import software.coley.collections.Unchecked;
 import software.coley.recaf.info.BasicFileInfo;
 import software.coley.recaf.info.FileInfo;
+import software.coley.recaf.info.JarFileInfo;
 import software.coley.recaf.info.JvmClassInfo;
 import software.coley.recaf.info.Named;
 import software.coley.recaf.info.StubClassInfo;
@@ -22,15 +23,28 @@
 import software.coley.recaf.path.PathNodes;
 import software.coley.recaf.path.ResourcePathNode;
 import software.coley.recaf.path.WorkspacePathNode;
+import software.coley.recaf.services.text.TextFormatConfig;
+import software.coley.recaf.services.workspace.io.BasicClassPatcher;
+import software.coley.recaf.services.workspace.io.BasicInfoImporter;
+import software.coley.recaf.services.workspace.io.BasicResourceImporter;
+import software.coley.recaf.services.workspace.io.InfoImporterConfig;
+import software.coley.recaf.services.workspace.io.ResourceImporter;
+import software.coley.recaf.services.workspace.io.ResourceImporterConfig;
+import software.coley.recaf.test.TestClassUtils;
 import software.coley.recaf.test.dummy.AccessibleFields;
 import software.coley.recaf.test.dummy.HelloWorld;
 import software.coley.recaf.test.dummy.StringConsumer;
 import software.coley.recaf.test.dummy.VariedModifierFields;
+import software.coley.recaf.ui.config.WorkspaceExplorerConfig;
+import software.coley.recaf.util.ZipCreationUtils;
+import software.coley.recaf.util.io.ByteSource;
+import software.coley.recaf.util.io.ByteSources;
 import software.coley.recaf.workspace.model.BasicWorkspace;
 import software.coley.recaf.workspace.model.Workspace;
 import software.coley.recaf.workspace.model.bundle.BasicFileBundle;
 import software.coley.recaf.workspace.model.bundle.BasicJvmClassBundle;
 import software.coley.recaf.workspace.model.bundle.JvmClassBundle;
+import software.coley.recaf.workspace.model.bundle.VersionedJvmClassBundle;
 import software.coley.recaf.workspace.model.resource.WorkspaceFileResource;
 import software.coley.recaf.workspace.model.resource.WorkspaceFileResourceBuilder;
 import software.coley.recaf.workspace.model.resource.WorkspaceResource;
@@ -39,10 +53,13 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Comparator;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Stream;
 
@@ -54,6 +71,8 @@
  * Tests for {@link WorkspaceTreeNode}.
  */
 class WorkspaceTreeNodeTest {
+	static ResourceImporter importer;
+	// Workspace model for testing - Used by multiple tests, but some will make their own.
 	static Workspace workspace;
 	static WorkspaceResource primaryResource;
 	static WorkspaceResource resourceWithEmbedded;
@@ -84,6 +103,11 @@ class WorkspaceTreeNodeTest {
 
 	@BeforeAll
 	static void setup() throws IOException {
+		importer = new BasicResourceImporter(
+				new BasicInfoImporter(new InfoImporterConfig(), new TextFormatConfig(), new BasicClassPatcher()),
+				new ResourceImporterConfig()
+		);
+
 		BasicFileBundle fileBundle = new BasicFileBundle();
 
 		primaryJvmBundle = fromClasses(
@@ -349,6 +373,57 @@ void nameOrdering() {
 		});
 	}
 
+	/**
+	 * While not an issue of name overloading in adjacent tree nodes, this test covers a case/regression where we
+	 * saw multiple classes with the same name in different versioned paths not being all shown.
+	 * This scenario has the effect of making all {@link VersionedJvmClassBundle} have map equality too which was
+	 * another issue with an older version of the path node path containment/equality logic.
+	 */
+	@Test
+	void multipleVersionedPaths() throws Exception {
+		String classPath = HelloWorld.class.getName().replace(".", "/");
+		String classPackage = classPath.substring(0, classPath.lastIndexOf('/'));
+		byte[] classBytes = TestClassUtils.fromRuntimeClass(HelloWorld.class).getBytecode();
+
+		// Create JAR with 'META-INF/versions/<dummyversion>/<dummypackage>/HelloWorld.class' for multiple versions.
+		byte[] zipBytes = ZipCreationUtils.builder()
+				.add(classPath + ".class", classBytes)
+				.add(JarFileInfo.MULTI_RELEASE_PREFIX + "9/" + classPath + ".class", classBytes)
+				.add(JarFileInfo.MULTI_RELEASE_PREFIX + "11/" + classPath + ".class", classBytes)
+				.add(JarFileInfo.MULTI_RELEASE_PREFIX + "16/" + classPath + ".class", classBytes)
+				.add(JarFileInfo.MULTI_RELEASE_PREFIX + "21/" + classPath + ".class", classBytes)
+				.add(JarFileInfo.MULTI_RELEASE_PREFIX + "25/" + classPath + ".class", classBytes)
+				.bytes();
+		ByteSource zipSource = ByteSources.wrap(zipBytes);
+
+		// Build the workspace and validate the versioned bundles exist.
+		WorkspaceResource resource = importer.importResource(zipSource);
+		BasicWorkspace workspace = new BasicWorkspace(resource);
+		assertEquals(5, resource.getVersionedJvmClassBundles().size(), "Expected 5 versioned bundles: 9, 11, 16, 21, 25");
+
+		// Build the tree model.
+		WorkspacePathNode rootPath = PathNodes.workspacePath(workspace);
+		WorkspaceRootTreeNode root = new WorkspaceRootTreeNode(new WorkspaceExplorerConfig(), rootPath);
+		root.build();
+
+		// Iterate over all versions, validate the path to the versioned class exists and that no duplicate tree paths exist.
+		final int baseline = -1;
+		int[] versions = new int[]{baseline, 9, 11, 16, 21, 25};
+		Set<BundlePathNode> visitedNodes = Collections.newSetFromMap(new IdentityHashMap<>());
+		for (int version : versions) {
+			String versionName = version == baseline ? "baseline" : String.valueOf(version);
+			ClassPathNode path = version == baseline ?
+					workspace.findJvmClass(classPath) :
+					workspace.findVersionedJvmClass(classPath, version);
+			assertNotNull(path, "Could not find class path for version: " + versionName);
+			WorkspaceTreeNode classNode = root.getNodeByPath(path);
+			assertNotNull(classNode, "Could not find tree node for class path for version: " + versionName);
+			BundlePathNode bundleNode = path.getParent().getParent();
+			assertTrue(visitedNodes.add(bundleNode), "Duplicate bundle node found for version: " + versionName);
+		}
+		assertEquals(6, visitedNodes.size(), "Expected 6 unique bundle nodes for versions: baseline, 9, 11, 16, 21, 25");
+	}
+
 	/**
 	 * The named path sorter was getting confused when checking "does 'a' have 'b' as a parent-directory" if either
 	 * String was empty, which is the case for classes in the default package. This led to inconsistent return value