jbonofre commented on code in PR #1736:
URL: https://github.com/apache/activemq/pull/1736#discussion_r2923296796
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java:
##########
@@ -1000,6 +1012,22 @@ public void setNestedMapAndListEnabled(boolean
structuredMapsEnabled) {
this.nestedMapAndListEnabled = structuredMapsEnabled;
}
+ public boolean isStrictCompliance() {
+ return strictCompliance;
+ }
+
+ /**
+ * If this flag is set then the connection follows strict Jakarta Messaging
+ * compliance. Enabling this will automatically force
nestedMapAndListEnabled to false.
+ */
+ public void setStrictCompliance(boolean strictCompliance) {
+ this.strictCompliance = strictCompliance;
+ if (strictCompliance) {
+ this.setNestedMapAndListEnabled(false);
+ LOG.info("Strict compliance enabled: nestedMapAndListEnabled has
been forced to false to align with Jakarta specifications.");
Review Comment:
nit: I would suggest to remove this log, or at least use `debug` instead of
`info`.
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java:
##########
@@ -141,6 +141,13 @@ public class ActiveMQConnection implements Connection,
TopicConnection, QueueCon
private RedeliveryPolicyMap redeliveryPolicyMap;
private MessageTransformer transformer;
+ /**
+ * If set to true, strict Jakarta Messaging 3.1 compliance is enforced.
+ * This rejects non-standard property types (like Character) and forces
+ * legacy features like nestedMapAndListEnabled to false.
Review Comment:
I'm not sure it's fully correct.
Javadoc says enabling strict compliance "automatically forces
nestedMapAndListEnabled to false."
This couples two independent concerns. If someone explicitly sets
`strictCompliance=true` and then `nestedMapAndListEnabled=true`, the second
call would override it, leading to inconsistent state.
Mayne we should consider either:
* just checking `strictCompliance` at validation time without side-effecting
other flags
* or document the ordering dependency clearly in the Javadoc
##########
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMessage.java:
##########
@@ -525,11 +527,15 @@ public void setProperties(Map<String, ?> properties)
throws JMSException {
protected void checkValidObject(Object value) throws
MessageFormatException {
boolean valid = value instanceof Boolean || value instanceof Byte ||
value instanceof Short || value instanceof Integer || value instanceof Long;
- valid = valid || value instanceof Float || value instanceof Double ||
value instanceof Character || value instanceof String || value == null;
+ valid = valid || value instanceof Float || value instanceof Double ||
value instanceof String || value == null;
Review Comment:
As pointing out by @jeanouii, you are right here to revert the `Character`
change in the validation.
However, as `Character` is now handled in the connection, the original line
works. The strict check should only reject `Character` when `strictCompliance`
is `true`.
Maybe it's easier (to read/maintenance) to split in two steps:
```
// Keep original line unchanged:
valid = valid || value instanceof Float || value instanceof Double
|| value instanceof Character || value instanceof String || value ==
null;
// Then add strict rejection:
if (valid && conn != null && conn.isStrictCompliance() && value instanceof
Character) {
throw new MessageFormatException("Character type not allowed under
strict Jakarta 3.1 compliance");
}
```
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java:
##########
@@ -408,6 +415,11 @@ protected ActiveMQConnection
createActiveMQConnection(String userName, String pa
protected ActiveMQConnection createActiveMQConnection(Transport transport,
JMSStatsImpl stats) throws Exception {
ActiveMQConnection connection = new ActiveMQConnection(transport,
getClientIdGenerator(),
getConnectionIdGenerator(), stats);
+
+ // Copy the compliance flags from the Factory to the Connection
+ connection.setStrictCompliance(isStrictCompliance());
+ connection.setNestedMapAndListEnabled(isNestedMapAndListEnabled());
Review Comment:
I think we should remove this line as it's a behavior change (and unrelated
to this PR).
Currently, the connection default is `nestedMapAndListEnabled=false` (if not
overridden by the connection URI).
With this line, the factory's value always overrides.
This is probably correct, but I would remove `setNestedMapAndListEnabled()`
from this PR.
--
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]
For further information, visit: https://activemq.apache.org/contact
