CAMEL-24164: camel-jaxb - Fix CamelJaxbPartClass header state pollution#24813
Conversation
…on in JaxbDataFormat Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
LGTM — important concurrency/state-pollution fix.
The bug: JaxbDataFormat is shared across all exchanges in a route step. Both unmarshal() and toElement() write the header-derived CamelJaxbPartClass value directly into the instance field this.partClass. This causes:
- Stickiness — once any exchange carries the header,
partClassstays non-null forever, silently using the leaked class for all subsequent exchanges without the header. - Data race — concurrent exchanges with different headers cross-contaminate through the shared field.
The fix: Introduces a local effectivePartClass variable initialized from this.partClass, overridden from the header when present. The instance field is never mutated. This matches the established pattern used by Jackson, CBOR, Gson, and Jsonb data formats.
Note: partNamespace is already safe — it's read into a local via getPartNamespace() and the header override mutates the method parameter partNamespaceOnDataFormat, not the instance field.
The test: Excellent coverage — sends Exchange 1 with CamelJaxbPartClass header, then Exchange 2 without, asserting no state leaks in both unmarshal and marshal paths. The assertions are specific (assertInstanceOf(PurchaseOrder.class, ...) / assertFalse(payload2.contains("<address:address"))) — directly proving the absence of pollution.
Bug has been present since 2017 (CAMEL-5723). Nice catch.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 59 tested, 29 compile-only — current: 59 all testedMaveniverse Scalpel detected 88 affected modules (current approach: 59).
|
oscerd
left a comment
There was a problem hiding this comment.
LGTM — real cross-exchange contamination fix. Mutating the shared this.partClass field per request meant a CamelJaxbPartClass header set for one exchange stuck around for the next (and raced under concurrency). Replacing it with a local effectivePartClass threaded through both toElement/marshal and the unmarshal path is logically equivalent across all configured×header combinations (header still takes precedence, full-vs-partial selection preserved) and eliminates the stickiness. The two-exchange isolation test (header on exchange 1, none on exchange 2, asserting exchange 2's payload isn't partial) targets exactly the deterministic manifestation. Clean.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Summary
Claude Code on behalf of Croway
The
JaxbDataFormat.unmarshal()andtoElement()methods write the header-derivedCamelJaxbPartClassvalue directly into the shared instance fieldthis.partClass. Since aDataFormatis shared across all exchanges in a route step, this causes:partClassremains non-null permanently — all subsequent exchanges without the header silently unmarshal/marshal using the leaked class.The fix uses a local variable (
effectivePartClass) instead of mutating the instance field, matching the pattern already used by Jackson, CBOR, Gson, and Jsonb data formats.The bug was introduced in 2017 (CAMEL-5723, commit
03519184b36a) and carried through the 2024 refactoring (commitf0b4bb237331).Changes
JaxbDataFormat.java: Replacethis.partClass = ...with localeffectivePartClassin bothunmarshal()andtoElement().JaxbPartClassHeaderStatePollutionTest.java: Two test cases (unmarshal + marshal) that send Exchange 1 with header, then Exchange 2 without, asserting no state leaks.Test plan
JaxbPartClassHeaderStatePollutionTestcovers both unmarshal and marshal pathsmvn verifyincomponents/camel-jaxb)JaxbDataFormatPartClassHeaderTestandJaxbDataFormatPartClassTeststill pass (header-driven and configured partClass paths)🤖 Generated with Claude Code