CAMEL-24052: Fix model dumpers permanently destroying EndpointDSL builders on RouteDefinition#24662
Open
Croway wants to merge 1 commit into
Open
CAMEL-24052: Fix model dumpers permanently destroying EndpointDSL builders on RouteDefinition#24662Croway wants to merge 1 commit into
Croway wants to merge 1 commit into
Conversation
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
approved these changes
Jul 13, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Summary: This PR fixes a real and impactful bug where model dumpers (XML, YAML, Java DSL) permanently destroyed EndpointConsumerBuilder / EndpointProducerBuilder references on live RouteDefinition objects. The most common trigger was AdviceWith.adviceWith() with the default logXml=true, but any JMX dumpRoutesAsXml() call would hit the same bug. Good work identifying the root cause and tracing it through all four dumper implementations.
Fix analysis:
I verified the clear() / setUri() behavior in the model classes:
FromDefinition.setUri()callsclear(), which nullsendpointConsumerBuilder,endpoint, anduri-- confirmed destructive.SendDefinition.setUri()callsclear(), which nullsendpointProducerBuilder,endpoint,uri, andendpointUriToString-- confirmed destructive.ToDynamicDefinition.setUri()does NOT callclear(), just setsuri-- the builder is preserved. The PR correctly usesrestorers.add(() -> to.setUri(null))instead of restoring the builder, which is the right approach here.
The restore logic is correct: setEndpointConsumerBuilder(builder) and setEndpointProducerBuilder(builder) both call clear() first (nulling the temporarily-set URI), then set the builder back to the original reference. The net effect is a clean restoration to the pre-dump state.
The finally blocks ensure restoration happens even if serialization throws an exception.
Observations (non-blocking):
-
The import order in
JaxbHelper.javaplacesorg.apache.camel.builder.*afterorg.apache.camel.model.ExpressionNode, which breaks alphabetical ordering. Please runmvn impsort:sorton thecamel-xml-jaxbmodule if the build's sourcecheck flags it. -
JaxbHelper.resolveEndpointDslUrisis apublic staticmethod whose return type changed fromvoidtoRunnable. The internal callers inmodelToXml()(lines 355, 363, 371, 378) now discard the returnedRunnable. This is harmless -- those call sites process freshly deserialized routes that have no EndpointDSL builders set -- but worth noting for future awareness. -
The
resolveEndpointDslUrisimplementation is duplicated across all four dumper classes. This is a pre-existing code smell, not introduced by this PR. -
There is still a narrow thread-safety window where the live
RouteDefinitionis temporarily mutated between URI resolution and serialization. This is a pre-existing concern that this PR cannot fully address without a larger refactoring (e.g., working on copies of the definitions). The PR does improve the situation by restoring state afterward.
Tests: Three well-structured test methods covering: (1) direct XML dumper invocation with FromDefinition, (2) adviceWith() integration with FromDefinition, and (3) adviceWith() integration with SendDefinition (producer builder). The route uses a non-primitive ExceptionHandler property to reproduce the hash=XXXXXXXX URI scenario. Good test design.
Scanner coverage: No static analysis tools available in this environment. Manual code review performed against the model source code (FromDefinition, SendDefinition, ToDynamicDefinition) to verify correctness of save/restore semantics. CI checks (build for JDK 17 and JDK 25) are still pending at review time.
This review does not replace specialized review tools such as CodeRabbit, Sourcery, or static analyzers such as SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| import org.apache.camel.converter.jaxp.XmlConverter; | ||
| import org.apache.camel.model.BasicExpressionNode; | ||
| import org.apache.camel.model.ExpressionNode; | ||
| import org.apache.camel.builder.EndpointConsumerBuilder; |
Contributor
There was a problem hiding this comment.
Minor: these two imports should be placed before the org.apache.camel.model.* group to maintain alphabetical order within the org.apache.camel package. Running mvn impsort:sort in the camel-xml-jaxb module should fix this automatically.
davsclaus
reviewed
Jul 13, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Review: CAMEL-24052 — Fix model dumpers permanently destroying EndpointDSL builders
Nice fix — the analysis of the setUri() → clear() destruction chain is thorough, and the save/restore pattern correctly handles the different semantics across FromDefinition, SendDefinition, and ToDynamicDefinition.
One import-ordering issue to fix before merge (details in inline comment). Otherwise this looks good.
Positive observations:
- Restorer logic correctly distinguishes
FromDefinition/SendDefinition(wheresetUri()callsclear(), destroying the builder) fromToDynamicDefinition(wheresetUri()does not callclear(), so only the URI needs resetting) finallyblocks ensure restoration even if serialization throwsWireTapDefinition extends ToDynamicDefinitionis properly covered byfilterTypeInOutputs- Tests cover the three key scenarios: XML dump directly,
adviceWithfor consumer builder,adviceWithfor producer builder - Non-primitive
ExceptionHandlerproperty reproduces thehash=XXXXXXXXURI behavior well
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
| import org.apache.camel.model.BasicExpressionNode; | ||
| import org.apache.camel.model.ExpressionNode; | ||
| import org.apache.camel.builder.EndpointConsumerBuilder; | ||
| import org.apache.camel.builder.EndpointProducerBuilder; |
Contributor
There was a problem hiding this comment.
The org.apache.camel.builder.* imports are placed after org.apache.camel.model.ExpressionNode, but alphabetically they belong between org.apache.camel.TypeConversionException and org.apache.camel.converter.jaxp.XmlConverter. This will fail mvn impsort:sort in CI.
The other three dumper files have the imports correctly placed — only this file needs fixing.
Running mvn impsort:sort in the core/camel-xml-jaxb module should auto-fix it.
…lders on RouteDefinition resolveEndpointDslUris() in LwModelToXMLDumper, JaxbModelToXMLDumper (via JaxbHelper), LwModelToYAMLDumper, and LwModelToJavaDumper called setUri() on live FromDefinition and SendDefinition objects before serialization. setUri() internally calls clear(), which permanently nulls the endpointConsumerBuilder/endpointProducerBuilder fields, causing Camel to lose pre-configured bean references (e.g. custom S3Client instances) and fall back to default unconfigured clients. Fix: resolveEndpointDslUris() now saves builder references before calling setUri() and returns a Runnable that restores them. Callers invoke the restorer in a finally block after serialization completes.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 476 tested, 28 compile-only — current: 475 all testedMaveniverse Scalpel detected 504 affected modules (current approach: 475).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resolveEndpointDslUris()in all four model dumpers (LwModelToXMLDumper,JaxbModelToXMLDumperviaJaxbHelper,LwModelToYAMLDumper,LwModelToJavaDumper) calledsetUri()on liveFromDefinitionandSendDefinitionobjects before serialization.setUri()internally callsclear(), which permanently nullsendpointConsumerBuilder/endpointProducerBuilder. For endpoints with non-primitive builder properties (e.g. a customS3Client), this causes Camel to lose the pre-configured bean reference and fall back to a default unconfigured client, resulting in route startup failures.The most common trigger is
AdviceWith.adviceWith()with the defaultlogXml=true, but any JMXdumpRoutesAsXml()call or startup dump on a context using Endpoint DSL hits the same bug.Changes
resolveEndpointDslUris()now saves builder references before callingsetUri()and returns aRunnablethat restores themfinallyblock after serialization completesAdviceWithEndpointDslMutationTestincamel-endpointdslcovering the XML dumper directly and throughadviceWith()for bothFromDefinitionandSendDefinitionbuildersRelated
Claude Code on behalf of Croway