gnodet-bot commented on code in PR #26753:
URL: https://github.com/apache/camel/pull/26753#discussion_r4075302720
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/ExpressionEvaluator.java:
##########
@@ -140,12 +182,87 @@ private static void evaluateLocally(
result.put("result", value != null ? value.toString() : null);
} catch (Exception e) {
result.put("status", "error");
- Throwable cause = e;
- while (cause.getCause() != null && cause.getCause() != cause) {
- cause = cause.getCause();
+ result.put("error", rootCause(e));
+ }
+ return true;
+ }
+
+ /**
+ * The catalog's own check of the text, which names where the syntax
breaks (its index), so an error says more than
+ * the evaluation's exception; only added when it finds something the
evaluation did not.
+ */
+ private static void syntaxCheck(
+ ToolContext ctx, ClassLoader loader, String lang, String
expression, boolean predicate, JsonObject result) {
+ if ("ok".equals(result.getString("status"))) {
+ return;
+ }
+ try {
+ LanguageValidationResult check = predicate
+ ? ctx.catalog().validateLanguagePredicate(loader, lang,
expression)
+ : ctx.catalog().validateLanguageExpression(loader, lang,
expression);
+ if (!check.isSuccess()) {
+ String error = check.getShortError() != null ?
check.getShortError() : check.getError();
+ if (error != null) {
+ result.put("syntaxError", error);
+ if (check.getIndex() >= 0) {
+ result.put("syntaxErrorAt", check.getIndex());
+ }
+ }
+ }
+ } catch (Exception e) {
+ // the catalog cannot check this language here; the evaluation's
own error stands
+ }
+ }
+
+ private static String rootCause(Throwable e) {
+ Throwable cause = e;
+ while (cause.getCause() != null && cause.getCause() != cause) {
+ cause = cause.getCause();
+ }
+ return cause.getMessage() != null ? cause.getMessage() :
cause.toString();
+ }
+
+ /** The groupId:artifactId:version of a language, from the catalog, or
null when the catalog does not know it. */
+ private static String languageArtifact(ToolContext ctx, String lang) {
+ try {
+ LanguageModel model = ctx.catalog().languageModel(lang);
+ if (model != null && model.getArtifactId() != null) {
+ return model.getGroupId() + ":" + model.getArtifactId() + ":"
+ model.getVersion();
+ }
+ } catch (Exception e) {
+ // the catalog does not know it
+ }
+ return null;
+ }
+
+ /**
+ * Downloads the component of a language and keeps its class loader, so
the next call does not download again.
+ * Returns null when there is nothing to download (an unknown name) or the
download fails (no network).
+ */
+ private static ClassLoader download(String gav) {
+ if (gav == null) {
+ return null;
+ }
+ ClassLoader cached = DOWNLOADED.get(gav);
+ if (cached != null) {
+ return cached;
+ }
+ String[] parts = gav.split(":");
+ try {
+ DependencyDownloaderClassLoader cl
+ = new
DependencyDownloaderClassLoader(ExpressionEvaluator.class.getClassLoader());
+ try (MavenDependencyDownloader downloader = new
MavenDependencyDownloader()) {
+ downloader.setClassLoader(cl);
+ downloader.start();
+ downloader.downloadDependency(parts[0], parts[1], parts[2]);
}
- String message = cause.getMessage();
- result.put("error", message != null ? message : cause.toString());
+ DOWNLOADED.put(gav, cl);
Review Comment:
⚠️ **Concurrency: `DOWNLOADED.put()` allows a double-download race.**
Two threads evaluating the same language simultaneously (e.g. two parallel
`camel_eval_expression` calls from an agent) both see `null` from
`DOWNLOADED.get()`, both download, and one `DependencyDownloaderClassLoader` (a
`URLClassLoader` wrapping JAR file handles) is leaked permanently in the static
map.
Use `computeIfAbsent` — but since the download is slow and blocking, do it
outside the lock and then race-safely stash the winner:
```suggestion
DOWNLOADED.putIfAbsent(gav, cl);
return DOWNLOADED.get(gav);
```
This still allows one redundant download in the rare race, but at least both
class loaders are usable (they resolve the same JARs) and the map ends up
consistent. If you want to close the loser explicitly:
```java
ClassLoader winner = DOWNLOADED.putIfAbsent(gav, cl);
if (winner != null) {
// another thread won; cl is unused — close it to release file handles
try { ((java.io.Closeable) cl).close(); } catch (Exception ignored) {}
return winner;
}
return cl;
```
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/ExpressionEvaluator.java:
##########
@@ -140,12 +182,87 @@ private static void evaluateLocally(
result.put("result", value != null ? value.toString() : null);
} catch (Exception e) {
result.put("status", "error");
- Throwable cause = e;
- while (cause.getCause() != null && cause.getCause() != cause) {
- cause = cause.getCause();
+ result.put("error", rootCause(e));
+ }
+ return true;
+ }
+
+ /**
+ * The catalog's own check of the text, which names where the syntax
breaks (its index), so an error says more than
+ * the evaluation's exception; only added when it finds something the
evaluation did not.
+ */
+ private static void syntaxCheck(
+ ToolContext ctx, ClassLoader loader, String lang, String
expression, boolean predicate, JsonObject result) {
+ if ("ok".equals(result.getString("status"))) {
+ return;
+ }
+ try {
+ LanguageValidationResult check = predicate
+ ? ctx.catalog().validateLanguagePredicate(loader, lang,
expression)
+ : ctx.catalog().validateLanguageExpression(loader, lang,
expression);
+ if (!check.isSuccess()) {
+ String error = check.getShortError() != null ?
check.getShortError() : check.getError();
+ if (error != null) {
+ result.put("syntaxError", error);
+ if (check.getIndex() >= 0) {
+ result.put("syntaxErrorAt", check.getIndex());
+ }
+ }
+ }
+ } catch (Exception e) {
+ // the catalog cannot check this language here; the evaluation's
own error stands
+ }
+ }
+
+ private static String rootCause(Throwable e) {
+ Throwable cause = e;
+ while (cause.getCause() != null && cause.getCause() != cause) {
+ cause = cause.getCause();
+ }
+ return cause.getMessage() != null ? cause.getMessage() :
cause.toString();
+ }
+
+ /** The groupId:artifactId:version of a language, from the catalog, or
null when the catalog does not know it. */
+ private static String languageArtifact(ToolContext ctx, String lang) {
+ try {
+ LanguageModel model = ctx.catalog().languageModel(lang);
+ if (model != null && model.getArtifactId() != null) {
+ return model.getGroupId() + ":" + model.getArtifactId() + ":"
+ model.getVersion();
+ }
+ } catch (Exception e) {
+ // the catalog does not know it
+ }
+ return null;
+ }
+
+ /**
+ * Downloads the component of a language and keeps its class loader, so
the next call does not download again.
+ * Returns null when there is nothing to download (an unknown name) or the
download fails (no network).
+ */
+ private static ClassLoader download(String gav) {
+ if (gav == null) {
+ return null;
+ }
+ ClassLoader cached = DOWNLOADED.get(gav);
+ if (cached != null) {
+ return cached;
+ }
+ String[] parts = gav.split(":");
+ try {
+ DependencyDownloaderClassLoader cl
+ = new
DependencyDownloaderClassLoader(ExpressionEvaluator.class.getClassLoader());
+ try (MavenDependencyDownloader downloader = new
MavenDependencyDownloader()) {
+ downloader.setClassLoader(cl);
+ downloader.start();
+ downloader.downloadDependency(parts[0], parts[1], parts[2]);
}
- String message = cause.getMessage();
- result.put("error", message != null ? message : cause.toString());
+ DOWNLOADED.put(gav, cl);
+ return cl;
+ } catch (Exception e) {
+ return null;
Review Comment:
🔍 **Silent swallow: download failure reason is discarded.**
When Maven resolution fails (network timeout, wrong repository, artifact not
found, unparseable GAV), the exception is silently dropped and `null` is
returned. The caller's error message names the GAV but not the cause, so the
model (or a developer debugging) sees:
> `"The language 'jq' is not on the classpath of this process and
org.apache.camel:camel-jq:4.x.y could not be downloaded; select a running
integration..."`
with no indication of *why* it couldn't be downloaded. At minimum, log the
cause at DEBUG level so it shows up in `camel jbang` output:
```suggestion
} catch (Exception e) {
LOG.debug("Failed to download {} for language evaluation: {}",
gav, e.getMessage());
return null;
```
(`LOG` being the existing `Logger` if this class has one, or a new
`LoggerFactory.getLogger(ExpressionEvaluator.class)` field.)
--
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]
