Merge pull request #9958 from HJianBo/refine-clients-http-return

fix(api): fix bad return error message if client id is not found
2023-02-14 20:13:44 +08:00 · 2023-02-14 20:13:44 +08:00 · 0753146f65
parent 43aab61a3a 0d63cfdc97
commit 0753146f65
5 changed files with 74 additions and 27 deletions
--- a/apps/emqx_management/src/emqx_management.app.src
+++ b/apps/emqx_management/src/emqx_management.app.src
@ -2,7 +2,7 @@
 {application, emqx_management, [
    {description, "EMQX Management API and CLI"},
    % strict semver, bump manually!
-    {vsn, "5.0.14"},
+    {vsn, "5.0.15"},
    {modules, []},
    {registered, [emqx_management_sup]},
    {applications, [kernel, stdlib, emqx_plugins, minirest, emqx]},
--- a/apps/emqx_management/src/emqx_mgmt_api_clients.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_clients.erl
@ -76,9 +76,10 @@

 -define(FORMAT_FUN, {?MODULE, format_channel_info}).

-define(CLIENT_ID_NOT_FOUND,
-    <<"{\"code\": \"RESOURCE_NOT_FOUND\", \"reason\": \"Client id not found\"}">>
-).
+-define(CLIENTID_NOT_FOUND, #{
+    code => 'CLIENTID_NOT_FOUND',
+    message => <<"Client ID not found">>
+}).

 api_spec() ->
    emqx_dashboard_swagger:spec(?MODULE, #{check_schema => true, translate_body => true}).
@ -219,7 +220,7 @@ schema("/clients/:clientid") ->
            responses => #{
                200 => hoconsc:mk(hoconsc:ref(?MODULE, client), #{}),
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        },
@ -232,7 +233,7 @@ schema("/clients/:clientid") ->
            responses => #{
                204 => <<"Kick out client successfully">>,
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -247,7 +248,7 @@ schema("/clients/:clientid/authorization/cache") ->
            responses => #{
                200 => hoconsc:mk(hoconsc:ref(?MODULE, authz_cache), #{}),
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        },
@ -256,9 +257,9 @@ schema("/clients/:clientid/authorization/cache") ->
            tags => ?TAGS,
            parameters => [{clientid, hoconsc:mk(binary(), #{in => path})}],
            responses => #{
-                204 => <<"Kick out client successfully">>,
+                204 => <<"Clean client authz cache successfully">>,
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -273,10 +274,11 @@ schema("/clients/:clientid/subscriptions") ->
            responses => #{
                200 => hoconsc:mk(
                    hoconsc:array(hoconsc:ref(emqx_mgmt_api_subscriptions, subscription)), #{}
-                ),
-                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
                )
+                %% returns [] if client not existed in cluster
+                %404 => emqx_dashboard_swagger:error_codes(
+                %    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
+                %)
            }
        }
    };
@ -291,7 +293,7 @@ schema("/clients/:clientid/subscribe") ->
            responses => #{
                200 => hoconsc:ref(emqx_mgmt_api_subscriptions, subscription),
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -307,7 +309,7 @@ schema("/clients/:clientid/subscribe/bulk") ->
            responses => #{
                200 => hoconsc:array(hoconsc:ref(emqx_mgmt_api_subscriptions, subscription)),
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -323,7 +325,7 @@ schema("/clients/:clientid/unsubscribe") ->
            responses => #{
                204 => <<"Unsubscribe OK">>,
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -339,7 +341,7 @@ schema("/clients/:clientid/unsubscribe/bulk") ->
            responses => #{
                204 => <<"Unsubscribe OK">>,
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -355,7 +357,7 @@ schema("/clients/:clientid/keepalive") ->
            responses => #{
                200 => hoconsc:mk(hoconsc:ref(?MODULE, client), #{}),
                404 => emqx_dashboard_swagger:error_codes(
-                    ['CLIENTID_NOT_FOUND'], <<"Client id not found">>
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                )
            }
        }
@ -621,7 +623,7 @@ set_keepalive(put, #{bindings := #{clientid := ClientID}, body := Body}) ->
        {ok, Interval} ->
            case emqx_mgmt:set_keepalive(emqx_mgmt_util:urldecode(ClientID), Interval) of
                ok -> lookup(#{clientid => ClientID});
-                {error, not_found} -> {404, ?CLIENT_ID_NOT_FOUND};
+                {error, not_found} -> {404, ?CLIENTID_NOT_FOUND};
                {error, Reason} -> {400, #{code => 'PARAMS_ERROR', message => Reason}}
            end
    end.
@ -669,7 +671,7 @@ list_clients(QString) ->
 lookup(#{clientid := ClientID}) ->
    case emqx_mgmt:lookup_client({clientid, ClientID}, ?FORMAT_FUN) of
        [] ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        ClientInfo ->
            {200, hd(ClientInfo)}
    end.
@ -677,7 +679,7 @@ lookup(#{clientid := ClientID}) ->
 kickout(#{clientid := ClientID}) ->
    case emqx_mgmt:kickout_client({ClientID, ?FORMAT_FUN}) of
        {error, not_found} ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        _ ->
            {204}
    end.
@ -685,7 +687,7 @@ kickout(#{clientid := ClientID}) ->
 get_authz_cache(#{clientid := ClientID}) ->
    case emqx_mgmt:list_authz_cache(ClientID) of
        {error, not_found} ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        {error, Reason} ->
            Message = list_to_binary(io_lib:format("~p", [Reason])),
            {500, #{code => <<"UNKNOW_ERROR">>, message => Message}};
@ -699,7 +701,7 @@ clean_authz_cache(#{clientid := ClientID}) ->
        ok ->
            {204};
        {error, not_found} ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        {error, Reason} ->
            Message = list_to_binary(io_lib:format("~p", [Reason])),
            {500, #{code => <<"UNKNOW_ERROR">>, message => Message}}
@ -709,7 +711,7 @@ subscribe(#{clientid := ClientID, topic := Topic} = Sub) ->
    Opts = maps:with([qos, nl, rap, rh], Sub),
    case do_subscribe(ClientID, Topic, Opts) of
        {error, channel_not_found} ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        {error, Reason} ->
            Message = list_to_binary(io_lib:format("~p", [Reason])),
            {500, #{code => <<"UNKNOW_ERROR">>, message => Message}};
@ -723,7 +725,7 @@ subscribe_batch(#{clientid := ClientID, topics := Topics}) ->
    %% has returned. So if one want to subscribe topics in this hook, it will fail.
    case ets:lookup(emqx_channel, ClientID) of
        [] ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        _ ->
            ArgList = [
                [ClientID, Topic, maps:with([qos, nl, rap, rh], Sub)]
@ -735,7 +737,7 @@ subscribe_batch(#{clientid := ClientID, topics := Topics}) ->
 unsubscribe(#{clientid := ClientID, topic := Topic}) ->
    case do_unsubscribe(ClientID, Topic) of
        {error, channel_not_found} ->
-            {404, ?CLIENT_ID_NOT_FOUND};
+            {404, ?CLIENTID_NOT_FOUND};
        {unsubscribe, [{Topic, #{}}]} ->
            {204}
    end.
@ -745,8 +747,8 @@ unsubscribe_batch(#{clientid := ClientID, topics := Topics}) ->
        {200, _} ->
            _ = emqx_mgmt:unsubscribe_batch(ClientID, Topics),
            {204};
-        {404, ?CLIENT_ID_NOT_FOUND} ->
-            {404, ?CLIENT_ID_NOT_FOUND}
+        {404, NotFound} ->
+            {404, NotFound}
    end.

 %%--------------------------------------------------------------------
--- a/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl
@ -247,6 +247,49 @@ t_keepalive(_Config) ->
    emqtt:disconnect(C1),
    ok.

+t_client_id_not_found(_Config) ->
+    AuthHeader = emqx_mgmt_api_test_util:auth_header_(),
+    Http = {"HTTP/1.1", 404, "Not Found"},
+    Body = "{\"code\":\"CLIENTID_NOT_FOUND\",\"message\":\"Client ID not found\"}",
+
+    PathFun = fun(Suffix) ->
+        emqx_mgmt_api_test_util:api_path(["clients", "no_existed_clientid"] ++ Suffix)
+    end,
+    ReqFun = fun(Method, Path) ->
+        emqx_mgmt_api_test_util:request_api(
+            Method, Path, "", AuthHeader, [], #{return_all => true}
+        )
+    end,
+
+    PostFun = fun(Method, Path, Data) ->
+        emqx_mgmt_api_test_util:request_api(
+            Method, Path, "", AuthHeader, Data, #{return_all => true}
+        )
+    end,
+
+    %% Client lookup
+    ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun([]))),
+    %% Client kickout
+    ?assertMatch({error, {Http, _, Body}}, ReqFun(delete, PathFun([]))),
+    %% Client Subscription list
+    ?assertMatch({ok, {{"HTTP/1.1", 200, "OK"}, _, "[]"}}, ReqFun(get, PathFun(["subscriptions"]))),
+    %% AuthZ Cache lookup
+    ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun(["authorization", "cache"]))),
+    %% AuthZ Cache clean
+    ?assertMatch({error, {Http, _, Body}}, ReqFun(delete, PathFun(["authorization", "cache"]))),
+    %% Client Subscribe
+    SubBody = #{topic => <<"testtopic">>, qos => 1, nl => 1, rh => 1},
+    ?assertMatch({error, {Http, _, Body}}, PostFun(post, PathFun(["subscribe"]), SubBody)),
+    ?assertMatch(
+        {error, {Http, _, Body}}, PostFun(post, PathFun(["subscribe", "bulk"]), [SubBody])
+    ),
+    %% Client Unsubscribe
+    UnsubBody = #{topic => <<"testtopic">>},
+    ?assertMatch({error, {Http, _, Body}}, PostFun(post, PathFun(["unsubscribe"]), UnsubBody)),
+    ?assertMatch(
+        {error, {Http, _, Body}}, PostFun(post, PathFun(["unsubscribe", "bulk"]), [UnsubBody])
+    ).
+
 time_string_to_epoch_millisecond(DateTime) ->
    time_string_to_epoch(DateTime, millisecond).

--- a/changes/v5.0.18/fix-9958.en.md
+++ b/changes/v5.0.18/fix-9958.en.md
@ -0,0 +1 @@
+Fix bad http response format when client ID is not found in `clients` APIs
--- a/changes/v5.0.18/fix-9958.zh.md
+++ b/changes/v5.0.18/fix-9958.zh.md
@ -0,0 +1 @@
+修复 `clients` API 在 Client ID 不存在时返回的错误的 HTTP 应答格式。
				`@ -0,0 +1 @@`
				Fix bad http response format when client ID is not found in `clients` APIs
				`@ -0,0 +1 @@`
				修复 `clients` API 在 Client ID 不存在时返回的错误的 HTTP 应答格式。