From 4b0ea562a28e0ac8ae874998fd8b12c40855b04c Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 21 Mar 2023 14:21:30 +0100 Subject: [PATCH] refactor(emqx_bridge): consistently use macros for http response --- apps/emqx_bridge/src/emqx_bridge_api.erl | 79 +++++++++++++----------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index e466b418d..917b44096 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/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(BridgeType, BridgeName), +-define(BRIDGE_NOT_FOUND(BRIDGE_TYPE, BRIDGE_NAME), ?NOT_FOUND( - <<"Bridge lookup failed: bridge named '", BridgeName/binary, "' of type ", - (atom_to_binary(BridgeType))/binary, " does not exist.">> + <<"Bridge lookup failed: bridge named '", BRIDGE_NAME/binary, "' of type ", + (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, - error_msg( - 'SERVICE_UNAVAILABLE', - 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(