[
https://issues.apache.org/jira/browse/MNG-7714?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17720573#comment-17720573
]
ASF GitHub Bot commented on MNG-7714:
-------------------------------------
elharo commented on code in PR #1099:
URL: https://github.com/apache/maven/pull/1099#discussion_r1187611327
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -199,6 +202,7 @@ public int compareTo(Item item) {
return -1;
case STRING_ITEM:
+ case COMBINATION_ITEM:
Review Comment:
include a return for every case, as relying on fallthrough is error-prone
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -422,6 +436,106 @@ public String toString() {
}
}
+ /**
+ * 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 stringValue;
+
+ Item digitValue;
+
+ CombinationItem(String v) {
+ int index = 0;
+ for (int i = 0; i < v.length(); i++) {
+ char c = v.charAt(i);
+ if (Character.isDigit(c)) {
Review Comment:
I don't think we want to include non-ASCII digits here.
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -422,6 +436,106 @@ public String toString() {
}
}
+ /**
+ * 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 stringValue;
+
+ Item digitValue;
+
+ CombinationItem(String v) {
Review Comment:
v --> value
##########
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -619,7 +748,13 @@ public final void parseVersion(String version) {
}
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) {
Review Comment:
What is "buf"? It's not a StringBuffer. Is there a more descriptive name for
this variable?
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -162,14 +169,14 @@ void testVersionsEqual() {
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");
+ checkVersionsEqualOrder("1Ga", "1");
Review Comment:
These really belong in a new test method. There's too much in this test
method already.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -383,4 +390,15 @@ void testMng7644() {
checkVersionsEqual("2.0." + x, "2.0.0." + x); // previously
ordered, now equals
}
}
+
+ @Test
+ public void testMng7714() {
+ String f = "1.0.final-redhat";
+ String sp1 = "1.0-sp1-redhat";
+ String sp2 = "1.0-sp-1-redhat";
+ String sp3 = "1.0-sp.1-redhat";
+ checkVersionsOrder(f, sp1);
Review Comment:
This is unclear since there's nothing in the method call to say which is
expected to be greater. If these can be tested directly, it would be easier to
follow. That is, show the comparisons between f and sp1, etc.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -147,10 +154,10 @@ void testVersionsEqual() {
checkVersionsEqual("1.0.0x", "1-x");
// aliases
- checkVersionsEqual("1ga", "1");
- checkVersionsEqual("1release", "1");
- checkVersionsEqual("1final", "1");
- checkVersionsEqual("1cr", "1rc");
+ checkVersionsEqualOrder("1ga", "1");
Review Comment:
I'd be more comfortable if existing tests didn't change
> sp < final
> ----------
>
> Key: MNG-7714
> URL: https://issues.apache.org/jira/browse/MNG-7714
> Project: Maven
> Issue Type: Bug
> Reporter: Elliotte Rusty Harold
> Assignee: Elliotte Rusty Harold
> Priority: Major
> Fix For: 4.0.0-alpha-6
>
>
> Ported from a comment on https://issues.apache.org/jira/browse/MNG-7701
> The claim is that sp < final, which if true is incorrect according to spec.
> It is easy to demonstrate that this is not fixed and also not in line with
> the spec, with just this one important example (yes this does break for us):
> $ jbang org.apache.maven:maven-artifact:3.8.6 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 < 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1.0.sp-1-redhat-1; tokens: [1, 0, sp, [1, [redhat,
> [1]]]]
> versus
> $ jbang org.apache.maven:maven-artifact:3.8.7 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 > 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1-sp-1-redhat-1; tokens: [1, [sp, [1, [redhat,
> [1]]]]]
> As you can see, our `sp` release is now ordered after our `final` release
> despite this clear text in the "spec":
> Non-numeric tokens ("qualifiers") have the alphabetical order, except for
> the following tokens which come first in this order: "alpha" < "beta" <
> "milestone" < "rc" = "cr" < "snapshot" < "" = "final" = "ga" < "sp"
> It's clear that this tokenization isn't really correct by any reasonable
> measurement, and breaking large amounts of (our) existing artifacts in the
> wild is definitely not OK.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
