davsclaus commented on code in PR #27841:
URL: https://github.com/apache/camel/pull/27841#discussion_r4284330376
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/ConfigurationValidateTools.java:
##########
@@ -106,8 +106,12 @@ public ConfigurationValidateResult
camel_configuration_validate(
private PropertyValidationInfo toPropertyValidationInfo(
int lineNumber, String text,
ConfigurationPropertiesValidationResult result) {
- boolean valid = result.isSuccess();
boolean accepted = result.isAccepted();
+ // A key that is not accepted by the catalog is by definition not
valid: the catalog
+ // returned no errors (because it returned early without checking
anything), but the
+ // property is outside the set of known camel.* groups, so reporting
valid=true would
+ // be misleading to the caller.
+ boolean valid = accepted && result.isSuccess();
Review Comment:
This makes `server.port=8080`, `quarkus.http.port`, `camel.beans.*` and
`camel.jbang.*` read as rejected. CAMEL-25538 item 4 asks for these to read as
"not checked": keep `valid = result.isSuccess()` for checked lines, report a
`checked` flag (or rename `accepted`), leave unchecked lines out of
`summary.valid`, add `summary.unchecked`, and describe it in the `@Tool`
description.
##########
core/camel-core-catalog/src/main/java/org/apache/camel/catalog/impl/AbstractCamelCatalog.java:
##########
@@ -1408,20 +1414,46 @@ private static boolean isValidSuffix(String suffix) {
return suffix == null || suffix.isEmpty() || suffix.equals(".");
}
- private static boolean acceptConfigurationPropertyKey(String key) {
+ private boolean acceptConfigurationPropertyKey(String key) {
if (key == null) {
return false;
}
- return key.startsWith("camel.component.")
+ // 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
+ MainModel model = mainModel();
+ if (model != null) {
+ for (MainModel.MainGroupModel group : model.getGroups()) {
+ String groupPrefix = group.getName() + ".";
+ if (key.startsWith(groupPrefix)) {
Review Comment:
Case- and dash-sensitive. camel-main binds `camel.error-registry.*`,
`camel.ai-observability.*` and `camel.telemetry-dev.*` (`MainHelper.optionKey`
and `startsWithIgnoreCase`), but these keys are not accepted here. Comparing
normalised forms would match the runtime, for example `key.replace("-",
"").toLowerCase(Locale.ENGLISH).startsWith(group.getName().replace("-",
"").toLowerCase(Locale.ENGLISH) + ".")`, and the same in `matchMainModelGroup`.
##########
dsl/camel-jbang/camel-jbang-mcp/src/test/java/org/apache/camel/dsl/jbang/core/commands/mcp/ConfigurationValidateToolsTest.java:
##########
@@ -180,6 +180,83 @@ void nonCamelKeyMarkedNotAccepted() {
ConfigurationValidateTools.PropertyValidationInfo info =
result.properties().get(0);
assertThat(info.accepted()).isFalse();
+ // accepted=false must imply valid=false (regression guard for
CAMEL-25538 bug #2)
+ assertThat(info.valid()).isFalse();
+ }
+
+ // --- 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.accepted()).isTrue();
+ assertThat(info.valid()).isTrue();
+ assertThat(info.errors()).isZero();
+ assertThat(info.issues()).isEmpty();
+ }
+
+ @Test
+ void camelServerInventedKeyIsAcceptedButNotValid() {
+ // 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.accepted()).isTrue();
+ assertThat(info.valid()).isFalse();
+ assertThat(info.issues()).anyMatch(i -> "unknown".equals(i.kind()));
+ }
+
+ @Test
+ void acceptedFalseImpliesValidFalseForNonCamelKey() {
+ // Invented key outside any known camel.* group: accepted=false AND
valid=false.
+ // Before the fix, valid was true despite accepted=false (CAMEL-25538
bug #2).
+ 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.accepted()).isFalse();
+ assertThat(info.valid()).isFalse();
+ // The summary must not count unaccepted keys as valid
+ assertThat(result.summary().valid()).isZero();
+ }
+
+ @Test
+ void camelManagementKeyIsAccepted() {
+ // 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.accepted()).isTrue();
+ assertThat(info.valid()).isTrue();
+ }
+
+ @Test
+ void camelVaultAwsKeyIsAccepted() {
+ // camel.vault.aws.* is a nested group (3-segment name) — must also be
accepted.
+ 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.accepted()).isTrue();
Review Comment:
Please also assert `valid`, and cover an invented option under a nested
group, which is where the new dot-offset logic matters:
```suggestion
assertThat(info.accepted()).isTrue();
assertThat(info.valid()).isTrue();
```
##########
core/camel-core-catalog/src/main/java/org/apache/camel/catalog/impl/AbstractCamelCatalog.java:
##########
@@ -1408,20 +1414,46 @@ private static boolean isValidSuffix(String suffix) {
return suffix == null || suffix.isEmpty() || suffix.equals(".");
}
- private static boolean acceptConfigurationPropertyKey(String key) {
+ private boolean acceptConfigurationPropertyKey(String key) {
if (key == null) {
return false;
}
- return key.startsWith("camel.component.")
+ // 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
+ MainModel model = mainModel();
Review Comment:
On a catalog without cache (`ValidateMojo` uses `new DefaultCamelCatalog()`)
this parses the 229 KB main metadata for every line, including keys outside
Camel, and again in `matchMainModelGroup` and the main branch. Returning early
with `if (!key.startsWith("camel.")) return false;` and loading the model once
in `validateConfigurationProperty` avoids that. The "camel-main not detected"
incapable branch can then no longer be reached, as a matched group implies a
model.
--
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]
