feat: implement MQTT wildcard subscription matching - #6906
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Aias00
left a comment
There was a problem hiding this comment.
Review: #6906 — feat: implement MQTT wildcard subscription matching
Verdict: ✅ Approve (with two follow-up suggestions, one a real correctness edge case)
This fills a genuinely broken feature — wildcard subscriptions silently never matched because Publish.send did an exact Map.get. The fix is well-structured.
What's correct
TopicMatcheris spec-accurate. I traced the algorithm across the test matrix and beyond:+matches exactly one level (sport/+/player1❌sport/tennis/stadium/player1, ✓sport/football/player1);#matches any number of levels incl. the parent (sport/#✓sport);#only matches when it's its own level (sport#/sport/tennis#correctly rejected);$-prefixed topics are not matched by a leading wildcard (#/+→ false) but are matched by explicit$SYS/#/$SYS/+— exactly MQTT-4.7.2-1;- null inputs return false (no NPE).
- The
get()exact-lookup method is retained and still used byadd()/remove()internally, so this isn't introducing dead code — good call keeping it. getChannelsByTopicis a clean O(N) scan that delegates entirely toTopicMatcher; no logic duplicated.TopicMatcherTestis thorough — exact, single-level, multi-level, mixed,$-topic, and null cases all covered.
Suggestions (non-blocking)
- Duplicate delivery on overlapping subscriptions (real, please track as a follow-up).
getChannelsByTopicdoesresult.addAll(entry.getValue())over every matching filter. If one client holds two overlapping subscriptions (e.g.sport/#and#), it appears under both keys, so a publish tosport/xadds the sameChanneltwice → the client receives the message twice. MQTT requires at most one delivery per publish per client. Collect into aSet<Channel>(orLinkedHashSetif you care about order/stability) before returning to avoid this. - Performance fast-path. Every publish now scans all subscriptions. For the very common case where the topic has an exact (non-wildcard) subscriber, you could
result.addAll(get(topic))first (O(1) exact hit) and then only scan filters containing+/#. Not necessary for correctness, just a scale consideration. - Minor: invalid filters (e.g.
#not as its own level, or trailing text after#) silently returnfalsehere. Optionally reject malformed filters at subscription time inSubscribe.addso bad subscriptions fail fast instead of silently never matching.
Verdict
Approving. The core matching logic is correct and well-tested, and get() is correctly preserved. Suggestion #1 (dedupe) is worth a quick follow-up PR before wildcard support sees production traffic with multi-subscription clients.
… filters Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Thank you for the code review on this PR. Fixes for the three review comments:
Tests added: TopicMatcherTest validation cases, new SubscribeRepositoryTest (dedup/fast-path), new SubscribeTest (rejection + SUBACK codes). |
Publish.publishWill used an exact SubscribeRepository.get() lookup, so clients subscribed to wildcard filters (e.g. status/#) never received wills published to concrete topics like status/client-001. Port the TopicMatcher and SubscribeRepository.getChannelsByTopic from apache#6906 and route will delivery through it, consistent with normal publish routing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.Summary:
Problem
Subscriptions using + (single-level wildcard) or # (multi-level wildcard) were silently broken. Publish.send() performed an exact-key lookup (ConcurrentHashMap.getOrDefault), so a publish to sensor/room1/temperature would never match a subscription filter like sensor/+/temperature.
Changes
- + matches exactly one topic level
- # matches any number of levels (must appear at the end of the filter)
- Wildcards at the first level do not match $-prefixed topics
the published topic using TopicMatcher.matches().
close #6851