[
https://issues.apache.org/jira/browse/ARTEMIS-5183?focusedWorklogId=951159&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-951159
]
ASF GitHub Bot logged work on ARTEMIS-5183:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 07/Jan/25 12:19
Start Date: 07/Jan/25 12:19
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #5410:
URL: https://github.com/apache/activemq-artemis/pull/5410#discussion_r1905356843
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/stomp/StompTest.java:
##########
@@ -1501,6 +1501,32 @@ public void testSubscribeToTopicWithNoLocal() throws
Exception {
conn.disconnect();
}
+ @Test
+ public void testSubscribeToTopicWithNoLocalSendWithStomp() throws Exception
{
+ conn.connect(defUser, defPass);
+ subscribeTopic(conn, null, null, null, true, true);
+
+ // send a message on the same connection => it should not be received is
noLocal = true on subscribe
Review Comment:
```suggestion
// send a message on the same connection => it should not be received
as noLocal = true on subscribe
```
(and place c&p from)
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/stomp/StompTest.java:
##########
@@ -1501,6 +1501,32 @@ public void testSubscribeToTopicWithNoLocal() throws
Exception {
conn.disconnect();
}
+ @Test
+ public void testSubscribeToTopicWithNoLocalSendWithStomp() throws Exception
{
+ conn.connect(defUser, defPass);
+ subscribeTopic(conn, null, null, null, true, true);
+
+ // send a message on the same connection => it should not be received is
noLocal = true on subscribe
+ send(conn, getTopicPrefix() + getTopicName(), null, "Hello World");
+
+ ClientStompFrame frame = conn.receiveFrame(100);
+ assertNull(frame, "No message should have been received since
subscription was removed");
+
+ // send message on another STOMP connection => it should be received
+ StompClientConnection conn2 =
StompClientConnectionFactory.createClientConnection(uri);
+ conn2.connect(defUser, defPass);
+ // a STOMP message does *not* automatically get a property named
__AMQ_CID
Review Comment:
```suggestion
// a STOMP message does *not* automatically get a property named
__AMQ_CID if there arent any noLocal subscriptions on the connection
```
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/stomp/StompTest.java:
##########
@@ -1501,6 +1501,32 @@ public void testSubscribeToTopicWithNoLocal() throws
Exception {
conn.disconnect();
}
+ @Test
+ public void testSubscribeToTopicWithNoLocalSendWithStomp() throws Exception
{
+ conn.connect(defUser, defPass);
+ subscribeTopic(conn, null, null, null, true, true);
+
+ // send a message on the same connection => it should not be received is
noLocal = true on subscribe
+ send(conn, getTopicPrefix() + getTopicName(), null, "Hello World");
+
+ ClientStompFrame frame = conn.receiveFrame(100);
+ assertNull(frame, "No message should have been received since
subscription was removed");
+
+ // send message on another STOMP connection => it should be received
+ StompClientConnection conn2 =
StompClientConnectionFactory.createClientConnection(uri);
+ conn2.connect(defUser, defPass);
+ // a STOMP message does *not* automatically get a property named
__AMQ_CID
+ send(conn2, getTopicPrefix() + getTopicName(), null, getName());
+ frame = conn.receiveFrame(10000);
+ assertNotNull(frame);
+ assertEquals(Stomp.Responses.MESSAGE, frame.getCommand());
+ assertEquals(getTopicPrefix() + getTopicName(),
frame.getHeader(Stomp.Headers.Send.DESTINATION));
+ assertEquals(getName(), frame.getBody());
+
+ conn.disconnect();
+ conn2.disconnect();
Review Comment:
Though there is fallback handling for _conn_, there isnt for _conn2_, so the
conn2.disconnect() might not happen if anything fails, might be worth adding a
try-finally, or try-with-resources if simple, or maybe use runAfter (could e.g
extract a utility method from the existing _conn_ fallback cleanup and pass it
_conn2_)
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/stomp/StompTest.java:
##########
@@ -1501,6 +1501,32 @@ public void testSubscribeToTopicWithNoLocal() throws
Exception {
conn.disconnect();
}
+ @Test
+ public void testSubscribeToTopicWithNoLocalSendWithStomp() throws Exception
{
+ conn.connect(defUser, defPass);
+ subscribeTopic(conn, null, null, null, true, true);
+
+ // send a message on the same connection => it should not be received is
noLocal = true on subscribe
+ send(conn, getTopicPrefix() + getTopicName(), null, "Hello World");
+
+ ClientStompFrame frame = conn.receiveFrame(100);
+ assertNull(frame, "No message should have been received since
subscription was removed");
Review Comment:
```suggestion
assertNull(frame, "No message should have been received since
subscription specified noLocal");
```
(and place c&p from)
Issue Time Tracking
-------------------
Worklog Id: (was: 951159)
Time Spent: 20m (was: 10m)
> STOMP noLocal filter ignores legitimate messages
> ------------------------------------------------
>
> Key: ARTEMIS-5183
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5183
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Components: STOMP
> Affects Versions: 2.37.0
> Reporter: Tommy Lindgren
> Assignee: Justin Bertram
> Priority: Major
> Labels: pull-request-available
> Time Spent: 20m
> Remaining Estimate: 0h
>
> If client 1 subscribes with {{noLocal=true}} to {{/topic/example}} and client
> 2 sends messages to {{/topic/example}} but doesn't use {{noLocal}}, then
> client 1 won't see the messages from client 2.
> If {{noLocal=true}}, {{StompConnect.java}} will add {{\__AMQ_CID<>xxx}} to
> the selector. However, the {{\__AMQ_CID}} header is only added to the message
> if {{noLocal}} is enabled, so the selector won't match anything (presumably
> because <> never matches NULL).
> Untested potential patch:
> {noformat}
> diff --git
> a/artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java
>
> b/artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java
> index bf0d09f955..29f9301f29 100644
> ---
> a/artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java
> +++
> b/artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java
> @@ -496,9 +496,8 @@ public final class StompConnection extends
> AbstractRemotingConnection {
> try {
> StompSession stompSession = getSession(txID);
>
> - if (stompSession.isNoLocal()) {
> - message.putStringProperty(CONNECTION_ID_PROPERTY_NAME_STRING,
> getID().toString());
> - }
> + message.putStringProperty(CONNECTION_ID_PROPERTY_NAME_STRING,
> getID().toString());
> +
> if (isEnableMessageID()) {
> message.putStringProperty("amqMessageId", "STOMP" +
> message.getMessageID());
> }{noformat}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
