Copilot commented on code in PR #12771:
URL: https://github.com/apache/maven/pull/12771#discussion_r3815103919
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/version/internal/DefaultPluginVersionResolver.java:
##########
@@ -270,6 +269,61 @@ private void selectVersion(DefaultPluginVersionResult
result, PluginVersionReque
}
}
+ /**
+ * Returns the newest version of {@code candidates} that passes {@link
#isCompatible}, or {@code null}.
+ */
+ private String selectCompatible(PluginVersionRequest request,
TreeSet<Version> candidates, String kind) {
+ if (candidates.isEmpty()) {
+ return null;
+ }
+ logger.info(
+ "Looking for compatible {} version of plugin {}:{}",
+ kind,
+ request.getGroupId(),
+ request.getArtifactId());
+ for (Version v : candidates) {
+ String ver = v.toString();
+ if (isCompatible(request, ver)) {
+ return ver;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * Does the repository offer at least one version that is neither a
snapshot nor a pre-release?
+ */
+ private boolean hasStableVersion(Versions versions) {
+ for (String ver : versions.versions.keySet()) {
+ if (!ver.endsWith("-SNAPSHOT") && !isPreRelease(ver)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Is this a pre-release version such as {@code 4.0.0-beta-1} or {@code
1.0-alpha-2}?
+ * <p>
+ * Decided with the version scheme itself rather than with a list of
qualifier names: a pre-release
+ * qualifier sorts <em>before</em> the version it qualifies ({@code
1.0-beta-1 < 1.0}), while a build
+ * or vendor qualifier does not ({@code 1.0-jre > 1.0}).
+ */
+ private boolean isPreRelease(String version) {
+ int qualifier = version.indexOf('-');
+ if (qualifier <= 0) {
+ return false;
+ }
+ try {
+ return versionScheme
+ .parseVersion(version)
+
.compareTo(versionScheme.parseVersion(version.substring(0, qualifier)))
+ < 0;
+ } catch (InvalidVersionSpecificationException e) {
+ return false;
+ }
+ }
+
private boolean isCompatible(PluginVersionRequest request, String version)
{
Review Comment:
The Javadoc defines ‘pre-release’ as alpha/beta/milestone/rc, but the
implementation classifies any qualifier that sorts before the base version as a
pre-release, which also includes `-SNAPSHOT` under common Maven version schemes
(and the new unit test currently asserts that). To avoid confusion, either (a)
explicitly exclude snapshots in `isPreRelease` and keep snapshot handling
separate, or (b) update the Javadoc (and related naming/messages) to state that
this method detects ‘unstable’ versions that sort before the base version,
including snapshots.
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/version/internal/DefaultPluginVersionResolver.java:
##########
@@ -270,6 +269,61 @@ private void selectVersion(DefaultPluginVersionResult
result, PluginVersionReque
}
}
+ /**
+ * Returns the newest version of {@code candidates} that passes {@link
#isCompatible}, or {@code null}.
+ */
+ private String selectCompatible(PluginVersionRequest request,
TreeSet<Version> candidates, String kind) {
+ if (candidates.isEmpty()) {
+ return null;
+ }
+ logger.info(
+ "Looking for compatible {} version of plugin {}:{}",
+ kind,
+ request.getGroupId(),
+ request.getArtifactId());
+ for (Version v : candidates) {
+ String ver = v.toString();
+ if (isCompatible(request, ver)) {
+ return ver;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * Does the repository offer at least one version that is neither a
snapshot nor a pre-release?
+ */
+ private boolean hasStableVersion(Versions versions) {
+ for (String ver : versions.versions.keySet()) {
+ if (!ver.endsWith("-SNAPSHOT") && !isPreRelease(ver)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
Review Comment:
`hasStableVersion` relies on string checks plus `isPreRelease`, which
returns `false` on unparseable versions; this can cause unparseable entries to
be treated as ‘stable’. Consider aligning this with the later parsing logic
(i.e., only count versions as stable if they are parseable by the configured
`versionScheme` and are neither snapshot nor pre-release). This keeps the
‘stable exists’ decision consistent with what can actually be selected.
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/version/internal/DefaultPluginVersionResolver.java:
##########
@@ -186,6 +186,21 @@ private void selectVersion(DefaultPluginVersionResult
result, PluginVersionReque
version = versions.latestVersion;
repo = versions.latestRepository;
}
+ // A pre-release (alpha/beta/milestone/rc) is a "release" as far as
repository metadata is
+ // concerned, but it is not what a user asking for an unversioned
plugin expects: such
+ // versions are typically built against unstable APIs. Prefer a stable
version whenever the
+ // repository offers one, and fall back to the pre-release only when
it does not.
+ if (version != null && isPreRelease(version) &&
hasStableVersion(versions)) {
+ logger.info(
+ "Metadata of plugin {}:{} points at pre-release version
{}, looking for a stable version",
+ request.getGroupId(),
+ request.getArtifactId(),
+ version);
+ version = null;
+ repo = null;
+ searchPerformed = true;
+ }
Review Comment:
The new selection behavior is covered by the added integration test, but the
core unit test class currently doesn’t exercise the `selectVersion` ordering
(stable → pre-release → snapshot) or the specific branch where `<release>`
points to a pre-release and a stable exists. Adding a focused unit test around
the resolver’s selection order would make regressions easier to catch without
relying on the heavier IT harness.
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/version/internal/DefaultPluginVersionResolver.java:
##########
@@ -214,34 +232,15 @@ private void selectVersion(DefaultPluginVersionResult
result, PluginVersionReque
}
}
- if (!releases.isEmpty()) {
- logger.info(
- "Looking for compatible RELEASE version of plugin
{}:{}",
- request.getGroupId(),
- request.getArtifactId());
- for (Version v : releases) {
- String ver = v.toString();
- if (isCompatible(request, ver)) {
- version = ver;
- repo = versions.versions.get(version);
- break;
- }
- }
+ version = selectCompatible(request, releases, "RELEASE");
+ if (version == null) {
+ version = selectCompatible(request, preReleases,
"pre-release");
}
-
- if (version == null && !snapshots.isEmpty()) {
- logger.info(
- "Looking for compatible SNAPSHOT version of plugin
{}:{}",
- request.getGroupId(),
- request.getArtifactId());
- for (Version v : snapshots) {
- String ver = v.toString();
- if (isCompatible(request, ver)) {
- version = ver;
- repo = versions.versions.get(version);
- break;
- }
- }
+ if (version == null) {
+ version = selectCompatible(request, snapshots, "SNAPSHOT");
+ }
Review Comment:
The `kind` values used for logging are inconsistent in casing/format
(`RELEASE`/`SNAPSHOT` vs `pre-release`). Standardizing these strings (e.g., all
uppercase or all lowercase, or aligning with existing Maven log terminology)
will keep log output consistent and easier to scan/grep.
--
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]
