[
https://issues.apache.org/jira/browse/MNG-7559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17633355#comment-17633355
]
ASF GitHub Bot commented on MNG-7559:
-------------------------------------
sultan commented on code in PR #845:
URL: https://github.com/apache/maven/pull/845#discussion_r1020917747
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -350,26 +371,27 @@ public String toString()
/**
* Represents a string in the version item list, usually a qualifier.
*/
- private static class StringItem
+ public static class StringItem
implements Item
{
- private static final List<String> QUALIFIERS =
- Arrays.asList( "alpha", "beta", "milestone", "rc", "snapshot",
"", "sp" );
+ // 'pre-release' < 'snapshot' < 'release'
+ private static final List<String> QUALIFIERS = Arrays.asList(
"snapshot", "", "sp" );
Review Comment:
the heart of the fix relies here, so better have this done right!
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -91,7 +112,7 @@
/**
* Represents a numeric item in the version item list that can be
represented with an int.
*/
- private static class IntItem
+ public static class IntItem
Review Comment:
will be treated in another PR
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <code>1.0alpha1 => [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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
Review Comment:
done on both sides (code/doc)
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <code>1.0alpha1 => [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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
+ * </ul>
+ * For other qualifiers, natural ordering is used:
+ * <ul>
+ * <li>alpha = a < beta = b < ea < milestone = m < preview < rc = cr <
snapshot < '' = final = ga = release < sp</li>
+ * </ul>
+ * </li>
+ * <li>a hyphen usually precedes a qualifier, and is always less important
than digits/number, for example
+ * 1.0.RC2 < 1.0-RC3 < 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>
Review Comment:
changed to refer to the website doc
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -73,7 +89,12 @@
private ListItem items;
- private interface Item
+ public ListItem getItems()
Review Comment:
will be treated in another PR
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -41,19 +42,34 @@
* <code>1.0alpha1 => [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, with the following exceptions:
+ * <ul>
+ * <li> 'snapshot' < '' < 'sp' </li>
+ * </ul>
+ * and alias => replacement:
+ * <ul>
+ * <li> 'a' => 'alpha' </li>
+ * <li> 'b' => 'beta' </li>
+ * <li> 'm' => 'milestone' </li>
+ * <li> 'cr' => 'rc' </li>
+ * <li> 'final' = 'ga' = 'release' => '' </li>
+ * </ul>
+ * </li>
+ * <li>
+ * Following semver rules is encouraged, and some qualifiers are discouraged:
+ * <ul>
+ * <li> The usage of 'CR' qualifier is discouraged. Use 'RC' instead. </li>
+ * <li> The usage of 'final', 'ga', 'release' qualifiers is discouraged.
Use no qualifier instead. </li>
+ * <li> The usage of 'SP' qualifier is discouraged. Use version increment
instead. </li>
+ * </ul>
+ * For other qualifiers, natural ordering is used:
+ * <ul>
+ * <li>alpha = a < beta = b < ea < milestone = m < preview < rc = cr <
snapshot < '' = final = ga = release < sp</li>
Review Comment:
this is just a mere example of how the alphabetical ordering works. since
this seems to be a problem, i will remove them from the doc
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -91,7 +112,7 @@
/**
* Represents a numeric item in the version item list that can be
represented with an int.
*/
- private static class IntItem
+ public static class IntItem
Review Comment:
i will make a separate PR for this particular problem then. but the problem
still needs to be addressed ...
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -404,27 +426,31 @@ public int getType()
@Override
public boolean isNull()
{
- return ( comparableQualifier( value ).compareTo(
RELEASE_VERSION_INDEX ) == 0 );
+ return QUALIFIERS.indexOf( value ) == RELEASE_VERSION_INDEX;
}
/**
- * Returns a comparable value for a qualifier.
- *
- * This method takes into account the ordering of known qualifiers
then unknown qualifiers with lexical
- * ordering.
+ * Compare the qualifiers of two artifact versions.
*
- * 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
+ * @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 String comparableQualifier( String qualifier )
+ public static int compareQualifiers( String qualifier1, String
qualifier2 )
{
- int i = QUALIFIERS.indexOf( qualifier );
+ 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 );
+ }
- return i == -1 ? ( QUALIFIERS.size() + "-" + qualifier ) :
String.valueOf( i );
+ // 'other qualifier' < 'snapshot' < '' < 'sp'
+ return Integer.compare( i1, i2 );
}
Review Comment:
the heart of the fix relies here, so better have this done right!
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -404,27 +426,31 @@ public int getType()
@Override
public boolean isNull()
{
- return ( comparableQualifier( value ).compareTo(
RELEASE_VERSION_INDEX ) == 0 );
+ return QUALIFIERS.indexOf( value ) == RELEASE_VERSION_INDEX;
}
/**
- * Returns a comparable value for a qualifier.
- *
- * This method takes into account the ordering of known qualifiers
then unknown qualifiers with lexical
- * ordering.
+ * Compare the qualifiers of two artifact versions.
*
- * 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
+ * @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 String comparableQualifier( String qualifier )
+ public static int compareQualifiers( String qualifier1, String
qualifier2 )
{
- int i = QUALIFIERS.indexOf( qualifier );
+ 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 );
+ }
- return i == -1 ? ( QUALIFIERS.size() + "-" + qualifier ) :
String.valueOf( i );
+ // 'other qualifier' < 'snapshot' < '' < 'sp'
+ return Integer.compare( i1, i2 );
}
Review Comment:
any qualifier other than 'SP' are less than 'release' or ''
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -267,7 +288,7 @@ public String toString()
/**
* Represents a numeric item in the version item list.
*/
- private static class BigIntegerItem
+ public static class BigIntegerItem
Review Comment:
will be treated in another PR
> ComparableVersion vs versions with custom qualifiers
> ----------------------------------------------------
>
> Key: MNG-7559
> URL: https://issues.apache.org/jira/browse/MNG-7559
> Project: Maven
> Issue Type: Bug
> Affects Versions: 3.8.3
> Reporter: Andrzej Jarmoniuk
> Priority: Major
> Attachments: image-2022-10-22-18-22-11-591.png
>
>
> Since I know that ComparableVersion was brought to Maven from
> versions-maven-plugin, it turns out the bug described here:
> https://github.com/mojohaus/versions-maven-plugin/issues/744
> also exists in maven, at least in 3.8.3.
> According to the maven version spec, versions containing a qualifier should
> be treated as less major than the same versions without the qualifier.
> Currently it's only the case for a few "standard" qualifiers, e.g. "-rc*",
> "-alpha", etc.
> However, it looks like "2.3-pfd" is deemed less major than "2.3".
> {code:java}
> @Test
> public void testComparableVersionWithCustomQualifier()
> {
> assertThat( new ComparableVersion( "2.3" ).compareTo( new
> ComparableVersion( "2.3-pfd" ) ),
> greaterThan( 0 ) );
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
