[MNG-7714] Fixed a scenario in version sorting where sp1 is less than final (#1099)

* Fixed a scenario in version sorting where sp1 is less than final

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

@@ -26,13 +26,14 @@ import java.util.Deque;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Properties;
 
 /**
  * <p>
  * Generic implementation of version comparison.
  * </p>
- *
+ * <p>
  * Features:
  * <ul>
  * <li>mixing of '<code>-</code>' (hyphen) and '<code>.</code>' (dot) separators,</li>
@@ -58,9 +59,9 @@ import java.util.Properties;
  *   over {@code 1.0.0.X1}.</li>
  * </ul>
  *
- * @see <a href="https://maven.apache.org/pom.html#version-order-specification">"Versioning" in the POM reference</a>
  * @author <a href="mailto:kenney@apache.org">Kenney Westerhof</a>
  * @author <a href="mailto:hboutemy@apache.org">Hervé Boutemy</a>
+ * @see <a href="https://maven.apache.org/pom.html#version-order-specification">"Versioning" in the POM reference</a>
  */
 public class ComparableVersion implements Comparable<ComparableVersion> {
     private static final int MAX_INTITEM_LENGTH = 9;
@@ -79,6 +80,7 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
         int BIGINTEGER_ITEM = 0;
         int STRING_ITEM = 1;
         int LIST_ITEM = 2;
+        int COMBINATION_ITEM = 5;
 
         int compareTo(Item item);
 
@@ -128,6 +130,8 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                     return -1;
 
                 case STRING_ITEM:
+                    return 1;
+                case COMBINATION_ITEM:
                     return 1; // 1.1 > 1-sp
 
                 case LIST_ITEM:
@@ -199,6 +203,8 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                     return -1;
 
                 case STRING_ITEM:
+                    return 1;
+                case COMBINATION_ITEM:
                     return 1; // 1.1 > 1-sp
 
                 case LIST_ITEM:
@@ -269,6 +275,8 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                     return value.compareTo(((BigIntegerItem) item).value);
 
                 case STRING_ITEM:
+                    return 1;
+                case COMBINATION_ITEM:
                     return 1; // 1.1 > 1-sp
 
                 case LIST_ITEM:
@@ -309,13 +317,11 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
     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 Properties ALIASES = new Properties();
 
         static {
-            ALIASES.put("ga", "");
-            ALIASES.put("final", "");
-            ALIASES.put("release", "");
             ALIASES.put("cr", "rc");
         }
 
@@ -353,25 +359,31 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
 
         @Override
         public boolean isNull() {
-            return (comparableQualifier(value).compareTo(RELEASE_VERSION_INDEX) == 0);
+            return value == null || value.isEmpty();
         }
 
         /**
          * Returns a comparable value for a qualifier.
-         *
+         * <p>
          * This method takes into account the ordering of known qualifiers then unknown qualifiers with lexical
          * ordering.
-         *
+         * <p>
-         * 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.
          *
          * @param qualifier
          * @return an equivalent value that can be used with lexical comparison
          */
         public static String comparableQualifier(String qualifier) {
+            if (RELEASE_QUALIFIERS.contains(qualifier)) {
+                return String.valueOf(QUALIFIERS.indexOf(""));
+            }
+
             int i = QUALIFIERS.indexOf(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.
             return i == -1 ? (QUALIFIERS.size() + "-" + qualifier) : String.valueOf(i);
         }
 
@@ -390,6 +402,13 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                 case STRING_ITEM:
                     return comparableQualifier(value).compareTo(comparableQualifier(((StringItem) item).value));
 
+                case COMBINATION_ITEM:
+                    int result = this.compareTo(((CombinationItem) item).getStringPart());
+                    if (result == 0) {
+                        return -1;
+                    }
+                    return result;
+
                 case LIST_ITEM:
                     return -1; // 1.any < 1-1
 
@@ -422,6 +441,106 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
         }
     }
 
+    /**
+     * Represents a combination in the version item list.
+     * It is usually a combination of a string and a number, with the string first and the number second.
+     */
+    private static class CombinationItem implements Item {
+
+        StringItem stringPart;
+
+        Item digitPart;
+
+        CombinationItem(String value) {
+            int index = 0;
+            for (int i = 0; i < value.length(); i++) {
+                char c = value.charAt(i);
+                if (Character.isDigit(c)) {
+                    index = i;
+                    break;
+                }
+            }
+
+            stringPart = new StringItem(value.substring(0, index), true);
+            digitPart = parseItem(true, value.substring(index));
+        }
+
+        @Override
+        public int compareTo(Item item) {
+            if (item == null) {
+                // 1-rc1 < 1, 1-ga1 > 1
+                return stringPart.compareTo(item);
+            }
+            int result = 0;
+            switch (item.getType()) {
+                case INT_ITEM:
+                case LONG_ITEM:
+                case BIGINTEGER_ITEM:
+                    return -1;
+
+                case STRING_ITEM:
+                    result = stringPart.compareTo(item);
+                    if (result == 0) {
+                        // X1 > X
+                        return 1;
+                    }
+                    return result;
+
+                case LIST_ITEM:
+                    return -1;
+
+                case COMBINATION_ITEM:
+                    result = stringPart.compareTo(((CombinationItem) item).getStringPart());
+                    if (result == 0) {
+                        return digitPart.compareTo(((CombinationItem) item).getDigitPart());
+                    }
+                    return result;
+                default:
+                    return 0;
+            }
+        }
+
+        public StringItem getStringPart() {
+            return stringPart;
+        }
+
+        public Item getDigitPart() {
+            return digitPart;
+        }
+
+        @Override
+        public int getType() {
+            return COMBINATION_ITEM;
+        }
+
+        @Override
+        public boolean isNull() {
+            return false;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+            CombinationItem that = (CombinationItem) o;
+            return Objects.equals(stringPart, that.stringPart) && Objects.equals(digitPart, that.digitPart);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(stringPart, digitPart);
+        }
+
+        @Override
+        public String toString() {
+            return stringPart.toString() + digitPart.toString();
+        }
+    }
+
     /**
      * Represents a version list item. This class is used both for the global item list and for sub-lists (which start
      * with '-(number)' in the version specification).
@@ -442,10 +561,14 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                 Item lastItem = get(i);
 
                 if (lastItem.isNull()) {
-                    // remove null trailing items: 0, "", empty list
+                    if (i == size() - 1 || get(i + 1).getType() == STRING_ITEM) {
-                    remove(i);
+                        remove(i);
-                } else if (!(lastItem instanceof ListItem)) {
+                    } else if (get(i + 1).getType() == LIST_ITEM) {
-                    break;
+                        Item item = ((ListItem) get(i + 1)).get(0);
+                        if (item.getType() == COMBINATION_ITEM || item.getType() == STRING_ITEM) {
+                            remove(i);
+                        }
+                    }
                 }
             }
         }
@@ -472,6 +595,8 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                     return -1; // 1-1 < 1.0.x
 
                 case STRING_ITEM:
+                    return 1;
+                case COMBINATION_ITEM:
                     return 1; // 1-1 > 1-sp
 
                 case LIST_ITEM:
@@ -549,6 +674,8 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
 
         boolean isDigit = false;
 
+        boolean isCombination = false;
+
         int startIndex = 0;
 
         for (int i = 0; i < version.length(); i++) {
@@ -558,43 +685,51 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                 if (i == startIndex) {
                     list.add(IntItem.ZERO);
                 } else {
-                    list.add(parseItem(isDigit, version.substring(startIndex, i)));
+                    list.add(parseItem(isCombination, isDigit, version.substring(startIndex, i)));
                 }
+                isCombination = false;
                 startIndex = i + 1;
             } else if (c == '-') {
                 if (i == startIndex) {
                     list.add(IntItem.ZERO);
                 } else {
-                    list.add(parseItem(isDigit, version.substring(startIndex, i)));
+                    // X-1 is going to be treated as X1
+                    if (!isDigit && i != version.length() - 1) {
+                        char c1 = version.charAt(i + 1);
+                        if (Character.isDigit(c1)) {
+                            isCombination = true;
+                            continue;
+                        }
+                    }
+                    list.add(parseItem(isCombination, isDigit, version.substring(startIndex, i)));
                 }
                 startIndex = i + 1;
 
-                list.add(list = new ListItem());
+                if (!list.isEmpty()) {
-                stack.push(list);
+                    list.add(list = new ListItem());
+                    stack.push(list);
+                }
+                isCombination = false;
             } else if (Character.isDigit(c)) {
                 if (!isDigit && i > startIndex) {
-                    // 1.0.0.X1 < 1.0.0-X2
+                    // X1
-                    // treat .X as -X for any string qualifier X
+                    isCombination = true;
+
                     if (!list.isEmpty()) {
                         list.add(list = new ListItem());
                         stack.push(list);
                     }
-
-                    list.add(new StringItem(version.substring(startIndex, i), true));
-                    startIndex = i;
-
-                    list.add(list = new ListItem());
-                    stack.push(list);
                 }
 
                 isDigit = true;
             } else {
                 if (isDigit && i > startIndex) {
-                    list.add(parseItem(true, version.substring(startIndex, i)));
+                    list.add(parseItem(isCombination, true, version.substring(startIndex, i)));
                     startIndex = i;
 
                     list.add(list = new ListItem());
                     stack.push(list);
+                    isCombination = false;
                 }
 
                 isDigit = false;
@@ -609,7 +744,7 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
                 stack.push(list);
             }
 
-            list.add(parseItem(isDigit, version.substring(startIndex)));
+            list.add(parseItem(isCombination, isDigit, version.substring(startIndex)));
         }
 
         while (!stack.isEmpty()) {
@@ -619,7 +754,13 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
     }
 
     private static Item parseItem(boolean isDigit, String buf) {
-        if (isDigit) {
+        return parseItem(false, isDigit, buf);
+    }
+
+    private static Item parseItem(boolean isCombination, boolean isDigit, String buf) {
+        if (isCombination) {
+            return new CombinationItem(buf.replace("-", ""));
+        } else if (isDigit) {
             buf = stripLeadingZeroes(buf);
             if (buf.length() <= MAX_INTITEM_LENGTH) {
                 // lower than 2^31
@@ -674,6 +815,7 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
     }
 
     // CHECKSTYLE_OFF: LineLength
+
     /**
      * Main to test version parsing and comparison.
      * <p>
@@ -688,7 +830,7 @@ public class ComparableVersion implements Comparable<ComparableVersion> {
      * </pre>
      *
      * @param args the version strings to parse and compare. You can pass arbitrary number of version strings and always
-     * two adjacent will be compared
+     *             two adjacent will be compared.
      */
     // CHECKSTYLE_ON: LineLength
     public static void main(String... args) {

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

@@ -101,6 +101,13 @@ class ComparableVersionTest {
         assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
     }
 
+    private void checkVersionsHaveSameOrder(String v1, String v2) {
+        ComparableVersion c1 = new ComparableVersion(v1);
+        ComparableVersion c2 = new ComparableVersion(v2);
+        assertEquals(0, c1.compareTo(c2), "expected " + v1 + " == " + v2);
+        assertEquals(0, c2.compareTo(c1), "expected " + v2 + " == " + v1);
+    }
+
     private void checkVersionsArrayEqual(String[] array) {
         // compare against each other (including itself)
         for (int i = 0; i < array.length; ++i)
@@ -145,11 +152,6 @@ class ComparableVersionTest {
         checkVersionsEqual("1x", "1.0.0-x");
         checkVersionsEqual("1.0x", "1-x");
         checkVersionsEqual("1.0.0x", "1-x");
-
-        // aliases
-        checkVersionsEqual("1ga", "1");
-        checkVersionsEqual("1release", "1");
-        checkVersionsEqual("1final", "1");
         checkVersionsEqual("1cr", "1rc");
 
         // special "aliases" a, b and m for alpha, beta and milestone
@@ -162,14 +164,6 @@ class ComparableVersionTest {
         checkVersionsEqual("1A", "1a");
         checkVersionsEqual("1B", "1b");
         checkVersionsEqual("1M", "1m");
-        checkVersionsEqual("1Ga", "1");
-        checkVersionsEqual("1GA", "1");
-        checkVersionsEqual("1RELEASE", "1");
-        checkVersionsEqual("1release", "1");
-        checkVersionsEqual("1RELeaSE", "1");
-        checkVersionsEqual("1Final", "1");
-        checkVersionsEqual("1FinaL", "1");
-        checkVersionsEqual("1FINAL", "1");
         checkVersionsEqual("1Cr", "1Rc");
         checkVersionsEqual("1cR", "1rC");
         checkVersionsEqual("1m3", "1Milestone3");
@@ -177,6 +171,21 @@ class ComparableVersionTest {
         checkVersionsEqual("1m3", "1MILESTONE3");
     }
 
+    @Test
+    void testVersionsHaveSameOrderButAreNotEqual() {
+        checkVersionsHaveSameOrder("1ga", "1");
+        checkVersionsHaveSameOrder("1release", "1");
+        checkVersionsHaveSameOrder("1final", "1");
+        checkVersionsHaveSameOrder("1Ga", "1");
+        checkVersionsHaveSameOrder("1GA", "1");
+        checkVersionsHaveSameOrder("1RELEASE", "1");
+        checkVersionsHaveSameOrder("1release", "1");
+        checkVersionsHaveSameOrder("1RELeaSE", "1");
+        checkVersionsHaveSameOrder("1Final", "1");
+        checkVersionsHaveSameOrder("1FinaL", "1");
+        checkVersionsHaveSameOrder("1FINAL", "1");
+    }
+
     @Test
     void testVersionComparing() {
         checkVersionsOrder("1", "2");
@@ -383,4 +392,15 @@ class ComparableVersionTest {
             checkVersionsEqual("2.0." + x, "2.0.0." + x); // previously ordered, now equals
         }
     }
+
+    @Test
+    public void testMng7714() {
+        ComparableVersion f = new ComparableVersion("1.0.final-redhat");
+        ComparableVersion sp1 = new ComparableVersion("1.0-sp1-redhat");
+        ComparableVersion sp2 = new ComparableVersion("1.0-sp-1-redhat");
+        ComparableVersion sp3 = new ComparableVersion("1.0-sp.1-redhat");
+        assertTrue(f.compareTo(sp1) < 0, "expected " + f + " < " + sp1);
+        assertTrue(f.compareTo(sp1) < 0, "expected " + f + " < " + sp2);
+        assertTrue(f.compareTo(sp1) < 0, "expected " + f + " < " + sp3);
+    }
 }