CAMEL-24900: camel-jcr - apply header filtering when mapping node properties to Exchange headers - #26748
Conversation
…perties to Exchange headers The JCR producer's CamelJcrGetById operation mapped every property of the retrieved node onto the Exchange without a HeaderFilterStrategy, and the insert path persisted all message headers (bar three JCR control keys) as node properties. camel-jcr defined no HeaderFilterStrategy, so Camel internal header names (e.g. CamelHttpUri) were copied through unfiltered in both directions. Make JcrEndpoint HeaderFilterStrategyAware with a default DefaultHeaderFilterStrategy, which filters names starting with Camel/camel case-insensitively. getById now runs each property name through applyFilterToExternalHeaders before setHeader, and insert runs each header through applyFilterToCamelHeaders; ordinary document properties are preserved. Adds JcrGetNodeByIdHeaderInjectionTest and an upgrade-guide note. Continues the header-filtering alignment done for camel-coap (CAMEL-24655). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Two issues to address before merge — one missing test coverage for the INSERT path, one misleading consumer DSL API.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
oscerd
added
port/camel-4.22.x
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
This is a well-executed instance of exactly the pattern the committer checklist in CLAUDE.md asks for, and the details that are easy to get wrong are right here. Specifically, I checked:
- Filter directions are correct —
applyFilterToCamelHeaderson the insert path (Camel → JCR) andapplyFilterToExternalHeadersongetById(JCR → Camel). Getting these backwards is the usual bug in this family and they're the right way round. - The default actually bites —
DefaultHeaderFilterStrategyinitialises bothinFilterStartsWithandoutFilterStartsWithto{"Camel", "camel"}withcaseInsensitive = trueandfilterOnMatch = true, so the four-casing test is testing something real rather than passing by accident. - No second inbound path was missed —
JcrProducer:84is the onlysetHeadercall in the component's main source, sogetByIdreally is the whole surface.
All 14 tests pass on the branch. Comments below are a test gap and some tidying; nothing blocking.
Main point: the insert-path filtering is documented in the upgrade guide but has no test — see the inline comment.
This review does not replace CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 693 tested, 24 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (4 direct + 8 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (24)
All tested modules (36 modules, 5m 34s total)Total reactor time: 5m 34s
Top 20 slowest modules:
|
- Add JcrInsertHeaderInjectionTest covering the insert-path filter: a Camel-prefixed header (four casings) is not persisted as a node property, while an ordinary header still is. - Scope the headerFilterStrategy option to "producer,filter". It is only used by the producer, so the previously generated consumer DSL overloads (a dead API) are removed. - Initialise headerFilterStrategy at its declaration to avoid a benign data race in the lazy getter, which is now a plain accessor. - Document why filterComponentHeaders is retained: it strips the JCR control keys unconditionally, independent of a custom HeaderFilterStrategy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Re-reviewed 0e8c49a (the follow-up since my review on 22 Sep). All four points are addressed, and the two judgement calls went the way I'd have chosen.
Addressed
- Insert-path test —
JcrInsertHeaderInjectionTestcloses the gap that was my main point, and it tests something real:filterComponentHeadersonly strips the three JCR control keys, so nothing but theHeaderFilterStrategycan be makingCamelHttpUridisappear. Mirroring the four casings from thegetByIdtest is the right symmetry, and assertingmy.contents.propertyis still stored keeps it honest about not over-filtering. - Lazy init in the getter — gone, field initialised at the declaration, getter is now a plain accessor. Race removed.
filterComponentHeaders— kept with a comment explaining exactly the custom-strategy case I raised. That was the alternative I said was defensible, and the comment means it no longer reads as dead code.- Dead null checks in
JcrProducer— see the note below; they are no longer dead, which is fine.
Two notes, neither blocking:
🟡 The catalog still doesn't tell anyone that filtering is on by default. This was the second half of my JcrEndpoint comment and the field initialiser can't fix it — the package plugin only emits defaultValue for literal constants, so an object-typed option never gets one. That leaves the upgrade guide as the only place the new default is written down; a user reading the component docs sees an option that looks unset. Worth folding into the description on the @UriParam, e.g. "... By default a DefaultHeaderFilterStrategy is used, which filters headers starting with Camel or camel." It's a regen, so only worth doing if you're touching the branch anyway.
🔵 The null guards changed meaning rather than becoming dead. With the lazy init gone, setHeaderFilterStrategy(null) now sticks instead of being healed by the next getHeaderFilterStrategy() call, so the != null / == null checks in process() are live again — and an explicit null silently disables the filtering this PR adds. No NPE, and treating null as "opt out entirely" is coherent with supplying a permissive custom strategy, so I'm not asking for a change. Flagging it only because it's the opposite of what my original nit assumed.
The label narrowing to producer,filter is correct and costs nothing — I checked main and headerFilterStrategy isn't an endpoint option on jcr at all today, so dropping the consumer builder methods removes nothing that was ever released.
From my side this is ready; I've left it as a comment rather than an approval so the sign-off is a human one.
Claude Code on behalf of davsclaus
This review does not replace CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Resolve upgrade-guide conflict in camel-4x-upgrade-guide-4_23.adoc by keeping both the camel-jcr and camel-mustache/camel-chunk sections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Both issues from the previous review are addressed — approving.
Resolved:
- ✅ Insert-path test —
JcrInsertHeaderInjectionTestcloses the coverage gap: four casing variants ofCamelHttpUriare sent on the insert exchange, the stored node is read back, andnode.hasProperty(variant)is assertedfalsewhilemy.contents.propertyis still present. MirrorsJcrGetNodeByIdHeaderInjectionTestsymmetrically. - ✅
@UriParamlabel — changed to"producer,filter". The regenerated DSL factory no longer emitsheaderFilterStrategy()onJcrEndpointConsumerBuilder; the catalog JSON records"label": "producer,filter"as expected.
One minor item not addressed (non-blocking, already noted by davsclaus):
- 🟡 The
@UriParamdescriptionfield still reads"To use a custom org.apache.camel.spi.HeaderFilterStrategy to filter header to and from Camel message."— it doesn't tell a user reading the component reference thatDefaultHeaderFilterStrategy(filteringCamel/camel-prefixed headers) is active by default. Since this is the whole behavioural change this PR introduces, a user scanning the option table has no hint that filtering is on unless they read the 4.23 upgrade guide. Worth folding into the description on a follow-up touch, e.g."... By default, a DefaultHeaderFilterStrategy is used, which filters headers whose names start with 'Camel' or 'camel'."
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Thanks both. Addressed the round in
Also merged Ready for another look. Claude Code on behalf of Andrea Cosentino (@oscerd) |
|
Thanks for the thorough re-review @davsclaus. On the two non-blocking notes:
Claude Code on behalf of Andrea Cosentino (@oscerd) |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Merge-only update since my last look. The jcr changes are unchanged and the upgrade-guide entry still renders correctly. The catalog-description wording (saying DefaultHeaderFilterStrategy is used by default) is still a nice-to-have, not a blocker. LGTM.
Claude Code on behalf of davsclaus
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
🔄 Backport BotConflicts (🤖 agent dispatched to resolve):
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs:
|
gnodet
removed
port/camel-4.22.x
…ng when mapping node properties to Exchange headers (#27043) The JCR producer applies a HeaderFilterStrategy when mapping between JCR node properties and Exchange headers (default DefaultHeaderFilterStrategy, filtering Camel/camel case-insensitively), so a node property cannot inject a Camel-internal header and Camel-internal headers are not persisted as node properties. Backport of #26748 to camel-4.22.x. The upgrade-guide entry lives on main only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ng when mapping node properties to Exchange headers (#27047) Backport of #26748 to camel-4.18.x. On this release line DefaultHeaderFilterStrategy does not filter Camel/camel headers by default (that default arrived in CAMEL-23543, on 4.21.x and later), so the JCR endpoint configures the filter explicitly to keep the same behaviour. The upgrade-guide entry lives on main only and is not included here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
5 participants
Motivation
The
jcrproducer'sCamelJcrGetByIdoperation mapped every property of the retrieved node onto the Exchange viamessage.setHeader(property.getName(), ...)without running the names through aHeaderFilterStrategy, and the insert path persisted all message headers (bar three JCR control keys) as node properties. Unlike most components,camel-jcrdefined noHeaderFilterStrategy, so Camel internal header names (e.g.CamelHttpUri) were copied through unfiltered in both directions.This continues the header-filtering alignment done for
camel-coap(CAMEL-24655).Jira: https://issues.apache.org/jira/browse/CAMEL-24900
Changes
JcrEndpointis nowHeaderFilterStrategyAware, exposing aheaderFilterStrategyendpoint option that defaults toDefaultHeaderFilterStrategy(filters names starting withCamel/camel, case-insensitively).CamelJcrGetByIdruns each property name throughapplyFilterToExternalHeadersbeforesetHeader; ordinary document properties are preserved.applyFilterToCamelHeaders, so Camel internal headers carried on the message are no longer persisted as node properties.JcrGetNodeByIdHeaderInjectionTest: aCamel-prefixed property name, in four casings, is not mapped onto the Exchange, while an ordinary document property still is.Testing
mvn -f components/camel-jcr/pom.xml test— all 14 tests pass, including the new injection test and the existing insert /getByIdround-trip tests.Claude Code on behalf of Andrea Cosentino (@oscerd)
🤖 Generated with Claude Code