Copilot commented on code in PR #15693:
URL: https://github.com/apache/grails-core/pull/15693#discussion_r3322063987
##########
grails-bootstrap/src/main/groovy/grails/plugins/VersionComparator.groovy:
##########
@@ -112,4 +99,97 @@ class VersionComparator implements Comparator<String> {
protected boolean isSnapshot(String version) {
SNAPSHOT_SUFFIXES.any { String it -> version?.endsWith(it) }
}
+
+ /**
+ * Splits a version into its leading numeric components and an optional
trailing qualifier.
+ * The first token that is not purely numeric ends the numeric section. A
token of the form
+ * {@code <digits>-<qualifier>} (for example {@code 0-RC1} from {@code
7.0.0-RC1}) contributes
+ * its leading digits to the numeric section and the remainder becomes the
qualifier.
+ */
+ private static ParsedVersion parse(String version) {
+ List<Integer> numbers = []
+ String qualifier = null
+ if (version) {
+ for (String token : version.split(/\./)) {
+ if (DIGITS.matcher(token).matches()) {
+ numbers.add(Integer.parseInt(token))
+ continue
+ }
+ Matcher matcher = NUMERIC_PREFIX.matcher(token)
+ if (matcher.matches()) {
+ numbers.add(Integer.parseInt(matcher.group(1)))
+ qualifier = normalizeQualifier(matcher.group(2))
+ } else {
+ qualifier = normalizeQualifier(token)
+ }
+ break
+ }
+ }
+ return new ParsedVersion(numbers, qualifier)
+ }
+
+ private static int compareNumbers(List<Integer> a, List<Integer> b) {
+ int max = Math.max(a.size(), b.size())
+ for (int i = 0; i < max; i++) {
+ int left = i < a.size() ? a.get(i) : 0
+ int right = i < b.size() ? b.get(i) : 0
+ int result = Integer.compare(left, right)
+ if (result != 0) {
+ return result
+ }
+ }
+ return 0
+ }
+
+ private static int compareQualifiers(String q1, String q2) {
+ int tier = Integer.compare(qualifierTier(q1), qualifierTier(q2))
+ if (tier != 0) {
+ return tier
+ }
+ return Integer.compare(qualifierNumber(q1), qualifierNumber(q2))
+ }
+
+ private static String normalizeQualifier(String qualifier) {
+ if (qualifier == null) {
+ return null
+ }
+ String upper = qualifier.toUpperCase()
+ return upper.contains(SNAPSHOT) ? SNAPSHOT : upper
+ }
+
+ private static int qualifierTier(String qualifier) {
+ if (qualifier == null) {
+ return TIER_FINAL
+ }
+ if (qualifier.contains(SNAPSHOT)) {
+ return TIER_SNAPSHOT
+ }
+ if (qualifier.startsWith(RELEASE_CANDIDATE)) {
+ return TIER_RELEASE_CANDIDATE
+ }
+ if (qualifier.startsWith(MILESTONE)) {
+ return TIER_MILESTONE
+ }
+ return TIER_FINAL
+ }
+
+ private static int qualifierNumber(String qualifier) {
+ if (qualifier == null) {
+ return 0
+ }
+ Matcher matcher = TRAILING_DIGITS.matcher(qualifier)
+ return matcher.find() ? Integer.parseInt(matcher.group(1)) : 0
+ }
Review Comment:
qualifierNumber() extracts trailing digits for *any* qualifier. That means
unrecognised qualifiers like "7.0.0-FOO2" will compare as newer than the final
release ("7.0.0"), which contradicts the class Javadoc stating unrecognised
qualifiers are treated as a final release and breaks the previous "ignore
unknown suffix" behavior. Restrict numeric comparison to milestone/RC
qualifiers only; treat everything else as 0.
##########
grails-bootstrap/src/main/groovy/grails/plugins/VersionComparator.groovy:
##########
@@ -18,79 +18,66 @@
*/
package grails.plugins
+import java.util.regex.Matcher
+import java.util.regex.Pattern
+
import groovy.transform.CompileStatic
/**
- * A comparator capable of sorting versions from from newest to oldest
+ * A comparator capable of sorting versions from newest to oldest.
+ *
Review Comment:
The class Javadoc says this comparator sorts versions "from newest to
oldest", but the comparison semantics (and the spec below) define natural
ordering where older < newer (e.g., 3.1.0 < 4.0.1). This is misleading for
callers using it with List#sort().
##########
grails-core/src/test/groovy/grails/plugins/VersionComparatorSpec.groovy:
##########
@@ -44,5 +44,48 @@ class VersionComparatorSpec extends Specification {
"3.0.0" | "3.0.0" || 0
"4.0.1" | "3.1.110" || 1
"4.0.1" | "3.0.0.BUILD-SNAPSHOT" || 1
+
+ // A pre-release (milestone/rc/snapshot) is older than the final
release of the same number
+ "7.0.0-M1" | "7.0.0" || -1
+ "7.0.0" | "7.0.0-M1" || 1
+ "7.0.0-RC1" | "7.0.0" || -1
+ "7.0.0-SNAPSHOT" | "7.0.0" || -1
+ "7.0.0" | "7.0.0-SNAPSHOT" || 1
+
+ // The numeric version is compared before the qualifier, so the patch
number is never lost
+ "7.0.5-M1" | "7.0.0" || 1
+ "7.0.0" | "7.0.5-M1" || -1
+ "7.0.1-M1" | "7.0.0" || 1
+ "7.0.0-RC1" | "6.9.9" || 1
+ "6.9.9" | "7.0.0-RC1" || -1
+
+ // Milestones and release candidates are ordered by their number,
numerically not lexically
+ "7.0.0-M1" | "7.0.0-M2" || -1
+ "7.0.0-M2" | "7.0.0-M1" || 1
+ "7.0.0-RC1" | "7.0.0-RC2" || -1
+ "7.0.0-RC10" | "7.0.0-RC2" || 1
+
+ // Qualifier tiers: milestone < release candidate < snapshot < final
+ "7.0.0-M2" | "7.0.0-RC1" || -1
+ "7.0.0-RC1" | "7.0.0-SNAPSHOT" || -1
+ "7.0.0-M9" | "7.0.0-SNAPSHOT" || -1
+
+ // The dotted and hyphenated qualifier forms are equivalent, and
matching is case insensitive
+ "7.0.0.M1" | "7.0.0-M1" || 0
+ "7.0.0.RC1" | "7.0.0-RC1" || 0
+ "7.0.0.BUILD-SNAPSHOT" | "7.0.0-SNAPSHOT" || 0
+ "7.0.0-rc3" | "7.0.0-RC3" || 0
Review Comment:
Since VersionComparator's Javadoc/intent is to treat unrecognised qualifiers
as a final release, add an assertion that an unknown qualifier (even with
trailing digits) compares equal to the base version. This guards against
regressions like treating "FOO2" as newer than the final release.
##########
grails-bootstrap/src/main/groovy/grails/plugins/VersionComparator.groovy:
##########
@@ -18,79 +18,66 @@
*/
package grails.plugins
+import java.util.regex.Matcher
+import java.util.regex.Pattern
+
import groovy.transform.CompileStatic
/**
- * A comparator capable of sorting versions from from newest to oldest
+ * A comparator capable of sorting versions from newest to oldest.
+ *
+ * <p>Versions are compared by their numeric components first (major, minor,
patch, ...),
+ * padding the shorter side with zeros so that {@code 7.0} and {@code 7.0.0}
are equal.
+ * When the numeric components are equal the version qualifier is used as a
tie-breaker
+ * following the same ordering as {@code
org.grails.datastore.mapping.core.grailsversion.GrailsVersion}:</p>
+ *
+ * <pre>
+ * 7.0.0-M1 < 7.0.0-M2 < 7.0.0-RC1 < 7.0.0-RC2 < 7.0.0-SNAPSHOT
< 7.0.0
+ * </pre>
+ *
+ * <p>In other words a milestone is older than a release candidate, a release
candidate is
+ * older than a snapshot, and any qualified (pre-release) version is older
than the final
+ * release of the same number. Both the dotted ({@code 7.0.0.M1}) and
hyphenated
+ * ({@code 7.0.0-M1}) qualifier forms are treated as equivalent. Unrecognised
qualifiers are
+ * treated as a final release to preserve backwards compatible behaviour.</p>
*/
@CompileStatic
class VersionComparator implements Comparator<String> {
+ private static final String SNAPSHOT = 'SNAPSHOT'
+ private static final String RELEASE_CANDIDATE = 'RC'
+ private static final String MILESTONE = 'M'
+
static private final List<String> SNAPSHOT_SUFFIXES = ['-SNAPSHOT',
'.BUILD-SNAPSHOT'].asImmutable()
+ private static final Pattern DIGITS = ~/\d+/
+ private static final Pattern NUMERIC_PREFIX = ~/(\d+)-(.+)/
+ private static final Pattern TRAILING_DIGITS = ~/(\d+)$/
+
+ private static final int TIER_FINAL = 4
+ private static final int TIER_SNAPSHOT = 3
+ private static final int TIER_RELEASE_CANDIDATE = 2
+ private static final int TIER_MILESTONE = 1
+
int compare(String o1, String o2) {
- int result = 0
- if (o1 == '*') {
- result = 1
- }
- else if (o2 == '*') {
- result = -1
+ String left = o1?.trim()
+ String right = o2?.trim()
+
+ if (left == '*') {
+ return 1
}
- else {
- def nums1
- try {
- def tokens = deSnapshot(o1).split(/\./)
- tokens = tokens.findAll { String it -> it.trim() ==~ /\d+/ }
- nums1 = tokens*.toInteger()
- }
- catch (NumberFormatException e) {
- throw new InvalidVersionException("Cannot compare versions,
left side [$o1] is invalid: ${e.message}")
- }
- def nums2
- try {
- def tokens = deSnapshot(o2).split(/\./)
- tokens = tokens.findAll { String it -> it.trim() ==~ /\d+/ }
- nums2 = tokens*.toInteger()
- }
- catch (NumberFormatException e) {
- throw new InvalidVersionException("Cannot compare versions,
right side [$o2] is invalid: ${e.message}")
- }
- boolean bigRight = nums2.size() > nums1.size()
- boolean bigLeft = nums1.size() > nums2.size()
- for (int i in 0..<nums1.size()) {
- if (nums2.size() > i) {
- result = nums1[i].compareTo(nums2[i])
- if (result != 0) {
- break
- }
- if (i == (nums1.size() - 1) && bigRight) {
- if (nums2[i + 1] != 0)
- result = -1
- break
- }
- }
- else if (bigLeft) {
- if (nums1[i] != 0)
- result = 1
- break
- }
- }
+ if (right == '*') {
+ return -1
}
+ ParsedVersion v1 = parse(left)
+ ParsedVersion v2 = parse(right)
+
Review Comment:
compare() no longer wraps numeric parsing failures in
InvalidVersionException as it did previously, so oversized numeric components
(or other parse failures) will now leak NumberFormatException and change the
public failure mode/message. To preserve backwards-compatible error behavior,
catch NumberFormatException around parse() and rethrow InvalidVersionException
with left/right context.
--
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]
