refactor(emqx_bridge): consistently use macros for http response

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -46,18 +46,33 @@
 
 -export([lookup_from_local_node/2]).
 
--define(BAD_REQUEST(Reason), {400, error_msg('BAD_REQUEST', Reason)}).
+%% [TODO] Move those to a commonly shared header file
+-define(ERROR_MSG(CODE, REASON), #{code => CODE, message => emqx_misc:readable_error_msg(REASON)}).
+
+-define(OK(CONTENT), {200, CONTENT}).
+
+-define(NO_CONTENT, 204).
+
+-define(BAD_REQUEST(CODE, REASON), {400, ?ERROR_MSG(CODE, REASON)}).
+-define(BAD_REQUEST(REASON), ?BAD_REQUEST('BAD_REQUEST', REASON)).
+
+-define(NOT_FOUND(REASON), {404, ?ERROR_MSG('NOT_FOUND', REASON)}).
+
+-define(INTERNAL_ERROR(REASON), {500, ?ERROR_MSG('INTERNAL_ERROR', REASON)}).
+
+-define(NOT_IMPLEMENTED, 501).
+
+-define(SERVICE_UNAVAILABLE(REASON), {503, ?ERROR_MSG('SERVICE_UNAVAILABLE', REASON)}).
+%% End TODO
 
 -define(BRIDGE_NOT_ENABLED,
     ?BAD_REQUEST(<<"Forbidden operation, bridge not enabled">>)
 ).
 
--define(NOT_FOUND(Reason), {404, error_msg('NOT_FOUND', Reason)}).
+-define(BRIDGE_NOT_FOUND(BRIDGE_TYPE, BRIDGE_NAME),
-
--define(BRIDGE_NOT_FOUND(BridgeType, BridgeName),
     ?NOT_FOUND(
-        <<"Bridge lookup failed: bridge named '", BridgeName/binary, "' of type ",
+        <<"Bridge lookup failed: bridge named '", BRIDGE_NAME/binary, "' of type ",
-            (atom_to_binary(BridgeType))/binary, " does not exist.">>
+            (atom_to_binary(BRIDGE_TYPE))/binary, " does not exist.">>
     )
 ).
 
@@ -480,7 +495,7 @@ schema("/bridges_probe") ->
 '/bridges'(post, #{body := #{<<"type">> := BridgeType, <<"name">> := BridgeName} = Conf0}) ->
     case emqx_bridge:lookup(BridgeType, BridgeName) of
         {ok, _} ->
-            {400, error_msg('ALREADY_EXISTS', <<"bridge already exists">>)};
+            ?BAD_REQUEST('ALREADY_EXISTS', <<"bridge already exists">>);
         {error, not_found} ->
             Conf = filter_out_request_body(Conf0),
             {ok, _} = emqx_bridge:create(BridgeType, BridgeName, Conf),
@@ -495,9 +510,9 @@ schema("/bridges_probe") ->
                 format_resource(Data, Node)
              || {Node, Bridges} <- lists:zip(Nodes, NodeBridges), Data <- Bridges
             ],
-            {200, zip_bridges([AllBridges])};
+            ?OK(zip_bridges([AllBridges]));
         {error, Reason} ->
-            {500, error_msg('INTERNAL_ERROR', Reason)}
+            ?INTERNAL_ERROR(Reason)
     end.
 
 '/bridges/:id'(get, #{bindings := #{id := Id}}) ->
@@ -529,16 +544,16 @@ schema("/bridges_probe") ->
                     end,
                 case emqx_bridge:check_deps_and_remove(BridgeType, BridgeName, AlsoDeleteActs) of
                     {ok, _} ->
-                        204;
+                        ?NO_CONTENT;
                     {error, {rules_deps_on_this_bridge, RuleIds}} ->
                         ?BAD_REQUEST(
                             {<<"Cannot delete bridge while active rules are defined for this bridge">>,
                                 RuleIds}
                         );
                     {error, timeout} ->
-                        {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
+                        ?SERVICE_UNAVAILABLE(<<"request timeout">>);
                     {error, Reason} ->
-                        {500, error_msg('INTERNAL_ERROR', Reason)}
+                        ?INTERNAL_ERROR(Reason)
                 end;
             {error, not_found} ->
                 ?BRIDGE_NOT_FOUND(BridgeType, BridgeName)
@@ -555,7 +570,7 @@ schema("/bridges_probe") ->
             ok = emqx_bridge_resource:reset_metrics(
                 emqx_bridge_resource:resource_id(BridgeType, BridgeName)
             ),
-            {204}
+            ?NO_CONTENT
         end
     ).
 
@@ -566,9 +581,9 @@ schema("/bridges_probe") ->
             Params1 = maybe_deobfuscate_bridge_probe(Params),
             case emqx_bridge_resource:create_dry_run(ConnType, maps:remove(<<"type">>, Params1)) of
                 ok ->
-                    204;
+                    ?NO_CONTENT;
                 {error, Reason} when not is_tuple(Reason); element(1, Reason) =/= 'exit' ->
-                    {400, error_msg('TEST_FAILED', emqx_misc:readable_error_msg(Reason))}
+                    ?BAD_REQUEST('TEST_FAILED', Reason)
             end;
         BadRequest ->
             BadRequest
@@ -602,7 +617,7 @@ do_lookup_from_all_nodes(BridgeType, BridgeName, SuccCode, FormatFun) ->
         {ok, [{error, not_found} | _]} ->
             ?BRIDGE_NOT_FOUND(BridgeType, BridgeName);
         {error, Reason} ->
-            {500, error_msg('INTERNAL_ERROR', Reason)}
+            ?INTERNAL_ERROR(Reason)
     end.
 
 lookup_from_local_node(BridgeType, BridgeName) ->
@@ -620,15 +635,15 @@ lookup_from_local_node(BridgeType, BridgeName) ->
             OperFunc ->
                 case emqx_bridge:disable_enable(OperFunc, BridgeType, BridgeName) of
                     {ok, _} ->
-                        204;
+                        ?NO_CONTENT;
                     {error, {pre_config_update, _, bridge_not_found}} ->
                         ?BRIDGE_NOT_FOUND(BridgeType, BridgeName);
                     {error, {_, _, timeout}} ->
-                        {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
+                        ?SERVICE_UNAVAILABLE(<<"request timeout">>);
                     {error, timeout} ->
-                        {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
+                        ?SERVICE_UNAVAILABLE(<<"request timeout">>);
                     {error, Reason} ->
-                        {500, error_msg('INTERNAL_ERROR', Reason)}
+                        ?INTERNAL_ERROR(Reason)
                 end
         end
     ).
@@ -943,9 +958,6 @@ filter_out_request_body(Conf) ->
     ],
     maps:without(ExtraConfs, Conf).
 
-error_msg(Code, Msg) ->
-    #{code => Code, message => emqx_misc:readable_error_msg(Msg)}.
-
 bin(S) when is_list(S) ->
     list_to_binary(S);
 bin(S) when is_atom(S) ->
@@ -956,30 +968,23 @@ bin(S) when is_binary(S) ->
 call_operation(NodeOrAll, OperFunc, Args = [_Nodes, BridgeType, BridgeName]) ->
     case is_ok(do_bpapi_call(NodeOrAll, OperFunc, Args)) of
         Ok when Ok =:= ok; is_tuple(Ok), element(1, Ok) =:= ok ->
-            204;
+            ?NO_CONTENT;
         {error, not_implemented} ->
             %% Should only happen if we call `start` on a node that is
             %% still on an older bpapi version that doesn't support it.
             maybe_try_restart(NodeOrAll, OperFunc, Args);
         {error, timeout} ->
-            {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
+            ?SERVICE_UNAVAILABLE(<<"Request timeout">>);
         {error, {start_pool_failed, Name, Reason}} ->
-            {503,
+            ?SERVICE_UNAVAILABLE(
-                error_msg(
+                bin(io_lib:format("Failed to start ~p pool for reason ~p", [Name, Reason]))
-                    'SERVICE_UNAVAILABLE',
+            );
-                    bin(
-                        io_lib:format(
-                            "failed to start ~p pool for reason ~p",
-                            [Name, Reason]
-                        )
-                    )
-                )};
         {error, not_found} ->
             ?BRIDGE_NOT_FOUND(BridgeType, BridgeName);
         {error, {node_not_found, Node}} ->
             ?NOT_FOUND(<<"Node not found: ", (atom_to_binary(Node))/binary>>);
         {error, Reason} when not is_tuple(Reason); element(1, Reason) =/= 'exit' ->
-            ?BAD_REQUEST(emqx_misc:readable_error_msg(Reason))
+            ?BAD_REQUEST(Reason)
     end.
 
 maybe_try_restart(all, start_bridges_to_all_nodes, Args) ->
@@ -987,7 +992,7 @@ maybe_try_restart(all, start_bridges_to_all_nodes, Args) ->
 maybe_try_restart(Node, start_bridge_to_node, Args) ->
     call_operation(Node, restart_bridge_to_node, Args);
 maybe_try_restart(_, _, _) ->
-    501.
+    ?NOT_IMPLEMENTED.
 
 do_bpapi_call(all, Call, Args) ->
     maybe_unwrap(