gemmellr commented on code in PR #4426:
URL: https://github.com/apache/activemq-artemis/pull/4426#discussion_r1158316804
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/multiprotocol/JMSMessageConsumerTest.java:
##########
@@ -193,4 +195,66 @@ private void
testDurableSubscriptionWithConfigurationManagedQueue(ConnectionSupp
assertEquals("color = 'BLUE'",
queue.getFilter().getFilterString().toString());
}
}
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndAMQP() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndOpenWire() throws
Exception {
+ testMapMessageConversion(createConnection(), createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndAMQP() throws Exception {
+ testMapMessageConversion(createCoreConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndCore() throws Exception {
+ testMapMessageConversion(createConnection(), createCoreConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndOpenWire() throws
Exception {
+ testMapMessageConversion(createCoreConnection(),
createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndCore() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(),
createCoreConnection());
+ }
+
+ private void testMapMessageConversion(Connection sender, Connection
consumer) throws Exception {
+ final String NAME = "myPropertyName";
+ final String VALUE = RandomUtil.randomString();
+
+ try {
+ Session session1 = sender.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ Session session2 = consumer.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+
+ javax.jms.Queue queue1 = session1.createQueue(getQueueName());
+ javax.jms.Queue queue2 = session2.createQueue(getQueueName());
Review Comment:
Should be able to use the same destination object for both, but if wanting
to avoid it then just inlining the creations might be neater given the
variables arent reused, avoiding them giving impression there are 'two queues'
when its just one.
##########
artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessageConverter.java:
##########
@@ -111,9 +111,13 @@ public static org.apache.activemq.artemis.api.core.Message
inbound(final Message
final ActiveMQBuffer body = coreMessage.getBodyBuffer();
final ByteSequence contents = messageSend.getContent();
- if (contents == null && coreType ==
org.apache.activemq.artemis.api.core.Message.TEXT_TYPE) {
- body.writeNullableString(null);
- } else if (contents != null) {
+ if (contents == null) {
+ if (coreType ==
org.apache.activemq.artemis.api.core.Message.TEXT_TYPE) {
+ body.writeNullableString(null);
+ } else if (coreType ==
org.apache.activemq.artemis.api.core.Message.MAP_TYPE) {
+ body.writeByte(DataConstants.NULL);
+ }
Review Comment:
The new tests dont appear to cover this case, the main addition of the PR
and how the issue was found?
Also feels like this could be unit tested rather than system tested.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/multiprotocol/JMSMessageConsumerTest.java:
##########
@@ -193,4 +195,66 @@ private void
testDurableSubscriptionWithConfigurationManagedQueue(ConnectionSupp
assertEquals("color = 'BLUE'",
queue.getFilter().getFilterString().toString());
}
}
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndAMQP() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndOpenWire() throws
Exception {
+ testMapMessageConversion(createConnection(), createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndAMQP() throws Exception {
+ testMapMessageConversion(createCoreConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndCore() throws Exception {
+ testMapMessageConversion(createConnection(), createCoreConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndOpenWire() throws
Exception {
+ testMapMessageConversion(createCoreConnection(),
createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndCore() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(),
createCoreConnection());
+ }
+
+ private void testMapMessageConversion(Connection sender, Connection
consumer) throws Exception {
+ final String NAME = "myPropertyName";
+ final String VALUE = RandomUtil.randomString();
+
+ try {
+ Session session1 = sender.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ Session session2 = consumer.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+
+ javax.jms.Queue queue1 = session1.createQueue(getQueueName());
+ javax.jms.Queue queue2 = session2.createQueue(getQueueName());
+
+ final MessageConsumer consumer2 = session2.createConsumer(queue2);
+
+ MessageProducer producer = session1.createProducer(queue1);
+ producer.setDeliveryMode(DeliveryMode.PERSISTENT);
+ sender.start();
+
+ MapMessage message = session1.createMapMessage();
+ message.setStringProperty(NAME, VALUE);
+ producer.send(message);
+
+ Message received = consumer2.receive(100);
Review Comment:
100 seems a little low for a message we do expect to receive, could be
unreliable in slow CI envs.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/multiprotocol/JMSMessageConsumerTest.java:
##########
@@ -193,4 +195,66 @@ private void
testDurableSubscriptionWithConfigurationManagedQueue(ConnectionSupp
assertEquals("color = 'BLUE'",
queue.getFilter().getFilterString().toString());
}
}
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndAMQP() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndOpenWire() throws
Exception {
+ testMapMessageConversion(createConnection(), createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndAMQP() throws Exception {
+ testMapMessageConversion(createCoreConnection(), createConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenAMQPAndCore() throws Exception {
+ testMapMessageConversion(createConnection(), createCoreConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenCoreAndOpenWire() throws
Exception {
+ testMapMessageConversion(createCoreConnection(),
createOpenWireConnection());
+ }
+
+ @Test(timeout = 30000)
+ public void testMapMessageConversionBetweenOpenWireAndCore() throws
Exception {
+ testMapMessageConversion(createOpenWireConnection(),
createCoreConnection());
+ }
+
+ private void testMapMessageConversion(Connection sender, Connection
consumer) throws Exception {
Review Comment:
Something like senderConnection and consumerConnection would seem more
descriptive...you could then follow through with 'sender[Foo]' and
'consumer[Foo]' convention for related objects and wouldnt need to call the
sole consumer "consumer2".
--
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]
