gnodet-bot commented on code in PR #26477:
URL: https://github.com/apache/camel/pull/26477#discussion_r4018749367
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogDocs.java:
##########
@@ -343,6 +355,222 @@ public static List<String> beansOfInterface(CamelCatalog
catalog, String interfa
return answer;
}
+ /** The languages whose script variables are documented, by their catalog
name (js is javascript, java is joor). */
+ private static final List<String> SCRIPT_LANGUAGES
+ = List.of("groovy", "js", "python", "python3", "quickjs", "java",
"template");
+
+ /** The template components that bind the same variable map, answered by
the template card. */
+ private static final List<String> TEMPLATE_COMPONENTS
+ = List.of("velocity", "freemarker", "mvel", "mustache", "chunk",
"stringtemplate", "thymeleaf", "jslt");
+
+ /**
+ * The compact API reference of a core Camel class (Exchange, Message,
CamelContext, Registry, ProducerTemplate,
+ * Processor, AggregationStrategy, Predicate, Expression, TypeConverter)
from the catalog, or the variables a script
+ * language binds; null when the name is neither. An older catalog that
has no API reference answers from the CLI's
+ * own, the API is the same.
+ *
+ * @param catalog the catalog
+ * @param name a simple or qualified class name, or a script language
(groovy, javascript, python, java)
+ * @param explicit whether kind=api was asked for, which makes a miss an
error with the names that exist
+ */
+ static JsonObject apiDoc(CamelCatalog catalog, String name, boolean
explicit) {
+ String n = name.trim();
+ String simple = n.substring(n.lastIndexOf('.') + 1);
+ CamelCatalog source = catalog.findApiReferenceNames().isEmpty() ?
ownCatalog() : catalog;
+ List<String> names = source.findApiReferenceNames();
+ String match = names.stream().filter(c ->
c.equalsIgnoreCase(simple)).findFirst().orElse(null);
+ if (match != null) {
+ ApiReferenceModel model = source.apiReferenceModel(match);
+ if (model != null && (n.equals(simple) ||
model.getJavaType().equalsIgnoreCase(n))) {
+ return apiReferenceDoc(catalog, model, names);
+ }
+ }
+ String lang = n.toLowerCase(Locale.ROOT);
+ if ("joor".equals(lang)) {
+ lang = "java";
+ } else if ("javascript".equals(lang)) {
+ lang = "js";
+ } else if (TEMPLATE_COMPONENTS.contains(lang)) {
+ // the mvel component is a template; the mvel language is asked
for without kind and answers as a language
+ lang = "template";
+ }
+ JsonObject script = scriptVariables(lang);
+ if (script != null) {
+ script.put("apis", new JsonArray(apiNames(names)));
+ return script;
+ }
+ if (explicit) {
+ JsonObject err = error("No API reference for '" + name + "'");
+ err.put("apis", new JsonArray(apiNames(names)));
+ return err;
+ }
+ return null;
+ }
+
+ /** The names an api lookup answers: the class cards and the script
languages. */
+ private static List<String> apiNames(List<String> classNames) {
+ List<String> answer = new ArrayList<>(classNames);
+ answer.addAll(SCRIPT_LANGUAGES);
+ return answer;
+ }
+
+ private static volatile CamelCatalog own;
+
Review Comment:
⚠️ **Broken double-checked locking — `ownCatalog()` can initialize twice
under race.**
`volatile` alone is not enough here. The current code is:
```java
private static volatile CamelCatalog own;
private static CamelCatalog ownCatalog() {
CamelCatalog c = own;
if (c == null) {
c = new DefaultCamelCatalog();
own = c;
}
return c;
}
```
Two threads can both read `own == null`, both construct a
`DefaultCamelCatalog`, and both write it — producing two distinct catalog
instances. One gets discarded; the other gets returned to the first caller. The
`volatile` guarantees visibility of the write but not atomicity of the
check-then-act. The cache inside `DefaultCamelCatalog` is separate per
instance, so callers may end up observing inconsistent cached state if one
thread reads from the stale instance before the write flushes.
Fix with proper double-checked locking (Java 5+ with `volatile` on the field
this works correctly if you add the inner null check):
```suggestion
private static volatile CamelCatalog own;
/** The catalog of the CLI's own Camel version, for the API reference an
older catalog does not carry. */
private static CamelCatalog ownCatalog() {
CamelCatalog c = own;
if (c == null) {
synchronized (CatalogDocs.class) {
c = own;
if (c == null) {
c = new DefaultCamelCatalog();
own = c;
}
}
}
return c;
}
```
Alternatively use a `static` initializer or `static` final field (no
lazy-init needed here since `DefaultCamelCatalog` is cheap and `CatalogDocs` is
loaded only when the tool runs):
```java
private static final CamelCatalog OWN_CATALOG = new DefaultCamelCatalog();
private static CamelCatalog ownCatalog() {
return OWN_CATALOG;
}
```
##########
tooling/camel-tooling-model/src/main/java/org/apache/camel/tooling/model/JsonMapper.java:
##########
@@ -350,6 +352,79 @@ public static PojoBeanModel
generatePojoBeanModel(JsonObject obj) {
return model;
}
+ public static ApiReferenceModel generateApiReferenceModel(String json) {
+ JsonObject obj = deserialize(json);
+ return generateApiReferenceModel(obj);
+ }
+
+ public static ApiReferenceModel generateApiReferenceModel(JsonObject obj) {
+ JsonObject mobj = (JsonObject) obj.get("api");
+ ApiReferenceModel model = new ApiReferenceModel();
+ parseModel(mobj, model);
+ parseArtifact(mobj, model);
+ JsonObject methods = (JsonObject) mobj.get("methods");
+ if (methods != null) {
+ for (Map.Entry<String, Object> entry : methods.entrySet()) {
+ JsonObject mp = (JsonObject) entry.getValue();
+ ApiReferenceModel.ApiMethodOptionModel method = new
ApiReferenceModel.ApiMethodOptionModel();
+ method.setName(entry.getKey());
+ Integer idx = mp.getInteger("index");
+ if (idx != null) {
+ method.setIndex(idx);
+ }
+ method.setJavaType(mp.getString("javaType"));
+ method.setDescription(mp.getString("description"));
+ method.setDeprecated(mp.getBooleanOrDefault("deprecated",
false));
+ method.setImportant(mp.getBooleanOrDefault("important",
false));
+ mp.getCollectionOrDefault("signatures", List.of()).forEach(o
-> method.addSignature(o.toString()));
+ mp.getCollectionOrDefault("examples", List.of()).forEach(o ->
method.addExample(o.toString()));
+ model.addOption(method);
+ }
+ }
+ return model;
+ }
+
+ public static String createParameterJsonSchema(ApiReferenceModel model) {
+ JsonObject wrapper = asJsonObject(model);
+ return serialize(wrapper);
+ }
+
+ public static JsonObject asJsonObject(ApiReferenceModel model) {
+ JsonObject obj = new JsonObject();
+ baseToJson(model, obj);
+ artifactToJson(model, obj);
+ obj.entrySet().removeIf(e -> e.getValue() == null);
+ JsonObject methods = new JsonObject();
+ List<ApiReferenceModel.ApiMethodOptionModel> options =
model.getOptions();
+ for (int i = 0; i < options.size(); i++) {
+ ApiReferenceModel.ApiMethodOptionModel m = options.get(i);
+ m.setIndex(i);
Review Comment:
⚠️ **`asJsonObject` mutates the model as a side effect of serialization.**
`m.setIndex(i)` modifies the option model in-place during what should be a
read-only serialization pass. This makes `asJsonObject()` non-idempotent:
calling it twice on the same model changes the `index` values the second time
only if the list order changed (e.g. after a sort). More critically, it makes
the serializer a hidden mutator, which is unexpected for callers that hold a
reference to the model.
This pattern already exists in other `asJsonObject` overloads (e.g. for
`EipModel`) so it is a pre-existing issue, but this new one introduces it
again. The fix is to use `i` (the loop variable) directly in the JSON output
and leave `m.setIndex()` out of this method:
```suggestion
ApiReferenceModel.ApiMethodOptionModel m = options.get(i);
JsonObject jo = new JsonObject();
jo.put("index", i);
```
--
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]
