Repository navigation
CAMEL-25879: camel-infinispan - the key option of the producer takes precedence over the key header - #28174
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Federico, the docs section is a good addition. I'd rather not flip the precedence of the key in the producer, though:
- The rest of Camel uses the opposite rule. Across Camel, a header on the message overrides the endpoint option:
CamelAwsS3KeyoverkeyName(AWS2S3Utils.determineKey),CamelCaffeineKeyoverkey(CaffeineCacheProducer). In this producer,CamelInfinispanValue,CamelInfinispanOldValue,CamelInfinispanDefaultValueandCamelInfinispanCacheNamealso still win over their options. After this PR,keywould be the only exception, which is harder to explain than the current rule. - The current behaviour is deliberate. The
keyoption was added as an alternative to the header in CAMEL-15609 (c022730), together with...HeaderHavePriorities, a test that asserts the header wins. Routes that set a defaultkeyon the endpoint and override it per message would break without any error: the value would go to the endpoint key. The same happens whenInfinispanEmbeddingsDataTypeTransformersets the key header. The upgrade guide note helps, but nothing fails, so users may not notice. - Bridge routes already remove the inbound headers. Consumer to producer is the same case as s3 to s3 or file to file. The new docs section already shows the fix (
removeHeaders("CamelInfinispan*")). I'd keep that section, reworded to say that the header takes precedence over thekeyoption, together with its catalog copies, and drop the producer change, the test rename and the upgrade guide entry.
CI is still running on the current head.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
LGTM, thanks Federico. Approving, which replaces my earlier request for changes: this is a hardening. A route that forwards events from an Infinispan consumer to a producer with a fixed key must not let the event's CamelInfinispanKey header pick the entry, and the upgrade guide tells routes that relied on overriding key per message what to change. The earlier point about CAMEL-15609 choosing header-first stands as history only; the new precedence is the intended one.
Small nit, optional: "An infinispan consumer" / "an infinispan producer" in the new docs section.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean fix — centralising the key-resolution logic into a single getKey() helper eliminates 13 duplicated message.getHeader(…, configuration::getKey) call sites cleanly. The test coverage and upgrade guide look good.
One minor observation inline.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
115e869 to
a6e9070
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the grammar nit ("An `infinispan`" / "An `infinispan-embedded`") is fixed in the second commit. The Javadoc suggestion on `getKey()` is still open.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
LGTM, thanks Federico. Since my approval the only change is the "an infinispan" wording, in both component docs and their catalog copies. I resolved that thread.
The bot's suggestion to add Javadoc on getKey() is optional. The docs section already explains that key wins over its header and the cache-name header wins over the endpoint.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 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: 12 of 707 tested, 23 compile-only — current: 12 all testedMaveniverse Scalpel detected 12 affected modules (current approach: 12). Skip-tests mode would test 12 modules (7 direct + 9 downstream), skip tests for 23 (generated code, meta-modules) Modules Scalpel would test (12)
Modules with tests skipped (23)
All tested modules (39 modules, 5m 34s total)Total reactor time: 5m 34s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
The new key lookup (endpoint option first) reaches all 13 operations that read a key, and the renamed test covers both sides. Net-new: the new docs snippet fails as written, and the upgrade note could say that the cache-name header still wins. Nothing blocking.
Blockers
- None
Other findings: 4 (1 important, 2 suggestions, 1 small)
1. [Important] The new snippet PUTs a null value (components/camel-infinispan/camel-infinispan/src/main/docs/infinispan-component.adoc:217-219, components/camel-infinispan/camel-infinispan-embedded/src/main/docs/infinispan-embedded-component.adoc:231-233, and the catalog copies)
After removeHeaders("CamelInfinispan*"), neither the CamelInfinispanValue header nor the value option is set, so onPut calls cache.put("myKey", null) (InfinispanProducer.java:72,88). An embedded cache rejects that: in Infinispan 16.3.0, CacheImpl.put calls assertKeyValueNotNull ("Null values are not supported!"). The CAMEL-25879 example had a value, and adding it back makes the snippet runnable: .to("infinispan:target?operation=PUT&key=myKey&value=myValue");
2. [Suggestion] Upgrade note and the cache-name header (docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc:1982-1990)
The note explains why a consumer-to-producer route with a fixed key used the key of the event, which reads as if that route works now. It still writes into the consumer's cache: InfinispanConsumer.java:51 sets CamelInfinispanCacheName on every event, and InfinispanManager.java:34 / InfinispanEmbeddedManager.java:137 use that header before the cache of the endpoint. That is why the Jira example removes it. One more sentence would help people upgrading: "The CamelInfinispanCacheName header still takes precedence over the cache of the endpoint, so such a route must remove the CamelInfinispan* headers first, see Key and cache name headers in the component docs."
3. [Suggestion] Show the precedence where tooling reads it (InfinispanConfiguration.java:29, InfinispanConstants.java:36)
The descriptions of the key option ("Set a specific key for producer operations") and the CamelInfinispanKey header don't mention the new rule. These texts feed the option and header tables, the catalog, IDE completion and the Endpoint DSL Javadoc, and this pair now behaves differently from the producer's other option/header pairs. Adding something like "When set, it takes precedence over the CamelInfinispanKey header." to the key description would make that visible; the infinispan JSON, catalog and Endpoint DSL then need regenerating.
4. [Small] Placement of the 4.24 note (camel-4x-upgrade-guide-4_24.adoc:1982)
The note is appended at the end of the file, where other open PRs add their 4.24 notes too, so each of those merges would force a rebase here. Moving it between two existing notes avoids that.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
a6e9070 to
5426bc7
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
All previous review findings are addressed. ✅
getKey()Javadoc — Correctly documents the endpoint-key-first precedence and the deliberate asymmetry withgetCache()(which letsCamelInfinispanCacheNameheader win over the endpoint cache).- Docs snippet — PUT now includes both
key=myKeyandvalue=myValue, making it runnable on embedded caches. - Key option description — Updated in catalog/DSL JSON to note precedence over the
CamelInfinispanKeyheader. - Upgrade note — Enriched with the
CamelInfinispanCacheNamecaveat and moved to alphabetical position (between milo and protobuf). - Test — Renamed and updated to confirm key-option-wins / value-header-wins asymmetry.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Federico. Since my last approval (a6e9070) the branch was rebased, and the new commits address the open points from Andrea's review and from gnodet-bot:
getKey()now has a Javadoc that explains the endpoint-first precedence and the opposite rule ofgetCache()(header first).- The docs snippet sets
value=myValue, so the PUT is runnable on an embedded cache. - The
keyoption description says it takes precedence over theCamelInfinispanKeyheader. The component and catalog JSON, the Endpoint DSL and the Component DSL (the "Regen" commit) are regenerated. - The 4.24 upgrade-guide entry for this behaviour change is in place. It now also says that
CamelInfinispanCacheNamestill wins over the cache of the endpoint, and it sits between the milo and protobuf entries rather than at the end of the file.
The code change itself is the same as what I approved.
Conventions check: the "Regen" commit has no CAMEL-25879: prefix. The squash-merge message will take care of that. CI was still building when I looked; dependency-review, license-check and upload are green.
This review covers project rules and conventions only. It does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of Claus Ibsen
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…precedence over the key header The producer resolved the key of every operation from the CamelInfinispanKey header first and fell back to the key option of the endpoint, so the events of an Infinispan consumer, which set CamelInfinispanKey to the key of the event, overrode a key configured on the producer endpoint. The producer now uses the key option when it is set, and the header only when the endpoint does not set key. The component docs describe the precedence of the key and cache name headers and how to forward consumer events to a producer, and the 4.24 upgrade guide lists the change.
…, key precedence in the option description and the upgrade note - the docs snippet sets a value, since a PUT without one fails on an embedded cache - the key option description and the getKey() Javadoc state that key wins over the CamelInfinispanKey header - the upgrade note says the CamelInfinispanCacheName header still wins, and moves away from the end of the file
b13b2ed to
57b0228
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings are confirmed addressed in the current head.The only change since my last review is the Regen commit (regenerated DSL/catalog files from the updated key option description in InfinispanConfiguration.java). No logic changes.This review was generated by an AI agent, Hermès on behalf of @gnodet.
5 participants
CAMEL-25879
The
infinispanandinfinispan-embeddedproducers resolved the key of every operation asmessage.getHeader(InfinispanConstants.KEY, configuration::getKey), so theCamelInfinispanKeyheader won over thekeyoption of the endpoint. An Infinispan consumer sets that header to the key of each cache event, so a route forwarding events to a producer with a fixedkeyworked on the key of the event instead.Changes
InfinispanProducer.getKey(Message): thekeyoption when it is set, otherwise theCamelInfinispanKeyheader. Used by the 13 operations that read a key.infinispan,infinispan-embedded, and the catalog copies): new section "Key and cache name headers" on the precedence of the two headers, and on removing theCamelInfinispan*headers between an Infinispan consumer and producer.CamelInfinispanCacheNamestill takes precedence over the cache name of the endpoint.keyon the endpoint and override it per message with the header must removekeyfrom the endpoint.Tests
publishKeyAndValueByExplicitlySpecifyingTheKeyAndValueOptionsHeaderHavePriorities(shared by the embedded and remote producer tests) asserted the previous precedence. It is renamed...KeyOptionHasPriorityand now expects the put on the key of the endpoint, while theCamelInfinispanValueheader still wins over thevalueoption. All 99 tests ofcamel-infinispan-embeddedpass; the remoteInfinispanRemoteProducerITuses the same test and was not run locally (needs a container).