gnodet-bot commented on code in PR #27841:
URL: https://github.com/apache/camel/pull/27841#discussion_r4291746164
##########
core/camel-core-catalog/src/main/java/org/apache/camel/catalog/impl/AbstractCamelCatalog.java:
##########
@@ -1232,33 +1235,42 @@ public ConfigurationPropertiesValidationResult
validateConfigurationProperty(Str
int posBracket = nOption.indexOf('[');
validateConfigurationProperty(posDot, posBracket, suffix,
nOption, result, rows, name, value, longKey);
}
- } else if (key.startsWith("main.")
- || key.startsWith("resilience4j.")
- || key.startsWith("faulttolerance.")
- || key.startsWith("threadpool.")
- || key.startsWith("lra.")
- || key.startsWith("health.")
- || key.startsWith("rest.")) {
- String name = StringHelper.before(key, ".");
- if (value != null) {
- MainModel model = mainModel();
- if (model == null) {
- result.addIncapable("camel-main not detected on
classpath");
- return result;
- }
- Map<String, BaseOptionModel> rows = new HashMap<>();
- model.getOptions().forEach(o ->
rows.put(dashToCamelCase(o.getName()), o));
-
- // lower case option and remove dash
- String nOption = longKey.replace("-",
"").toLowerCase(Locale.ENGLISH);
-
- // look for suffix or array index after 2nd dot
- int secondDot = nOption.indexOf('.', nOption.indexOf('.') + 1)
+ 1;
+ } else {
+ // Check if the key matches a main-model group (dynamic — replaces
the hardcoded list)
+ MainModel.MainGroupModel matchedGroup = matchMainModelGroup(key,
cachedMainModel);
+ if (matchedGroup != null) {
+ String name = StringHelper.before(key, ".");
+ if (value != null) {
+ // cachedMainModel is guaranteed non-null here:
acceptConfigurationPropertyKey returned true
+ // for a camel.* key that is not
component/dataformat/language, which means matchMainModelGroup
+ // already found a match — so the model must have been
available.
+ MainModel model = cachedMainModel;
+ if (model == null) {
+ result.addIncapable("camel-main not detected on
classpath");
+ return result;
+ }
Review Comment:
💡 **Dead code:** the comment at line 1244 says `cachedMainModel` is
*guaranteed non-null* here (because `matchMainModelGroup` already found a
match, which requires a non-null model), yet the `if (model == null)` branch at
line 1248 is still present. Either remove the branch or drop the "guaranteed"
claim — keeping both contradicts itself and could mislead future readers about
the contract.
##########
core/camel-core-catalog/src/main/java/org/apache/camel/catalog/impl/AbstractCamelCatalog.java:
##########
@@ -1185,7 +1185,10 @@ public ConfigurationPropertiesValidationResult
validateConfigurationProperty(Str
}
ConfigurationPropertiesValidationResult result = new
ConfigurationPropertiesValidationResult();
- boolean accept = acceptConfigurationPropertyKey(key);
+ // Load the main model once here to avoid repeated JSON parsing in
acceptConfigurationPropertyKey
+ // and matchMainModelGroup (both called for camel.* keys that are not
component/dataformat/language).
+ MainModel cachedMainModel = key != null && key.startsWith("camel.") ?
mainModel() : null;
Review Comment:
⚠️ **Performance regression:** `mainModel()` is called eagerly for **all**
`camel.*` keys, including `camel.component.*`, `camel.dataformat.*`, and
`camel.language.*` — which never use the result (they hit the static-prefix
fast-path at lines 1432–1435 and return before the model is accessed).
`mainModel()` parses a 229 KB JSON on every call with no cache in
`DefaultCamelCatalog`, so every component/dataformat/language property lookup
now pays a full parse that it didn't before.
```suggestion
// Load the main model once, but only for dynamic main-model keys.
// Component, dataformat, and language keys are accepted statically
(no model needed).
MainModel cachedMainModel = key != null && key.startsWith("camel.")
&& !key.startsWith("camel.component.")
&& !key.startsWith("camel.dataformat.")
&& !key.startsWith("camel.language.") ? mainModel() : null;
```
##########
core/camel-core-catalog/src/main/java/org/apache/camel/catalog/impl/AbstractCamelCatalog.java:
##########
@@ -1408,20 +1420,60 @@ private static boolean isValidSuffix(String suffix) {
return suffix == null || suffix.isEmpty() || suffix.equals(".");
}
- private static boolean acceptConfigurationPropertyKey(String key) {
+ private boolean acceptConfigurationPropertyKey(String key, MainModel
cachedModel) {
if (key == null) {
return false;
}
- return key.startsWith("camel.component.")
+ // Keys outside the camel.* namespace are never checked by this catalog
+ if (!key.startsWith("camel.")) {
+ return false;
+ }
+ // Always-accepted static prefixes (component/dataformat/language have
their own models)
+ if (key.startsWith("camel.component.")
|| key.startsWith("camel.dataformat.")
- || key.startsWith("camel.language.")
- || key.startsWith("camel.main.")
- || key.startsWith("camel.resilience4j.")
- || key.startsWith("camel.faulttolerance.")
- || key.startsWith("camel.threadpool.")
- || key.startsWith("camel.health.")
- || key.startsWith("camel.lra.")
- || key.startsWith("camel.rest.");
+ || key.startsWith("camel.language.")) {
+ return true;
+ }
+ // Dynamic: accept any prefix that matches a group in the main-model
metadata.
+ // camel-main binds keys after normalising dashes and case
(MainHelper.optionKey /
+ // startsWithIgnoreCase), so we compare normalised forms to match e.g.
+ // camel.error-registry.* against the group name "camel.errorRegistry".
+ String normKey = key.replace("-", "").toLowerCase(Locale.ENGLISH);
+ MainModel model = cachedModel != null ? cachedModel : mainModel();
+ if (model != null) {
+ for (MainModel.MainGroupModel group : model.getGroups()) {
+ String normGroup = group.getName().replace("-",
"").toLowerCase(Locale.ENGLISH) + ".";
+ if (normKey.startsWith(normGroup)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
Review Comment:
💡 **Duplication:** `acceptConfigurationPropertyKey` (lines 1423–1452) and
`matchMainModelGroup` (lines 1461–1477) both iterate `model.getGroups()` with
normalise-and-`startsWith` logic. They differ only in key convention (full
`camel.*` key vs stripped key), which makes a direct delegation tricky but is
worth a comment explaining why both exist separately, or consider unifying by
always passing the stripped key.
##########
dsl/camel-jbang/camel-jbang-mcp/src/test/java/org/apache/camel/dsl/jbang/core/commands/mcp/ConfigurationValidateToolsTest.java:
##########
@@ -172,14 +172,114 @@ void multipleLinesAggregated() {
}
@Test
- void nonCamelKeyMarkedNotAccepted() {
+ void nonCamelKeyMarkedNotChecked() {
+ // Non-Camel keys (e.g. server.port) are not checked by this catalog:
checked=false,
+ // valid=false, and counted in summary.unchecked (not in
summary.errors or summary.valid).
ConfigurationValidateTools tools = createTools();
ConfigurationValidateTools.ConfigurationValidateResult result
= tools.camel_configuration_validate("server.port=8080", null,
null, null);
ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
- assertThat(info.accepted()).isFalse();
+ assertThat(info.checked()).isFalse();
+ assertThat(info.valid()).isFalse();
+ assertThat(result.summary().unchecked()).isEqualTo(1);
+ assertThat(result.summary().errors()).isZero();
+ assertThat(result.summary().valid()).isZero();
+ }
+
+ // --- CAMEL-25538 regression tests ---
+
+ @Test
+ void camelServerPortIsAcceptedAndValid() {
+ // camel.server.* was previously not in the hardcoded prefix list — it
must now be
+ // accepted and validated against the main-model metadata.
+ ConfigurationValidateTools tools = createTools();
+
+ ConfigurationValidateTools.ConfigurationValidateResult result
+ = tools.camel_configuration_validate("camel.server.port=9999",
null, null, null);
+
+ assertThat(result.summary().total()).isEqualTo(1);
+ ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
+ assertThat(info.checked()).isTrue();
+ assertThat(info.valid()).isTrue();
+ assertThat(info.errors()).isZero();
+ assertThat(info.issues()).isEmpty();
+ }
+
+ @Test
+ void camelServerInventedKeyIsCheckedButNotValid() {
+ // An invented key under an accepted camel.server.* group must be
reported as unknown,
+ // not silently passed as valid.
+ ConfigurationValidateTools tools = createTools();
+
+ ConfigurationValidateTools.ConfigurationValidateResult result
+ =
tools.camel_configuration_validate("camel.server.bindPort=9999", null, null,
null);
+
+ ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
+ assertThat(info.checked()).isTrue();
+ assertThat(info.valid()).isFalse();
+ assertThat(info.issues()).anyMatch(i -> "unknown".equals(i.kind()));
+ }
+
+ @Test
+ void uncheckedKeyImpliesValidFalseAndNotCountedInErrors() {
+ // Invented key outside any known camel.* group: checked=false AND
valid=false.
+ // Before the fix, valid was true despite accepted=false (CAMEL-25538
bug #2).
+ // The key must not appear in summary.errors — it is unchecked, not
rejected.
+ ConfigurationValidateTools tools = createTools();
+
+ ConfigurationValidateTools.ConfigurationValidateResult result
+ =
tools.camel_configuration_validate("camel.invented.completely.unknown=foo",
null, null, null);
+
+ ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
+ assertThat(info.checked()).isFalse();
+ assertThat(info.valid()).isFalse();
+ // The summary must not count unchecked keys as valid or as errors
+ assertThat(result.summary().valid()).isZero();
+ assertThat(result.summary().errors()).isZero();
+ assertThat(result.summary().unchecked()).isEqualTo(1);
+ }
+
+ @Test
+ void camelManagementKeyIsCheckedAndValid() {
+ // camel.management.* was also missing from the old hardcoded list.
+ ConfigurationValidateTools tools = createTools();
+
+ ConfigurationValidateTools.ConfigurationValidateResult result
+ =
tools.camel_configuration_validate("camel.management.enabled=true", null, null,
null);
+
+ ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
+ assertThat(info.checked()).isTrue();
+ assertThat(info.valid()).isTrue();
+ }
+
+ @Test
+ void camelVaultAwsKeyIsCheckedAndValid() {
+ // camel.vault.aws.* is a nested group (3-segment name) — must also be
accepted.
+ // The dot-offset logic that skips the group prefix is exercised here.
+ ConfigurationValidateTools tools = createTools();
+
+ ConfigurationValidateTools.ConfigurationValidateResult result
+ =
tools.camel_configuration_validate("camel.vault.aws.accessKey=AKIA1234", null,
null, null);
+
+ ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
+ assertThat(info.checked()).isTrue();
+ assertThat(info.valid()).isTrue();
Review Comment:
💡 `camelVaultAwsKeyIsCheckedAndValid` asserts `checked=true` and
`valid=true` but has no assertion on `issues()`. Since this test is
specifically validating a nested group key that exercises the dot-offset logic,
asserting `info.issues().isEmpty()` would guard against silent validation
errors that happen to leave `valid=true`.
--
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]
