Copilot commented on code in PR #177:
URL:
https://github.com/apache/maven-toolchains-plugin/pull/177#discussion_r3629966839
##########
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java:
##########
@@ -370,24 +370,25 @@ 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 oa = parseInt(a[i]);
+ int ob = parseInt(b[i]);
+ if (oa != ob) {
+ return Integer.compare(oa, ob);
}
Review Comment:
`parseInt(a[i])` assumes each dot-separated segment is a pure integer.
Common JDK version formats frequently include non-numeric characters in a
segment (e.g., `1.8.0_202`, `17.0.2+8`, `11-ea`), which will currently parse as
`0` (via the catch) and can collapse distinct versions into ties or produce
incorrect ordering (e.g., update/build numbers become ignored). Consider
parsing the *leading numeric prefix* of each segment (and optionally comparing
any remaining suffix as a tiebreaker), or switching to a version parser
designed for this (e.g., `Runtime.Version` where applicable, or Maven’s
`ComparableVersion`/`DefaultArtifactVersion`) so real-world JDK version strings
are ordered correctly.
##########
src/main/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscoverer.java:
##########
@@ -370,24 +370,25 @@ 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 oa = parseInt(a[i]);
+ int ob = parseInt(b[i]);
+ if (oa != ob) {
+ return Integer.compare(oa, ob);
}
}
- return a.length - b.length;
+ return Integer.compare(a.length, b.length);
})
.reversed();
}
+ private static int parseInt(String s) {
+ try {
+ return Integer.parseInt(s);
+ } catch (NumberFormatException e) {
+ return 0;
+ }
+ }
Review Comment:
This comparator can run frequently during discovery/sorting, and using
exceptions for control flow (`NumberFormatException`) is relatively expensive.
If you keep a custom parser, prefer a non-throwing approach (e.g., scan
characters and accumulate digits until the first non-digit) to avoid exception
overhead while still handling segments like `0_202` or `2+8`.
##########
src/test/java/org/apache/maven/plugins/toolchain/jdk/ToolchainDiscovererTest.java:
##########
@@ -51,4 +56,32 @@ void testDiscovery() {
assertTrue(persistedToolchains.getToolchains().stream()
.anyMatch(tc -> tc.getProvides().containsKey(CURRENT)));
}
+
+ @Test
+ void testVersionComparator() {
+ ToolchainDiscoverer discoverer = new ToolchainDiscoverer();
+
+ ToolchainModel jdk8 = new ToolchainModel();
+ jdk8.setType("jdk");
+ jdk8.addProvide("version", "8");
+
+ ToolchainModel jdk11 = new ToolchainModel();
+ jdk11.setType("jdk");
+ jdk11.addProvide("version", "11");
+
+ ToolchainModel jdk17 = new ToolchainModel();
+ jdk17.setType("jdk");
+ jdk17.addProvide("version", "17");
+
+ List<ToolchainModel> list = new ArrayList<>();
+ list.add(jdk8);
+ list.add(jdk17);
+ list.add(jdk11);
+
+ 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"));
+ }
Review Comment:
The new comparator behavior depends heavily on how non-trivial segments are
parsed (e.g., dotted versions, underscores, `+build`, `-ea`). This test only
covers single-number versions, so it won’t catch regressions introduced by
`parseInt` returning `0` on non-numeric segments. Please add cases like
`17.0.2` vs `17.0.10`, `1.8.0_202` vs `1.8.0_121`, and `11-ea` vs `11` (or
whatever ordering the project expects) to lock down correct ordering for
real-world JDK version strings.
--
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]
