Fix Jakarta 3.1 compliance for Character property types by pradeep85841 · Pull Request #1736 · apache/activemq

pradeep85841 · 2026-03-07T15:39:29Z

This PR addresses a spec compliance issue in ActiveMQMessage regarding allowed property types.

TCK Alignment: Updated checkValidObject to reject Character types when isNestedMapAndListEnabled is set to false, as required by the Jakarta Messaging 3.1 specification.

Backward Compatibility: Maintained support for Character types when the legacy flag is true (default) to prevent regressions for existing users.

Validation: Added unit tests to verify that MessageFormatException is correctly thrown in strict mode while verifying successful property assignment in legacy mode.

…hile keeping legacy support

activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQMessagePropertyTest.java

jbonofre · 2026-03-07T16:31:29Z

In order to be Jakarta Messaging 3.1 fully compliant, I would suggest to set nestedMapAndListEnabled to false by default, in order to not accept Character.

…iance

pradeep85841 · 2026-03-07T16:53:09Z

Set nestedMapAndListEnabled to false by default for Jakarta 3.1 compliance

jeanouii

I think changing the default from true to false is impacting for customers because it does not only impact Character but also Map and List.
The change in the connection is incomplete if we want to do so, we would need also to change the factory.
If everyone agrees to change default, I don't have a special opinion on this, we should mention it's to align with specification by default and it needs to be clear in the documentation and in the release notes.

Finally, I'd put some more love in the tests if you don't mind.
The embedded broker is started automatically by the factory but it's not stopped. That can cause flaky tests. Also the connections should also be using try-with-resources. This is small non blocking comment.

Overall, good job with the PR

jeanouii · 2026-03-11T13:48:51Z

activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java

    private long optimizeAcknowledgeTimeOut = 0;
    private long optimizedAckScheduledAckInterval = 0;
-    private boolean nestedMapAndListEnabled = true;
+    private boolean nestedMapAndListEnabled = false;


I don't think this has any effect. The factory creating the connection defaults to true and overrides this value I think. To be verified and discarded if I'm not correct.

pradeep85841 · 2026-03-11T15:50:36Z

Thank you @jeanouii for the detailed review and suggestions. I agree with your points.

I will update the ActiveMQConnectionFactory as well to set the default to false. This ensures the setting correctly propagates to all connections as intended for Jakarta 3.1 compliance.

And will refactor the unit tests to use try-with-resources for better connection management. I will also ensure the embedded broker is explicitly stopped to avoid any flakiness in the CI.

I am working on these changes and will push the updated code shortly

jbonofre

The PR moves Character from the "always valid" set into the "only valid when nested maps enabled".

But Character is not related to maps and lists.

I think modifying nestedMapAndListEnabled is semantically wrong in this context.

I would suggest to create a specific flag for Character, or maybe reject Character if the spec never allowed it.

jbonofre · 2026-03-11T16:00:40Z

activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java

    private long optimizeAcknowledgeTimeOut = 0;
    private long optimizedAckScheduledAckInterval = 0;
-    private boolean nestedMapAndListEnabled = true;
+    private boolean nestedMapAndListEnabled = false;


Changing nestedMapAndListEnabled to false by default is a significant behavioral change that goes well beyond Character rejection.

With false, it means Map and List property values will also be rejected by default. This will break users who rely on the legacy nested map/list support (it's the behavior for years).

I would suggest to not change in the context of this PR.

Thank you for the guidance. I completely understand your point—changing the nestedMapAndListEnabled default is a very big change and will surely break many legacy users who have been using Maps and Lists for years. I will revert this default back to true.

As suggested, I will introduce a specific flag, complianceMode (or strictCompliance), to handle the Character rejection separately. This way, we can achieve Jakarta 3.1 compliance without affecting the long-standing behavior of Maps and Lists.

I will update the PR with these changes and the refactored tests. Please let me know if this sounds correct to you

I like the idea of not having a specific flag just for this. We are just starting working on JMS 3.1 compliance and we may encounter other situations so it may well end up by having many flags.
Having one flag for turning the entire broker into a compatible mode seems relevant to me.
We could as well force (and log of course) nestedMapAndListEnabled to false when the new strictCompliance flag is enabled? Thoughts?

@jeanouii agree, that was basically my proposal.

pradeep85841 · 2026-03-11T18:14:09Z

Introduced a strictCompliance flag in ActiveMQConnection and ActiveMQConnectionFactory.

When strictCompliance=true, it automatically forces nestedMapAndListEnabled=false and logs an info message.

Updated ActiveMQMessage to reject Character properties (and other non-standard types) only when strictCompliance is enabled.

Refactored tests to verify the master switch

jeanouii · 2026-03-11T18:31:37Z

activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMessage.java

-                if (!(value instanceof Map || value instanceof List)) {
-                    throw new MessageFormatException("Only objectified primitive objects, String, Map and List types are allowed but was: " + value + " type: " + value.getClass());
+                // Standard rules: only primitives and Strings are allowed
+                if (!(value instanceof Map || value instanceof List || value instanceof Character)) {


I think now that you have the other flag, you can revert the Character part here, what do you think?

jeanouii

Small comment to revert the changes in the other flag if possible

jeanouii · 2026-03-11T20:05:52Z

@jbonofre I think that looks good now, what do you think?

It's a good compromise, isn't it?

jbonofre · 2026-03-12T09:17:46Z

activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMessage.java


        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;


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/ActiveMQConnection.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java

…ection

pradeep85841 · 2026-03-12T16:34:43Z

Reverted the original validation line and added a secondary check to reject Character only when strictCompliance is enabled.

Removed the side-effect where strictCompliance would force nestedMapAndListEnabled to false, preventing inconsistent states as suggested.

Removed the INFO log in the setter and limited flag propagation in the factory to only include the new compliance flag.

Updated the unit tests to verify that strictCompliance correctly enforces Jakarta 3.1 rules even if legacy flags are explicitly enabled.

jeanouii

LGTM
Unless there are hard blockers I'll go ahead and merge later today or tomorrow.

@pradeep85841 Thank you so much for the contributions and your patience.
I appreciate you being very careful about the feedback.

jbonofre

The latest iteration looks solid and addresses all prior review feedback.

Thanks !

Fixing Jakarta 3.1 TCK issue: Added strict check for Character type w…

c319a09

…hile keeping legacy support

jbonofre self-requested a review March 7, 2026 15:53

jbonofre requested changes Mar 7, 2026

View reviewed changes

activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQMessagePropertyTest.java Show resolved Hide resolved

Add missing ASF license header to test file

8850abc

Set nestedMapAndListEnabled to false by default for Jakarta 3.1 compl…

e1b2deb

…iance

jeanouii reviewed Mar 11, 2026

View reviewed changes

jbonofre requested changes Mar 11, 2026

View reviewed changes

pradeep85841 added 2 commits March 11, 2026 21:49

Align default configuration and tests with Jakarta 3.1 compliance

fa14a9c

Introduce strictCompliance master flag for Jakarta 3.1 alignment

a03670c

jeanouii reviewed Mar 11, 2026

View reviewed changes

Add strictCompliance flag and preserve legacy validation logic

3770470

jbonofre requested changes Mar 12, 2026

View reviewed changes

Decouple strictCompliance from legacy flags and isolate Character rej…

6fdef5a

…ection

jeanouii approved these changes Mar 12, 2026

View reviewed changes

jbonofre approved these changes Mar 12, 2026

View reviewed changes

jeanouii merged commit cada078 into apache:main Mar 12, 2026
10 checks passed

pradeep85841 deleted the fix/jakarta-31-property-compliance branch March 13, 2026 11:19

Conversation

pradeep85841 commented Mar 7, 2026

Uh oh!

Uh oh!

jbonofre commented Mar 7, 2026

Uh oh!

pradeep85841 commented Mar 7, 2026

Uh oh!

jeanouii left a comment

Choose a reason for hiding this comment

Uh oh!

jeanouii Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

pradeep85841 commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

jbonofre Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

pradeep85841 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jeanouii Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jbonofre Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

pradeep85841 commented Mar 11, 2026

Uh oh!

jeanouii Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jeanouii left a comment

Choose a reason for hiding this comment

Uh oh!

jeanouii commented Mar 11, 2026

Uh oh!

jbonofre Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pradeep85841 commented Mar 12, 2026

Uh oh!

jeanouii left a comment

Choose a reason for hiding this comment

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pradeep85841 commented Mar 11, 2026 •

edited

Loading