[MNG-7559] Fix version comparison with case insensitive lexical order

sultan · sultan · commit 28feee8b7e6b · 2025-11-19T22:35:12.000+01:00
diff --git a/compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java b/compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java
@@ -23,11 +23,12 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Deque;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Objects;
-import java.util.Properties;
 
 /**
  * <p>
@@ -41,20 +42,21 @@
  *     <code>1.0alpha1 =&gt; [1, [alpha, 1]]</code></li>
  * <li>Unlimited number of version components,</li>
  * <li>Version components in the text can be digits or strings,</li>
- * <li>Strings are checked for well-known qualifiers, and the qualifier ordering is used for version ordering.
- *     Well-known qualifiers (case-insensitive) are, in order from least to greatest:<ol>
- *     <li><code>alpha</code> or <code>a</code></li>
- *     <li><code>beta</code> or <code>b</code></li>
- *     <li><code>milestone</code> or <code>m</code></li>
- *     <li><code>rc</code> or <code>cr</code></li>
- *     <li><code>snapshot</code></li>
- *     <li><code>ga</code> or <code>final</code></li>
- *     <li><code>sp</code></li>
- *     </ol>
- *     Unknown qualifiers are considered after known qualifiers,
- *     with lexical order (case-insensitive in the English locale).
- *     <code>ga</code> and <code>final</code> sort the same as not having a qualifier.
- *   </li>
+ * <li>
+ *   Following semver rules is encouraged, and some qualifiers are discouraged (no matter the case sensitivity):
+ *   <ul>
+ *     <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ *     <li> The usage of 'Final', 'GA', and 'Release' qualifiers is discouraged. Use no qualifier instead. </li>
+ *     <li> The usage of 'SP' qualifier is discouraged. Increment the patch version instead. </li>
+ *   </ul>
+ *   String qualifiers are ordered lexically (case insensitive in the English locale), with the following exceptions:
+ *   <ul>
+ *     <li><code> alpha = a &lt; beta = b &lt; milestone = m &lt; rc = cr
+ *         &lt; snapshot &lt; '' = final = ga = release &lt; sp </code></li>
+ *     <li>Other qualifiers are ordered lexically (case insensitive in the English locale),
+ *         and considered less than Snapshot and Release.</li>
+ *   </ul>
+ * </li>
  * <li>A hyphen usually precedes a qualifier, and is always less important than digits/number. For example
  *   {@code 1.0.RC2 < 1.0-RC3 < 1.0.1}; but prefer {@code 1.0.0-RC2} over {@code 1.0.0.RC2}, and more
  *   generally: {@code 1.0.X2 < 1.0-X3 < 1.0.1} for any string {@code X}; but prefer {@code 1.0.0-X1}
@@ -295,41 +297,37 @@ public String toString() {
      * Represents a string in the version item list, usually a qualifier.
      */
     private static class StringItem implements Item {
-        private static final List<String> QUALIFIERS =
-                Arrays.asList("alpha", "beta", "milestone", "rc", "snapshot", "", "sp");
-        private static final List<String> RELEASE_QUALIFIERS = Arrays.asList("ga", "final", "release");
+        private static final List<String> QUALIFIERS = Arrays.asList("snapshot", "", "sp");
+        private static final List<String> RELEASE_QUALIFIERS = Arrays.asList("final", "ga", "release");
 
-        private static final Properties ALIASES = new Properties();
+        private static final Map<String, String> ALIASES = new HashMap<>(4);
 
         static {
             ALIASES.put("cr", "rc");
+            ALIASES.put("final", "");
+            ALIASES.put("ga", "");
+            ALIASES.put("release", "");
         }
 
         /**
-         * A comparable value for the empty-string qualifier. This one is used to determine if a given qualifier makes
+         * An index value for the empty-string qualifier. This one is used to determine if a given qualifier makes
          * the version older than one without a qualifier, or more recent.
          */
-        private static final String RELEASE_VERSION_INDEX = String.valueOf(QUALIFIERS.indexOf(""));
+        private static final int RELEASE_VERSION_INDEX = QUALIFIERS.indexOf("");
 
         private final String value;
 
         StringItem(String value, boolean followedByDigit) {
             if (followedByDigit && value.length() == 1) {
                 // a1 = alpha-1, b1 = beta-1, m1 = milestone-1
-                switch (value.charAt(0)) {
-                    case 'a':
-                        value = "alpha";
-                        break;
-                    case 'b':
-                        value = "beta";
-                        break;
-                    case 'm':
-                        value = "milestone";
-                        break;
-                    default:
-                }
+                value = switch (value.charAt(0)) {
+                    case 'a' -> "alpha";
+                    case 'b' -> "beta";
+                    case 'm' -> "milestone";
+                    default -> value;
+                };
             }
-            this.value = ALIASES.getProperty(value, value);
+            this.value = ALIASES.getOrDefault(value, value);
         }
 
         @Override
@@ -351,27 +349,55 @@ public boolean isNull() {
          *
          * @param qualifier
          * @return an equivalent value that can be used with lexical comparison
+         * @deprecated Use {@link #compareQualifiers(String, String)} instead
          */
+        @Deprecated
         public static String comparableQualifier(String qualifier) {
+            // Just returning an Integer with the index here is faster, but requires a lot of if/then/else to check for
+            // -1 or QUALIFIERS.size and then resort to lexical ordering. Most comparisons are decided by the first
+            // character, so this is still fast. If more characters are needed then it requires a lexical sort anyway.
+
             if (RELEASE_QUALIFIERS.contains(qualifier)) {
-                return String.valueOf(QUALIFIERS.indexOf(""));
+                qualifier = "";
             }
 
-            int i = QUALIFIERS.indexOf(qualifier);
+            int index = QUALIFIERS.indexOf(qualifier) + 1;
 
-            // Just returning an Integer with the index here is faster, but requires a lot of if/then/else to check for
-            // -1
-            //  or QUALIFIERS.size and then resort to lexical ordering. Most comparisons are decided by the first
-            // character,
-            // so this is still fast. If more characters are needed then it requires a lexical sort anyway.
-            return i == -1 ? (QUALIFIERS.size() + "-" + qualifier) : String.valueOf(i);
+            return index == 0 ? ("0-" + qualifier) : String.valueOf(index);
+        }
+
+        /**
+         * Compare the qualifiers of two artifact versions.
+         *
+         * @param qualifier1 qualifier of first artifact
+         * @param qualifier2 qualifier of second artifact
+         * @return a negative integer, zero, or a positive integer as the first argument is less than, equal to, or
+         * greater than the second
+         */
+        public static int compareQualifiers(String qualifier1, String qualifier2) {
+            if (RELEASE_QUALIFIERS.contains(qualifier1)) {
+                qualifier1 = "";
+            }
+            if (RELEASE_QUALIFIERS.contains(qualifier2)) {
+                qualifier2 = "";
+            }
+            int i1 = QUALIFIERS.indexOf(qualifier1);
+            int i2 = QUALIFIERS.indexOf(qualifier2);
+
+            // if both pre-release, then use natural lexical ordering
+            if (i1 == -1 && i2 == -1) {
+                return qualifier1.compareTo(qualifier2);
+            }
+
+            // 'other qualifier' < 'snapshot' < '' < 'sp'
+            return Integer.compare(i1, i2);
         }
 
         @Override
         public int compareTo(Item item) {
             if (item == null) {
                 // 1-rc < 1, 1-ga > 1
-                return comparableQualifier(value).compareTo(RELEASE_VERSION_INDEX);
+                return Integer.compare(QUALIFIERS.indexOf(value), RELEASE_VERSION_INDEX);
             }
             switch (item.getType()) {
                 case INT_ITEM:
@@ -380,7 +406,7 @@ public int compareTo(Item item) {
                     return -1; // 1.any < 1.1 ?
 
                 case STRING_ITEM:
-                    return comparableQualifier(value).compareTo(comparableQualifier(((StringItem) item).value));
+                    return compareQualifiers(value, ((StringItem) item).value);
 
                 case COMBINATION_ITEM:
                     int result = this.compareTo(((CombinationItem) item).getStringPart());
diff --git a/compat/maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java b/compat/maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java
@@ -44,13 +44,16 @@ private ComparableVersion newComparable(String version) {
     }
 
     private static final String[] VERSIONS_QUALIFIER = {
+        "1-abc",
         "1-alpha2snapshot",
         "1-alpha2",
         "1-alpha-123",
         "1-beta-2",
         "1-beta123",
+        "1-def",
         "1-m2",
         "1-m11",
+        "1-pom-1",
         "1-rc",
         "1-cr2",
         "1-rc123",
@@ -59,18 +62,15 @@ private ComparableVersion newComparable(String version) {
         "1-sp",
         "1-sp2",
         "1-sp123",
-        "1-abc",
-        "1-def",
-        "1-pom-1",
         "1-1-snapshot",
         "1-1",
         "1-2",
         "1-123"
     };
 
     private static final String[] VERSIONS_NUMBER = {
-        "2.0", "2.0.a", "2-1", "2.0.2", "2.0.123", "2.1.0", "2.1-a", "2.1b", "2.1-c", "2.1-1", "2.1.0.1", "2.2",
-        "2.123", "11.a2", "11.a11", "11.b2", "11.b11", "11.m2", "11.m11", "11", "11.a", "11b", "11c", "11m"
+        "2.0.a", "2.0", "2-1", "2.0.2", "2.0.123", "2.1-a", "2.1b", "2.1-c", "2.1.0", "2.1-1", "2.1.0.1", "2.2",
+        "2.123", "11.a", "11.a2", "11.a11", "11b", "11.b2", "11.b11", "11c", "11m", "11.m2", "11.m11", "11"
     };
 
     private void checkVersionsOrder(String[] versions) {
@@ -212,7 +212,7 @@ void testVersionComparing() {
 
         checkVersionsOrder("2.0-1", "2.0.1");
         checkVersionsOrder("2.0.1-klm", "2.0.1-lmn");
-        checkVersionsOrder("2.0.1", "2.0.1-xyz");
+        checkVersionsOrder("2.0.1-xyz", "2.0.1"); // now 2.0.1-xyz < 2.0.1 as of MNG-7559
 
         checkVersionsOrder("2.0.1", "2.0.1-123");
         checkVersionsOrder("2.0.1-xyz", "2.0.1-123");
@@ -328,13 +328,9 @@ void testLexicographicOrder() {
      */
     @Test
     void testMng5568() {
-        String a = "6.1.0";
-        String b = "6.1.0rc3";
-        String c = "6.1H.5-beta"; // this is the unusual version string, with 'H' in the middle
-
-        checkVersionsOrder(b, a); // classical
-        checkVersionsOrder(b, c); // now b < c, but before MNG-5568, we had b > c
-        checkVersionsOrder(a, c);
+        checkVersionsOrder("6.1H.5-beta", "6.1.0rc3"); // now H < RC as of MNG-7559
+        checkVersionsOrder("6.1.0rc3", "6.1.0"); // classical
+        checkVersionsOrder("6.1H.5-beta", "6.1.0"); // transitivity
     }
 
     /**
@@ -458,6 +454,23 @@ void testReuse() {
         assertEquals(c1, c2, "reused instance should be equivalent to new instance");
     }
 
+    /**
+     * Test <a href="https://issues.apache.org/jira/browse/MNG-7559">MNG-7559</a> edge cases
+     * -pfd < final, ga, release
+     * 2.0.1.MR < 2.0.1
+     * 9.4.1.jre16 > 9.4.1.jre16-preview
+     */
+    @Test
+    void testMng7559() {
+        // checking general cases
+        checkVersionsOrder(new String[] {"ab", "alpha", "beta", "cd", "ea", "milestone", "mr", "pfd", "preview", "RC"});
+        // checking identified issues respect the general case
+        checkVersionsOrder("2.3-pfd", "2.3");
+        checkVersionsOrder("2.0.1.MR", "2.0.1");
+        checkVersionsOrder("9.4.1.jre16-preview", "9.4.1.jre16");
+        checkVersionsOrder("1-ga-1", "1-sp-1");
+    }
+
     /**
      * Test <a href="https://issues.apache.org/jira/browse/MNG-7644">MNG-7644</a> edge cases
      * 1.0.0.RC1 &lt; 1.0.0-RC2 and more generally:
diff --git a/compat/maven-artifact/src/test/java/org/apache/maven/artifact/versioning/DefaultArtifactVersionTest.java b/compat/maven-artifact/src/test/java/org/apache/maven/artifact/versioning/DefaultArtifactVersionTest.java
@@ -118,7 +118,7 @@ void testVersionComparing() {
         assertVersionOlder("2.0-1", "2.0.1");
 
         assertVersionOlder("2.0.1-klm", "2.0.1-lmn");
-        assertVersionOlder("2.0.1", "2.0.1-xyz");
+        assertVersionOlder("2.0.1-xyz", "2.0.1"); // now 2.0.1-xyz < 2.0.1 as of MNG-7559
         assertVersionOlder("2.0.1-xyz-1", "2.0.1-1-xyz");
 
         assertVersionOlder("2.0.1", "2.0.1-123");
