Copilot commented on code in PR #179:
URL:
https://github.com/apache/maven-toolchains-plugin/pull/179#discussion_r3630885988
##########
src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java:
##########
@@ -51,4 +56,87 @@ void testDiscovery() {
assertTrue(persistedToolchains.getToolchains().stream()
.anyMatch(tc -> tc.getProvides().containsKey(CURRENT)));
}
+
+ @Test
+ void testVersionComparatorSimple() {
+ ToolchainDiscoverer discoverer = new ToolchainDiscoverer();
+
+ List<ToolchainModel> 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<ToolchainModel> 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<ToolchainModel> 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"));
+ }
Review Comment:
The new comparator logic introduces suffix handling (including examples like
`17.0.2+8` in the PR description), but the tests don’t cover ordering when both
segments contain suffixes and only the suffix differs (e.g., `17.0.2+8` vs
`17.0.2+4`, or two different `-ea`-style qualifiers). Adding a test for “suffix
vs suffix” comparisons would catch the current `return 0` behavior and pin down
the intended ordering semantics.
##########
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java:
##########
@@ -370,24 +370,52 @@ Comparator<ToolchainModel> 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;
+ }
Review Comment:
`parseVersionSegment` can overflow `int` for sufficiently long numeric
segments, silently wrapping and producing incorrect ordering. If version
strings can come from external/toolchain metadata, this is a correctness issue.
Consider parsing into a `long` (or clamping at `Integer.MAX_VALUE` /
`Long.MAX_VALUE`) and comparing using the wider type, while still keeping the
“no exceptions” fast path.
##########
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java:
##########
@@ -370,24 +370,52 @@ Comparator<ToolchainModel> 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;
+ }
Review Comment:
The comparator returns `0` for segments that are not equal but share the
same leading digits and both have a suffix (e.g., `"2+8"` vs `"2+4"`, `"11-ea"`
vs `"11-beta"`). Returning `0` for unequal values can lead to non-deterministic
ordering and can break uses of the comparator in sorted sets/maps (distinct
items may be treated as duplicates). Consider adding a deterministic
tie-breaker when `na == nb` and both have suffixes (e.g., compare the suffix
substrings after the digit prefix, potentially with special handling for
numeric build/update suffixes like `+8` / `_202`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
