wy471x commented on PR #6906:
URL: https://github.com/apache/shenyu/pull/6906#issuecomment-5295559755
> ## 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
> * **`TopicMatcher` is 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** by
`add()`/`remove()` internally, so this isn't introducing dead code — good call
keeping it.
> * **`getChannelsByTopic`** is a clean O(N) scan that delegates entirely to
`TopicMatcher`; no logic duplicated.
> * **`TopicMatcherTest` is thorough** — exact, single-level, multi-level,
mixed, `$`-topic, and null cases all covered.
>
> ### Suggestions (non-blocking)
> 1. **Duplicate delivery on overlapping subscriptions (real, please track
as a follow-up).** `getChannelsByTopic` does `result.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 to
`sport/x` adds the same `Channel` twice → the client receives the message
twice. MQTT requires at most one delivery per publish per client. Collect into
a `Set<Channel>` (or `LinkedHashSet` if you care about order/stability) before
returning to avoid this.
> 2. **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.
> 3. **Minor:** invalid filters (e.g. `#` not as its own level, or trailing
text after `#`) silently return `false` here. Optionally reject malformed
filters at subscription time in `Subscribe.add` so 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.
Thank you for the code review on this PR.
Fixes for the three review comments:
1. Duplicate delivery on overlapping subscriptions —
SubscribeRepository.getChannelsByTopic now collects channels into a
LinkedHashSet before returning, so a client holding
overlapping filters (e.g. sport/# and #) receives at most one delivery per
publish, per MQTT spec.
2. Performance fast-path — exact topic subscribers are added via an O(1)
map lookup first; the wildcard scan then skips filters without +/#.
3. Malformed filters fail fast — added TopicMatcher.isValidFilter
(MQTT-4.7.1 rules); Subscribe registers only valid filters and sends SUBACK
return code 0x80 (FAILURE) for
invalid ones.
Tests added: TopicMatcherTest validation cases, new SubscribeRepositoryTest
(dedup/fast-path), new SubscribeTest (rejection + SUBACK codes).
--
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]
