oscerd commented on code in PR #27409:
URL: https://github.com/apache/camel/pull/27409#discussion_r4193653024
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -3020,7 +3020,7 @@ protected void autoConfigurationFromProperties(
// load properties from ENV (override existing)
if
(mainConfigurationProperties.isAutoConfigurationEnvironmentVariablesEnabled()) {
Map<String, String> env = MainHelper
- .filterEnvVariables(new String[] { "camel.component.",
"camel.dataformat.", "camel.language." });
+ .filterEnvVariables(new String[] { "CAMEL_COMPONENT_",
"CAMEL_DATAFORMAT_", "CAMEL_LANGUAGE_" });
Review Comment:
**`autoConfigurationFailFast=false` does not stop the startup failures this
line makes reachable**
The Upgrade Note says a stray variable fails the startup *unless
`autoConfigurationFailFast=false`*, but the flag only reaches
`setPropertiesOnTarget`, so it only covers an unknown option on a target that
resolves. The two most likely stray-variable cases fail no matter how it is set:
- **A name that does not resolve**: the supplier lambdas in
`doAutoConfigurationFromProperties` throw `IllegalArgumentException("... cannot
find component|dataformat|language with name ...")` unconditionally. Example:
`CAMEL_COMPONENT_KAFKA_BROKERS` injected through a shared `envFrom` ConfigMap
into an application that does not ship camel-kafka.
- **An empty value**: `MainHelper.computeProperties` calls
`validateOptionAndValue`, which throws `... because value is empty`. Empty
variables are common (`X=${UNSET}` in compose, `value: ""` in a pod spec), and
by then `putAllOverride` has already removed the matching
`application.properties` key.
The same keys in a properties file fail the same way, so the behaviour is
consistent. But since these variables were silently ignored since 3.9, this is
what existing deployments will hit after upgrading, and the only switch is
`autoConfigurationEnvironmentVariablesEnabled=false`, which also turns off
`CAMEL_MAIN_*`. The upgrade-guide entry should not offer
`autoConfigurationFailFast=false` as the way out. Skipping (with a WARN) names
from the ENV source that do not resolve, or a warn-only mode, would give a
targeted opt-out.
##########
core/camel-main/src/test/java/org/apache/camel/main/MainHelperTest.java:
##########
@@ -87,4 +87,68 @@ public void testAddCustomComponentEnvVariables() {
Assertions.assertEquals("false",
prop.getProperty("camel.component.foo.pretty-print"));
}
+ /**
+ * Verifies that overlapping component names (e.g. NETTY vs NETTY_HTTP,
SJMS vs SJMS2, FILE vs FILE_WATCH) are
+ * resolved by longest-match so that each variable is mapped to the
correct component.
+ */
+ @Test
+ void testLongestMatchComponentEnvVariables() {
+ Map<String, String> env = MainHelper.filterEnvVariables(new String[] {
"CAMEL_COMPONENT_" });
+ // netty-http must not be mapped to netty
+ env.put("CAMEL_COMPONENT_NETTY_HTTP_MUTE_EXCEPTION", "true");
+ // sjms2 must not be mapped to sjms
+ env.put("CAMEL_COMPONENT_SJMS2_RECOVERY_INTERVAL", "2000");
+ // file-watch must not be mapped to file
+ env.put("CAMEL_COMPONENT_FILE_WATCH_POLL_TIMEOUT", "5000");
Review Comment:
**Some test variables use options that do not exist**
Three of the test variables use options that do not exist: `file-watch` has
no `pollTimeout` (it has `pollThreads`, `queueSize`, ...), `avro` has
`instanceClassName` rather than `schemaClassName`, and `avroJackson` has no
`prettyPrinter`. The assertions only check the mapped key, so the tests pass.
But these are the only examples of the variable form in the PR, and set for
real (or reused in the end-to-end test asked for in the existing thread) they
would fail the startup. Real options keep the same coverage, e.g.
`CAMEL_COMPONENT_FILE_WATCH_QUEUE_SIZE` and
`CAMEL_DATAFORMAT_AVRO_INSTANCE_CLASS_NAME`.
##########
core/camel-main/src/main/java/org/apache/camel/main/MainHelper.java:
##########
@@ -476,6 +474,21 @@ public static void validateOptionAndValue(String key,
String option, String valu
}
}
+ /**
+ * Returns the longest name from {@code names} such that {@code envVarKey}
starts with that name followed by
+ * {@code _}, or {@link Optional#empty()} if no such name exists.
+ * <p>
+ * Using the longest match is critical when the catalog contains prefix
pairs such as {@code CAMEL_COMPONENT_NETTY}
+ * / {@code CAMEL_COMPONENT_NETTY_HTTP} or {@code CAMEL_LANGUAGE_JS} /
{@code CAMEL_LANGUAGE_JSONPATH}: a simple
+ * {@code findFirst()} on a {@link java.util.HashSet} gives a
non-deterministic result, whereas the longest match
+ * always selects the most specific name.
+ */
+ private static Optional<String> longestMatch(Set<String> names, String
envVarKey) {
+ return names.stream()
+ .filter(name -> envVarKey.startsWith(name + "_"))
+ .max(Comparator.comparingInt(String::length));
Review Comment:
**Longest match still splits `rest.apiDoc` (and custom-named instances) at
the wrong place**
Longest match fixes the hash-order dependence, but the underscore form still
cannot show where the name ends, and there is one real collision in the catalog
today. `rest` has the component option `apiDoc`, while `rest-api` is a
component of its own (options: `bridgeErrorHandler`, `consumerComponentName`,
`autowiredEnabled`). So `CAMEL_COMPONENT_REST_API_DOC=api.json` becomes
`camel.component.rest-api.doc`, an unknown option. An extra instance registered
under a name that starts with a catalog name is split the same way (`kafka-dr`
→ `CAMEL_COMPONENT_KAFKA_DR_BROKERS` becomes
`camel.component.kafka.dr-brokers`). A heuristic is fine here, since only the
names are known, but the docs could state the limitation and the workarounds:
`CAMEL_COMPONENT_REST_APIDOC` binds because the option is matched ignoring
case, or use `camel.component.rest.apiDoc={{env:...}}`.
--
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]
