fix version comparison

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

@@ -41,22 +41,39 @@
  *     <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>a hyphen usually precedes a qualifier, and is always less important than something preceded with a dot.</li>
+ * <li>
+ *   String qualifiers are ordered lexically (case insensitive), with the following exceptions:
+ *   <ul>
+ *     <li> 'snapshot' &lt; '' &lt; 'sp' </li>
+ *   </ul>
+ *   and alias -&gt; replacement (all case insensitive):
+ *   <ul>
+ *     <li> 'a' -&gt; 'alpha' </li>
+ *     <li> 'b' -&gt; 'beta' </li>
+ *     <li> 'm' -&gt; 'milestone' </li>
+ *     <li> 'cr' -&gt; 'rc' </li>
+ *     <li> 'final' -&gt; '' </li>
+ *     <li> 'final' -&gt; '' </li>
+ *     <li> 'final' -&gt; '' </li>
+ *   </ul>
+ * </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>
+ *   For other qualifiers, natural ordering is used (case insensitive):
+ *   <ul>
+ * <li>alpha = a &lt; beta = b &lt; milestone = m &lt; rc = cr &lt; snapshot &lt; '' = final = ga = release &lt; sp</li>
+ *   </ul>
+ * </li>
+ * <li>a hyphen usually precedes a qualifier, and is always less important than digits/number, for example
+ *   1.0.RC2 &lt; 1.0-RC3 &lt; 1.0.1 ; but prefer '1.0.0-RC1' over '1.0.0.RC1' </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>
  */
@@ -132,7 +149,7 @@ public int compareTo( Item item )
             {
                 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;
@@ -218,7 +235,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;
 
@@ -353,23 +370,23 @@ public String toString()
     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();
+
         static
         {
+            ALIASES.put( "cr", "rc" );
             ALIASES.put( "ga", "" );
             ALIASES.put( "final", "" );
             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;
 
@@ -404,7 +421,7 @@ public int getType()
         @Override
         public boolean isNull()
         {
-            return ( comparableQualifier( value ).compareTo( RELEASE_VERSION_INDEX ) == 0 );
+            return QUALIFIERS.indexOf( value ) == RELEASE_VERSION_INDEX;
         }
 
         /**
@@ -419,12 +436,38 @@ 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 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 )
+        {
+            int i1 = QUALIFIERS.indexOf( qualifier1 );
+            int i2 = QUALIFIERS.indexOf( qualifier2 );
+
+            // if both pre-release, then use natural lexical ordering
+            if ( i1 == -1 && i2 == -1 )
+            {
+                // alpha < beta < ea < milestone < preview < rc
+                return qualifier1.compareTo( qualifier2 );
+            }
+
+            // 'other qualifier' < 'snapshot' < '' < 'sp'
+            return Integer.compare( i1, i2 );
         }
 
         @Override
@@ -433,7 +476,7 @@ 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() )
             {
@@ -443,7 +486,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
@@ -676,6 +719,14 @@ else if ( Character.isDigit( c ) )
             {
                 if ( !isDigit && i > startIndex )
                 {
+                    // 1.0.0.RC1 < 1.0.0-RC2
+                    // treat .RC as -RC
+                    if ( !list.isEmpty() )
+                    {
+                        list.add( list = new ListItem() );
+                        stack.push( list );
+                    }
+
                     list.add( new StringItem( version.substring( startIndex, i ), true ) );
                     startIndex = i;
 
@@ -702,6 +753,14 @@ else if ( Character.isDigit( c ) )
 
         if ( version.length() > startIndex )
         {
+            // 1.0.0.RC1 < 1.0.0-RC2
+            // treat .RC as -RC
+            if ( !isDigit && !list.isEmpty() )
+            {
+                list.add( list = new ListItem() );
+                stack.push( list );
+            }
+
             list.add( parseItem( isDigit, version.substring( startIndex ) ) );
         }
 

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

@@ -46,13 +46,13 @@ private Comparable newComparable( String version )
     }
 
     private static final String[] VERSIONS_QUALIFIER =
-        { "1-alpha2snapshot", "1-alpha2", "1-alpha-123", "1-beta-2", "1-beta123", "1-m2", "1-m11", "1-rc", "1-cr2",
-            "1-rc123", "1-SNAPSHOT", "1", "1-sp", "1-sp2", "1-sp123", "1-abc", "1-def", "1-pom-1", "1-1-snapshot",
+        { "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", "1-SNAPSHOT", "1", "1-sp", "1-sp2", "1-sp123", "1-1-snapshot",
             "1-1", "1-2", "1-123" };
 
     private static final String[] VERSIONS_NUMBER =
-        { "2.0", "2-1", "2.0.a", "2.0.0.a", "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 )
     {
@@ -189,7 +189,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" );
@@ -204,13 +204,9 @@ public void testVersionComparing()
      */
     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
     }
 
     /**
@@ -337,4 +333,21 @@ 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
+     * 1.0.0.RC1 < 1.0.0-RC2
+     * -pfd < final, ga, release
+     * 2.0.1.MR < 2.0.1
+     * 9.4.1.jre16 > 9.4.1.jre16-preview
+     */
+    public void testMng7559() {
+        checkVersionsOrder( "1.0.0.RC1", "1.0.0-RC2" );
+        checkVersionsOrder( "4.0.0.Beta3", "4.0.0-RC2" );
+        checkVersionsOrder( "2.3-pfd", "2.3" );
+        checkVersionsOrder( "2.0.1.MR", "2.0.1" );
+        checkVersionsOrder( "9.4.1.jre16-preview", "9.4.1.jre16" );
+        checkVersionsEqual( "2.0.a", "2.0.0.a" ); // previously ordered, now equals
+        checkVersionsOrder( "1-sp-1", "1-ga-1" ); // proving website documentation right.
+    }
 }

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

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