yuanbiao
/
emqx
1
0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #11211 from sstrigler/EMQX-9963-refactor-all-http-ap-is-to-respond-404-on-the-deletion-of-the-resource-is-non-exist 

consistently return 404 if resource not found
This commit is contained in:
Stefan Strigler 2023-07-10 16:25:31 +02:00 committed by GitHub
parent bd0ab87c76 da052e0a5e
commit 93a9772743
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 173 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
apps/emqx_gateway/src/emqx_gateway_api_clients.erl
View File

@ -57,7 +57,8 @@
qs2ms/2,
run_fuzzy_filter/2,
format_channel_info/1,
format_channel_info/2
format_channel_info/2,
client_info_mountpoint/1
]).
-define(TAGS, [<<"Gateway Clients">>]).
@ -177,8 +178,12 @@ clients_insta(delete, #{
}
}) ->
with_gateway(Name0, fun(GwName, _) ->
_ = emqx_gateway_http:kickout_client(GwName, ClientId),
case emqx_gateway_http:kickout_client(GwName, ClientId) of
{error, not_found} ->
return_http_error(404, "Client not found");
_ ->
{204}
end
end).
%% List the established subscriptions with mountpoint
@ -234,8 +239,13 @@ subscriptions(delete, #{
}
}) ->
with_gateway(Name0, fun(GwName, _) ->
case lookup_topic(GwName, ClientId, Topic) of
{ok, _} ->
_ = emqx_gateway_http:client_unsubscribe(GwName, ClientId, Topic),
{204}
{204};
{error, not_found} ->
return_http_error(404, "Resource not found")
end
end).
%%--------------------------------------------------------------------
@ -260,6 +270,34 @@ extra_sub_props(Props) ->
#{subid => maps:get(<<"subid">>, Props, undefined)}
).
lookup_topic(GwName, ClientId, Topic) ->
Mountpoints = emqx_gateway_http:lookup_client(
GwName,
ClientId,
{?MODULE, client_info_mountpoint}
),
case emqx_gateway_http:list_client_subscriptions(GwName, ClientId) of
{ok, Subscriptions} ->
case
[
S
|| S = #{topic := Topic0} <- Subscriptions,
Mountpoint <- Mountpoints,
Topic0 == emqx_mountpoint:mount(Mountpoint, Topic)
]
of
[] ->
{error, not_found};
Filtered ->
{ok, Filtered}
end;
Error ->
Error
end.
client_info_mountpoint({_, #{clientinfo := #{mountpoint := Mountpoint}}, _}) ->
Mountpoint.
%%--------------------------------------------------------------------
%% QueryString to MatchSpec

7
apps/emqx_gateway/src/emqx_gateway_api_listeners.erl
View File

@ -112,8 +112,13 @@ listeners(post, #{bindings := #{name := Name0}, body := LConf}) ->
listeners_insta(delete, #{bindings := #{name := Name0, id := ListenerId}}) ->
with_gateway(Name0, fun(_GwName, _) ->
case emqx_gateway_conf:listener(ListenerId) of
{ok, _Listener} ->
ok = emqx_gateway_http:remove_listener(ListenerId),
{204}
{204};
{error, not_found} ->
return_http_error(404, "Listener not found")
end
end);
listeners_insta(get, #{bindings := #{name := Name0, id := ListenerId}}) ->
with_gateway(Name0, fun(_GwName, _) ->

2
apps/emqx_gateway/src/emqx_gateway_http.erl
View File

@ -550,7 +550,7 @@ with_gateway(GwName0, Fun) ->
return_http_error(400, [K, " is required"]);
%% Exceptions from emqx_gateway_utils:parse_listener_id/1
error:{invalid_listener_id, Id} ->
return_http_error(400, ["Invalid listener id: ", Id]);
return_http_error(404, ["Listener not found: ", Id]);
%% Exceptions from emqx:get_config/1
error:{config_not_found, Path0} ->
Path = lists:concat(

52
apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl
View File

@ -409,6 +409,7 @@ t_listeners_tcp(_) ->
{204, _} = request(delete, "/gateways/stomp/listeners/stomp:tcp:def"),
{404, _} = request(get, "/gateways/stomp/listeners/stomp:tcp:def"),
{404, _} = request(delete, "/gateways/stomp/listeners/stomp:tcp:def"),
ok.
t_listeners_max_conns(_) ->
@ -480,9 +481,19 @@ t_listeners_authn(_) ->
{200, ConfResp3} = request(get, Path),
assert_confs(AuthConf2, ConfResp3),
{404, _} = request(get, Path ++ "/users/not_exists"),
{404, _} = request(delete, Path ++ "/users/not_exists"),
{204, _} = request(delete, Path),
%% FIXME: 204?
{204, _} = request(get, Path),
BadPath = "/gateways/stomp/listeners/stomp:tcp:not_exists/authentication/users/foo",
{404, _} = request(get, BadPath),
{404, _} = request(delete, BadPath),
{404, _} = request(get, "/gateways/stomp/listeners/not_exists"),
{404, _} = request(delete, "/gateways/stomp/listeners/not_exists"),
ok.
t_listeners_authn_data_mgmt(_) ->
@ -575,6 +586,47 @@ t_listeners_authn_data_mgmt(_) ->
ok.
t_clients(_) ->
GwConf = #{
name => <<"mqttsn">>,
gateway_id => 1,
broadcast => true,
predefined => [#{id => 1, topic => <<"t/a">>}],
enable_qos3 => true,
listeners => [
#{name => <<"def">>, type => <<"udp">>, bind => <<"1884">>}
]
},
init_gw("mqttsn", GwConf),
Path = "/gateways/mqttsn/clients",
MyClient = Path ++ "/my_client",
MyClientSubscriptions = MyClient ++ "/subscriptions",
{200, NoClients} = request(get, Path),
?assertMatch(#{data := []}, NoClients),
ClientSocket = emqx_gateway_test_utils:sn_client_connect(<<"my_client">>),
{200, _} = request(get, MyClient),
{200, Clients} = request(get, Path),
?assertMatch(#{data := [#{clientid := <<"my_client">>}]}, Clients),
{201, _} = request(post, MyClientSubscriptions, #{topic => <<"test/topic">>}),
{200, Subscriptions} = request(get, MyClientSubscriptions),
?assertMatch([#{topic := <<"test/topic">>}], Subscriptions),
{204, _} = request(delete, MyClientSubscriptions ++ "/test%2Ftopic"),
{200, []} = request(get, MyClientSubscriptions),
{404, _} = request(delete, MyClientSubscriptions ++ "/test%2Ftopic"),
{204, _} = request(delete, MyClient),
{404, _} = request(delete, MyClient),
{404, _} = request(get, MyClient),
{404, _} = request(get, MyClientSubscriptions),
{404, _} = request(post, MyClientSubscriptions, #{topic => <<"foo">>}),
{404, _} = request(delete, MyClientSubscriptions ++ "/topic"),
{200, NoClients2} = request(get, Path),
?assertMatch(#{data := []}, NoClients2),
emqx_gateway_test_utils:sn_client_disconnect(ClientSocket),
ok.
t_authn_fuzzy_search(_) ->
init_gw("stomp"),
AuthConf = #{

16
apps/emqx_gateway/test/emqx_gateway_cli_SUITE.erl
View File

@ -54,6 +54,8 @@ end).
"}\n"
).
-import(emqx_gateway_test_utils, [sn_client_connect/1, sn_client_disconnect/1]).
%%--------------------------------------------------------------------
%% Setup
%%--------------------------------------------------------------------
@ -303,17 +305,3 @@ acc_print(Acc) ->
after 200 ->
Acc
end.
sn_client_connect(ClientId) ->
{ok, Socket} = gen_udp:open(0, [binary]),
_ = emqx_sn_protocol_SUITE:send_connect_msg(Socket, ClientId),
?assertEqual(
<<3, 16#05, 0>>,
emqx_sn_protocol_SUITE:receive_response(Socket)
),
Socket.
sn_client_disconnect(Socket) ->
_ = emqx_sn_protocol_SUITE:send_disconnect_msg(Socket, undefined),
gen_udp:close(Socket),
ok.

16
apps/emqx_gateway/test/emqx_gateway_test_utils.erl
View File

@ -19,6 +19,8 @@
-compile(export_all).
-compile(nowarn_export_all).
-include_lib("eunit/include/eunit.hrl").
assert_confs(Expected0, Effected) ->
Expected = maybe_unconvert_listeners(Expected0),
case do_assert_confs(root, Expected, Effected) of
@ -181,3 +183,17 @@ url(Path, Qs) ->
auth(Headers) ->
[emqx_mgmt_api_test_util:auth_header_() | Headers].
sn_client_connect(ClientId) ->
{ok, Socket} = gen_udp:open(0, [binary]),
_ = emqx_sn_protocol_SUITE:send_connect_msg(Socket, ClientId),
?assertEqual(
<<3, 16#05, 0>>,
emqx_sn_protocol_SUITE:receive_response(Socket)
),
Socket.
sn_client_disconnect(Socket) ->
_ = emqx_sn_protocol_SUITE:send_disconnect_msg(Socket, undefined),
gen_udp:close(Socket),
ok.

44
apps/emqx_management/src/emqx_mgmt_api_listeners.erl
View File

@ -45,14 +45,10 @@
update/3
]).
-include_lib("emqx/include/emqx.hrl").
-include_lib("hocon/include/hoconsc.hrl").
-define(NODE_LISTENER_NOT_FOUND, <<"Node name or listener id not found">>).
-define(NODE_NOT_FOUND_OR_DOWN, <<"Node not found or Down">>).
-define(LISTENER_NOT_FOUND, <<"Listener id not found">>).
-define(LISTENER_ID_INCONSISTENT, <<"Path and body's listener id not match">>).
-define(ADDR_PORT_INUSE, <<"Addr port in use">>).
namespace() -> "listeners".
@ -156,7 +152,7 @@ schema("/listeners/:id") ->
parameters => [?R_REF(listener_id)],
responses => #{
204 => <<"Listener deleted">>,
400 => error_codes(['BAD_REQUEST'])
404 => error_codes(['BAD_LISTENER_ID'])
}
}
};
@ -405,20 +401,8 @@ list_listeners(get, #{query_string := Query}) ->
list_listeners(post, #{body := Body}) ->
create_listener(Body).
crud_listeners_by_id(get, #{bindings := #{id := Id0}}) ->
Listeners =
[
Conf#{
<<"id">> => Id,
<<"type">> => Type,
<<"bind">> := iolist_to_binary(
emqx_listeners:format_bind(maps:get(<<"bind">>, Conf))
)
}
|| {Id, Type, Conf} <- emqx_listeners:list_raw(),
Id =:= Id0
],
case Listeners of
crud_listeners_by_id(get, #{bindings := #{id := Id}}) ->
case find_listeners_by_id(Id) of
[] -> {404, #{code => 'BAD_LISTENER_ID', message => ?LISTENER_NOT_FOUND}};
[L] -> {200, L}
end;
@ -449,9 +433,12 @@ crud_listeners_by_id(post, #{body := Body}) ->
create_listener(Body);
crud_listeners_by_id(delete, #{bindings := #{id := Id}}) ->
{ok, #{type := Type, name := Name}} = emqx_listeners:parse_listener_id(Id),
case ensure_remove(Type, Name) of
{ok, _} -> {204};
{error, Reason} -> {400, #{code => 'BAD_REQUEST', message => err_msg(Reason)}}
case find_listeners_by_id(Id) of
[_L] ->
{ok, _Res} = ensure_remove(Type, Name),
{204};
[] ->
{404, #{code => 'BAD_LISTENER_ID', message => ?LISTENER_NOT_FOUND}}
end.
parse_listener_conf(Conf0) ->
@ -585,6 +572,19 @@ do_list_listeners() ->
<<"listeners">> => Listeners
}.
find_listeners_by_id(Id) ->
[
Conf#{
<<"id">> => Id0,
<<"type">> => Type,
<<"bind">> := iolist_to_binary(
emqx_listeners:format_bind(maps:get(<<"bind">>, Conf))
)
}
|| {Id0, Type, Conf} <- emqx_listeners:list_raw(),
Id0 =:= Id
].
wrap_rpc({badrpc, Reason}) ->
{error, Reason};
wrap_rpc(Res) ->

3
apps/emqx_management/src/emqx_mgmt_api_plugins.erl
View File

@ -132,6 +132,7 @@ schema("/plugins/:name") ->
parameters => [hoconsc:ref(name)],
responses => #{
204 => <<"Uninstall successfully">>,
400 => emqx_dashboard_swagger:error_codes(['PARAM_ERROR'], <<"Bad parameter">>),
404 => emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Plugin Not Found">>)
}
}
@ -484,6 +485,8 @@ ensure_action(Name, restart) ->
return(Code, ok) ->
{Code};
return(_, {error, #{error := "bad_info_file", return := {enoent, _} = Reason}}) ->
{404, #{code => 'NOT_FOUND', message => iolist_to_binary(io_lib:format("~p", [Reason]))}};
return(_, {error, Reason}) ->
{400, #{code => 'PARAM_ERROR', message => iolist_to_binary(io_lib:format("~p", [Reason]))}}.

7
apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl
View File

@ -399,12 +399,15 @@ crud_listeners_by_id(ListenerId, NewListenerId, MinListenerId, BadId, Type, Port
?assertEqual([], delete(MinPath)),
?assertEqual({error, not_found}, is_running(NewListenerId)),
?assertMatch({error, {"HTTP/1.1", 404, _}}, request(get, NewPath, [], [])),
?assertEqual([], delete(NewPath)),
?assertMatch({error, {"HTTP/1.1", 404, _}}, request(delete, NewPath, [], [])),
ok.
t_delete_nonexistent_listener(Config) when is_list(Config) ->
NonExist = emqx_mgmt_api_test_util:api_path(["listeners", "tcp:nonexistent"]),
?assertEqual([], delete(NonExist)),
?assertMatch(
{error, {_, 404, _}},
request(delete, NonExist, [], [])
),
ok.
t_action_listeners(Config) when is_list(Config) ->

8
apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl
View File

@ -133,6 +133,14 @@ t_bad_plugin(Config) ->
)
).
t_delete_non_existing(_Config) ->
Path = emqx_mgmt_api_test_util:api_path(["plugins", "non_exists"]),
?assertMatch(
{error, {_, 404, _}},
emqx_mgmt_api_test_util:request_api(delete, Path)
),
ok.
list_plugins() ->
Path = emqx_mgmt_api_test_util:api_path(["plugins"]),
case emqx_mgmt_api_test_util:request_api(get, Path) of

2
apps/emqx_retainer/src/emqx_retainer.app.src
View File

@ -2,7 +2,7 @@
{application, emqx_retainer, [
{description, "EMQX Retainer"},
% strict semver, bump manually!
{vsn, "5.0.14"},
{vsn, "5.0.15"},
{modules, []},
{registered, [emqx_retainer_sup]},
{applications, [kernel, stdlib, emqx, emqx_ctl]},

11
apps/emqx_retainer/src/emqx_retainer_api.erl
View File

@ -102,6 +102,7 @@ schema(?PREFIX ++ "/message/:topic") ->
parameters => parameters(),
responses => #{
204 => <<>>,
404 => error_codes(['NOT_FOUND'], ?DESC(message_not_exist)),
400 => error_codes(
['BAD_REQUEST'],
?DESC(unsupported_backend)
@ -187,8 +188,16 @@ with_topic(get, #{bindings := Bindings}) ->
end;
with_topic(delete, #{bindings := Bindings}) ->
Topic = maps:get(topic, Bindings),
case emqx_retainer_mnesia:page_read(undefined, Topic, 1, 1) of
{ok, []} ->
{404, #{
code => <<"NOT_FOUND">>,
message => <<"Viewed message doesn't exist">>
}};
{ok, _} ->
emqx_retainer_mnesia:delete_message(undefined, Topic),
{204}.
{204}
end.
format_message(#message{
id = ID,

1
apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl
View File

@ -218,6 +218,7 @@ t_lookup_and_delete(_) ->
{ok, []} = request_api(delete, API),
{error, {"HTTP/1.1", 404, "Not Found"}} = request_api(get, API),
{error, {"HTTP/1.1", 404, "Not Found"}} = request_api(delete, API),
ok = emqtt:disconnect(C1).

1
changes/ce/fix-11211.en.md Normal file
View File

@ -0,0 +1 @@
Consistently return `404` for `DELETE` operations on non-existent resources.