davsclaus commented on code in PR #26310: URL: https://github.com/apache/camel/pull/26310#discussion_r3989778047
########## components/camel-javascript/src/main/java/org/apache/camel/language/js/JavaScriptLanguage.java: ########## @@ -16,21 +16,62 @@ */ package org.apache.camel.language.js; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.apache.camel.Expression; import org.apache.camel.Predicate; +import org.apache.camel.Service; import org.apache.camel.spi.ScriptingLanguage; import org.apache.camel.spi.annotations.Language; +import org.apache.camel.support.LRUCacheFactory; import org.apache.camel.support.TypedLanguageSupport; import org.graalvm.polyglot.Context; +import org.graalvm.polyglot.Engine; import org.graalvm.polyglot.Source; import org.graalvm.polyglot.Value; -import static org.graalvm.polyglot.Source.newBuilder; - +/** + * Camel expression language for JavaScript via <a href="https://www.graalvm.org/javascript/";>GraalJS</a>. + * <p> + * One {@link Engine} is shared by all evaluations of a language instance so parsed and compiled scripts are reused; a + * fresh {@link Context} is still created per evaluation so scripts stay isolated from each other. The engine is created Review Comment: "The engine is created lazily on first use" is not what happens. `start()` calls `engine()` unconditionally, and `AbstractCamelContext.doResolveLanguage(...)` starts a `Language` as soon as it is resolved: ```java if (language instanceof Service service) { CamelContextAware.trySetCamelContext(service, camelContext); ServiceHelper.initService(service); startService(service); } ``` So the `Engine` is built the moment anything resolves the `js` language — during route startup, not on first evaluation. That is probably what you want (pay the GraalJS engine-build cost at startup rather than on the first message), but the comment should say so, since "lazily on first use" sets a startup-cost expectation that is not met. Either drop the `engine()` call from `start()` and keep the sentence, or keep `start()` and reword to something like "created when the language is started and closed when it is stopped; the `engine()` accessor also builds it on demand for expressions used before start". ########## components/camel-javascript/src/main/java/org/apache/camel/language/js/JavaScriptLanguage.java: ########## @@ -45,14 +86,140 @@ public Expression createExpression(String expression) { @Override public <T> T evaluate(String script, Map<String, Object> bindings, Class<T> resultType) { script = loadResource(script); - try (Context cx = JavaScriptHelper.newContext()) { - Value b = cx.getBindings("js"); - bindings.forEach(b::putMember); - Source source = newBuilder("js", script, "Unnamed") - .mimeType("application/javascript+module").buildLiteral(); - Value o = cx.eval(source); - Object answer = o != null ? o.as(resultType) : null; - return resultType.cast(answer); + try (Context cx = newContext()) { + if (bindings != null) { + Value b = cx.getBindings("js"); + bindings.forEach(b::putMember); + } + Value o = cx.eval(source(script)); + Object answer = materialize(o); + if (answer == null || resultType == Object.class || resultType.isInstance(answer)) { + return resultType.cast(answer); + } + if (getCamelContext() != null) { + return getCamelContext().getTypeConverter().convertTo(resultType, answer); Review Comment: `TypeConverter.convertTo(...)` returns `null` when it has no converter for the pair, rather than throwing. Previously this path was `o.as(resultType)`, which raised a `ClassCastException`/`PolyglotException` on an impossible conversion. So a `ScriptingLanguage` caller asking for a type the materialized value cannot become now silently gets `null` instead of an error pointing at the script. `mandatoryConvertTo(...)` keeps the old "fail loudly" contract and gives a much better message than the polyglot exception did: ```suggestion return getCamelContext().getTypeConverter().mandatoryConvertTo(resultType, answer); ``` If a lenient `null` is deliberate here, worth a short comment saying so — it is the kind of thing that reads as an oversight later. -- 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]
