[ISSUE #19] fix '#' check in topicFilter #25
Conversation
|
please make sure the ci pass |
| topicFilter = "t/t1/#/t3/"; | ||
| Assert.assertFalse(TopicUtils.isMatch(topic, topicFilter)); | ||
|
|
||
| topicFilter = "t/t1#/"; |
There was a problem hiding this comment.
Will "t/t1#/" appear?
Maybe misuse. Clients probably reject this type, but it's better for the server to check it itself.
| @@ -99,9 +99,7 @@ private void refreshWildcards(String firstTopic) { | |||
| queueNames.add(pubTopic); | |||
| Set<String> wildcards = matchWildcards(pubTopic); | |||
| if (wildcards != null && !wildcards.isEmpty()) { | |||
There was a problem hiding this comment.
line 101 the return wildcards may never be null and the if judgment can be removed or not ？
There was a problem hiding this comment.
I prefer not to removed. It's not expensive and could avoid problems that may arise from a change of matchWildcards() in the future.
mqtt-common/src/test/java/org/apache/rocketmq/mqtt/common/test/TestTopicUtils.java
Show resolved
Hide resolved
| @@ -61,9 +61,7 @@ public void cleanResource(String clientId,String channelId) { | |||
|
|
|||
| public void put(CacheType cacheType, String channelId, int mqttMsgId) { | |||
| ConcurrentMap<String, Set<Integer>> cache = whichCache(cacheType); | |||
| if (!cache.containsKey(channelId)) { | |||
There was a problem hiding this comment.
Adding judgment is mainly to avoid unnecessary temporary object generation
There was a problem hiding this comment.
Oh, that's my mistake. I just replaced them all, and don't notice this cache operation will be frequent.
| @@ -61,9 +61,7 @@ public void cleanResource(String clientId,String channelId) { | |||
|
|
|||
| public void put(CacheType cacheType, String channelId, int mqttMsgId) { | |||
| ConcurrentMap<String, Set<Integer>> cache = whichCache(cacheType); | |||
| if (!cache.containsKey(channelId)) { | |||
There was a problem hiding this comment.
Oh, that's my mistake. I just replaced them all, and don't notice this cache operation will be frequent.
| @@ -120,9 +120,7 @@ public void addSubscription(Session session, Set<Subscription> subscriptions) { | |||
| } | |||
|
|
|||
| synchronized (topicCache) { | |||
| if (!topicCache.containsKey(topicFilter)) { | |||
There was a problem hiding this comment.
This place is for subscribe request, that will not be frequent, can I keep it? @pingww
link #19, some code can be optimized:
ADDFLAG & JINFLAG should be replaced with PLUS_SIGN & NUMBER_SIGN refer to unicode
Some redundant containsKey before putIfAbsent()
buildIntanceName -> buildInstanceName
TopicUtils.isMatch("t/t1/t2/", "t/#/t2/") should be false but return true now.(I know some client will do this check like poho, however, it is better for the server to do a validation as well)