davsclaus commented on code in PR #26748:
URL: https://github.com/apache/camel/pull/26748#discussion_r4072418076
##########
components/camel-jcr/src/main/java/org/apache/camel/component/jcr/JcrProducer.java:
##########
@@ -57,6 +59,10 @@ public void process(Exchange exchange) throws Exception {
Map<String, Object> headers =
filterComponentHeaders(message.getHeaders());
for (String key : headers.keySet()) {
Object header = message.getHeader(key);
+ if (headerFilterStrategy != null
+ &&
headerFilterStrategy.applyFilterToCamelHeaders(key, header, exchange)) {
Review Comment:
🟠**This direction has no test.**
The upgrade guide documents both halves of the change, and
`JcrGetNodeByIdHeaderInjectionTest` covers the `getById` half thoroughly (four
casings, plus an ordinary property still mapping). But nothing asserts the
insert half: that a `CamelHttpUri` header carried on the message is no longer
persisted as a node property.
There's an existing insert test to extend — setting a `Camel`-prefixed
header on the message, running the insert, then reading the node back and
asserting the property is absent while an ordinary header is present, would
mirror the `getById` test nicely.
##########
components/camel-jcr/src/main/java/org/apache/camel/component/jcr/JcrEndpoint.java:
##########
@@ -72,6 +75,9 @@ public class JcrEndpoint extends DefaultEndpoint {
private long sessionLiveCheckInterval = 60000L;
@UriParam
private String workspaceName;
+ @UriParam(label = "filter",
+ description = "To use a custom
org.apache.camel.spi.HeaderFilterStrategy to filter header to and from Camel
message.")
+ private HeaderFilterStrategy headerFilterStrategy;
Review Comment:
🟠**Lazy init in the getter is a data race, and it hides the default from
the catalog.**
`getHeaderFilterStrategy()` assigns to this non-volatile field, and
`JcrProducer.process()` calls it on every exchange — so concurrent producers
can race on the assignment. In practice it's benign (the instances are
equivalent and effectively immutable), but there's no reason to take the race,
and the Java memory model doesn't formally rule out publishing a
partially-constructed object.
More practically: because the field starts null, the generated `jcr.json`
carries no `defaultValue` for this option, so nothing in the catalog or the
component docs tells a user that `Camel*` filtering is now applied by default —
which is the whole behavioural change this PR makes.
Initialising at the declaration, the way `NatsConfiguration:119` does, fixes
both at once and lets the getter become a plain accessor:
```suggestion
@UriParam(label = "filter",
description = "To use a custom
org.apache.camel.spi.HeaderFilterStrategy to filter header to and from Camel
message.")
private HeaderFilterStrategy headerFilterStrategy = new
DefaultHeaderFilterStrategy();
```
##########
components/camel-jcr/src/main/java/org/apache/camel/component/jcr/JcrProducer.java:
##########
@@ -81,7 +87,11 @@ public void process(Exchange exchange) throws Exception {
} else {
value = converter.convertTo(aClass, exchange,
property.getValue());
}
- message.setHeader(property.getName(), value);
+ String name = property.getName();
+ if (headerFilterStrategy == null
+ ||
!headerFilterStrategy.applyFilterToExternalHeaders(name, value, exchange)) {
Review Comment:
🟡 Nit: `headerFilterStrategy` can't be null here —
`getJcrEndpoint().getHeaderFilterStrategy()` lazy-inits it — so both this `==
null` and the `!= null` on the insert path are dead. Harmless, but they suggest
a nullable contract that doesn't exist. (Initialising the field at its
declaration, as suggested on `JcrEndpoint`, makes that unambiguous.)
##########
components/camel-jcr/src/main/java/org/apache/camel/component/jcr/JcrProducer.java:
##########
@@ -57,6 +59,10 @@ public void process(Exchange exchange) throws Exception {
Map<String, Object> headers =
filterComponentHeaders(message.getHeaders());
Review Comment:
🟡 `filterComponentHeaders` is now redundant. All three keys it strips are
`Camel`-prefixed — `JCR_NODE_NAME` = `CamelJcrNodeName`, `JCR_OPERATION` =
`CamelJcrOperation`, `JCR_NODE_TYPE` = `CamelJcrNodeType` — so
`applyFilterToCamelHeaders` on the next line already excludes every one of them
under the default strategy.
Worth removing so there's one filtering mechanism rather than two
overlapping ones. The one thing to weigh first: a user who configures a
*custom* `headerFilterStrategy` that doesn't filter `Camel*` would then start
persisting the JCR control headers as node properties, which
`filterComponentHeaders` currently prevents unconditionally. If that's a case
you want to keep guarding, leaving it is defensible — but then it deserves a
comment saying so, because it reads as dead code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
