fix(exhook): refine exhook and exhook api

Refine status code.
Fix ssl ciphers raw list.
Make sure config update error be thrown.

apps/emqx_exhook/src/emqx_exhook_api.erl

@@ -23,14 +23,26 @@
 -include_lib("emqx/include/logger.hrl").
 -include_lib("hocon/include/hoconsc.hrl").
 
--export([api_spec/0, paths/0, schema/1, fields/1, namespace/0]).
+-export([
+    api_spec/0,
+    paths/0,
+    schema/1,
+    fields/1,
+    namespace/0
+]).
 
--export([exhooks/2, action_with_name/2, move/2, server_hooks/2]).
+-export([
+    exhooks/2,
+    action_with_name/2,
+    move/2,
+    server_hooks/2
+]).
 
--import(hoconsc, [mk/2, ref/1, enum/1, array/1, map/2]).
+-import(hoconsc, [mk/1, mk/2, ref/1, enum/1, array/1, map/2]).
 -import(emqx_dashboard_swagger, [schema_with_example/2, error_codes/2]).
 
 -define(TAGS, [<<"exhooks">>]).
+-define(NOT_FOURD, 'NOT_FOUND').
 -define(BAD_REQUEST, 'BAD_REQUEST').
 -define(BAD_RPC, 'BAD_RPC').
 
@@ -58,14 +70,15 @@ schema(("/exhooks")) ->
         get => #{
             tags => ?TAGS,
             desc => ?DESC(list_all_servers),
-            responses => #{200 => mk(array(ref(detail_server_info)), #{})}
+            responses => #{200 => mk(array(ref(detail_server_info)))}
         },
         post => #{
             tags => ?TAGS,
             desc => ?DESC(add_server),
             'requestBody' => server_conf_schema(),
             responses => #{
-                201 => mk(ref(detail_server_info), #{}),
+                200 => mk(ref(detail_server_info)),
+                400 => error_codes([?BAD_REQUEST], <<"Already exists">>),
                 500 => error_codes([?BAD_RPC], <<"Bad RPC">>)
             }
         }
@@ -78,8 +91,8 @@ schema("/exhooks/:name") ->
             desc => ?DESC(get_detail),
             parameters => params_server_name_in_path(),
             responses => #{
-                200 => mk(ref(detail_server_info), #{}),
+                200 => mk(ref(detail_server_info)),
-                400 => error_codes([?BAD_REQUEST], <<"Bad Request">>)
+                404 => error_codes([?NOT_FOURD], <<"Server not found">>)
             }
         },
         put => #{
@@ -88,8 +101,9 @@ schema("/exhooks/:name") ->
             parameters => params_server_name_in_path(),
             'requestBody' => server_conf_schema(),
             responses => #{
-                200 => <<>>,
+                200 => mk(ref(detail_server_info)),
                 400 => error_codes([?BAD_REQUEST], <<"Bad Request">>),
+                404 => error_codes([?NOT_FOURD], <<"Server not found">>),
                 500 => error_codes([?BAD_RPC], <<"Bad RPC">>)
             }
         },
@@ -99,6 +113,7 @@ schema("/exhooks/:name") ->
             parameters => params_server_name_in_path(),
             responses => #{
                 204 => <<>>,
+                404 => error_codes([?NOT_FOURD], <<"Server not found">>),
                 500 => error_codes([?BAD_RPC], <<"Bad RPC">>)
             }
         }
@@ -111,7 +126,7 @@ schema("/exhooks/:name/hooks") ->
             desc => ?DESC(get_hooks),
             parameters => params_server_name_in_path(),
             responses => #{
-                200 => mk(array(ref(list_hook_info)), #{}),
+                200 => mk(array(ref(list_hook_info))),
                 400 => error_codes([?BAD_REQUEST], <<"Bad Request">>)
             }
         }
@@ -149,7 +164,7 @@ fields(detail_server_info) ->
         {metrics, mk(ref(metrics), #{desc => ?DESC(server_metrics)})},
         {node_metrics, mk(array(ref(node_metrics)), #{desc => ?DESC(node_metrics)})},
         {node_status, mk(array(ref(node_status)), #{desc => ?DESC(node_status)})},
-        {hooks, mk(array(ref(hook_info)), #{})}
+        {hooks, mk(array(ref(hook_info)))}
     ] ++ emqx_exhook_schema:server_config();
 fields(list_hook_info) ->
     [
@@ -220,13 +235,19 @@ server_conf_schema() ->
 %% API
 %%--------------------------------------------------------------------
 exhooks(get, _) ->
-    Confs = emqx:get_config([exhook, servers]),
+    Confs = get_raw_config(),
     Infos = nodes_all_server_info(Confs),
     {200, Infos};
-exhooks(post, #{body := Body}) ->
+exhooks(post, #{body := #{<<"name">> := Name} = Body}) ->
-    {ok, _} = emqx_exhook_mgr:update_config([exhook, servers], {add, Body}),
+    case emqx_exhook_mgr:update_config([exhook, servers], {add, Body}) of
-    #{<<"name">> := Name} = Body,
+        {ok, _} ->
-    get_nodes_server_info(Name).
+            get_nodes_server_info(Name);
+        {error, already_exists} ->
+            {400, #{
+                code => <<"BAD_REQUEST">>,
+                message => <<"Already exists">>
+            }}
+    end.
 
 action_with_name(get, #{bindings := #{name := Name}}) ->
     get_nodes_server_info(Name);
@@ -237,20 +258,13 @@ action_with_name(put, #{bindings := #{name := Name}, body := Body}) ->
             {update, Name, Body}
         )
     of
-        {ok, not_found} ->
+        {ok, _} ->
-            {400, #{
+            get_nodes_server_info(Name);
-                code => <<"BAD_REQUEST">>,
+        {error, not_found} ->
+            {404, #{
+                code => <<"NOT_FOUND">>,
                 message => <<"Server not found">>
             }};
-        {ok, {error, Reason}} ->
-            {400, #{
-                code => <<"BAD_REQUEST">>,
-                message => unicode:characters_to_binary(
-                    io_lib:format("Error Reason:~p~n", [Reason])
-                )
-            }};
-        {ok, _} ->
-            {200};
         {error, Error} ->
             {500, #{
                 code => <<"BAD_RPC">>,
@@ -265,7 +279,12 @@ action_with_name(delete, #{bindings := #{name := Name}}) ->
         )
     of
         {ok, _} ->
-            {200};
+            {204};
+        {error, not_found} ->
+            {404, #{
+                code => <<"BAD_REQUEST">>,
+                message => <<"Server not found">>
+            }};
         {error, Error} ->
             {500, #{
                 code => <<"BAD_RPC">>,
@@ -284,8 +303,8 @@ move(post, #{bindings := #{name := Name}, body := #{<<"position">> := RawPositio
             of
                 {ok, ok} ->
                     {204};
-                {ok, not_found} ->
+                {error, not_found} ->
-                    {400, #{
+                    {404, #{
                         code => <<"BAD_REQUEST">>,
                         message => <<"Server not found">>
                     }};
@@ -303,8 +322,8 @@ move(post, #{bindings := #{name := Name}, body := #{<<"position">> := RawPositio
     end.
 
 server_hooks(get, #{bindings := #{name := Name}}) ->
-    Confs = emqx:get_config([exhook, servers]),
+    Confs = get_raw_config(),
-    case lists:search(fun(#{name := CfgName}) -> CfgName =:= Name end, Confs) of
+    case lists:search(fun(#{<<"name">> := CfgName}) -> CfgName =:= Name end, Confs) of
         false ->
             {400, #{
                 code => <<"BAD_REQUEST">>,
@@ -316,10 +335,10 @@ server_hooks(get, #{bindings := #{name := Name}}) ->
     end.
 
 get_nodes_server_info(Name) ->
-    Confs = emqx:get_config([exhook, servers]),
+    Confs = get_raw_config(),
-    case lists:search(fun(#{name := CfgName}) -> CfgName =:= Name end, Confs) of
+    case lists:search(fun(#{<<"name">> := CfgName}) -> CfgName =:= Name end, Confs) of
         false ->
-            {400, #{
+            {404, #{
                 code => <<"BAD_REQUEST">>,
                 message => <<"Server not found">>
             }};
@@ -336,7 +355,7 @@ nodes_all_server_info(ConfL) ->
     Default = emqx_exhook_metrics:new_metrics_info(),
     node_all_server_info(ConfL, AllInfos, Default, []).
 
-node_all_server_info([#{name := ServerName} = Conf | T], AllInfos, Default, Acc) ->
+node_all_server_info([#{<<"name">> := ServerName} = Conf | T], AllInfos, Default, Acc) ->
     Info = fill_cluster_server_info(AllInfos, [], [], ServerName, Default),
     AllInfo = maps:merge(Conf, Info),
     node_all_server_info(T, AllInfos, Default, [AllInfo | Acc]);
@@ -449,6 +468,14 @@ call_cluster(Fun) ->
 %% Internal Funcs
 %%--------------------------------------------------------------------
 
+get_raw_config() ->
+    RawConfig = emqx:get_raw_config([exhook, servers], []),
+    Schema = #{roots => emqx_exhook_schema:fields(exhook), fields => #{}},
+    Conf = #{<<"servers">> => RawConfig},
+    Options = #{only_fill_defaults => true},
+    #{<<"servers">> := Servers} = hocon_tconf:check_plain(Schema, Conf, Options),
+    Servers.
+
 position_example() ->
     #{
         front =>

apps/emqx_exhook/src/emqx_exhook_mgr.erl

@@ -147,22 +147,24 @@ update_config(KeyPath, UpdateReq) ->
             Error
     end.
 
-pre_config_update(_, {add, Conf}, OldConf) ->
+pre_config_update(_, {add, #{<<"name">> := Name} = Conf}, OldConf) ->
-    {ok, OldConf ++ [Conf]};
+    case lists:any(fun(#{<<"name">> := ExistedName}) -> ExistedName =:= Name end, OldConf) of
+        true -> throw(already_exists);
+        false -> {ok, OldConf ++ [Conf]}
+    end;
 pre_config_update(_, {update, Name, Conf}, OldConf) ->
     case replace_conf(Name, fun(_) -> Conf end, OldConf) of
-        not_found -> {error, not_found};
+        not_found -> throw(not_found);
         NewConf -> {ok, NewConf}
     end;
 pre_config_update(_, {delete, ToDelete}, OldConf) ->
-    {ok,
+    case do_delete(ToDelete, OldConf) of
-        lists:dropwhile(
+        not_found -> throw(not_found);
-            fun(#{<<"name">> := Name}) -> Name =:= ToDelete end,
+        NewConf -> {ok, NewConf}
-            OldConf
+    end;
-        )};
 pre_config_update(_, {move, Name, Position}, OldConf) ->
     case do_move(Name, Position, OldConf) of
-        not_found -> {error, not_found};
+        not_found -> throw(not_found);
         NewConf -> {ok, NewConf}
     end;
 pre_config_update(_, {enable, Name, Enable}, OldConf) ->
@@ -173,7 +175,7 @@ pre_config_update(_, {enable, Name, Enable}, OldConf) ->
             OldConf
         )
     of
-        not_found -> {error, not_found};
+        not_found -> throw(not_found);
         NewConf -> {ok, NewConf}
     end.
 
@@ -420,6 +422,19 @@ move_to([H | T], Position, Server, HeadL) ->
 move_to([], _Position, _Server, _HeadL) ->
     not_found.
 
+-spec do_delete(binary(), list(server_options())) ->
+    not_found | list(server_options()).
+do_delete(ToDelete, OldConf) ->
+    case lists:any(fun(#{<<"name">> := ExistedName}) -> ExistedName =:= ToDelete end, OldConf) of
+        true ->
+            lists:dropwhile(
+                fun(#{<<"name">> := Name}) -> Name =:= ToDelete end,
+                OldConf
+            );
+        false ->
+            not_found
+    end.
+
 -spec reorder(list(server_options()), servers()) -> servers().
 reorder(ServerL, Servers) ->
     Orders = reorder(ServerL, 1, Servers),