Skip illegal properties instead of failing bridging of full message

[davidvoit]

If for example a java qpid client is sending a message, and the bridge is using a core protocol receiver the "x-opt-jms-msg-type" can't be copied and would trigger a invalidJavaIdentifier exception.

As it seems these properties are only special properties, it's better to just skip these properties and just log the error. We only trace log the error as every qpid message triggers this error.

[davidvoit]

I can create a jira issue for this one, but first want to check if this is something you want to handle.

[clebertsuconic]

You're mentioning qpid mesasge. Are you sending it originally as AMQP?

• Why are you using the JMS Bridge? You could use the broker connection bridge. (JMS Bridge will convert messages as Core, While the AMQP Bridge will keep the message as is (it will even keep core messages as core).
• Perhaps there's an issue on the converter from AMQP To Core

A test would elucidate this I think.

[davidvoit]

We two way bridge between our old JMS vendor and artemis. We use the addMessageIdHeader option and a selector to avoid endless loops. Core protocol is used to support XATransactions between both "worlds".

After the migration our applications mostly using AMQP

[davidvoit]

I can provide the original error only on monday - sorry

[davidvoit]

"Why are you using the JMS Bridge? You could use the broker connection bridge. (JMS Bridge will convert messages as Core, While the AMQP Bridge will keep the message as is (it will even keep core messages as core)."

To make this more clear, this error is triggered on the artemis -> other jms vendor path of the bridge.

To add the original messageid as a new property all properties are cleared in the code block and readded. Here we trigger this bug. The property is in the Core protocol message and on the readding it's not allowed.

[clebertsuconic]

I see.. a test would be great.. perhaps a mock test to describe exactly the issue.

[davidvoit]

Sure, will work on something on monday. Will also create a jira issue then. Thanks for the support on these issues btw :-)

[davidvoit]

Exception:

AMQ139012: The property name 'JMS_AMQP_MA_x-opt-jms-msg-type' is not a valid java identifier.
       at org.apache.activemq.artemis.jms.client.ActiveMQMessage.checkProperty(ActiveMQMessage.java:904)
       at org.apache.activemq.artemis.jms.client.ActiveMQMessage.setObjectProperty(ActiveMQMessage.java:687)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl.copyProperties(JMSBridgeImpl.java:1620)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl.addMessageIDInHeader(JMSBridgeImpl.java:1569)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl.sendMessages(JMSBridgeImpl.java:1518)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl.sendBatchXA(JMSBridgeImpl.java:1438)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl.sendBatch(JMSBridgeImpl.java:1361)
       at org.apache.activemq.artemis.jms.bridge.impl.JMSBridgeImpl$SourceReceiver.run(JMSBridgeImpl.java:1722)
       at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
       at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
       at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
       at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
       at java.base/java.lang.Thread.run(Thread.java:1583)

[davidvoit]

This would be a standalone unit test of the bug without the JMSBridge:

    @Test
    public void testDummy() throws JMSException {
        ClientMessage clientMessage = new ClientMessageImpl(ActiveMQMapMessage.TYPE, true, 0, System.currentTimeMillis(), (byte) 4, 1000);
        var mockSession = Mockito.mock(ClientSession.class);
        Mockito.when(mockSession.createMessage(
                Mockito.anyByte(),
                Mockito.anyBoolean(),
                Mockito.anyLong(),
                Mockito.anyLong(),
                Mockito.anyByte()))
            .thenReturn(clientMessage);

        var newMsg = ActiveMQMessage.createMessage(clientMessage, mockSession);

        newMsg.getCoreMessage().putShortProperty("JMS_AMQP_ORIGINAL_ENCODING", (short)7);
        newMsg.getCoreMessage().putBooleanProperty("JMS_AMQP_HEADER", true);
        newMsg.getCoreMessage().putBooleanProperty("JMS_AMQP_HEADERDURABLE", true);
        newMsg.getCoreMessage().putByteProperty("JMS_AMQP_MA_x-opt-jms-msg-type", (byte)2);
        newMsg.getCoreMessage().putByteProperty("JMS_AMQP_MA_x-opt-jms-dest", (byte)0);
        newMsg.getCoreMessage().putStringProperty("NATIVE_MESSAGE_ID", "ID:ae7d84f6-b32b-4135-9abf-96865914989b:1:1:1-1");

        Enumeration<String> en = newMsg.getPropertyNames();

        Map<String, Object> oldProps = null;

        while (en.hasMoreElements()) {
            String propName = en.nextElement();

            if (oldProps == null) {
                oldProps = new HashMap<>();
            }

            oldProps.put(propName, newMsg.getObjectProperty(propName));
        }

        newMsg.clearProperties();


        if (oldProps != null) {
            for (Map.Entry<String, Object> entry : oldProps.entrySet()) {
                String propName = entry.getKey();

                newMsg.setObjectProperty(propName, entry.getValue());
            }
        }
    }

[davidvoit]

Added a unit test which does a full JMSBridge run. Also added a run which would trigger the bug

[davidvoit]

Sorry forget this unusual 3 char indention - not used to checkstyle files, but did run it manuelly and it's now passing:

$ java -cp ../checkstyle-13.7.0-all.jar:../sevntu-checks-1.44.1.jar com.puppycrawl.tools.checkstyle.Main -c etc/checkstyle.xml -f xml artemis-jms-server/src/test/java/org/apache/activemq/artemis/jms/bridge/JMSBridgeRunTest.java 
<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="13.7.0">
<file name="/home/david/Programmieren/IdeaProjects/artemis/artemis-jms-server/src/test/java/org/apache/activemq/artemis/jms/bridge/JMSBridgeRunTest.java">
</file>
</checkstyle>

Sorry about that

[davidvoit]

Ok that is something new to me. Question here should the checks be also running under the jakarta side of things or is it enough if they are only run on the jms side?

I will add now the test dependencies, but I don't know if I should add the surefire stuff?

[jbertram]

For what it's worth, you can run the Checkstyle checks using the dev profile, e.g.:

mvn -Pdev install

See more in the hacking guide.

[davidvoit]

mvn test did now work for both jms aswell for the jakarta subprojects. On my intellij system dependencies are little bit finicky as there is some dependency loop.

Will run now the mvn -Pdev install run without ide stuff. But I think it should be fine now

Update:

$ mvn -Pdev install
[...]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

[davidvoit]

Should I create jira issue for this?
Do you want to fix it like here with the try/catch or in jms client side (Is this even correct, I think it's wanted that these attributtes are read-only???) Or both?