From 48a50c9137a89fbe6fd55a94dec8987ff5822b97 Mon Sep 17 00:00:00 2001
From: Andrew Mayorov <encube.ul@gmail.com>
Date: Fri, 4 Aug 2023 16:24:13 +0400
Subject: [PATCH] fix(topicidx): fix missing matches when 'a/b' and 'a/b/#'
 both exist

Thanks to @HJianBo for spotting this issue. The approach to fix it
is different though: we try to keep the "recurrency branch factor"
to a minimum, instead introducing new condition for the case when
filter does not match, but iteration with `ets:next/2` is not yet
finished for the prefix.

Co-Authored-By: JianBo He <heeejianbo@gmail.com>
---
 apps/emqx/src/emqx_topic_index.erl        | 43 ++++++++++++-----------
 apps/emqx/test/emqx_topic_index_SUITE.erl | 18 ++++++++++
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/apps/emqx/src/emqx_topic_index.erl b/apps/emqx/src/emqx_topic_index.erl
index 48c685f04..29d8694af 100644
--- a/apps/emqx/src/emqx_topic_index.erl
+++ b/apps/emqx/src/emqx_topic_index.erl
@@ -59,10 +59,14 @@ match(Topic, Tab) ->
 
 match(Words, RPrefix, Tab) ->
     Prefix = lists:reverse(RPrefix),
-    K = ets:next(Tab, {Prefix, {}}),
-    case match_filter(Prefix, K, Words == []) of
+    match(ets:next(Tab, {Prefix, {}}), Prefix, Words, RPrefix, Tab).
+
+match(K, Prefix, Words, RPrefix, Tab) ->
+    case match_next(Prefix, K, Words) of
         true ->
             K;
+        skip ->
+            match(ets:next(Tab, K), Prefix, Words, RPrefix, Tab);
         stop ->
             false;
         Matched ->
@@ -100,9 +104,11 @@ matches(Words, RPrefix, Acc, Tab) ->
     matches(ets:next(Tab, {Prefix, {}}), Prefix, Words, RPrefix, Acc, Tab).
 
 matches(K, Prefix, Words, RPrefix, Acc, Tab) ->
-    case match_filter(Prefix, K, Words == []) of
+    case match_next(Prefix, K, Words) of
         true ->
             matches(ets:next(Tab, K), Prefix, Words, RPrefix, match_add(K, Acc), Tab);
+        skip ->
+            matches(ets:next(Tab, K), Prefix, Words, RPrefix, Acc, Tab);
         stop ->
             Acc;
         Matched ->
@@ -122,29 +128,26 @@ match_add(K = {_Filter, ID}, Acc = #{}) ->
 match_add(K, Acc) ->
     [K | Acc].
 
-match_filter(Prefix, {Filter, _ID}, NotPrefix) ->
-    case match_filter(Prefix, Filter) of
-        exact ->
-            % NOTE: exact match is `true` only if we match whole topic, not prefix
-            NotPrefix;
-        Match ->
-            Match
-    end;
-match_filter(_, '$end_of_table', _) ->
+match_next(Prefix, {Filter, _ID}, Suffix) ->
+    match_filter(Prefix, Filter, Suffix);
+match_next(_, '$end_of_table', _) ->
     stop.
 
-match_filter([], []) ->
-    exact;
-match_filter([], ['#']) ->
+match_filter([], [], []) ->
+    true;
+match_filter([], [], _Suffix) ->
+    % NOTE: we matched the prefix, but there may be more matches next
+    skip;
+match_filter([], ['#'], _Suffix) ->
     % NOTE: naturally, '#' < '+', so this is already optimal for `match/2`
     true;
-match_filter([], ['+' | _]) ->
+match_filter([], ['+' | _], _Suffix) ->
     plus;
-match_filter([], [_H | _]) ->
+match_filter([], [_H | _], _Suffix) ->
     false;
-match_filter([H | T1], [H | T2]) ->
-    match_filter(T1, T2);
-match_filter([H1 | _], [H2 | _]) when H2 > H1 ->
+match_filter([H | T1], [H | T2], Suffix) ->
+    match_filter(T1, T2, Suffix);
+match_filter([H1 | _], [H2 | _], _Suffix) when H2 > H1 ->
     % NOTE: we're strictly past the prefix, no need to continue
     stop.
 
diff --git a/apps/emqx/test/emqx_topic_index_SUITE.erl b/apps/emqx/test/emqx_topic_index_SUITE.erl
index b5faba4d9..75e1adaf6 100644
--- a/apps/emqx/test/emqx_topic_index_SUITE.erl
+++ b/apps/emqx/test/emqx_topic_index_SUITE.erl
@@ -144,6 +144,24 @@ t_match_unique(_) ->
         [id(M) || M <- emqx_topic_index:matches(<<"a/b/c">>, Tab, [unique])]
     ).
 
+t_match_wildcards(_) ->
+    Tab = emqx_topic_index:new(),
+    emqx_topic_index:insert(<<"a/b">>, id1, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/b/#">>, id2, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/b/#">>, id3, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/b/c">>, id4, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/b/+">>, id5, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/b/d">>, id6, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/+/+">>, id7, <<>>, Tab),
+    emqx_topic_index:insert(<<"a/+/#">>, id8, <<>>, Tab),
+
+    Records = [id(M) || M <- matches(<<"a/b/c">>, Tab)],
+    ?assertEqual([id2, id3, id4, id5, id7, id8], lists:sort(Records)),
+
+    Records1 = [id(M) || M <- matches(<<"a/b">>, Tab)],
+    ?assertEqual([id1, id2, id3, id8], lists:sort(Records1)),
+    ok.
+
 match(T, Tab) ->
     emqx_topic_index:match(T, Tab).