Repository navigation
CAMEL-25533: camel-jbang - apply camel.server.* settings to the HTTP server started for platform-http - #27667
Conversation
…server started for platform-http camel run starts an embedded HTTP server when a route uses platform-http, also when camel.server.enabled is not set, but created it with the default configuration: only --port was taken (from the run settings), so camel.server.port, host, path and the other camel.server.* settings of application.properties were ignored without any message, while the auto-configuration summary listed them. camel-main binds them only on the server it starts itself, when camel.server.enabled=true. The implicit server now binds the camel.server.* properties the same way, with placeholders resolved; --port keeps precedence, and the run settings record the port actually used.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid bug fix that correctly mirrors the property-binding pattern from BaseMainSupport into the implicit platform-http server path. The property loading and binding logic looks correct — loadProperties with MainHelper::optionKey as the key mapper, followed by setPropertiesOnTarget, matches what camel-main does when the server is explicitly enabled.
One minor cleanup item below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| OrderedLocationProperties options = new OrderedLocationProperties(); | ||
| for (Object k : prop.keySet()) { | ||
| String key = k.toString(); | ||
| if (startsWithIgnoreCase(key, PREFIX_SERVER)) { |
There was a problem hiding this comment.
💡 Suggestion (minor): The startsWithIgnoreCase(key, PREFIX_SERVER) guard inside this loop appears to be dead code.
Both sources of keys in prop start with camel.server.:
loadProperties(name -> startsWithIgnoreCase(name, PREFIX_SERVER), MainHelper::optionKey)— the filter ensures it, andoptionKey(dash-to-camelCase) preserves thecamel.server.prefix- The JVM property merge just above also filters on
PREFIX_SERVERbefore inserting intoprop
The if is never false, so it and its outer } can be removed — simplifying the loop to just the inner if (!"enabled"...) guard.
There was a problem hiding this comment.
Removed in e19f3db: every source is already filtered on camel.server., so the loop now only skips enabled.
Claude Code on behalf of Croway
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 25 (1 retried attempt) Recovered flaky tests on JDK 25 (1)
🔬 Scalpel shadow comparison — Scalpel: 6 of 704 tested, 9 compile-only — current: 6 all testedMaveniverse Scalpel detected 6 affected modules (current approach: 6). Skip-tests mode would test 6 modules (1 direct + 6 downstream), skip tests for 9 (generated code, meta-modules) Modules Scalpel would test (6)
Modules with tests skipped (9)
All tested modules (16 modules, 2m 35s total)Total reactor time: 2m 35s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the fix is correct and safe: only explicit user settings are honoured, --port still wins, and no default is relaxed.
- +1 to gnodet-bot's open thread on the
startsWithIgnoreCase(key, PREFIX_SERVER)guard: both sources are already filtered by the prefix, so it's dead code and the thread should be resolved before merge. - Env vars gap (inline).
- Optional design idea: camel-main already binds all
camel.server.*ontomainConfigurationProperties.httpServer()before returning early whenenabledis false (BaseMainSupport.setHttpServerProperties), covering env, JVM props, placeholders and fail-fast. KameletMain could hand that already bound object toMainHttpServerFactoryinstead of re-binding here. - Question: with
camel.server.enabled=true, camel-main setsuseGlobalSslContextParameters=trueautomatically when a global SSL context exists. The implicit server doesn't. Intended? (Not a regression.)
Remember to sync any follow-up commit into the 4.22.x backport #27668.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying. It is a static review against the project conventions and does not replace static analysis or specialized review tools.
|
@Croway would be good to fix this so we can have it in 4.23.0 |
|
@davsclaus on it |
…nt variables to the HTTP server started for platform-http Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 participants
Description
CAMEL-25533
Under
camel run, a route that usesplatform-httpstarts an embedded HTTP server even whencamel.server.enabledis not set: when the component is resolved,MainHttpServerFactory.setupHttpServer(camel-kamelet-main) creates the server from a newHttpServerConfigurationPropertiesand only reads the port from the run settings (written for--port). camel-main binds thecamel.server.*properties onto its own configuration, butBaseMainSupport.setHttpServerPropertiesreturns early whencamel.server.enabledis not true, so that configuration is never used. The result:camel.server.port=9090inapplication.propertiesis ignored without any message, the server listens on 8080, and the auto-configuration summary still lists[application.properties] camel.server.port = 9090. The same goes forhost,path,maxBodySizeand the rest. We hit this while evaluating AI coding agents on migrations to Camel: they spent many tool calls looking for the port, some concluded that placeholders are not supported forcamel.server.port, and 5 of 10 trials ended up on the wrong port.Change: the implicitly started server binds the
camel.server.*properties (properties files, initial and override properties, thenCAMEL_SERVER_*OS environment variables and JVM system properties overriding them, as in camel-main) withMainHelper.setPropertiesOnTarget, as camel-main does forcamel.server.enabled=true, so placeholders are resolved.--portkeeps precedence, and the run settings now record the port actually used (socamel exportand the other commands see it).camel.server.enabledis not bound, the server is started because platform-http is in use, as before.Why not make
camel.server.portimplycamel.server.enabled=true: that would be in camel-main and change plain Camel Main applications, where the property alone does not start a server today, while the implicit server exists only undercamel run. Applying the settings to the server that is actually started is the least surprising, and keepscamel runwith and withoutcamel.server.enabled=trueconsistent.Tests:
MainHttpServerFactoryTest(new): default port without properties,camel.server.port/host/path/max-body-sizeapplied, placeholder in the port, nothing started in silent mode, a JVM system property overridingapplication.properties. The property tests fail without the change (8080). The environment variables were checked by running the test withCAMEL_SERVER_PATH=/env(the default-path test then fails with/env), as the module has no way to set an environment variable in a test.hello.camel.yaml(from: platform-http:/hello) and anapplication.propertiescontaining onlycamel.server.port=9090,camel run hello.camel.yaml application.propertieswith 4.22.1 logsVert.x HttpServer started on 0.0.0.0:8080; with the patched class it logs0.0.0.0:9090and only 9090 answers.camel.server.port={{app.port}}withapp.port=9092binds 9092.Target
mainbranch)Tracking
Apache Camel coding standards and style
camel-kamelet-main) and ran its tests; the build changes no generated or formatted files.