oscerd commented on code in PR #27421:
URL: https://github.com/apache/camel/pull/27421#discussion_r4193663895
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogDocs.java:
##########
@@ -345,12 +345,45 @@ private static JsonObject eipDoc(
String matchedTerm) {
String doc = includeDoc ? catalog.asciiDoc(model.getName() + "-eip") :
null;
JsonObject result = eipDoc(model, filter, scope, doc);
+ if (filter != null && scope != OptionScope.NONE &&
result.getIntegerOrDefault("matchedOptions", 0) == 0) {
+ putNestedOptions(catalog, model, filter, result);
+ }
if (matchedTerm != null) {
result.put("matchedTerm", matchedTerm);
}
return result;
}
+ /**
+ * The options of the elements of an EIP that match the filter, when none
of its own do: logStackTrace is an option
+ * of the redeliveryPolicy of onException, and a filter on onException
found nothing (CAMEL-25370).
+ */
+ private static void putNestedOptions(CamelCatalog catalog, EipModel model,
String filter, JsonObject result) {
+ JsonArray nested = new JsonArray();
+ for (BaseOptionModel opt : model.getOptions()) {
+ if (!"element".equals(opt.getKind())) {
+ continue;
+ }
+ EipModel element = catalog.eipModel(opt.getName());
Review Comment:
**The nested search finds the element's model by option name, so name
collisions pick an unrelated model**
`catalog.eipModel(opt.getName())` assumes the option is named after its
model. That holds for `onException.redeliveryPolicy`, but resolves to an
unrelated model for e.g. `templatedRoute.bean`
(`List<BeanFactoryDefinition<...>>`, resolved to the `bean` EIP's
`BeanDefinition`, so `templatedRoute optionsFilter=method` answers `bean:
{method: ...}`, while a templated route bean has
`initMethod`/`factoryMethod`/`builderMethod` and its YAML key is `beans:`),
`responseMessage.header` (→ the `header` language) and `beanFactory.script` (a
`String`, → the `script` EIP). Checking that the option's javaType (inside
`List<...>`, without generics) equals `element.getJavaType()` would skip
exactly these and keep the intended matches.
Two smaller points: `under` is the catalog element name and the hint always
shows a map, so `routeTemplate.templateParameter` (YAML `parameters:`, a list)
and `choice.when` (a list) get shapes the schema does not accept; and
`errorHandler` itself gets no `nestedOptions`, because `errorHandlerType` has
no model of that name (`camel_catalog_doc errorHandler
optionsFilter=deadLetterUri` still finds nothing).
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogDocs.java:
##########
@@ -1535,6 +1568,15 @@ public static JsonObject optionToJson(BaseOptionModel
opt, String scope) {
enums.addAll(opt.getEnums());
o.put("enumValues", enums);
}
+ if ("element".equals(opt.getKind()) && opt.getOneOfs() != null &&
!opt.getOneOfs().isEmpty()) {
Review Comment:
**The oneOf hint fires for every element with a oneOf list, including
`outputs` (27 EIPs), where it is wrong**
This condition matches every `element` option with a `oneOf` list, not only
the ones written as one of their kinds. It is right for
`errorHandler.errorHandlerType`, `marshal`/`unmarshal`'s `dataFormatType`,
`loadBalance.loadBalancerType`, `resequence.resequencerConfig`,
`tokenizer.tokenizerImplementation` and `route.input`, but wrong for the rest:
- `outputs` (kind element, type array, required, oneOf of ~72 processors) is
an option of 27 EIP models (filter, split, loop, multicast, doTry, onException,
aggregate, when, otherwise, route, ...). Their default `camel_catalog_doc`
answer now says `write one of oneOf as the key, not 'outputs': for example
a2aSubTask: {...}`, while in the YAML DSL these go under `steps:`.
`FilterDefinition` has `steps` and no `to`/`log` (`additionalProperties:
false`), so following the hint fails validation; for `route` the steps go under
`from:`.
- `setHeaders.headers` / `setVariables.variables` (oneOf `[setHeader]` /
`[setVariable]`): the hint says to write `setHeader:` instead of `headers:`,
but the YAML is `setHeaders: {headers: [...]}`.
- `choice.when`, `choice.otherwise` and `doTry.doCatch` have their own name
as their only oneOf entry, so the hint contradicts itself (`not 'when': for
example when: {...}`).
The 72-name list also makes those 27 answers noticeably larger, which works
against the small-model goal. Limiting the hint to single-valued elements
written as one of their kinds keeps every correct case:
```suggestion
if ("element".equals(opt.getKind()) &&
"object".equals(opt.getType()) && opt.getOneOfs() != null
&& !opt.getOneOfs().isEmpty() &&
!opt.getOneOfs().contains(opt.getName())) {
```
A negative assertion in `CatalogDocsTest` (no `oneOf`/`yaml` on `filter`'s
`outputs` or `setHeaders`'s `headers`) would keep it from coming back.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogSamples.java:
##########
@@ -90,6 +90,10 @@ public final class CatalogSamples {
Map.entry("redeliveryPolicy", "onException"),
Map.entry("deadLetterChannel", "errorHandler"),
Map.entry("defaultErrorHandler", "errorHandler"),
+ Map.entry("noErrorHandler", "errorHandler"),
+ Map.entry("jtaTransactionErrorHandler", "errorHandler"),
+ Map.entry("springTransactionErrorHandler", "errorHandler"),
+ Map.entry("refErrorHandler", "errorHandler"),
Review Comment:
**"the sample shows it in place" is not true for the four kinds the samples
do not show**
With these entries, `resolveExact` maps `defaultErrorHandler`,
`jtaTransactionErrorHandler`, `springTransactionErrorHandler` and
`refErrorHandler` to `errorHandler`, and `eipAnswer` adds `<name> is a part of
errorHandler; the sample shows it in place`. The two shipped `errorHandler`
samples only contain `deadLetterChannel` and `noErrorHandler`, so the note is
not true for those four. That matters most for `refErrorHandler`, whose shape
(`refErrorHandler: myHandler` or `{ref: ...}`) cannot be inferred from a
deadLetterChannel example. A note for these kinds could say the samples show
deadLetterChannel and noErrorHandler, that `<kind>` goes in the same place, and
that `camel_catalog_doc <kind>` gives its options.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogDocs.java:
##########
@@ -345,12 +345,45 @@ private static JsonObject eipDoc(
String matchedTerm) {
String doc = includeDoc ? catalog.asciiDoc(model.getName() + "-eip") :
null;
JsonObject result = eipDoc(model, filter, scope, doc);
+ if (filter != null && scope != OptionScope.NONE &&
result.getIntegerOrDefault("matchedOptions", 0) == 0) {
Review Comment:
**With includeOptions=required, the fallback runs although the EIP's own
options matched, and ignores the scope**
`matchedOptions` counts the options the scope kept, not the ones the filter
matched. With `includeOptions=required`, `camel_catalog_doc onException
optionsFilter=redelivery` matches six of the EIP's own options (none required),
so `matchedOptions: 0` / `omittedOptions: 6`, and the fallback still runs and
adds `redeliveryPolicy` options, none of them required either, because
`putNestedOptions` never applies the scope. That contradicts the javadoc ("when
none of its own do"). Also requiring `omittedOptions == 0` here and passing the
scope into `putNestedOptions` would keep them consistent.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/CatalogSamples.java:
##########
@@ -648,7 +652,17 @@ static String normalizeKind(String kind) {
return k;
}
+ /** The kinds of error handler, each the one key under errorHandler:. */
+ static final Set<String> ERROR_HANDLERS = Set.of("errorHandler",
"deadLetterChannel", "defaultErrorHandler",
+ "noErrorHandler", "jtaTransactionErrorHandler",
"springTransactionErrorHandler", "refErrorHandler");
+
static String placement(String key) {
+ if (ERROR_HANDLERS.contains(key)) {
+ // a step it is not, and it has two places (CAMEL-25370)
+ return "errorHandler: with the kind (deadLetterChannel,
defaultErrorHandler, noErrorHandler) as its one key,"
+ + " either as a top-level list item - errorHandler: for
every route, or under one route, next to from:,"
+ + " for that route only; never a step";
Review Comment:
**The placement leaves out routeConfiguration**
`routeConfiguration` is a third place for the error handler
(`RouteConfigurationDefinition` has `errorHandler`, and
route-configuration.adoc has a "Route Configuration with Error Handler"
section), and it is the usual way to share one handler among a group of routes
without making it global:
```suggestion
+ " for that route only, or inside a routeConfiguration
for the routes that use it; never a step";
```
--
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]
