gnodet-bot commented on code in PR #28761:
URL: https://github.com/apache/camel/pull/28761#discussion_r4429968292
##########
core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java:
##########
@@ -1012,7 +1030,17 @@ public boolean isExternalRedelivered(Message message) {
@Override
public void copyInternalProperties(Exchange target) {
- ((AbstractExchange)
target).internalProperties.putAll(internalProperties);
+ AbstractExchange exchange = (AbstractExchange) target;
+ if (toEndpoint != null) {
Review Comment:
💡 **Suggestion:** Consider adding a short comment explaining why `null`
means "not set / removed" rather than "to be propagated". The old `putAll`
approach could propagate explicit `null` values from the `EnumMap`; the new
field-based guard cannot distinguish *never set* from *explicitly removed*, so
the behavior has subtly changed. A one-liner like `// null means not set or
explicitly removed — skip copy` would make the intent clear to the next reader.
##########
core/camel-core/src/test/java/org/apache/camel/support/AbstractExchangeTest.java:
##########
@@ -77,4 +89,190 @@ void shouldPreserveDataTypeOnCopy() {
assertSame(type2, ((DataTypeAware) e2.getMessage()).getDataType());
}
+ @Test
+ void shouldNotCreateInternalPropertiesForToEndpoint() {
+ // every send sets this key, so it must not cost every exchange the
map sized for all the keys
+ DefaultExchange exchange = new DefaultExchange(context);
+ assertNull(exchange.internalProperties);
+
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ assertEquals(URI,
exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertNull(exchange.internalProperties);
+
+ AbstractExchange copy = (AbstractExchange) exchange.copy();
+ assertNull(copy.internalProperties);
+ }
+
+ @Test
+ void shouldKeepToEndpointBesideOtherInternalProperties() {
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ exchange.setProperty(ExchangePropertyKey.CHARSET_NAME, "UTF-8");
+
+ assertNotNull(exchange.internalProperties);
+ assertEquals(URI,
exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertEquals("UTF-8",
exchange.getProperty(ExchangePropertyKey.CHARSET_NAME));
+
+ assertEquals("UTF-8",
exchange.removeProperty(ExchangePropertyKey.CHARSET_NAME));
+ assertEquals(URI,
exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ }
+
+ @Test
+ void shouldReadOtherInternalPropertiesWithoutTheMap() {
+ DefaultExchange exchange = new DefaultExchange(context);
+
+ assertNull(exchange.getProperty(ExchangePropertyKey.CHARSET_NAME));
+ assertNull(exchange.getProperty(Exchange.CHARSET_NAME));
+ assertNull(exchange.removeProperty(ExchangePropertyKey.CHARSET_NAME));
+ assertTrue(exchange.getInternalProperties().isEmpty());
+ assertTrue(exchange.getAllProperties().isEmpty());
+ }
+
+ @Test
+ void shouldAccessToEndpointByKeyAndByName() {
+ // the name is the public constant that routes and components use
+ DefaultExchange exchange = new DefaultExchange(context);
+
+ exchange.setProperty(Exchange.TO_ENDPOINT, URI);
+ assertEquals(URI,
exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertEquals(URI, exchange.getProperty(Exchange.TO_ENDPOINT));
+ assertEquals(URI, exchange.getProperty(Exchange.TO_ENDPOINT,
String.class));
+
+ assertEquals(URI, exchange.removeProperty(Exchange.TO_ENDPOINT));
+ assertNull(exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ assertEquals(URI,
exchange.removeProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertNull(exchange.getProperty(Exchange.TO_ENDPOINT));
+ }
+
+ @Test
+ void shouldListToEndpointWithTheInternalProperties() {
+ // the dev consoles and tracers dump the exchange with these
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ exchange.setProperty("foo", "bar");
+
+ assertEquals(Map.of("CamelToEndpoint", URI),
exchange.getInternalProperties());
+ assertEquals(Map.of("CamelToEndpoint", URI, "foo", "bar"),
exchange.getAllProperties());
+ }
+
+ @Test
+ void shouldCopyToEndpointIndependently() {
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+
+ Exchange copy = exchange.copy();
+ assertEquals(URI, copy.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+
+ copy.setProperty(ExchangePropertyKey.TO_ENDPOINT, "mock://bar");
+ assertEquals(URI,
exchange.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ }
+
+ @Test
+ void shouldCopyInternalPropertiesIndependently() {
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.CHARSET_NAME, "UTF-8");
+
+ Exchange copy = exchange.copy();
+ assertEquals("UTF-8",
copy.getProperty(ExchangePropertyKey.CHARSET_NAME));
+
+ copy.setProperty(ExchangePropertyKey.CHARSET_NAME, "ISO-8859-1");
+ assertEquals("UTF-8",
exchange.getProperty(ExchangePropertyKey.CHARSET_NAME));
+ }
+
+ @Test
+ void shouldCreateCopyWithToEndpoint() {
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ exchange.setProperty("foo", "bar");
+
+ Exchange copy =
exchange.getExchangeExtension().createCopyWithProperties(context);
+
+ assertEquals(URI, copy.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertEquals("bar", copy.getProperty("foo"));
+ assertNull(((AbstractExchange) copy).internalProperties);
+ }
+
+ @Test
+ void shouldCreateCopyWithInternalProperties() {
+ DefaultExchange exchange = new DefaultExchange(context);
+ exchange.setProperty(ExchangePropertyKey.CHARSET_NAME, "UTF-8");
+
+ Exchange copy =
exchange.getExchangeExtension().createCopyWithProperties(context);
+
+ assertEquals("UTF-8",
copy.getProperty(ExchangePropertyKey.CHARSET_NAME));
+ }
+
+ @Test
+ void shouldCopyInternalPropertiesToTarget() {
+ DefaultExchange source = new DefaultExchange(context);
+ source.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ source.setProperty(ExchangePropertyKey.CHARSET_NAME, "UTF-8");
+
+ DefaultExchange target = new DefaultExchange(context);
+ source.getExchangeExtension().copyInternalProperties(target);
+
+ assertEquals(URI, target.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertEquals("UTF-8",
target.getProperty(ExchangePropertyKey.CHARSET_NAME));
+ // the target has its own map, so it can change without changing the
source
+ target.setProperty(ExchangePropertyKey.CHARSET_NAME, "ISO-8859-1");
+ assertEquals("UTF-8",
source.getProperty(ExchangePropertyKey.CHARSET_NAME));
+ }
+
+ @Test
+ void shouldKeepPropertiesOfTargetThatTheSourceDoesNotHave() {
+ DefaultExchange target = new DefaultExchange(context);
+ target.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
+ target.setProperty(ExchangePropertyKey.CHARSET_NAME, "UTF-8");
+
+ new
DefaultExchange(context).getExchangeExtension().copyInternalProperties(target);
+
+ assertEquals(URI, target.getProperty(ExchangePropertyKey.TO_ENDPOINT));
+ assertEquals("UTF-8",
target.getProperty(ExchangePropertyKey.CHARSET_NAME));
Review Comment:
💡 **Suggestion:** This test covers a source whose `TO_ENDPOINT` was *never
set* (defaults to `null`). Consider also adding a test for the set→remove→copy
path:
```java
@Test
void shouldNotOverwriteTargetToEndpointWhenSourceHadItRemoved() {
DefaultExchange source = new DefaultExchange(context);
source.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
source.removeProperty(ExchangePropertyKey.TO_ENDPOINT); // explicitly
nulled
DefaultExchange target = new DefaultExchange(context);
target.setProperty(ExchangePropertyKey.TO_ENDPOINT, URI);
source.getExchangeExtension().copyInternalProperties(target);
// source had TO_ENDPOINT removed — target's value should be preserved
assertEquals(URI, target.getProperty(ExchangePropertyKey.TO_ENDPOINT));
}
```
This documents the intended semantics of the `if (toEndpoint != null)` guard.
--
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]
