Re: [PR] feat: Improve the matching of required versions [maven]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on code in PR #11770:
URL: https://github.com/apache/maven/pull/11770#discussion_r3646437422


##########
maven-core/src/main/java/org/apache/maven/toolchain/RequirementMatcherFactory.java:
##########
@@ -79,6 +81,29 @@ public boolean matches(String requirement) {
             }
         }
 
+        private VersionRange convertRequirementToVersionRange(String requirement)
+                throws InvalidVersionSpecificationException {
+            // Specific for Version _requirement_ matching;
+            // If the version is a simple integer (like "25")
+            // then treat this as the requirement "the major version is 25"
+            if (Pattern.matches("^[0-9]+$", requirement)) {
+                int majorVersion = Integer.parseInt(requirement);
+                return VersionRange.createFromVersionSpec("[" + majorVersion + "," + (majorVersion + 1) + ")");
+            }
+
+            // If the version is a major.minor (like "1.5")
+            // then treat this as the requirement "the major version is 1 and the minor is 5"
+            if (Pattern.matches("^[0-9]\\.[0-9]+$", requirement)) {

Review Comment:
   Bug: `[0-9]` matches only a single digit for the major version. Requirements like `"21.3"`, `"17.0"`, or `"11.0"` will not match this pattern and will fall through to exact-match behavior — exactly the most common modern JDK versions.
   
   The major-only pattern on line 89 correctly uses `[0-9]+`.
   
   ```suggestion
               if (Pattern.matches("^[0-9]+\\.[0-9]+$", requirement)) {
   ```



##########
maven-core/src/test/java/org/apache/maven/toolchain/RequirementMatcherFactoryTest.java:
##########
@@ -50,11 +50,18 @@ public void testCreateExactMatcher() {
     public void testCreateVersionMatcher() {
         RequirementMatcher matcher;
         matcher = RequirementMatcherFactory.createVersionMatcher("1.5.2");
-        assertFalse(matcher.matches("1.5"));
-        assertTrue(matcher.matches("1.5.2"));
+        assertTrue(matcher.matches("1")); // Major matches
+        assertTrue(matcher.matches("1.5")); // Major.Minor matches
+        assertTrue(matcher.matches("1.5.2")); // Full match

Review Comment:
   Consider adding test cases with multi-digit major versions to cover the regex bug above. For example:
   
   ```java
   matcher = RequirementMatcherFactory.createVersionMatcher("21.0.2");
   assertTrue(matcher.matches("21"));    // Major-only
   assertTrue(matcher.matches("21.0"));  // Major.minor
   assertFalse(matcher.matches("17"));   // Wrong major
   ```
   
   This would immediately expose the single-digit regex limitation.



-- 
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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org