fix: create consistent interface 'with_node' for API access

apps/emqx/include/emqx_api_lib.hrl

@@ -0,0 +1,36 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2021-2023 EMQ Technologies Co., Ltd. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%--------------------------------------------------------------------
+
+-ifndef(EMQX_API_LIB_HRL).
+-define(EMQX_API_LIB_HRL, true).
+
+-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)}).
+-endif.

apps/emqx/src/emqx_api_lib.erl

@@ -0,0 +1,69 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%--------------------------------------------------------------------
+
+-module(emqx_api_lib).
+
+-export([
+    with_node/2,
+    with_node_or_cluster/2
+]).
+
+-include("emqx_api_lib.hrl").
+
+-define(NODE_NOT_FOUND(NODE), ?NOT_FOUND(<<"Node not found: ", NODE/binary>>)).
+
+%%--------------------------------------------------------------------
+%% exported API
+%%--------------------------------------------------------------------
+-spec with_node(binary(), fun((atom()) -> {ok, term()} | {error, term()})) ->
+    ?OK(term()) | ?NOT_FOUND(binary()) | ?BAD_REQUEST(term()).
+with_node(BinNode, Fun) ->
+    case lookup_node(BinNode) of
+        {ok, Node} ->
+            handle_result(Fun(Node));
+        not_found ->
+            ?NODE_NOT_FOUND(BinNode)
+    end.
+
+-spec with_node_or_cluster(binary(), fun((atom()) -> {ok, term()} | {error, term()})) ->
+    ?OK(term()) | ?NOT_FOUND(iolist()) | ?BAD_REQUEST(term()).
+with_node_or_cluster(<<"all">>, Fun) ->
+    handle_result(Fun(all));
+with_node_or_cluster(Node, Fun) ->
+    with_node(Node, Fun).
+
+%%--------------------------------------------------------------------
+%% Internal
+%%--------------------------------------------------------------------
+
+-spec lookup_node(binary()) -> {ok, atom()} | not_found.
+lookup_node(BinNode) ->
+    case emqx_misc:safe_to_existing_atom(BinNode, utf8) of
+        {ok, Node} ->
+            case lists:member(Node, mria:running_nodes()) of
+                true ->
+                    {ok, Node};
+                false ->
+                    not_found
+            end;
+        _Error ->
+            not_found
+    end.
+
+handle_result({ok, Result}) ->
+    ?OK(Result);
+handle_result({error, Reason}) ->
+    ?BAD_REQUEST(Reason).

apps/emqx/test/emqx_api_lib_SUITE.erl

@@ -0,0 +1,101 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%--------------------------------------------------------------------
+
+-module(emqx_api_lib_SUITE).
+
+-compile(export_all).
+-compile(nowarn_export_all).
+
+-include("emqx_api_lib.hrl").
+-include_lib("eunit/include/eunit.hrl").
+
+-define(DUMMY, dummy_module).
+
+all() -> emqx_common_test_helpers:all(?MODULE).
+
+init_per_suite(Config) ->
+    emqx_common_test_helpers:boot_modules(all),
+    emqx_common_test_helpers:start_apps([]),
+    Config.
+
+end_per_suite(_Config) ->
+    emqx_common_test_helpers:stop_apps([]).
+
+init_per_testcase(_Case, Config) ->
+    meck:new(?DUMMY, [non_strict]),
+    meck:expect(?DUMMY, expect_not_called, 1, fun(Node) -> throw({blow_this_up, Node}) end),
+    meck:expect(?DUMMY, expect_success, 1, {ok, success}),
+    meck:expect(?DUMMY, expect_error, 1, {error, error}),
+    Config.
+
+end_per_testcase(_Case, _Config) ->
+    meck:unload(?DUMMY).
+
+t_with_node(_) ->
+    test_with(fun emqx_api_lib:with_node/2, [<<"all">>]).
+
+t_with_node_or_cluster(_) ->
+    test_with(fun emqx_api_lib:with_node_or_cluster/2, []),
+    meck:reset(?DUMMY),
+    ?assertEqual(
+        ?OK(success),
+        emqx_api_lib:with_node_or_cluster(
+            <<"all">>,
+            fun ?DUMMY:expect_success/1
+        )
+    ),
+    ?assertMatch([{_, {?DUMMY, expect_success, [all]}, {ok, success}}], meck:history(?DUMMY)).
+
+%% helpers
+test_with(TestFun, ExtraBadNodes) ->
+    % make sure this is an atom
+    'unknownnode@unknownnohost',
+    BadNodes =
+        [
+            <<"undefined">>,
+            <<"this_should_not_be_an_atom">>,
+            <<"unknownnode@unknownnohost">>
+        ] ++ ExtraBadNodes,
+    [ensure_not_found(TestFun(N, fun ?DUMMY:expect_not_called/1)) || N <- BadNodes],
+    ensure_not_called(?DUMMY, expect_not_called),
+    ensure_not_existing_atom(<<"this_should_not_be_an_atom">>),
+
+    GoodNode = node(),
+
+    ?assertEqual(
+        ?OK(success),
+        TestFun(GoodNode, fun ?DUMMY:expect_success/1)
+    ),
+
+    ?assertEqual(
+        ?BAD_REQUEST(error),
+        TestFun(GoodNode, fun ?DUMMY:expect_error/1)
+    ),
+    ok.
+
+ensure_not_found(Result) ->
+    ?assertMatch({404, _}, Result).
+
+ensure_not_called(Mod, Fun) ->
+    ?assert(not meck:called(Mod, Fun, '_')).
+
+ensure_not_existing_atom(Bin) ->
+    try binary_to_existing_atom(Bin) of
+        _ -> throw(is_atom)
+    catch
+        error:badarg ->
+            ok
+    end.

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -20,6 +20,7 @@
 -include_lib("typerefl/include/types.hrl").
 -include_lib("hocon/include/hoconsc.hrl").
 -include_lib("emqx/include/logger.hrl").
+-include_lib("emqx/include/emqx_api_lib.hrl").
 -include_lib("emqx_bridge/include/emqx_bridge.hrl").
 
 -import(hoconsc, [mk/2, array/1, enum/1]).
@@ -46,25 +47,6 @@
 
 -export([lookup_from_local_node/2]).
 
-%% [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">>)
 ).

apps/emqx_dashboard/src/emqx_dashboard_monitor_api.erl

@@ -121,32 +121,27 @@ fields(sampler_current) ->
 
 monitor(get, #{query_string := QS, bindings := Bindings}) ->
     Latest = maps:get(<<"latest">>, QS, infinity),
-    RawNode = maps:get(node, Bindings, all),
-    with_node(RawNode, dashboard_samplers_fun(Latest)).
+    RawNode = maps:get(node, Bindings, <<"all">>),
+    emqx_api_lib:with_node_or_cluster(RawNode, dashboard_samplers_fun(Latest)).
 
 dashboard_samplers_fun(Latest) ->
     fun(NodeOrCluster) ->
         case emqx_dashboard_monitor:samplers(NodeOrCluster, Latest) of
-            {badrpc, _} = Error -> Error;
+            {badrpc, _} = Error -> {error, Error};
             Samplers -> {ok, Samplers}
         end
     end.
 
 monitor_current(get, #{bindings := Bindings}) ->
-    RawNode = maps:get(node, Bindings, all),
-    with_node(RawNode, fun emqx_dashboard_monitor:current_rate/1).
-
-with_node(RawNode, Fun) ->
-    case emqx_misc:safe_to_existing_atom(RawNode, utf8) of
-        {ok, NodeOrCluster} ->
-            case Fun(NodeOrCluster) of
-                {badrpc, {Node, Reason}} ->
-                    {404, 'NOT_FOUND', io_lib:format("Node not found: ~p (~p)", [Node, Reason])};
-                {ok, Result} ->
-                    {200, Result}
-            end;
-        _Error ->
-            {404, 'NOT_FOUND', io_lib:format("Node not found: ~p", [RawNode])}
+    RawNode = maps:get(node, Bindings, <<"all">>),
+    emqx_api_lib:with_node_or_cluster(RawNode, fun current_rate/1).
+
+current_rate(Node) ->
+    case emqx_dashboard_monitor:current_rate(Node) of
+        {badrpc, _} = BadRpc ->
+            {error, BadRpc};
+        {ok, _} = OkResult ->
+            OkResult
     end.
 
 %% -------------------------------------------------------------------------------------------------

apps/emqx_management/src/emqx_mgmt_api_nodes.erl

@@ -17,7 +17,6 @@
 
 -behaviour(minirest_api).
 
--include_lib("emqx/include/emqx.hrl").
 -include_lib("typerefl/include/types.hrl").
 
 -import(hoconsc, [mk/2, ref/1, ref/2, enum/1, array/1]).
@@ -25,8 +24,6 @@
 -define(NODE_METRICS_MODULE, emqx_mgmt_api_metrics).
 -define(NODE_STATS_MODULE, emqx_mgmt_api_stats).
 
--define(SOURCE_ERROR, 'SOURCE_ERROR').
-
 %% Swagger specs from hocon schema
 -export([
     api_spec/0,
@@ -88,7 +85,7 @@ schema("/nodes/:node") ->
                             ref(node_info),
                             #{desc => <<"Get node info successfully">>}
                         ),
-                        400 => node_error()
+                        404 => not_found()
                     }
             }
     };
@@ -106,7 +103,7 @@ schema("/nodes/:node/metrics") ->
                             ref(?NODE_METRICS_MODULE, node_metrics),
                             #{desc => <<"Get node metrics successfully">>}
                         ),
-                        400 => node_error()
+                        404 => not_found()
                     }
             }
     };
@@ -124,7 +121,7 @@ schema("/nodes/:node/stats") ->
                             ref(?NODE_STATS_MODULE, node_stats_data),
                             #{desc => <<"Get node stats successfully">>}
                         ),
-                        400 => node_error()
+                        404 => not_found()
                     }
             }
     }.
@@ -136,7 +133,7 @@ fields(node_name) ->
     [
         {node,
             mk(
-                atom(),
+                binary(),
                 #{
                     in => path,
                     description => <<"Node name">>,
@@ -250,55 +247,46 @@ nodes(get, _Params) ->
     list_nodes(#{}).
 
 node(get, #{bindings := #{node := NodeName}}) ->
-    get_node(NodeName).
+    emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun get_node/1)).
 
 node_metrics(get, #{bindings := #{node := NodeName}}) ->
-    get_metrics(NodeName).
+    emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun emqx_mgmt:get_metrics/1)).
 
 node_stats(get, #{bindings := #{node := NodeName}}) ->
-    get_stats(NodeName).
+    emqx_api_lib:with_node(NodeName, to_ok_result_fun(fun emqx_mgmt:get_stats/1)).
 
 %%--------------------------------------------------------------------
 %% api apply
 
 list_nodes(#{}) ->
-    NodesInfo = [format(Node, NodeInfo) || {Node, NodeInfo} <- emqx_mgmt:list_nodes()],
+    NodesInfo = [format(NodeInfo) || {_Node, NodeInfo} <- emqx_mgmt:list_nodes()],
     {200, NodesInfo}.
 
 get_node(Node) ->
-    case emqx_mgmt:lookup_node(Node) of
-        {error, _} ->
-            {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}};
-        NodeInfo ->
-            {200, format(Node, NodeInfo)}
-    end.
-
-get_metrics(Node) ->
-    case emqx_mgmt:get_metrics(Node) of
-        {error, _} ->
-            {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}};
-        Metrics ->
-            {200, Metrics}
-    end.
-
-get_stats(Node) ->
-    case emqx_mgmt:get_stats(Node) of
-        {error, _} ->
-            {400, #{code => 'SOURCE_ERROR', message => <<"rpc_failed">>}};
-        Stats ->
-            {200, Stats}
-    end.
+    format(emqx_mgmt:lookup_node(Node)).
 
 %%--------------------------------------------------------------------
 %% internal function
 
-format(_Node, Info = #{memory_total := Total, memory_used := Used}) ->
+format(Info = #{memory_total := Total, memory_used := Used}) ->
     Info#{
         memory_total := emqx_mgmt_util:kmg(Total),
         memory_used := emqx_mgmt_util:kmg(Used)
     };
-format(_Node, Info) when is_map(Info) ->
+format(Info) when is_map(Info) ->
     Info.
 
-node_error() ->
-    emqx_dashboard_swagger:error_codes([?SOURCE_ERROR], <<"Node error">>).
+to_ok_result({error, _} = Error) ->
+    Error;
+to_ok_result({ok, _} = Ok) ->
+    Ok;
+to_ok_result(Result) ->
+    {ok, Result}.
+
+to_ok_result_fun(Fun) when is_function(Fun) ->
+    fun(Arg) ->
+        to_ok_result(Fun(Arg))
+    end.
+
+not_found() ->
+    emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Node not found">>).

apps/emqx_management/test/emqx_mgmt_api_nodes_SUITE.erl

@@ -68,7 +68,7 @@ t_nodes_api(_) ->
 
     BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode"]),
     ?assertMatch(
-        {error, {_, 400, _}},
+        {error, {_, 404, _}},
         emqx_mgmt_api_test_util:request_api(get, BadNodePath)
     ).
 
@@ -94,7 +94,7 @@ t_node_stats_api(_) ->
 
     BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode", "stats"]),
     ?assertMatch(
-        {error, {_, 400, _}},
+        {error, {_, 404, _}},
         emqx_mgmt_api_test_util:request_api(get, BadNodePath)
     ).
 
@@ -112,7 +112,7 @@ t_node_metrics_api(_) ->
 
     BadNodePath = emqx_mgmt_api_test_util:api_path(["nodes", "badnode", "metrics"]),
     ?assertMatch(
-        {error, {_, 400, _}},
+        {error, {_, 404, _}},
         emqx_mgmt_api_test_util:request_api(get, BadNodePath)
     ).
 

changes/ce/fix-10237.en.md

@@ -0,0 +1 @@
+Ensure we return `404` status code for unknown node names in `/nodes/:node[/metrics|/stats]` API.