From 1d32c266e3d23a1c4b8183320f4a1248824a1e4b Mon Sep 17 00:00:00 2001 From: Elliotte Rusty Harold Date: Wed, 22 Jul 2026 13:37:39 +0000 Subject: [PATCH 1/2] Fix version comparator: use numeric comparison with non-numeric segment support (#166) - Replace String.compareTo() with numeric comparison of leading digit prefix in each version segment - Use non-throwing character scan instead of parseInt for performance - Handle non-numeric suffixes (e.g. 1.8.0_202, 17.0.2+8, 11-ea) by comparing leading digit prefix first, then treating segments with suffixes as lower than clean numeric segments - Add tests: simple versions, multi-part, multi-digit segments, underscore suffixes, and EA/pre-release variants --- .../toolchain/jdk/ToolchainDiscoverer.java | 52 ++++++++--- .../jdk/ToolchainDiscovererTest.java | 88 +++++++++++++++++++ 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java b/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java index fa9455d..ff6a73f 100644 --- a/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java +++ b/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java @@ -370,24 +370,52 @@ Comparator version() { String[] b = v2.split("\\."); int length = Math.min(a.length, b.length); for (int i = 0; i < length; i++) { - String oa = a[i]; - String ob = b[i]; - if (!Objects.equals(oa, ob)) { - // A null element is less than a non-null element - if (oa == null || ob == null) { - return oa == null ? -1 : 1; - } - int v = oa.compareTo(ob); - if (v != 0) { - return v; - } + int cmp = compareVersionSegments(a[i], b[i]); + if (cmp != 0) { + return cmp; } } - return a.length - b.length; + return Integer.compare(a.length, b.length); }) .reversed(); } + private static int compareVersionSegments(String sa, String sb) { + int na = parseVersionSegment(sa); + int nb = parseVersionSegment(sb); + if (na != nb) { + return Integer.compare(na, nb); + } + boolean suffixA = hasSuffix(sa); + boolean suffixB = hasSuffix(sb); + if (suffixA != suffixB) { + return suffixA ? -1 : 1; + } + return 0; + } + + private static boolean hasSuffix(String s) { + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + if (c < '0' || c > '9') { + return true; + } + } + return false; + } + + private static int parseVersionSegment(String s) { + int n = 0; + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + if (c < '0' || c > '9') { + break; + } + n = n * 10 + (c - '0'); + } + return n; + } + private Set findJdks() { if (foundJdks == null) { synchronized (this) { diff --git a/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java b/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java index 11657c0..301603e 100644 --- a/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java +++ b/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java @@ -18,7 +18,11 @@ */ package org.apache.maven.plugins.toolchain.jdk; +import java.util.ArrayList; +import java.util.List; + import org.apache.maven.toolchain.model.PersistedToolchains; +import org.apache.maven.toolchain.model.ToolchainModel; import org.codehaus.plexus.util.xml.Xpp3Dom; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledOnJre; @@ -27,6 +31,7 @@ import org.slf4j.LoggerFactory; import static org.apache.maven.plugins.toolchain.jdk.ToolchainDiscoverer.CURRENT; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -51,4 +56,87 @@ void testDiscovery() { assertTrue(persistedToolchains.getToolchains().stream() .anyMatch(tc -> tc.getProvides().containsKey(CURRENT))); } + + @Test + void testVersionComparatorSimple() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("8")); + list.add(toolchain("17")); + list.add(toolchain("11")); + + list.sort(discoverer.version()); + + assertEquals("17", list.get(0).getProvides().getProperty("version")); + assertEquals("11", list.get(1).getProvides().getProperty("version")); + assertEquals("8", list.get(2).getProvides().getProperty("version")); + } + + @Test + void testVersionComparatorMultiPart() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("11.0.1")); + list.add(toolchain("11.0.31")); + list.add(toolchain("17.0.1")); + list.add(toolchain("1.8")); + + list.sort(discoverer.version()); + + assertEquals("17.0.1", list.get(0).getProvides().getProperty("version")); + assertEquals("11.0.31", list.get(1).getProvides().getProperty("version")); + assertEquals("11.0.1", list.get(2).getProvides().getProperty("version")); + assertEquals("1.8", list.get(3).getProvides().getProperty("version")); + } + + @Test + void testVersionComparatorMultiDigitSegments() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("17.0.2")); + list.add(toolchain("17.0.10")); + + list.sort(discoverer.version()); + + assertEquals("17.0.10", list.get(0).getProvides().getProperty("version")); + assertEquals("17.0.2", list.get(1).getProvides().getProperty("version")); + } + + @Test + void testVersionComparatorWithNonNumericSuffix() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("1.8.0_202")); + list.add(toolchain("1.8.0_121")); + + list.sort(discoverer.version()); + + assertEquals("1.8.0_202", list.get(0).getProvides().getProperty("version")); + assertEquals("1.8.0_121", list.get(1).getProvides().getProperty("version")); + } + + @Test + void testVersionComparatorWithEAPreRelease() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("11-ea")); + list.add(toolchain("11")); + + list.sort(discoverer.version()); + + assertEquals("11", list.get(0).getProvides().getProperty("version")); + assertEquals("11-ea", list.get(1).getProvides().getProperty("version")); + } + + private static ToolchainModel toolchain(String version) { + ToolchainModel model = new ToolchainModel(); + model.setType("jdk"); + model.addProvide("version", version); + return model; + } } From 283472d755a32b3956f99dd24446a4c137a573ca Mon Sep 17 00:00:00 2001 From: Elliotte Rusty Harold Date: Wed, 22 Jul 2026 14:37:18 +0000 Subject: [PATCH 2/2] Address copilot review: long overflow, suffix-vs-suffix tiebreaker, add suffix test --- .../toolchain/jdk/ToolchainDiscoverer.java | 27 +++++++++---------- .../jdk/ToolchainDiscovererTest.java | 14 ++++++++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java b/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java index ff6a73f..bda806a 100644 --- a/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java +++ b/src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java @@ -381,31 +381,30 @@ Comparator version() { } private static int compareVersionSegments(String sa, String sb) { - int na = parseVersionSegment(sa); - int nb = parseVersionSegment(sb); - if (na != nb) { - return Integer.compare(na, nb); + int cmp = Long.compare(parseVersionSegment(sa), parseVersionSegment(sb)); + if (cmp != 0) { + return cmp; } - boolean suffixA = hasSuffix(sa); - boolean suffixB = hasSuffix(sb); - if (suffixA != suffixB) { - return suffixA ? -1 : 1; + String suffixA = suffix(sa); + String suffixB = suffix(sb); + if (suffixA.isEmpty() != suffixB.isEmpty()) { + return suffixA.isEmpty() ? 1 : -1; } - return 0; + return suffixA.compareTo(suffixB); } - private static boolean hasSuffix(String s) { + private static String suffix(String s) { for (int i = 0; i < s.length(); i++) { char c = s.charAt(i); if (c < '0' || c > '9') { - return true; + return s.substring(i); } } - return false; + return ""; } - private static int parseVersionSegment(String s) { - int n = 0; + private static long parseVersionSegment(String s) { + long n = 0; for (int i = 0; i < s.length(); i++) { char c = s.charAt(i); if (c < '0' || c > '9') { diff --git a/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java b/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java index 301603e..3a2a056 100644 --- a/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java +++ b/src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java @@ -133,6 +133,20 @@ void testVersionComparatorWithEAPreRelease() { assertEquals("11-ea", list.get(1).getProvides().getProperty("version")); } + @Test + void testVersionComparatorSuffixVsSuffix() { + ToolchainDiscoverer discoverer = new ToolchainDiscoverer(); + + List list = new ArrayList<>(); + list.add(toolchain("17.0.2+8")); + list.add(toolchain("17.0.2+4")); + + list.sort(discoverer.version()); + + assertEquals("17.0.2+8", list.get(0).getProvides().getProperty("version")); + assertEquals("17.0.2+4", list.get(1).getProvides().getProperty("version")); + } + private static ToolchainModel toolchain(String version) { ToolchainModel model = new ToolchainModel(); model.setType("jdk");