From 33e011aff551b9e9efb187e590dd2fbb1537b0dc Mon Sep 17 00:00:00 2001 From: Erik Timan Date: Fri, 20 Jan 2023 13:48:35 +0100 Subject: [PATCH 1/4] fix(emqx_management): handle multiple routes in topics/{topic} API The topics/{topic} API endpoint would return 500 - Internal Error if a topic had multiple routes. This is now fixed by returning a list of routes. --- .../emqx_management/src/emqx_mgmt_api_topics.erl | 7 ++++--- .../test/emqx_mgmt_api_topics_SUITE.erl | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_topics.erl b/apps/emqx_management/src/emqx_mgmt_api_topics.erl index a64badd3a..4100269e5 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_topics.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_topics.erl @@ -75,7 +75,7 @@ schema("/topics/:topic") -> tags => ?TAGS, parameters => [topic_param(path)], responses => #{ - 200 => hoconsc:mk(hoconsc:ref(topic), #{}), + 200 => hoconsc:mk(hoconsc:array(hoconsc:ref(topic)), #{}), 404 => emqx_dashboard_swagger:error_codes(['TOPIC_NOT_FOUND'], <<"Topic not found">>) } @@ -130,8 +130,9 @@ lookup(#{topic := Topic}) -> case emqx_router:lookup_routes(Topic) of [] -> {404, #{code => ?TOPIC_NOT_FOUND, message => <<"Topic not found">>}}; - [Route] -> - {200, format(Route)} + Routes when is_list(Routes) -> + Formatted = [format(Route) || Route <- Routes], + {200, Formatted} end. %%%============================================================================================== diff --git a/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl index dcea88d59..70bf1a780 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl @@ -72,8 +72,18 @@ t_nodes_api(_) -> ), %% get topics/:topic + %% We add another route here to ensure that the response handles + %% multiple routes for a single topic + DummyNode = 'dummy-node-name', + ok = emqx_router:add_route(Topic, DummyNode), RoutePath = emqx_mgmt_api_test_util:api_path(["topics", Topic]), {ok, RouteResponse} = emqx_mgmt_api_test_util:request_api(get, RoutePath), - RouteData = emqx_json:decode(RouteResponse, [return_maps]), - ?assertEqual(Topic, maps:get(<<"topic">>, RouteData)), - ?assertEqual(Node, maps:get(<<"node">>, RouteData)). + ok = emqx_router:delete_route(Topic, DummyNode), + + [ + #{<<"topic">> := Topic, <<"node">> := Node1}, + #{<<"topic">> := Topic, <<"node">> := Node2} + ] = emqx_json:decode(RouteResponse, [return_maps]), + + DummyNodeBin = atom_to_binary(DummyNode), + ?assertEqual(lists:usort([Node, DummyNodeBin]), lists:usort([Node1, Node2])). From 85d3c5cfd83ee980924c91a42edf2c388104298c Mon Sep 17 00:00:00 2001 From: Erik Timan Date: Fri, 20 Jan 2023 14:42:08 +0100 Subject: [PATCH 2/4] chore: update changes --- changes/v5.0.16/fix-9824.en.md | 1 + changes/v5.0.16/fix-9824.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/v5.0.16/fix-9824.en.md create mode 100644 changes/v5.0.16/fix-9824.zh.md diff --git a/changes/v5.0.16/fix-9824.en.md b/changes/v5.0.16/fix-9824.en.md new file mode 100644 index 000000000..29aa93264 --- /dev/null +++ b/changes/v5.0.16/fix-9824.en.md @@ -0,0 +1 @@ +The `topics/{topic}` API endpoint would return `500 - Internal Error` if a topic had multiple routes. This is fixed by returning a list of routes. diff --git a/changes/v5.0.16/fix-9824.zh.md b/changes/v5.0.16/fix-9824.zh.md new file mode 100644 index 000000000..143a39c16 --- /dev/null +++ b/changes/v5.0.16/fix-9824.zh.md @@ -0,0 +1 @@ +修复:当存在多个路由信息时,topics/{topic} 将会返回 500 - Internal Error 的问题,现在将会正确的返回路由信息列表。 From 03cabf6b26c786a1d643c025d2261e99773bd837 Mon Sep 17 00:00:00 2001 From: Erik Timan Date: Mon, 30 Jan 2023 08:51:40 +0100 Subject: [PATCH 3/4] chore: bump app VSN --- apps/emqx_management/src/emqx_management.app.src | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/emqx_management/src/emqx_management.app.src b/apps/emqx_management/src/emqx_management.app.src index ccb53dac4..158d65b6b 100644 --- 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.12"}, + {vsn, "5.0.13"}, {modules, []}, {registered, [emqx_management_sup]}, {applications, [kernel, stdlib, emqx_plugins, minirest, emqx]}, From 5b3a77e3c7f44a54e2a7a17ca8764eede76dde11 Mon Sep 17 00:00:00 2001 From: Erik Timan Date: Mon, 30 Jan 2023 14:02:27 +0100 Subject: [PATCH 4/4] test(emqx_management): fix flaky route handling in get topic test This reworks a test case to use a second slave node. This ensures that an added route is permanently in the routing table. The old version reverted the routing table quickly since the node name given wasn't a real node. --- .../test/emqx_mgmt_api_topics_SUITE.erl | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl index 70bf1a780..8f9b224ef 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_topics_SUITE.erl @@ -19,18 +19,25 @@ -compile(nowarn_export_all). -include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). + +-define(ROUTE_TAB, emqx_route). all() -> emqx_common_test_helpers:all(?MODULE). init_per_suite(Config) -> emqx_mgmt_api_test_util:init_suite(), - Config. + Slave = emqx_common_test_helpers:start_slave(some_node, []), + [{slave, Slave} | Config]. -end_per_suite(_) -> +end_per_suite(Config) -> + Slave = ?config(slave, Config), + emqx_common_test_helpers:stop_slave(Slave), + mria:clear_table(?ROUTE_TAB), emqx_mgmt_api_test_util:end_suite(). -t_nodes_api(_) -> +t_nodes_api(Config) -> Node = atom_to_binary(node(), utf8), Topic = <<"test_topic">>, {ok, Client} = emqtt:start_link(#{ @@ -74,16 +81,15 @@ t_nodes_api(_) -> %% get topics/:topic %% We add another route here to ensure that the response handles %% multiple routes for a single topic - DummyNode = 'dummy-node-name', - ok = emqx_router:add_route(Topic, DummyNode), + Slave = ?config(slave, Config), + ok = emqx_router:add_route(Topic, Slave), RoutePath = emqx_mgmt_api_test_util:api_path(["topics", Topic]), {ok, RouteResponse} = emqx_mgmt_api_test_util:request_api(get, RoutePath), - ok = emqx_router:delete_route(Topic, DummyNode), + ok = emqx_router:delete_route(Topic, Slave), [ #{<<"topic">> := Topic, <<"node">> := Node1}, #{<<"topic">> := Topic, <<"node">> := Node2} ] = emqx_json:decode(RouteResponse, [return_maps]), - DummyNodeBin = atom_to_binary(DummyNode), - ?assertEqual(lists:usort([Node, DummyNodeBin]), lists:usort([Node1, Node2])). + ?assertEqual(lists:usort([Node, atom_to_binary(Slave)]), lists:usort([Node1, Node2])).