elharo commented on code in PR #12947:
URL: https://github.com/apache/maven/pull/12947#discussion_r3893987547


##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -68,6 +68,16 @@ public class ComparableVersion implements 
Comparable<ComparableVersion> {
 
     private static final int MAX_LONGITEM_LENGTH = 18;
 
+    /**
+     * Maximum accepted length of a version string. Version strings routinely 
come from external
+     * repository metadata; without a bound, every {@code -} separator nests 
another list whose

Review Comment:
    metadata; without -->  metadata. Without



##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -808,6 +835,53 @@ public int hashCode() {
         return items.hashCode();
     }
 
+    /**
+     * Returns a hash code consistent with the ordering defined by {@link 
#compareTo(ComparableVersion)}:
+     * two versions that compare as equal get the same value even when their 
parsed representations
+     * differ, e.g. {@code 1-ga} ({@code [1, [ga]]}) and {@code 1} ({@code 
[1]}).
+     * <p>
+     * {@link #hashCode()} cannot provide this: it is structural, matching the 
structural
+     * {@link #equals(Object)} of this class. This method exists for classes 
such as
+     * {@link DefaultArtifactVersion} whose {@code equals} is defined as 
{@code compareTo == 0} and
+     * whose {@code hashCode} must therefore follow ordering equality (two 
equal objects must have
+     * equal hash codes).
+     *
+     * @return a hash code such that {@code a.compareTo(b) == 0} implies 
{@code a.orderingHashCode() == b.orderingHashCode()}
+     */
+    int orderingHashCode() {
+        return orderingHash(items);
+    }
+
+    private static int orderingHash(Item item) {
+        return switch (item.getType()) {
+            case Item.LIST_ITEM -> {
+                ListItem list = (ListItem) item;
+                int end = list.size();
+                // trailing items that compare as equal to null do not affect 
ordering: 1-ga == 1
+                while (end > 0 && list.get(end - 1).compareTo(null) == 0) {
+                    end--;
+                }
+                int hash = 1;
+                for (int i = 0; i < end; i++) {
+                    hash = 31 * hash + orderingHash(list.get(i));
+                }
+                yield hash;
+            }
+            // qualifiers that compare as equal ("ga", "final", "release" and 
the empty qualifier) must hash alike

Review Comment:
   good catch



##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -450,7 +460,14 @@ private static class CombinationItem implements Item {
         public int compareTo(Item item) {
             if (item == null) {
                 // 1-rc1 < 1, 1-ga1 > 1
-                return stringPart.compareTo(item);
+                int result = stringPart.compareTo(item);
+                if (result == 0) {
+                    // the string part is equivalent to the release qualifier 
("ga", "final", "release"),

Review Comment:
   have we tested 1-ga2 vs 1-ga11?



##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -68,6 +68,16 @@ public class ComparableVersion implements 
Comparable<ComparableVersion> {
 
     private static final int MAX_LONGITEM_LENGTH = 18;
 
+    /**
+     * Maximum accepted length of a version string. Version strings routinely 
come from external
+     * repository metadata; without a bound, every {@code -} separator nests 
another list whose
+     * comparison, equality, hash code and canonicalization recurse one frame 
per level, and digit
+     * runs longer than {@value #MAX_LONGITEM_LENGTH} characters are parsed 
into {@link BigInteger}
+     * at quadratic cost. 256 characters is far beyond any real-world version 
identifier while
+     * keeping the nesting depth (at most about half the length) and numeric 
items small.
+     */
+    private static final int MAX_VERSION_LENGTH = 256;

Review Comment:
   I don't know. I'd probably take a 50-50 bet that there are longer version 
strings out there. Would it be feasible to go to 1024? or higher? At what 
length do we actually have problems?



##########
compat/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java:
##########
@@ -68,6 +68,16 @@ public class ComparableVersion implements 
Comparable<ComparableVersion> {
 
     private static final int MAX_LONGITEM_LENGTH = 18;
 
+    /**
+     * Maximum accepted length of a version string. Version strings routinely 
come from external
+     * repository metadata; without a bound, every {@code -} separator nests 
another list whose
+     * comparison, equality, hash code and canonicalization recurse one frame 
per level, and digit

Review Comment:
   hash code, and



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to