jamesnetherton commented on PR #8926:
URL: https://github.com/apache/camel-quarkus/pull/8926#issuecomment-5101049043
Looks good to me mostly. Some feedback:
### 1. `AiToolPresent` should also check for `camel-langchain4j-agent`
`AiToolPresent` only verifies `AiToolRegistry` (from `camel-ai-tool`). But
the build steps it gates also register `CamelAiToolProvider`, whose static
initializer eagerly calls `resolveToToolSpecification()` — which requires
`AiToolSpecToLangChain4j` from `camel-langchain4j-agent`.
If a user has `camel-ai-tool` + `camel-quarkus-support-langchain4j` but
forgets the agent dependency, the application crashes at startup with
`ExceptionInInitializerError`. Adding a second class check makes this fail
gracefully — the bean simply isn't registered:
```java
@Override
public boolean getAsBoolean() {
ClassLoader cl = Thread.currentThread().getContextClassLoader();
try {
cl.loadClass("org.apache.camel.component.ai.tool.AiToolRegistry");
cl.loadClass("org.apache.camel.component.langchain4j.agent.AiToolSpecToLangChain4j");
return true;
} catch (ClassNotFoundException e) {
return false;
}
}
```
---
### 2. Eager static initializer is fragile
```java
private static final Method TO_TOOL_SPECIFICATION =
resolveToToolSpecification();
```
This runs when `CamelAiToolProvider` is first loaded. The
`configureCamelAiToolTags` build step accesses `CamelAiToolProvider.TAG_MAP` (a
non-constant static field), which triggers class initialization during
`STATIC_INIT`. If the converter class is absent, the application fails to start
— even if the user hasn't configured any `ai-tool:` routes yet.
Lazy initialization would limit the failure to when `provideTools()` is
actually called, giving a more actionable error:
```java
private static class ConverterHolder {
static final Method METHOD = resolveToToolSpecification();
}
// Then use ConverterHolder.METHOD in toToolSpecification()
```
(If finding 1 is addressed, this becomes less critical since the build step
won't run at all without both dependencies — but it's still a nice defensive
layer.)
---
### 3. ThreadLocal save/restore for nested interceptor calls
`CamelAiToolsInterceptor` unconditionally clears the ThreadLocal in its
`finally` block. If one `@CamelAiTools`-annotated service calls another (nested
on the same thread), the outer tag is lost:
```
outer interceptor sets "tagA"
inner interceptor sets "tagB"
inner service executes (sees "tagB") ✓
inner finally clears tag
outer service continues (sees null, should see "tagA") ✗
```
Low-probability scenario, but an easy fix — save and restore:
```java
@AroundInvoke
Object aroundInvoke(InvocationContext ctx) throws Exception {
Class<?> targetClass = ctx.getTarget().getClass();
String tag = resolveTag(targetClass);
String previous = CamelAiToolProvider.getCurrentTag();
if (tag != null) {
CamelAiToolProvider.setCurrentTag(tag);
}
try {
return ctx.proceed();
} finally {
if (previous != null) {
CamelAiToolProvider.setCurrentTag(previous);
} else {
CamelAiToolProvider.clearCurrentTag();
}
}
}
```
(Requires adding a `static String getCurrentTag()` method to
`CamelAiToolProvider` that returns `CURRENT_TAG.get()`.)
---
### 4. Consider Gizmo code generation instead of runtime reflection
`CamelAiToolProvider` uses `Class.forName()` + `getMethod()` + `invoke()` to
call `AiToolSpecToLangChain4j.toToolSpecification()` reflectively. This works,
but runtime reflection is not idiomatic in Quarkus extensions — the philosophy
is to push work to build time.
A Gizmo-generated bridge class would eliminate the reflection, the
`ReflectiveClassBuildItem`, and the eager static initializer:
1. Define an interface in the runtime module: `AiToolSpecConverter {
ToolSpecification toToolSpecification(AiToolSpec spec); }`
2. Generate an implementation at build time using Gizmo (all class
references as strings — no compile-time dependency on the agent jar)
3. `CamelAiToolProvider` injects the interface via CDI
This would also make findings 2 and 3's static initializer concerns moot,
since the Gizmo class only exists if the build step runs.
---
### 5. Empty `@CamelAiTools("")` tag value not validated
The Jandex scan checks for `annotation.value() == null` (handles
`@CamelAiTools` with no explicit value), but `@CamelAiTools("")` passes through
and creates a mapping with an empty-string tag. `registry.getToolsByTag("")`
would return only default-pool tools (no tool is tagged with `""`), which could
silently confuse users. Should also reject blank values:
```java
String tagValue = annotation.value().asString();
if (tagValue == null || tagValue.isBlank()) {
LOG.warnf("@CamelAiTools on %s has no/blank value — skipping",
className);
continue;
}
```
---
--
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]
