prosgarz35 commented on PR #3194:
URL: https://github.com/apache/james-project/pull/3194#issuecomment-5834555798
# PR #3194 — Proposed Code Changes (Detailed Plan)
> **Scope:** Artemis-related files only. Files `.gitattributes`,
`crowdsec/james.log`, and `angus-mail.version` are intentionally excluded.
>
> Each section specifies: exact file path → exact line numbers → current
code → replacement code → rationale.
---
## Change 1 — Bug Fix: BrokerExtension `server-id` mismatch
### Problem
When `BrokerExtension` creates an embedded Artemis broker it assigns a
unique `server-id` via `BROKER_COUNTER.incrementAndGet()`. Because
`AtomicInteger` starts at **0**, the very first call to `incrementAndGet()`
returns **1**. This means the first broker starts listening on InVM server-id
**1**.
However, `JMSCacheableMailQueueTest.setUp()` hard-codes `"vm://0"` as the
connection URL, which targets server-id **0** — a server that does not exist.
This mismatch is currently masked only by an Artemis InVM quirk where a
default server-id=0 may be implicitly reused. As soon as tests run in parallel,
or the JVM boot order changes, connections either fail or land on a different
broker instance, producing silent data mix-up between test classes.
---
### File 1 of 2 — `BrokerExtension.java`
**Path:**
```
server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/BrokerExtension.java
```
**Line 47 — current:**
```java
private static final AtomicInteger BROKER_COUNTER = new AtomicInteger(0);
```
**Line 47 — replacement:**
```java
private static final AtomicInteger BROKER_COUNTER = new AtomicInteger(-1);
```
**Why:** `new AtomicInteger(-1)` means the first `incrementAndGet()` call
returns **0**, so the first broker gets `server-id=0`. This makes the InVM URL
`vm://0` in the test correct for the first test class. Subsequent classes get
ids 1, 2, … but each class uses its own `BrokerExtension` instance in its own
JVM fork (surefire forks per-class by default), so there is no conflict.
---
**Lines 61–74 — current:**
```java
public BrokerExtension() throws Exception {
brokerId = BROKER_COUNTER.incrementAndGet();
Configuration config = new ConfigurationImpl()
.setSecurityEnabled(false)
.setJMXManagementEnabled(false)
.setPersistenceEnabled(false)
.addAcceptorConfiguration(new TransportConfiguration(
InVMAcceptorFactory.class.getName(),
java.util.Collections.singletonMap("server-id",
String.valueOf(brokerId))
))
.setName("test-broker-" + brokerId);
broker = new EmbeddedActiveMQ();
broker.setConfiguration(config);
}
```
**No change to lines 61–74.** The constructor body stays identical — only
the initial value of `BROKER_COUNTER` changes (line 47 above). With `-1` as the
start, `brokerId` will be `0` for the first instance, and the rest of the code
is already correct.
---
### File 2 of 2 — `JMSCacheableMailQueueTest.java`
**Path:**
```
server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSCacheableMailQueueTest.java
```
**Lines 55–56 — current:**
```java
// Use InVM transport - broker ID matches BrokerExtension instance
counter
ActiveMQConnectionFactory connectionFactory = new
ActiveMQConnectionFactory("vm://0");
```
**Lines 55–56 — replacement:**
```java
// InVM transport: server-id=0 matches the first BrokerExtension
instance (BROKER_COUNTER starts at -1)
ActiveMQConnectionFactory connectionFactory = new
ActiveMQConnectionFactory("vm://0");
```
**Why:** The URL itself (`"vm://0"`) does not change — only the comment is
updated to explain *why* `0` is correct, i.e., that `BROKER_COUNTER` is
initialized at `-1` so the first `incrementAndGet()` yields `0`.
---
## Change 2 — DRY: Extract duplicated hex-encoding loop
### Problem
Two methods in `JMSCacheableMailQueue` — `encodeAttributePropertyName` and
`encodePerRecipientHeaderPropertyName` — contain **identical**
character-encoding loops. The only difference is the string prefix prepended to
the result. If the escaping format ever needs to change (e.g. switching from
`_XXXX_` to a different scheme), both methods must be updated independently.
This violates DRY.
---
### File — `JMSCacheableMailQueue.java`
**Path:**
```
server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSCacheableMailQueue.java
```
**Lines 162–186 — current:**
```java
protected static String encodeAttributePropertyName(String
attributeName) {
StringBuilder sb = new StringBuilder(JAMES_MAIL_ATTR_PROP_PREFIX);
for (int i = 0; i < attributeName.length(); i++) {
char c = attributeName.charAt(i);
if (Character.isJavaIdentifierPart(c)) {
sb.append(c);
} else {
sb.append(String.format("_%04x_", (int) c));
}
}
return sb.toString();
}
protected static String encodePerRecipientHeaderPropertyName(String
recipientAddress) {
StringBuilder sb = new
StringBuilder(JAMES_MAIL_PER_RECIPIENT_HEADERS).append('_');
for (int i = 0; i < recipientAddress.length(); i++) {
char c = recipientAddress.charAt(i);
if (Character.isJavaIdentifierPart(c)) {
sb.append(c);
} else {
sb.append(String.format("_%04x_", (int) c));
}
}
return sb.toString();
}
```
**Lines 162–186 — replacement:**
```java
private static String encodeAsJmsPropertyName(String prefix, String
input) {
StringBuilder sb = new StringBuilder(prefix);
for (int i = 0; i < input.length(); i++) {
char c = input.charAt(i);
if (Character.isJavaIdentifierPart(c)) {
sb.append(c);
} else {
sb.append(String.format("_%04x_", (int) c));
}
}
return sb.toString();
}
protected static String encodeAttributePropertyName(String
attributeName) {
return encodeAsJmsPropertyName(JAMES_MAIL_ATTR_PROP_PREFIX,
attributeName);
}
protected static String encodePerRecipientHeaderPropertyName(String
recipientAddress) {
return encodeAsJmsPropertyName(JAMES_MAIL_PER_RECIPIENT_HEADERS +
"_", recipientAddress);
}
```
**Why:**
- Removes ~12 lines of duplicated code.
- The encoding loop exists in exactly one place; any future change to the
`_%04x_` escape format is a single-line edit.
- Public API of both `protected static` methods is unchanged — callers and
tests are unaffected.
- `encodeAsJmsPropertyName` is `private static` — it is an internal detail,
not part of the public interface.
- Note: `JAMES_MAIL_PER_RECIPIENT_HEADERS + "_"` is a compile-time-constant
concatenation; it produces exactly the same prefix as the original `new
StringBuilder(JAMES_MAIL_PER_RECIPIENT_HEADERS).append('_')`.
---
## Change 3 — KISS: Simplify `mailAttribute()` — remove unnecessary
`Throwing.function` wrapper
### Problem
`mailAttribute()` currently uses `Throwing.function` (from the
`fge/throwing-lambdas` library) to wrap a lambda that already fully catches
every exception it can throw. The outer `Throwing.function` wrapper serves no
purpose: no checked exception escapes the lambda. The result is a nested
structure — lambda inside `Throwing.function` inside `.apply()` — that obscures
simple sequential fallback logic.
---
### File — `JMSCacheableMailQueue.java`
**Path:**
```
server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSCacheableMailQueue.java
```
**Lines 541–570 — current:**
```java
private Stream<Attribute> mailAttribute(Message message, String name) {
// Now cast the property back to Serializable and set it as
attribute.
// See JAMES-1241. Property name is encoded to ensure it is a valid
JMS identifier.
Object attrValue = Throwing.function((String prop) -> {
try {
Object val = message.getObjectProperty(prop);
if (val != null) {
return val;
}
} catch (Exception e) {
// Fall back to raw name if property name check failed
}
try {
return message.getObjectProperty(name);
} catch (Exception e) {
return null;
}
}).apply(encodeAttributePropertyName(name));
if (attrValue instanceof String) {
try {
return Stream.of(new Attribute(AttributeName.of(name),
AttributeValue.fromJsonString((String) attrValue)));
} catch (IOException e) {
LOGGER.error("Error deserializing mail attribute {} with
value {}", name, attrValue, e);
}
} else {
LOGGER.error("Not supported mail attribute {} of type {} for
mail {}", name, attrValue, name);
}
return Stream.empty();
}
```
**Lines 541–570 — replacement:**
```java
private Stream<Attribute> mailAttribute(Message message, String name) {
// Property name is encoded to ensure it is a valid JMS identifier.
See JAMES-1241.
// Try encoded name first; fall back to raw name for backward
compatibility.
Object attrValue = null;
try {
attrValue =
message.getObjectProperty(encodeAttributePropertyName(name));
} catch (JMSException e) {
// ignore — fall through to legacy fallback
}
if (attrValue == null) {
try {
attrValue = message.getObjectProperty(name);
} catch (JMSException e) {
// ignore
}
}
if (attrValue instanceof String) {
try {
return Stream.of(new Attribute(AttributeName.of(name),
AttributeValue.fromJsonString((String) attrValue)));
} catch (IOException e) {
LOGGER.error("Error deserializing mail attribute {} with
value {}", name, attrValue, e);
}
} else {
LOGGER.error("Not supported mail attribute {} of type {} for
mail {}", name, attrValue, name);
}
return Stream.empty();
}
```
**Why:**
- Removes the `Throwing.function` wrapper — the functional interface was
only needed to satisfy the compiler about `JMSException`, but since both
`getObjectProperty` calls were already surrounded by `catch (Exception e)`, the
wrapper did nothing.
- Replaces `catch (Exception e)` with the specific `catch (JMSException e)`
— now we catch exactly what the API declares, not everything. This is better
practice: unchecked exceptions (e.g. `NullPointerException` from a broken
broker) are no longer silently swallowed.
- Logic is identical: try encoded name → if null, try raw name → if still
null/non-String, log error.
- Code flows top-to-bottom with no hidden lambdas; a new contributor can
read it without knowing the `fge/throwing-lambdas` library.
- No behavioral change. The existing `JMSCacheableMailQueuePropertyNameTest`
already covers the encode/decode round-trip; the fallback path is also tested.
---
## Change 4 — Inconsistency: Align `CLIENT_BLOCK_ON_DURABLE_SEND` /
`CLIENT_BLOCK_ON_ACKNOWLEDGE` defaults
### Problem
In `ActiveMQConfiguration.java`, both client-side blocking flags default to
`true`:
```java
private static final boolean CLIENT_BLOCK_ON_DURABLE_SEND_DEFAULT = true;
private static final boolean CLIENT_BLOCK_ON_ACKNOWLEDGE_DEFAULT = true;
```
But the canonical `activemq.properties` sample file (the one that documents
the recommended settings to operators) specifies:
```properties
artemis.client.block.on.durable.send=false
artemis.client.block.on.acknowledge=false
```
**Impact:** deployments that do not provide an `activemq.properties` file
(currently: JPA app, JPA-SMTP app, Spring app — they have no sample config for
this) use `getDefault()` which calls `from(new BaseConfiguration())`. With
`DEFAULT = true`, those deployments silently run in blocking mode: every
`send()` to a durable queue blocks the caller thread until the broker
acknowledges persistence. This is the high-latency / low-throughput path.
Apps that *do* provide the sample file (postgres-app) get `false` — the
non-blocking, high-throughput path.
The two groups of apps behave differently with no visible indication to the
operator.
---
### File — `ActiveMQConfiguration.java`
**Path:**
```
server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQConfiguration.java
```
**Line 50 — current:**
```java
private static final boolean CLIENT_BLOCK_ON_DURABLE_SEND_DEFAULT = true;
```
**Line 50 — replacement:**
```java
private static final boolean CLIENT_BLOCK_ON_DURABLE_SEND_DEFAULT =
false;
```
---
**Line 53 — current:**
```java
private static final boolean CLIENT_BLOCK_ON_ACKNOWLEDGE_DEFAULT = true;
```
**Line 53 — replacement:**
```java
private static final boolean CLIENT_BLOCK_ON_ACKNOWLEDGE_DEFAULT = false;
```
**Why:**
- `false` (non-blocking) is the recommended value: already documented and
shipped in `activemq.properties`.
- Non-blocking send delegates persistence responsibility to the Artemis
journal engine, which batches writes efficiently. This is the correct default
for a mail queue broker embedded in the same JVM — there is no network latency,
and the journal fsync (`journalSyncTransactional=true`) still guarantees
durability.
- `true` (blocking) makes sense only for remote brokers where you need
round-trip confirmation. For InVM transport (`vm://0`), blocking is always
wasteful.
- Changing the default to `false` makes *all* James deployments (with or
without a custom properties file) behave consistently and in the recommended
way.
> **Note:** if any deployment scenario genuinely requires blocking sends
(e.g. a custom remote broker setup), the operator can override via
`artemis.client.block.on.durable.send=true` in their properties file. The
ability to configure this is preserved.
---
## Summary of All Changes
| # | File | Lines affected | Type | Risk |
|---|------|---------------|------|------|
| 1a | `BrokerExtension.java` | 47 | Bug fix | Low — test-only |
| 1b | `JMSCacheableMailQueueTest.java` | 55 | Comment update | None |
| 2 | `JMSCacheableMailQueue.java` | 162–186 | DRY refactor | Low —
`protected static` API unchanged |
| 3 | `JMSCacheableMailQueue.java` | 541–570 | KISS refactor | Low — logic
identical, narrower catch |
| 4 | `ActiveMQConfiguration.java` | 50, 53 | Default value fix | Medium —
behavior change for apps without properties file |
> **Change 4** is the only one with production behavior impact. All other
changes are either test-only or pure refactors with identical runtime behavior.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
