Copilot commented on code in PR #15696:
URL: https://github.com/apache/grails-core/pull/15696#discussion_r3322085705
##########
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-level Javadoc says this comparator sorts versions "from newest to
oldest", but the comparator contract/tests show it orders from oldest to newest
(e.g., `versions.sort(false, comparator)` in the spec expects
ascending/oldest-first order). Please align the documentation with the actual
ordering to avoid misuse by future callers.
##########
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))
+ }
Review Comment:
`compareQualifiers` treats unknown qualifiers as `TIER_FINAL` as intended,
but then still compares their trailing digits. That makes an unrecognised
qualifier like `7.0.0-FOO2` sort *newer* than the final `7.0.0`, which
contradicts the documented/previous behavior that unknown qualifiers should be
treated as a final release (i.e., equal to no qualifier). Only milestone/RC
qualifiers should participate in numeric suffix ordering once tiers are equal.
##########
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))
+ }
Review Comment:
`compareQualifiers` treats unknown qualifiers as `TIER_FINAL` as intended,
but then still compares their trailing digits. That makes an unrecognised
qualifier like `7.0.0-FOO2` sort *newer* than the final `7.0.0`, which
contradicts the documented/previous behavior that unknown qualifiers should be
treated as a final release (i.e., equal to no qualifier). Only milestone/RC
qualifiers should participate in numeric suffix ordering once tiers are equal.
##########
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-level Javadoc says this comparator sorts versions "from newest to
oldest", but the comparator contract/tests show it orders from oldest to newest
(e.g., `versions.sort(false, comparator)` in the spec expects
ascending/oldest-first order). Please align the documentation with the actual
ordering to avoid misuse by future callers.
##########
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:
The updated comparator intentionally treats unrecognised qualifiers as a
final release, but there isn't a spec case asserting this behavior (especially
with trailing digits, which previously could be mis-ordered). Adding an
explicit assertion here would prevent regressions like `7.0.0-FOO2` comparing
newer than `7.0.0`.
--
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]
