Apply review comments

esl · Dec 5, 2022 · 5e66fe0 · 5e66fe0
1 parent bd0c840
commit 5e66fe0
Show file tree

Hide file tree

Showing 7 changed files with 275 additions and 119 deletions.
diff --git a/big_tests/tests/graphql_session_SUITE.erl b/big_tests/tests/graphql_session_SUITE.erl
@@ -303,7 +303,10 @@ admin_list_sessions_story(Config, _Alice, AliceB, _Bob) ->
     ?assertEqual(1, length(Sessions2)),
     Res3 = list_sessions(unprep(BisDomain), Config),
     Sessions3 = get_ok_value(Path, Res3),
-    ?assertEqual(1, length(Sessions3)).
+    ?assertEqual(1, length(Sessions3)),
+    % List sessions for a non-existing domain
+    Res4 = list_sessions(<<"nonexisting">>, Config),
+    ?assertEqual(<<"Domain not found">>, get_err_msg(Res4)).
 
 admin_count_sessions(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}, {alice_bis, 1}, {bob, 1}],
@@ -320,7 +323,10 @@ admin_count_sessions_story(Config, _Alice, AliceB, _Bob) ->
     Res2 = count_sessions(BisDomain, Config),
     ?assertEqual(1,  get_ok_value(Path, Res2)),
     Res3 = count_sessions(unprep(BisDomain), Config),
-    ?assertEqual(1,  get_ok_value(Path, Res3)).
+    ?assertEqual(1,  get_ok_value(Path, Res3)),
+    % Count sessions for a non-existing domain
+    Res4 = count_sessions(<<"nonexisting">>, Config),
+    ?assertEqual(<<"Domain not found">>, get_err_msg(Res4)).
 
 admin_list_user_sessions(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 2}, {bob, 1}],
@@ -334,7 +340,11 @@ admin_list_user_sessions_story(Config, Alice, Alice2, _Bob) ->
     ExpectedRes = lists:map(fun escalus_utils:jid_to_lower/1, [S1JID, S2JID]),
     Sessions = get_ok_value(Path, Res),
     ?assertEqual(2, length(Sessions)),
-    check_users(ExpectedRes, Sessions).
+    check_users(ExpectedRes, Sessions),
+    % Check for a non-existing user
+    Domain = domain(),
+    Res2 = list_user_sessions(<<"alien@", Domain/binary>>, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res2)).
 
 admin_count_user_resources(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 3}], fun admin_count_user_resources_story/4).
@@ -343,7 +353,11 @@ admin_count_user_resources_story(Config, Alice, _Alice2, _Alice3) ->
     Path = [data, session, countUserResources],
     JID = escalus_client:full_jid(Alice),
     Res = count_user_resources(JID, Config),
-    ?assertEqual(3, get_ok_value(Path, Res)).
+    ?assertEqual(3, get_ok_value(Path, Res)),
+    % Check for a non-existing user
+    Domain = domain(),
+    Res2 = count_user_resources(<<"alien@", Domain/binary>>, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res2)).
 
 admin_get_user_resource(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 3}], fun admin_get_user_resource_story/4).
@@ -356,7 +370,11 @@ admin_get_user_resource_story(Config, Alice, Alice2, _Alice3) ->
     ?assertEqual(escalus_client:resource(Alice2), get_ok_value(Path, Res)),
     % Provide a wrong resource number
     Res2 = get_user_resource(JID, 4, Config),
-    ?assertNotEqual(nomatch, binary:match(get_err_msg(Res2), <<"Wrong resource number">>)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Res2), <<"Wrong resource number">>)),
+    % Check for a non-existing user
+    Domain = domain(),
+    Res3 = get_user_resource(<<"alien@", Domain/binary>>, 1, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res3)).
 
 admin_count_users_with_status(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}, {alice_bis, 1}],
@@ -376,7 +394,10 @@ admin_count_users_with_status_story(Config, Alice, AliceB) ->
     assert_count_users_with_status(1, unprep(domain_helper:domain()), AwayStatus, Config),
     % Count users with dnd status globally
     escalus_client:send(AliceB, DndPresence),
-    assert_count_users_with_status(1, null, DndStatus, Config).
+    assert_count_users_with_status(1, null, DndStatus, Config),
+    % Count users with dnd status for a non-existing domain
+    Res = count_users_with_status(<<"nonexisting">>, DndStatus, Config),
+    ?assertEqual(<<"Domain not found">>, get_err_msg(Res)).
 
 admin_list_users_with_status(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}, {alice_bis, 1}],
@@ -398,7 +419,10 @@ admin_list_users_with_status_story(Config, Alice, AliceB) ->
     assert_list_users_with_status([AliceJID], unprep(domain()), AwayStatus, Config),
     % List users with dnd status globally
     escalus_client:send(AliceB, DndPresence),
-    assert_list_users_with_status([AliceBJID], null, DndStatus, Config).
+    assert_list_users_with_status([AliceBJID], null, DndStatus, Config),
+    % List users with dnd status for a non-existing domain
+    Res = count_users_with_status(<<"nonexisting">>, DndStatus, Config),
+    ?assertEqual(<<"Domain not found">>, get_err_msg(Res)).
 
 admin_kick_user_session(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 2}], fun admin_kick_user_session_story/3).
@@ -420,7 +444,11 @@ admin_kick_user_session_story(Config, Alice1, Alice2) ->
     ?assertNotEqual(nomatch, binary:match(get_coercion_err_msg(Res3), <<"jid_without_resource">>)),
     % Kick active session without reason text
     Res4 = kick_user_session(JIDA2, null, Config),
-    ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Res4), <<"kicked">>)).
+    ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Res4), <<"kicked">>)),
+    % Kick a non-existing user
+    Domain = domain(),
+    Res5 = kick_user_session(<<"alien@", Domain/binary, "/mobile">>, null, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res5)).
 
 admin_kick_user(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 2}], fun admin_kick_user_story/3).
@@ -431,7 +459,11 @@ admin_kick_user_story(Config, Alice1, _Alice2) ->
     Res1 = kick_user(AliceJID, Reason, Config),
     Res2 = get_ok_value([data, session, kickUser], Res1),
     ?assertEqual(2, length(Res2)),
-    ?assertEqual([true, true], [Kicked || #{<<"kicked">> := Kicked} <- Res2]).
+    ?assertEqual([true, true], [Kicked || #{<<"kicked">> := Kicked} <- Res2]),
+    % Kick a non-existing user
+    Domain = domain(),
+    Res5 = kick_user(<<"alien@", Domain/binary>>, null, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res5)).
 
 admin_set_presence(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}], fun admin_set_presence_story/2).
@@ -446,10 +478,14 @@ admin_set_presence_story(Config, Alice) ->
     % Send short JID
     Res = set_presence(ShortJID, Type, Show, Status, Priority, Config),
     ?assertNotEqual(nomatch, binary:match(get_err_msg(Res), <<"resource is empty">>)),
+    % Non-existing user
+    Domain = domain(),
+    Res2 = set_presence(<<"alien@", Domain/binary, "/mobile">>, Type, Show, Status, Priority, Config),
+    ?assertEqual(<<"Given user does not exist">>, get_err_msg(Res2)),
     % Send full JID
     Path = [data, session, setPresence, message],
-    Res2 = set_presence(JID, Type, Show, Status, Priority, Config),
-    ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Res2), <<"set successfully">>)),
+    Res3 = set_presence(JID, Type, Show, Status, Priority, Config),
+    ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Res3), <<"set successfully">>)),
     Presence = escalus:wait_for_stanza(Alice),
     ?assertNot(escalus_pred:is_presence_with_show(<<"online">>, Presence)),
     escalus:assert(is_presence_with_type, [<<"available">>], Presence),

diff --git a/src/admin_extra/service_admin_extra_sessions.erl b/src/admin_extra/service_admin_extra_sessions.erl
@@ -213,9 +213,14 @@ user_sessions_info(User, Host) ->
 format_sessions(Sessions) ->
     lists:map(fun(S) -> format_session(S) end, Sessions).
 
-format_session({USR, Conn, IP, Port, Prio, Node, Uptime}) ->
-    IPS = inet:ntoa(IP),
-    {jid:to_binary(USR), atom_to_list(Conn), IPS, Port, Prio, atom_to_list(Node), Uptime}.
+format_session({USR, Conn, Address, Prio, Node, Uptime}) ->
+    {IP, Port} = from_address(Address),
+    {jid:to_binary(USR), atom_to_list(Conn), IP, Port, Prio, atom_to_list(Node), Uptime}.
+
+from_address(undefined) ->
+    {undefined, undefined};
+from_address({IP, Port}) ->
+    {inet:ntoa(IP), Port}.
 
 format_status_users(Sessions) ->
     lists:map(fun(S) -> format_status_user(S) end, Sessions).

diff --git a/src/graphql/admin/mongoose_graphql_session_admin_mutation.erl b/src/graphql/admin/mongoose_graphql_session_admin_mutation.erl
@@ -23,9 +23,12 @@ kick_user_session(#{<<"user">> := JID, <<"reason">> := KickReason}) ->
         Error -> make_error(Error, #{jid => jid:to_binary(JID), reason => KickReason})
     end.
 
--spec kick_user(map()) -> {ok, [mongoose_session_api:kick_user_result()]}.
+-spec kick_user(map()) -> {ok, [mongoose_session_api:kick_user_result()]} | {error, resolver_error()}.
 kick_user(#{<<"user">> := JID, <<"reason">> := KickReason}) ->
-    mongoose_session_api:kick_sessions(JID, KickReason).
+    case mongoose_session_api:kick_sessions(JID, KickReason) of
+        {ok, Result} -> {ok, Result};
+        Error -> make_error(Error, #{jid => jid:to_binary(JID), reason => KickReason})
+    end.
 
 -spec set_presence(map()) -> {ok, map()} | {error, resolver_error()}.
 set_presence(#{<<"user">> := JID, <<"type">> := Type,

diff --git a/src/graphql/admin/mongoose_graphql_session_admin_query.erl b/src/graphql/admin/mongoose_graphql_session_admin_query.erl
@@ -5,8 +5,10 @@
 
 -ignore_xref([execute/4]).
 
+-include("../mongoose_graphql_types.hrl").
+
 -import(mongoose_graphql_session_helper, [format_sessions/1, format_status_users/1]).
--import(mongoose_graphql_helper, [format_result/2]).
+-import(mongoose_graphql_helper, [format_result/2, make_error/2]).
 
 execute(_Ctx, _Obj, <<"listSessions">>, Args) ->
     list_sessions(Args);
@@ -23,44 +25,57 @@ execute(_Ctx, _Obj, <<"listUsersWithStatus">>, Args) ->
 execute(_Ctx, _Obj, <<"countUsersWithStatus">>, Args) ->
     count_users_with_status(Args).
 
--spec list_sessions(map()) -> {ok, mongoose_graphql_session_helper:session_list()}.
+-spec list_sessions(map()) -> {ok, mongoose_graphql_session_helper:session_list()} | {error, resolver_error()}.
 list_sessions(#{<<"domain">> := null}) ->
     {ok, Sessions} = mongoose_session_api:list_sessions(),
     {ok, format_sessions(Sessions)};
 list_sessions(#{<<"domain">> := Domain}) ->
-    {ok, Sessions} = mongoose_session_api:list_sessions(Domain),
-    {ok, format_sessions(Sessions)}.
+    case mongoose_session_api:list_sessions(Domain) of
+        {ok, Sessions} ->
+            {ok, format_sessions(Sessions)};
+        Error ->
+            make_error(Error, #{domain => Domain})
+    end.
 
--spec count_sessions(map()) -> {ok, non_neg_integer()}.
+-spec count_sessions(map()) -> {ok, non_neg_integer()} | {error, resolver_error()}.
 count_sessions(#{<<"domain">> := null}) ->
     mongoose_session_api:count_sessions();
 count_sessions(#{<<"domain">> := Domain}) ->
-    mongoose_session_api:count_sessions(Domain).
+    format_result(mongoose_session_api:count_sessions(Domain), #{domain => Domain}).
 
 -spec list_user_sessions(map()) -> {ok, mongoose_graphql_session_helper:session_list()}.
 list_user_sessions(#{<<"user">> := JID}) ->
-    {ok, Sessions} = mongoose_session_api:list_user_sessions(JID),
-    {ok, format_sessions(Sessions)}.
+    case mongoose_session_api:list_user_sessions(JID) of
+        {ok, Sessions} ->
+            {ok, format_sessions(Sessions)};
+        Error ->
+            make_error(Error, #{user => jid:to_binary(JID)})
+    end.
 
--spec count_user_resources(map()) -> {ok, non_neg_integer()}.
+-spec count_user_resources(map()) -> {ok, non_neg_integer()} | {error, resolver_error()}.
 count_user_resources(#{<<"user">> := JID}) ->
-    mongoose_session_api:num_resources(JID).
+    format_result(mongoose_session_api:num_resources(JID), #{jid => jid:to_binary(JID)}).
 
--spec get_user_resource(map()) -> {ok, jid:lresource()}.
+-spec get_user_resource(map()) -> {ok, jid:lresource()} | {error, resolver_error()}.
 get_user_resource(#{<<"user">> := JID, <<"number">> := ResNumber}) ->
     Result = mongoose_session_api:get_user_resource(JID, ResNumber),
     format_result(Result, #{user => jid:to_binary(JID), number => ResNumber}).
 
--spec list_users_with_status(map()) -> {ok, mongoose_graphql_session_helper:status_user_list()}.
+-spec list_users_with_status(map()) -> {ok, mongoose_graphql_session_helper:status_user_list()}
+                                     | {error, resolver_error()}.
 list_users_with_status(#{<<"domain">> := null, <<"status">> := Status}) ->
     {ok, StatusUsers} = mongoose_session_api:list_status_users(Status),
     {ok, format_status_users(StatusUsers)};
 list_users_with_status(#{<<"domain">> := Domain, <<"status">> := Status}) ->
-    {ok, StatusUsers} = mongoose_session_api:list_status_users(Domain, Status),
-    {ok, format_status_users(StatusUsers)}.
+    case mongoose_session_api:list_status_users(Domain, Status) of
+        {ok, StatusUsers} ->
+            {ok, format_status_users(StatusUsers)};
+        Error ->
+            make_error(Error, #{domain => Domain, status => Status})
+    end.
 
--spec count_users_with_status(map()) -> {ok, non_neg_integer()}.
+-spec count_users_with_status(map()) -> {ok, non_neg_integer()} | {error, resolver_error()}.
 count_users_with_status(#{<<"domain">> := null, <<"status">> := Status}) ->
-    mongoose_session_api:num_status_users(Status);
+    format_result(mongoose_session_api:num_status_users(Status), #{status => Status});
 count_users_with_status(#{<<"domain">> := Domain, <<"status">> := Status}) ->
-    mongoose_session_api:num_status_users(Domain, Status).
+    format_result(mongoose_session_api:num_status_users(Domain, Status), #{domain => Domain, status => Status}).
diff --git a/src/graphql/mongoose_graphql_muc_helper.erl b/src/graphql/mongoose_graphql_muc_helper.erl
@@ -13,13 +13,15 @@
 -include("mod_muc_room.hrl").
 
 -spec add_user_resource(jid:jid(), binary() | null) ->
-    {ok, jid:jid()} | {no_session, iolist()}.
+    {ok, jid:jid()} | {atom(), binary()}.
 add_user_resource(JID, null) ->
     case mongoose_session_api:get_user_resource(JID, 1) of
         {ok, Resource} ->
             {ok, jid:replace_resource(JID, Resource)};
-        {wrong_res_number, _ErrMsg}->
-            {no_session, "Given user does not have any session"}
+        {wrong_res_number, _} ->
+            {no_session, <<"Given user does not have any session">>};
+        Error ->
+            Error
     end;
 add_user_resource(JID, Resource) ->
     {ok, jid:replace_resource(JID, Resource)}.

diff --git a/src/graphql/mongoose_graphql_session_helper.erl b/src/graphql/mongoose_graphql_session_helper.erl
@@ -16,16 +16,24 @@ format_sessions(Sessions) ->
     lists:map(fun(S) -> {ok, format_session(S)} end, Sessions).
 
 -spec format_session(mongoose_session_api:session_info()) -> session_data().
-format_session({USR, Conn, IP, Port, Prio, Node, Uptime}) ->
-    IPS = inet:ntoa(IP),
+format_session({USR, Conn, Address, Prio, Node, Uptime}) ->
+    {IP, Port} = from_address(Address),
     #{<<"user">> => jid:to_binary(USR),
-      <<"connection">> => atom_to_binary(Conn),
-      <<"ip">> => iolist_to_binary(IPS),
+      <<"connection">> => from_conn(Conn),
+      <<"ip">> => IP,
       <<"port">> => Port,
       <<"priority">> => Prio,
       <<"node">> => atom_to_binary(Node),
       <<"uptime">> => Uptime}.
 
+from_conn(undefined) -> null;
+from_conn(Conn) -> atom_to_binary(Conn).
+
+from_address(undefined) ->
+    {null, null};
+from_address({IP, Port}) ->
+    {iolist_to_binary(inet:ntoa(IP)), Port}.
+
 -spec format_status_users([mongoose_session_api:status_user_info()]) -> status_user_list().
 format_status_users(Sessions) ->
     lists:map(fun(S) -> {ok, format_status_user(S)} end, Sessions).