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

Author: sultan

maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java

@@ -23,10 +23,11 @@
 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.Properties;
+import java.util.Map;
 
 /**
  * <p>
@@ -40,25 +41,28 @@
  *     <code>1.0alpha1 =&gt; [1, 0, 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:<ul>
- *     <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>(the empty string)</code> or <code>ga</code> or <code>final</code></li>
- *     <li><code>sp</code></li>
- *     </ul>
- *     Unknown qualifiers are considered after known qualifiers, with lexical order (always case insensitive),
- *   </li>
+ * <li>
+ *   Following semver rules is encouraged, and some qualifiers are discouraged (no matter the case):
+ *   <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), 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),
+ *         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-RC1} over {@code 1.0.0.RC1}, 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}
  *   over {@code 1.0.0.X1}.</li>
  * </ul>
  *
- * @see <a href="https://cwiki.apache.org/confluence/display/MAVENOLD/Versioning">"Versioning" on Maven Wiki</a>
+ * @see <a href="https://maven.apache.org/pom.html#Version_Order_Specification">Version Order Specification</a>
  * @author <a href="mailto:kenney@apache.org">Kenney Westerhof</a>
  * @author <a href="mailto:hboutemy@apache.org">Hervé Boutemy</a>
  */
@@ -122,7 +126,7 @@ public int compareTo(Item item) {
             switch (item.getType()) {
                 case INT_ITEM:
                     int itemValue = ((IntItem) item).value;
-                    return (value < itemValue) ? -1 : ((value == itemValue) ? 0 : 1);
+                    return Integer.compare(value, itemValue);
                 case LONG_ITEM:
                 case BIGINTEGER_ITEM:
                     return -1;
@@ -194,7 +198,7 @@ public int compareTo(Item item) {
                     return 1;
                 case LONG_ITEM:
                     long itemValue = ((LongItem) item).value;
-                    return (value < itemValue) ? -1 : ((value == itemValue) ? 0 : 1);
+                    return Long.compare(value, itemValue);
                 case BIGINTEGER_ITEM:
                     return -1;
 
@@ -307,23 +311,22 @@ 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> QUALIFIERS = Arrays.asList("snapshot", "", "sp");
 
-        private static final Properties ALIASES = new Properties();
+        private static final Map<String, String> ALIASES = new HashMap<>(4);
 
         static {
-            ALIASES.put("ga", "");
+            ALIASES.put("cr", "rc");
             ALIASES.put("final", "");
+            ALIASES.put("ga", "");
             ALIASES.put("release", "");
-            ALIASES.put("cr", "rc");
         }
 
         /**
-         * 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;
 
@@ -343,7 +346,7 @@ private static class StringItem implements Item {
                     default:
                 }
             }
-            this.value = ALIASES.getProperty(value, value);
+            this.value = ALIASES.getOrDefault(value, value);
         }
 
         @Override
@@ -353,7 +356,7 @@ public int getType() {
 
         @Override
         public boolean isNull() {
-            return (comparableQualifier(value).compareTo(RELEASE_VERSION_INDEX) == 0);
+            return QUALIFIERS.indexOf(value) == RELEASE_VERSION_INDEX;
         }
 
         /**
@@ -368,18 +371,41 @@ 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) {
-            int i = QUALIFIERS.indexOf(qualifier);
+            int index = QUALIFIERS.indexOf(qualifier) + 1;
+
+            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) {
+            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);
+            }
 
-            return i == -1 ? (QUALIFIERS.size() + "-" + qualifier) : String.valueOf(i);
+            // '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:
@@ -388,7 +414,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 LIST_ITEM:
                     return -1; // 1.any < 1-1

maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java

@@ -43,13 +43,16 @@ private Comparable 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",
@@ -58,18 +61,15 @@ private Comparable 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) {
@@ -195,7 +195,7 @@ public 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");
@@ -209,13 +209,9 @@ public void testVersionComparing() {
      * <a href="https://netbeans.org/bugzilla/show_bug.cgi?id=226100">226100</a>
      */
     public 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
     }
 
     /**
@@ -333,6 +329,22 @@ public void testReuse() {
         assertEquals("reused instance should be equivalent to new instance", c1, c2);
     }
 
+    /**
+     * 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
+     */
+    public 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-sp-1", "1-ga-1"); // proving website documentation right.
+    }
+
     /**
      * Test <a href="https://issues.apache.org/jira/browse/MNG-7644">MNG-7644</a> edge cases
      * 1.0.0.RC1 < 1.0.0-RC2 and more generally:

maven-artifact/src/test/java/org/apache/maven/artifact/versioning/DefaultArtifactVersionTest.java

@@ -113,7 +113,7 @@ public 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");