Merge pull request #13647 from thalesmg/20240819-r58-clients-v2-filter-ip

chore(clients v2 mgmt api): return prettier errors when user furnishes bad values

apps/emqx_management/src/emqx_mgmt_api_clients.erl

@@ -16,6 +16,8 @@
 
 -module(emqx_mgmt_api_clients).
 
+-feature(maybe_expr, enable).
+
 -behaviour(minirest_api).
 
 -include_lib("typerefl/include/types.hrl").
@@ -981,14 +983,16 @@ do_list_clients_v2(Nodes, Cursor, QString0) ->
 do_list_clients_v2(_Nodes, Cursor = done, _QString, Acc) ->
     format_results(Acc, Cursor);
 do_list_clients_v2(Nodes, Cursor = #{type := ?CURSOR_TYPE_ETS, node := Node}, QString0, Acc0) ->
-    {Rows, NewCursor} = do_ets_select(Nodes, QString0, Cursor),
-    Acc1 = maps:update_with(rows, fun(Rs) -> [{Node, Rows} | Rs] end, Acc0),
-    Acc = #{limit := Limit, n := N} = maps:update_with(n, fun(N) -> N + length(Rows) end, Acc1),
-    case N >= Limit of
-        true ->
-            format_results(Acc, NewCursor);
-        false ->
-            do_list_clients_v2(Nodes, NewCursor, QString0, Acc)
+    maybe
+        {ok, {Rows, NewCursor}} ?= do_ets_select(Nodes, QString0, Cursor),
+        Acc1 = maps:update_with(rows, fun(Rs) -> [{Node, Rows} | Rs] end, Acc0),
+        Acc = #{limit := Limit, n := N} = maps:update_with(n, fun(N) -> N + length(Rows) end, Acc1),
+        case N >= Limit of
+            true ->
+                format_results(Acc, NewCursor);
+            false ->
+                do_list_clients_v2(Nodes, NewCursor, QString0, Acc)
+        end
     end;
 do_list_clients_v2(Nodes, _Cursor = #{type := ?CURSOR_TYPE_DS, iterator := Iter0}, QString0, Acc0) ->
     #{limit := Limit} = Acc0,
@@ -1031,12 +1035,28 @@ format_results(Acc, Cursor) ->
     ?OK(Resp).
 
 do_ets_select(Nodes, QString0, #{node := Node, node_idx := NodeIdx, cont := Cont} = _Cursor) ->
-    {_, QString1} = emqx_mgmt_api:parse_qstring(QString0, ?CLIENT_QSCHEMA),
-    Limit = maps:get(<<"limit">>, QString0, 10),
-    {Rows, #{cont := NewCont, node_idx := NewNodeIdx}} = ets_select(
-        QString1, Limit, Node, NodeIdx, Cont
-    ),
-    {Rows, next_ets_cursor(Nodes, NewNodeIdx, NewCont)}.
+    maybe
+        {ok, {_, QString1}} ?= parse_qstring(QString0),
+        Limit = maps:get(<<"limit">>, QString0, 10),
+        {Rows, #{cont := NewCont, node_idx := NewNodeIdx}} = ets_select(
+            QString1, Limit, Node, NodeIdx, Cont
+        ),
+        {ok, {Rows, next_ets_cursor(Nodes, NewNodeIdx, NewCont)}}
+    end.
+
+parse_qstring(QString) ->
+    try
+        {ok, emqx_mgmt_api:parse_qstring(QString, ?CLIENT_QSCHEMA)}
+    catch
+        throw:{bad_value_type, {Key, ExpectedType, AcutalValue}} ->
+            Message = list_to_binary(
+                io_lib:format(
+                    "the ~s parameter expected type is ~s, but the value is ~s",
+                    [Key, ExpectedType, emqx_utils_conv:str(AcutalValue)]
+                )
+            ),
+            ?BAD_REQUEST('INVALID_PARAMETER', Message)
+    end.
 
 maybe_run_fuzzy_filter(Rows, QString0) ->
     {_NClauses, {QString1, FuzzyQString1}} = emqx_mgmt_api:parse_qstring(QString0, ?CLIENT_QSCHEMA),

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

@@ -69,7 +69,8 @@ persistent_session_testcases() ->
         t_persistent_sessions6,
         t_persistent_sessions_subscriptions1,
         t_list_clients_v2,
-        t_list_clients_v2_exact_filters
+        t_list_clients_v2_exact_filters,
+        t_list_clients_v2_bad_query_string_parameters
     ].
 non_persistent_cluster_testcases() ->
     [
@@ -1896,7 +1897,7 @@ t_list_clients_v2_exact_filters(Config) ->
             C3B = connect_client(#{port => Port1, clientid => ClientId3}),
             C4B = connect_client(#{port => Port2, clientid => ClientId4}),
 
-            lists:foreach(fun stop_and_commit/1, [C1, C2, C3B, C4B]),
+            lists:foreach(fun disconnect_and_destroy_session/1, [C1, C2, C3B, C4B]),
             lists:foreach(fun emqtt:stop/1, [C5, C6]),
 
             ok
@@ -1905,6 +1906,29 @@ t_list_clients_v2_exact_filters(Config) ->
     ),
     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) ->
+    ?check_trace(
+        begin
+            QueryParams1 = [
+                {"ip_address", "10.50.0.0:60748"}
+            ],
+            Res1 = simplify_result(list_v2_request(QueryParams1, Config)),
+            ?assertMatch(
+                {400, #{
+                    <<"message">> :=
+                        <<"the ip_address parameter expected type is ip, but the value is",
+                            _/binary>>
+                }},
+                Res1
+            ),
+            ok
+        end,
+        []
+    ),
+    ok.
+
 t_cursor_serde_prop(_Config) ->
     ?assert(proper:quickcheck(cursor_serde_prop(), [{numtests, 100}, {to_file, user}])).