Merge pull request #14055 from savonarola/1022-fix-client-filtering

fix(client_api): apply query to search results

apps/emqx_management/src/emqx_mgmt_api_clients.erl

@@ -1004,7 +1004,7 @@ do_list_clients_v2(Nodes, _Cursor = #{type := ?CURSOR_TYPE_DS, iterator := Iter0
     {Rows0, Iter} = emqx_persistent_session_ds_state:session_iterator_next(Iter0, Limit),
     NewCursor = next_ds_cursor(Iter),
     Rows1 = check_for_live_and_expired(Rows0),
-    Rows = maybe_run_fuzzy_filter(Rows1, QString0),
+    Rows = run_filters(Rows1, QString0),
     Acc1 = maps:update_with(rows, fun(Rs) -> [{undefined, Rows} | Rs] end, Acc0),
     Acc = #{n := N} = maps:update_with(n, fun(N) -> N + length(Rows) end, Acc1),
     case N >= Limit of
@@ -1063,19 +1063,22 @@ parse_qstring(QString) ->
             ?BAD_REQUEST('INVALID_PARAMETER', Message)
     end.
 
-maybe_run_fuzzy_filter(Rows, QString0) ->
+run_filters(Rows, QString0) ->
     {_NClauses, {QString1, FuzzyQString1}} = emqx_mgmt_api:parse_qstring(QString0, ?CLIENT_QSCHEMA),
-    {_QString, FuzzyQString} = adapt_custom_filters(QString1, FuzzyQString1),
+    {QString, FuzzyQString} = adapt_custom_filters(QString1, FuzzyQString1),
     FuzzyFilterFn = fuzzy_filter_fun(FuzzyQString),
-    case FuzzyFilterFn of
-        undefined ->
-            Rows;
-        {Fn, Args} ->
-            lists:filter(
-                fun(E) -> erlang:apply(Fn, [E | Args]) end,
-                Rows
-            )
-    end.
+    lists:filter(
+        fun(Row) ->
+            does_row_match_query(Row, QString) andalso
+                does_row_match_fuzzy_filter(Row, FuzzyFilterFn)
+        end,
+        Rows
+    ).
+
+does_row_match_fuzzy_filter(_Row, undefined) ->
+    true;
+does_row_match_fuzzy_filter(Row, {Fn, Args}) ->
+    erlang:apply(Fn, [Row | Args]).
 
 %% These filters, while they are able to be adapted to efficient ETS match specs, must be
 %% used as fuzzy filters when iterating over offlient persistent clients, which live
@@ -1084,7 +1087,7 @@ adapt_custom_filters(Qs, Fuzzy) ->
     lists:foldl(
         fun
             ({Field, '=:=', X}, {QsAcc, FuzzyAcc}) when
-                Field =:= username
+                Field =:= username orelse Field =:= clientid
             ->
                 Xs = wrap(X),
                 {QsAcc, [{Field, in, Xs} | FuzzyAcc]};
@@ -1711,6 +1714,55 @@ ms(connected_at, X) ->
 ms(created_at, X) ->
     #{session => #{created_at => X}}.
 
+%%--------------------------------------------------------------------
+%% Filter funcs
+%% These functions are used to filter durable clients. Durable clients are not stored in
+%% ETS tables, so we cannot use matchspecs to filter them. Instead, we filter ready rows,
+%% like fuzzy filters do.
+%%
+%% These functions are used with clients_v2 API.
+
+does_chan_info_match({ip_address, '=:=', IpAddress}, #{conninfo := #{peername := {IpAddress, _}}}) ->
+    true;
+does_chan_info_match({conn_state, '=:=', State}, #{conn_state := State}) ->
+    true;
+does_chan_info_match({clean_start, '=:=', CleanStart}, #{conninfo := #{clean_start := CleanStart}}) ->
+    true;
+does_chan_info_match({proto_ver, '=:=', ProtoVer}, #{conninfo := #{proto_ver := ProtoVer}}) ->
+    true;
+does_chan_info_match({connected_at, '>=', ConnectedAtFrom}, #{
+    conninfo := #{connected_at := ConnectedAt}
+}) when
+    ConnectedAt >= ConnectedAtFrom
+->
+    true;
+does_chan_info_match({connected_at, '=<', ConnectedAtTo}, #{
+    conninfo := #{connected_at := ConnectedAt}
+}) when
+    ConnectedAt =< ConnectedAtTo
+->
+    true;
+does_chan_info_match({created_at, '>=', CreatedAtFrom}, #{session := #{created_at := CreatedAt}}) when
+    CreatedAt >= CreatedAtFrom
+->
+    true;
+does_chan_info_match({created_at, '=<', CreatedAtTo}, #{session := #{created_at := CreatedAt}}) when
+    CreatedAt =< CreatedAtTo
+->
+    true;
+does_chan_info_match(_, _) ->
+    false.
+
+does_row_match_query(
+    {_Id, #{metadata := #{offline_info := #{chan_info := ChanInfo}}}}, CompiledQueryString
+) ->
+    lists:all(
+        fun(FieldQuery) -> does_chan_info_match(FieldQuery, ChanInfo) end,
+        CompiledQueryString
+    );
+does_row_match_query(_, _) ->
+    false.
+
 %%--------------------------------------------------------------------
 %% Match funcs
 

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

@@ -70,6 +70,7 @@ persistent_session_testcases() ->
         t_persistent_sessions_subscriptions1,
         t_list_clients_v2,
         t_list_clients_v2_exact_filters,
+        t_list_clients_v2_regular_filters,
         t_list_clients_v2_bad_query_string_parameters
     ].
 non_persistent_cluster_testcases() ->
@@ -1910,6 +1911,55 @@ t_list_clients_v2_exact_filters(Config) ->
     ),
     ok.
 
+%% Checks that regular filters (non fuzzy and not username) work in clients_v2 API.
+t_list_clients_v2_regular_filters(Config) ->
+    [N1, _N2] = ?config(nodes, Config),
+    Port1 = get_mqtt_port(N1, tcp),
+    Id = fun(Bin) -> iolist_to_binary([atom_to_binary(?FUNCTION_NAME), <<"-">>, Bin]) end,
+    ?check_trace(
+        begin
+            ConnectedAt = binary_to_list(emqx_utils_calendar:now_to_rfc3339()),
+            ClientId1 = Id(<<"ps1-offline">>),
+            Username1 = Id(<<"u1">>),
+            C1 = connect_client(#{
+                port => Port1,
+                clientid => ClientId1,
+                clean_start => false,
+                username => Username1
+            }),
+            stop_and_commit(C1),
+            %% Let the client goes offline from emqx_cm
+            timer:sleep(100),
+
+            QueryParams1 = [
+                {"limit", "100"},
+                {"gte_connected_at", ConnectedAt}
+            ],
+            Res1 = list_all_v2(QueryParams1, Config),
+            ?assertContainsClientids(Res1, [ClientId1]),
+
+            QueryParams2 = [
+                {"limit", "100"},
+                {"lte_connected_at", ConnectedAt}
+            ],
+            Res2 = list_all_v2(QueryParams2, Config),
+            ?assertMatch(
+                [
+                    #{
+                        <<"data">> := [],
+                        <<"meta">> := #{<<"count">> := 0}
+                    }
+                ],
+                Res2
+            ),
+
+            ?tp(warning, destroy_session, #{clientid => ClientId1}),
+            ok = erpc:call(N1, emqx_persistent_session_ds, destroy_session, [ClientId1])
+        end,
+        []
+    ),
+    ok.
+
 %% Checks that we return pretty errors when user uses bad value types for a query string
 %% parameter.
 t_list_clients_v2_bad_query_string_parameters(Config) ->

changes/ce/fix-14055.en.md

@@ -0,0 +1 @@
+In `/clients_v2` API, respect filtering arguments when querying offline clients with durable sessions. Previously, only `username` specifier was respected, and all other filtering arguments were ignored.