gnodet-bot commented on code in PR #28581:
URL: https://github.com/apache/camel/pull/28581#discussion_r4398570525
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/plugin/PluginAdd.java:
##########
@@ -164,4 +191,162 @@ public Integer doCall() throws Exception {
return 0;
}
+ /**
+ * Selects the newest release of a community plugin built for this Camel
CLI, and downloads it, so a release that
+ * cannot be used never gets into the configuration. Sets {@link #version}
to the selected release, or leaves it
+ * null to use LATEST when no release can be selected.
+ *
+ * @return false when the selected release cannot be downloaded
+ */
+ private boolean selectVersion(JsonObject plugin) throws Exception {
+ String cli = cliVersion();
+ Result result;
+ DependencyDownloader downloader = newDownloader();
+ try {
+ result = new PluginVersionResolver(downloader).resolve(groupId,
artifactId, cli);
+ } finally {
+ downloader.stop();
+ downloader.close();
+ }
+
+ if (result instanceof Selected selected) {
+ if (!useRelease(plugin, selected.version(), selected.compat())) {
+ return false;
+ }
+ printer().printf("Selected %s %s for Camel CLI %s (%s)%n", name,
version, cli, selected.compat().describe());
+ } else if (result instanceof Older older) {
+ if (!useRelease(plugin, older.version(), older.compat())) {
+ return false;
+ }
+ printer().printWarn(
+ "No release of %s is built for Camel CLI %s. Selected %s
%s, the newest release for an older Camel version (%s)."
+ .formatted(name, cli, name, version,
older.compat().describe()));
+ } else if (result instanceof NoMatch noMatch) {
+ StringBuilder sb = new StringBuilder();
+ switch (noMatch.reason()) {
+ case CLI_NEWER -> sb.append("No release of %s is built for
Camel CLI %s yet. The newest, %s %s, is %s."
+ .formatted(name, cli, name, noMatch.newestVersion(),
noMatch.newest().describe()));
+ case CLI_OLDER -> sb.append("No release of %s is built for
Camel CLI %s or an older Camel version."
+ .formatted(name, cli));
+ default -> sb.append("No release of %s tells which Camel
versions it supports.".formatted(name));
+ }
+ sb.append(" Using LATEST.");
+ if (!noMatch.newestPerLine().isEmpty()) {
+ sb.append(System.lineSeparator()).append("Newest release per
Camel version:");
+ noMatch.newestPerLine().forEach((line, v) ->
sb.append(System.lineSeparator())
+ .append(" Camel %s: %s %s".formatted(line, name, v)));
+ sb.append(System.lineSeparator())
+ .append("To use one of them: camel plugin add %s
--version <version>".formatted(name));
+ }
+ printer().printWarn(sb.toString());
+ } else if (result instanceof Unavailable unavailable) {
+ printer().printWarn("Could not check plugin compatibility (%s).
Using LATEST.".formatted(unavailable.reason()));
+ }
+ return true;
+ }
+
+ /**
+ * Downloads the release, and stores the Camel versions it supports in the
plugin entry.
+ *
+ * @return false when the release cannot be downloaded
+ */
+ private boolean useRelease(JsonObject plugin, String release, Compat
compat) {
+ version = release;
+ String releaseGav = "%s:%s:%s".formatted(groupId, artifactId, version);
+ Optional<Plugin> resolved;
+ try {
+ resolved = downloadPlugin(releaseGav);
+ } catch (Exception e) {
+ printer().printErr("Cannot download " + releaseGav, e);
+ return false;
+ }
+ if (resolved.isEmpty()) {
+ printer().printErr("Cannot download " + releaseGav);
+ return false;
+ }
+ plugin.put("camelVersions", compat.range());
+ plugin.put("camelVersionsSource", compat.source().label());
Review Comment:
⚠️ **`Older` path stores a range that excludes the CLI — creates an infinite
warning loop.**
When `Older` is selected, `compat.range()` = e.g. `[4.18.0,4.19.0)` is
stored as `camelVersions`. On every subsequent command,
`PluginHelper.compatibilityCheck()` evaluates
`PluginCompatibility.accepts("[4.18.0,4.19.0)", "4.21.0")` → `false`, and warns:
> `forage 1.4.2 is built for Camel [4.18.0,4.19.0) but this CLI is 4.21.0.
Run 'camel plugin add forage' to select a matching release.`
Running `camel plugin add forage` again selects the same `Older` release (no
better one exists for CLI 4.21), stores the same range, and the warning fires
again on the next command. The test at
`PluginAddTest.shouldSelectNearestOlderReleaseWhenBetweenCamelVersions()`
asserts `camelVersions = "[4.18.0,4.19.0)"` for this case, locking in the
looping behaviour.
**Fix (two options — as @davsclaus suggested):**
1. For `Older` results, skip `plugin.put("camelVersions", ...)` (or set it
to `null`), so `compatibilityCheck` silently passes. The user already saw the
warning during `plugin add`.
2. Store a different marker (e.g. a dedicated `"camelVersionsOlderFallback"`
flag) and teach `compatibilityCheck` to emit a one-time message that does _not_
say "run `camel plugin add` again" — since doing so changes nothing.
Either way, please add an assertion to the test that
`plugin.get("camelVersions")` is `null` (option 1) or that no warning fires on
the second load (option 2).
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PluginVersionResolver.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dsl.jbang.core.common;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.regex.Pattern;
+
+import org.apache.camel.dsl.jbang.core.common.PluginCompatibility.Compat;
+import org.apache.camel.dsl.jbang.core.common.PluginCompatibility.Source;
+import org.apache.camel.main.download.DependencyDownloader;
+import org.apache.camel.tooling.maven.MavenArtifactDescriptor;
+import org.eclipse.aether.util.version.GenericVersionScheme;
+import org.eclipse.aether.version.InvalidVersionSpecificationException;
+import org.eclipse.aether.version.Version;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Selects the newest release of a plugin that supports the version of the
Camel CLI, from the resolved POMs of its
+ * releases (see {@link PluginCompatibility}).
+ * <p>
+ * The releases are grouped by release line of the plugin (its major and minor
version, such as {@code 1.6} for
+ * {@code 1.6.0}, {@code 1.6.1} and {@code 1.6.2}), and only the POM of the
newest release of each line is read, newest
+ * line first, so a release for an older Camel version is found with few reads.
+ */
+public final class PluginVersionResolver {
+
+ /**
+ * The most release lines of the plugin whose newest POM is read, newest
line first.
+ */
+ static final int MAX_RELEASE_LINES = 10;
+
+ private static final Logger LOG =
LoggerFactory.getLogger(PluginVersionResolver.class);
+
+ private static final GenericVersionScheme VERSION_SCHEME = new
GenericVersionScheme();
+ // a release without qualifier, such as 1.6.2 (no SNAPSHOT, RC, or other
qualifier)
+ private static final Pattern RELEASE = Pattern.compile("^\\d+(\\.\\d+)*$");
+
+ public sealed interface Result permits Selected, Older, NoMatch,
Unavailable {
+ }
+
+ /**
+ * The newest release that supports the Camel CLI.
+ */
+ public record Selected(String version, Compat compat) implements Result {
+ }
+
+ /**
+ * No release read supports the Camel CLI, which is between the Camel
versions of the releases read: the newest
+ * release for the nearest older Camel version.
+ *
+ * @param version the release
+ * @param compat what is known about the release
+ * @param newestPerLine the newest release per Camel line (such as {@code
4.22 -> 1.6.2}), newest release first,
+ * from the releases read
+ */
+ public record Older(String version, Compat compat, Map<String, String>
newestPerLine) implements Result {
+ }
+
+ /**
+ * Why no release can be selected.
+ */
+ public enum Reason {
+ /**
+ * The Camel CLI is newer than the Camel versions of all the releases
read.
+ */
+ CLI_NEWER,
+ /**
+ * The Camel CLI is older than the Camel versions of all the releases
read.
+ */
+ CLI_OLDER,
+ /**
+ * No release read tells which Camel versions it supports.
+ */
+ UNKNOWN
+ }
+
+ /**
+ * No release can be selected.
+ *
+ * @param reason why
+ * @param newest what is known about the newest release
+ * @param newestVersion the newest release
+ * @param newestPerLine the newest release per Camel line (such as {@code
4.22 -> 1.6.2}), newest release first,
+ * from the releases read
+ */
+ public record NoMatch(Reason reason, Compat newest, String newestVersion,
Map<String, String> newestPerLine)
+ implements
+ Result {
+ }
+
+ /**
+ * The releases or their POMs could not be read, such as when offline.
+ */
+ public record Unavailable(String reason) implements Result {
+ }
+
+ private record Read(String version, Compat compat) {
+ }
+
+ private final DependencyDownloader downloader;
+
+ /**
+ * @param downloader a started downloader, configured with the
repositories to read the releases and POMs from
+ */
+ public PluginVersionResolver(DependencyDownloader downloader) {
+ this.downloader = downloader;
+ }
+
+ /**
+ * Selects the newest release of the plugin that supports the Camel CLI.
+ *
+ * @param groupId the groupId of the plugin
+ * @param artifactId the artifactId of the plugin
+ * @param cliVersion the version of the Camel CLI
+ */
+ public Result resolve(String groupId, String artifactId, String
cliVersion) {
+ List<String> versions;
+ try {
+ // the releases change, so check the repositories again (the POM
of a release does not change)
+ versions = downloader.resolveAvailableVersions(groupId,
artifactId, true);
+ } catch (Exception e) {
+ LOG.debug("Cannot list versions of {}:{}: {}", groupId,
artifactId, e.getMessage(), e);
+ return new Unavailable(
+ e.getMessage() != null
+ ? e.getMessage()
+ : "cannot list the releases of " + groupId + ":" +
artifactId);
+ }
+
+ // the newest release of each release line, newest line first
+ Map<String, String> lines = new LinkedHashMap<>();
+ for (String version : newestFirst(versions)) {
+ lines.putIfAbsent(releaseLine(version), version);
+ }
+ if (lines.isEmpty()) {
+ return new Unavailable("no release of " + groupId + ":" +
artifactId + " found");
+ }
+
+ String failure = null;
+ List<Read> reads = new ArrayList<>();
+ Map<String, String> newestPerLine = new LinkedHashMap<>();
+ for (String version : lines.values()) {
+ if (reads.size() >= MAX_RELEASE_LINES) {
+ break;
+ }
+ Compat compat;
+ try {
+ MavenArtifactDescriptor descriptor
+ = downloader.resolveArtifactDescriptor(groupId,
artifactId, version, null);
+ compat = PluginCompatibility.of(descriptor);
+ } catch (Exception e) {
+ LOG.debug("Cannot read the POM of {}:{}:{}: {}", groupId,
artifactId, version, e.getMessage(), e);
+ failure = e.getMessage();
+ // counts as a read, so an unreachable repository does not
read every release
+ reads.add(new Read(version, null));
Review Comment:
⚠️ **A failed POM read silently skips the entire release line — no test
covers this.**
`lines` is built with `putIfAbsent(releaseLine(version), version)`, holding
only the **newest patch** of each release line. When that one read fails (lines
169–174), the whole line is skipped — potentially moving selection to an older
line that was previously fine (the `fallsBackToAnOlderRelease` case davsclaus
described). Before this PR the resolver would try the next patch; that safety
net is gone.
`FakePluginDownloader` already supports per-version failures: a version
absent from `descriptors` throws `DownloadException`. A test would be:
```java
@Test
void fallsBackToOlderPatchWhenNewestPatchIsUnreadable() {
FakePluginDownloader downloader = new FakePluginDownloader()
.release("2.0.0", "4.22.5", null); // 2.0.0 readable and matches
downloader.versions.add(0, "2.0.1"); // 2.0.1 in listing, no
descriptor → throws
Result result = resolve(downloader, "4.22.0");
// Should select 2.0.0, not skip the whole 2.0 line
assertThat(result).isInstanceOfSatisfying(Selected.class,
s -> assertThat(s.version()).isEqualTo("2.0.0"));
}
```
Please add this test and adjust the resolver to walk the remaining patches
of a line before moving on, as @davsclaus suggested.
--
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]
