mod_roster: Change hook type from #roster{} to #roster_item{}

The problem with #roster{} is that every new record entry is also stored
in the mnesia roster table.  Adding the mix_participant_id there makes
no sense because the normal roster items are no MIX channels.  Using
\#roster_item{} for the hook and #roster{} for storing the normal items
seems to be a better idea.

6 files changed, 78 insertions, 101 deletions

diff --git a/src/ejabberd_c2s.erl b/src/ejabberd_c2s.erl
index 6f103894..ec5c610e 100644
--- a/src/ejabberd_c2s.erl
+++ b/src/ejabberd_c2s.erl
@@ -777,9 +777,9 @@ broadcast_presence_unavailable(#{jid := JID, pres_a := PresA} = State, Pres,
 		    Roster = ejabberd_hooks:run_fold(roster_get, LServer,
 						     [], [{LUser, LServer}]),
 		    lists:foldl(
-			fun(#roster{jid = LJID, subscription = Sub}, Acc)
+			fun(#roster_item{jid = ItemJID, subscription = Sub}, Acc)
 			       when Sub == both; Sub == from ->
-			    maps:put(LJID, 1, Acc);
+			    maps:put(jid:tolower(ItemJID), 1, Acc);
 			   (_, Acc) ->
 			       Acc
 			end, #{BareJID => 1}, Roster);
@@ -813,8 +813,7 @@ broadcast_presence_available(#{jid := JID} = State,
 				    [], [{LUser, LServer}]),
     {FJIDs, TJIDs} =
 	lists:foldl(
-	  fun(#roster{jid = LJID, subscription = Sub}, {F, T}) ->
-		  To = jid:make(LJID),
+	  fun(#roster_item{jid = To, subscription = Sub}, {F, T}) ->
 		  F1 = if Sub == both orelse Sub == from ->
 			       Pres1 = xmpp:set_to(Pres, To),
 			       case privacy_check_packet(State, Pres1, out) of
@@ -843,10 +842,9 @@ broadcast_presence_available(#{jid := JID} = State,
     Items = ejabberd_hooks:run_fold(
 	      roster_get, LServer, [], [{LUser, LServer}]),
     JIDs = lists:foldl(
-	     fun(#roster{jid = LJID, subscription = Sub}, Tos)
+	     fun(#roster_item{jid = To, subscription = Sub}, Tos)
 		   when Sub == both orelse Sub == from ->
-		     To = jid:make(LJID),
-		     P = xmpp:set_to(Pres, jid:make(LJID)),
+		     P = xmpp:set_to(Pres, To),
 		     case privacy_check_packet(State, P, out) of
 			 allow -> [To|Tos];
 			 deny -> Tos
diff --git a/src/mod_admin_extra.erl b/src/mod_admin_extra.erl
index 19aa983f..b0dffd2d 100644
--- a/src/mod_admin_extra.erl
+++ b/src/mod_admin_extra.erl
@@ -1333,16 +1333,15 @@ get_roster(User, Server) ->
 %% several times, each one in a different group.
 make_roster_xmlrpc(Roster) ->
     lists:foldl(
-      fun(Item, Res) ->
-	      JIDS = jid:encode(Item#roster.jid),
-	      Nick = Item#roster.name,
-	      Subs = atom_to_list(Item#roster.subscription),
-	      Ask = atom_to_list(Item#roster.ask),
-	      Groups = case Item#roster.groups of
+      fun(#roster_item{jid = JID, name = Nick, subscription = Sub, ask = Ask} = Item, Res) ->
+	      JIDS = jid:encode(JID),
+	      Subs = atom_to_list(Sub),
+	      Asks = atom_to_list(Ask),
+	      Groups = case Item#roster_item.groups of
 			   [] -> [<<>>];
 			   Gs -> Gs
 		       end,
-	      ItemsX = [{JIDS, Nick, Subs, Ask, Group} || Group <- Groups],
+	      ItemsX = [{JIDS, Nick, Subs, Asks, Group} || Group <- Groups],
 	      ItemsX ++ Res
       end,
       [],
diff --git a/src/mod_mix_pam.erl b/src/mod_mix_pam.erl
index 9491d217..ca3b4e6e 100644
--- a/src/mod_mix_pam.erl
+++ b/src/mod_mix_pam.erl
@@ -212,22 +212,20 @@ process_iq(#iq{type = set,
 process_iq(IQ) ->
     xmpp:make_error(IQ, unsupported_query_error(IQ)).
 
--spec get_mix_roster_items([#roster{}], {binary(), binary()}) -> [#roster{}].
+-spec get_mix_roster_items([#roster_item{}], {binary(), binary()}) -> [#roster_item{}].
 get_mix_roster_items(Acc, {LUser, LServer}) ->
     JID = jid:make(LUser, LServer),
     case get_channels(JID) of
         {ok, Channels} ->
             lists:map(
-                fun({#jid{luser=Channel, lserver=Service}, Id}) ->
-                    #roster{
-                        jid = {Channel, Service, <<>>},
+                fun({ItemJID, Id}) ->
+                    #roster_item{
+                        jid = ItemJID,
                         name = <<>>,
                         subscription = both,
-                        ask = none,
+                        ask = undefined,
                         groups = [<<"Channels">>],
-                        askmessage = <<>>,
-                        xs = [],
-                        mix_participant_id = Id
+                        mix_channel = #mix_roster_channel{'participant-id' = Id}
                     }
                 end, Channels);
         _ ->
@@ -288,21 +286,19 @@ process_leave(#iq{from = From,
     end.
 
 -spec process_join_result(iq(), iq()) -> ok.
-process_join_result(#iq{from = Channel,
+process_join_result(#iq{from = #jid{} = Channel,
 			type = result, sub_els = [#mix_join{id = ID} = Join]},
 		    #iq{to = To} = IQ) ->
     case add_channel(To, Channel, ID) of
 	ok ->
 	    % Do roster push
-	    #jid{luser = ChannelName, lserver = Service} = Channel,
-	    mod_roster:push_item(To, #roster{}, #roster{
-		jid = {ChannelName, Service, <<>>},
+	    mod_roster:push_item(To, #roster_item{jid = #jid{}}, #roster_item{
+		jid = Channel,
 		name = <<>>,
 		subscription = none,
-		ask = none,
+		ask = undefined,
 		groups = [],
-		askmessage = <<>>,
-		mix_participant_id = ID
+		mix_channel = #mix_roster_channel{'participant-id' = ID}
 	    }),
 	    % send IQ result
 	    ChanID = make_channel_id(Channel, ID),
@@ -319,11 +315,9 @@ process_join_result(Err, IQ) ->
 process_leave_result(#iq{from = Channel, type = result, sub_els = [#mix_leave{} = Leave]},
 		     #iq{to = User} = IQ) ->
     % Do roster push
-    #jid{luser = ChannelName, lserver = Service} = Channel,
     mod_roster:push_item(User,
-	#roster{jid = {ChannelName, Service, <<>>}, subscription = none},
-	#roster{jid = {ChannelName, Service, <<>>},
-	        subscription = remove}),
+	#roster_item{jid = Channel, subscription = none},
+	#roster_item{jid = Channel, subscription = remove}),
     % send iq result
     ResIQ = xmpp:make_iq_result(IQ, #mix_client_leave{leave = Leave}),
     ejabberd_router:route(ResIQ);
diff --git a/src/mod_roster.erl b/src/mod_roster.erl
index 27fac313..fdf9de9f 100644
--- a/src/mod_roster.erl
+++ b/src/mod_roster.erl
@@ -41,7 +41,7 @@
 -behaviour(gen_mod).
 
 -export([start/2, stop/1, reload/3, process_iq/1, export/1,
-	 import_info/0, process_local_iq/1, get_user_roster/2,
+	 import_info/0, process_local_iq/1, get_user_roster_items/2,
 	 import/5, get_roster/2, push_item/3,
 	 import_start/2, import_stop/2, is_subscribed/2,
 	 c2s_self_presence/1, in_subscription/2,
@@ -92,7 +92,7 @@ start(Host, Opts) ->
     Mod:init(Host, Opts),
     init_cache(Mod, Host, Opts),
     ejabberd_hooks:add(roster_get, Host, ?MODULE,
-		       get_user_roster, 50),
+		       get_user_roster_items, 50),
     ejabberd_hooks:add(roster_in_subscription, Host,
 		       ?MODULE, in_subscription, 50),
     ejabberd_hooks:add(roster_out_subscription, Host,
@@ -114,7 +114,7 @@ start(Host, Opts) ->
 
 stop(Host) ->
     ejabberd_hooks:delete(roster_get, Host, ?MODULE,
-			  get_user_roster, 50),
+			  get_user_roster_items, 50),
     ejabberd_hooks:delete(roster_in_subscription, Host,
 			  ?MODULE, in_subscription, 50),
     ejabberd_hooks:delete(roster_out_subscription, Host,
@@ -205,9 +205,8 @@ process_local_iq(#iq{lang = Lang} = IQ) ->
 
 -spec roster_hash([#roster{}]) -> binary().
 roster_hash(Items) ->
-    str:sha(term_to_binary(lists:sort([R#roster{groups =
-						    lists:sort(Grs)}
-				       || R = #roster{groups = Grs}
+    str:sha(term_to_binary(lists:sort([R#roster_item{groups = lists:sort(Grs)}
+				       || R = #roster_item{groups = Grs}
 					      <- Items]))).
 
 %% Returns a list that may contain an xmlelement with the XEP-237 feature if it's enabled.
@@ -227,7 +226,6 @@ get_versioning_feature(Acc, Host) ->
 
 -spec roster_version(binary(), binary()) -> undefined | binary().
 roster_version(LServer, LUser) ->
-    US = {LUser, LServer},
     case mod_roster_opt:store_current_id(LServer) of
       true ->
 	  case read_roster_version(LUser, LServer) of
@@ -235,8 +233,7 @@ roster_version(LServer, LUser) ->
 	    {ok, V} -> V
 	  end;
       false ->
-	  roster_hash(ejabberd_hooks:run_fold(roster_get, LServer,
-					      [], [US]))
+	  roster_hash(run_roster_get_hook(LUser, LServer))
     end.
 
 -spec read_roster_version(binary(), binary()) -> {ok, binary()} | error.
@@ -279,7 +276,6 @@ process_iq_get(#iq{to = To, from = From,
 		   sub_els = [#roster_query{ver = RequestedVersion, mix_annotate = MixAnnotate}]} = IQ) ->
     LUser = To#jid.luser,
     LServer = To#jid.lserver,
-    US = {LUser, LServer},
     MixEnabled = MixAnnotate == #mix_roster_annotate{},
     {ItemsToSend, VersionToSend} =
 	case {mod_roster_opt:versioning(LServer),
@@ -288,32 +284,22 @@ process_iq_get(#iq{to = To, from = From,
 		case read_roster_version(LUser, LServer) of
 		    error ->
 			RosterVersion = write_roster_version(LUser, LServer),
-			{lists:map(fun encode_item/1,
-				   ejabberd_hooks:run_fold(
-				     roster_get, To#jid.lserver, [], [US])),
-			 RosterVersion};
+			{run_roster_get_hook(LUser, LServer), RosterVersion};
 		    {ok, RequestedVersion} ->
 			{false, false};
 		    {ok, NewVersion} ->
-			{lists:map(fun encode_item/1,
-				   ejabberd_hooks:run_fold(
-				     roster_get, To#jid.lserver, [], [US])),
-			 NewVersion}
+			{run_roster_get_hook(LUser, LServer), NewVersion}
 		end;
 	    {true, false} when RequestedVersion /= undefined ->
-		RosterItems = ejabberd_hooks:run_fold(
-				roster_get, To#jid.lserver, [], [US]),
+		RosterItems = run_roster_get_hook(LUser, LServer),
 		case roster_hash(RosterItems) of
 		    RequestedVersion ->
 			{false, false};
 		    New ->
-			{lists:map(fun encode_item/1, RosterItems), New}
+			{RosterItems, New}
 		end;
 	    _ ->
-		{lists:map(fun encode_item/1,
-			   ejabberd_hooks:run_fold(
-			     roster_get, To#jid.lserver, [], [US])),
-		 false}
+		{run_roster_get_hook(LUser, LServer), false}
 	end,
     % Store that MIX annotation is enabled (for roster pushes)
     set_mix_annotation_enabled(From, MixEnabled),
@@ -334,16 +320,21 @@ process_iq_get(#iq{to = To, from = From,
 			    ver = Version}
       end).
 
--spec get_user_roster([#roster{}], {binary(), binary()}) -> [#roster{}].
-get_user_roster(Acc, {LUser, LServer}) ->
-    Items = get_roster(LUser, LServer),
-    lists:filter(fun (#roster{subscription = none,
-			      ask = in}) ->
-			 false;
-		     (_) -> true
-		 end,
-		 Items)
-      ++ Acc.
+-spec run_roster_get_hook(binary(), binary()) -> [#roster_item{}].
+run_roster_get_hook(LUser, LServer) ->
+    ejabberd_hooks:run_fold(roster_get, LServer, [], [{LUser, LServer}]).
+
+-spec get_filtered_roster(binary(), binary()) -> [#roster{}].
+get_filtered_roster(LUser, LServer) ->
+    lists:filter(
+	fun (#roster{subscription = none, ask = in}) -> false;
+	    (_) -> true
+	end,
+	get_roster(LUser, LServer)).
+
+-spec get_user_roster_items([#roster_item{}], {binary(), binary()}) -> [#roster_item{}].
+get_user_roster_items(Acc, {LUser, LServer}) ->
+    lists:map(fun encode_item/1, get_filtered_roster(LUser, LServer)) ++ Acc.
 
 -spec get_roster(binary(), binary()) -> [#roster{}].
 get_roster(LUser, LServer) ->
@@ -435,11 +426,7 @@ encode_item(Item) ->
 			   both -> subscribe;
 			   _ -> undefined
 		       end,
-		 groups = Item#roster.groups,
-		 mix_channel = case Item#roster.mix_participant_id of
-				   <<>> -> undefined;
-				   _ -> #mix_roster_channel{'participant-id' = Item#roster.mix_participant_id}
-			       end}.
+		 groups = Item#roster.groups}.
 
 -spec decode_item(roster_item(), #roster{}, boolean()) -> #roster{}.
 decode_item(#roster_item{subscription = remove} = Item, R, _) ->
@@ -494,7 +481,7 @@ set_item_and_notify_clients(To, #roster_item{jid = PeerJID} = RosterItem,
 	end,
     case transaction(LUser, LServer, [PeerLJID], F) of
 	{atomic, {OldItem, NewItem}} ->
-	    push_item(To, OldItem, NewItem),
+	    push_item(To, encode_item(OldItem), encode_item(NewItem)),
 	    case NewItem#roster.subscription of
 		remove ->
 		    send_unsubscribing_presence(To, OldItem);
@@ -505,7 +492,7 @@ set_item_and_notify_clients(To, #roster_item{jid = PeerJID} = RosterItem,
 	    {error, Reason}
     end.
 
--spec push_item(jid(), #roster{}, #roster{}) -> ok.
+-spec push_item(jid(), #roster_item{}, #roster_item{}) -> ok.
 push_item(To, OldItem, NewItem) ->
     #jid{luser = LUser, lserver = LServer} = To,
     Ver = case mod_roster_opt:versioning(LServer) of
@@ -518,10 +505,10 @@ push_item(To, OldItem, NewItem) ->
 	      push_item(To1, OldItem, NewItem, Ver)
       end, ejabberd_sm:get_user_resources(LUser, LServer)).
 
--spec push_item(jid(), #roster{}, #roster{}, undefined | binary()) -> ok.
+-spec push_item(jid(), #roster_item{}, #roster_item{}, undefined | binary()) -> ok.
 push_item(To, OldItem, NewItem, Ver) ->
     route_presence_change(To, OldItem, NewItem),
-    [Item] = process_items_mix([encode_item(NewItem)], To),
+    [Item] = process_items_mix([NewItem], To),
     IQ = #iq{type = set, to = To,
 	     from = jid:remove_resource(To),
 	     id = <<"push", (p1_rand:get_string())/binary>>,
@@ -529,11 +516,11 @@ push_item(To, OldItem, NewItem, Ver) ->
 				      items = [Item]}]},
     ejabberd_router:route(IQ).
 
--spec route_presence_change(jid(), #roster{}, #roster{}) -> ok.
+-spec route_presence_change(jid(), #roster_item{}, #roster_item{}) -> ok.
 route_presence_change(From, OldItem, NewItem) ->
-    OldSub = OldItem#roster.subscription,
-    NewSub = NewItem#roster.subscription,
-    To = jid:make(NewItem#roster.jid),
+    OldSub = OldItem#roster_item.subscription,
+    NewSub = NewItem#roster_item.subscription,
+    To = NewItem#roster_item.jid,
     NewIsFrom = NewSub == both orelse NewSub == from,
     OldIsFrom = OldSub == both orelse OldSub == from,
     if NewIsFrom andalso not OldIsFrom ->
@@ -657,7 +644,9 @@ process_subscription(Direction, User, Server, JID1,
 		       NewItem#roster.ask == in ->
 			    ok;
 		       true ->
-			    push_item(jid:make(User, Server), OldItem, NewItem)
+			    push_item(jid:make(User, Server),
+				      encode_item(OldItem),
+				      encode_item(NewItem))
 		    end,
 		    true;
 		none ->
@@ -835,7 +824,7 @@ in_auto_reply(_, _, _) -> none.
 remove_user(User, Server) ->
     LUser = jid:nodeprep(User),
     LServer = jid:nameprep(Server),
-    Items = get_user_roster([], {LUser, LServer}),
+    Items = get_filtered_roster(LUser, LServer),
     send_unsubscription_to_rosteritems(LUser, LServer, Items),
     Mod = gen_mod:db_mod(LServer, ?MODULE),
     Mod:remove_user(LUser, LServer),
diff --git a/src/mod_shared_roster.erl b/src/mod_shared_roster.erl
index 333a64a4..067d4504 100644
--- a/src/mod_shared_roster.erl
+++ b/src/mod_shared_roster.erl
@@ -185,8 +185,8 @@ cache_nodes(Mod, Host) ->
         false -> ejabberd_cluster:get_nodes()
     end.
 
--spec get_user_roster([#roster{}], {binary(), binary()}) -> [#roster{}].
-get_user_roster(Items, {U, S} = US) ->
+-spec get_user_roster([#roster_item{}], {binary(), binary()}) -> [#roster_item{}].
+get_user_roster(Items, {_, S} = US) ->
     {DisplayedGroups, Cache} = get_user_displayed_groups(US),
     SRUsers = lists:foldl(
 	fun(Group, Acc1) ->
@@ -216,10 +216,10 @@ get_user_roster(Items, {U, S} = US) ->
 	    end
 	end,
 	SRUsers, Items),
-    SRItems = [#roster{usj = {U, S, {U1, S1, <<"">>}},
-		       us = US, jid = {U1, S1, <<"">>},
-		       name = get_rosteritem_name(U1, S1),
-		       subscription = both, ask = none, groups = GroupLabels}
+    SRItems = [#roster_item{jid = jid:make(U1, S1),
+			    name = get_rosteritem_name(U1, S1),
+			    subscription = both, ask = undefined,
+			    groups = GroupLabels}
 	       || {{U1, S1}, GroupLabels} <- dict:to_list(SRUsersRest)],
     SRItems ++ NewItems1.
 
@@ -855,16 +855,14 @@ displayed_to_groups(GroupName, LServer) ->
 
 push_item(User, Server, Item) ->
     mod_roster:push_item(jid:make(User, Server),
-			 Item#roster{subscription = none},
+			 Item#roster_item{subscription = none},
 			 Item).
 
 push_roster_item(User, Server, ContactU, ContactS, ContactN,
 		 GroupLabel, Subscription) ->
-    Item = #roster{usj =
-		       {User, Server, {ContactU, ContactS, <<"">>}},
-		   us = {User, Server}, jid = {ContactU, ContactS, <<"">>},
-		   name = ContactN, subscription = Subscription, ask = none,
-		   groups = [GroupLabel]},
+    Item = #roster_item{jid = jid:make(ContactU, ContactS),
+			name = ContactN, subscription = Subscription, ask = undefined,
+			groups = [GroupLabel]},
     push_item(User, Server, Item).
 
 -spec c2s_self_presence({presence(), ejabberd_c2s:state()})
diff --git a/src/mod_shared_roster_ldap.erl b/src/mod_shared_roster_ldap.erl
index 216dd4ce..2b4ec1da 100644
--- a/src/mod_shared_roster_ldap.erl
+++ b/src/mod_shared_roster_ldap.erl
@@ -109,8 +109,8 @@ depends(_Host, _Opts) ->
 %%--------------------------------------------------------------------
 %% Hooks
 %%--------------------------------------------------------------------
--spec get_user_roster([#roster{}], {binary(), binary()}) -> [#roster{}].
-get_user_roster(Items, {U, S} = US) ->
+-spec get_user_roster([#roster_item{}], {binary(), binary()}) -> [#roster_item{}].
+get_user_roster(Items, US) ->
     SRUsers = get_user_to_groups_map(US, true),
     {NewItems1, SRUsersRest} = lists:mapfoldl(fun (Item,
 						   SRUsers1) ->
@@ -135,10 +135,9 @@ get_user_roster(Items, {U, S} = US) ->
 						      end
 					      end,
 					      SRUsers, Items),
-    SRItems = [#roster{usj = {U, S, {U1, S1, <<"">>}},
-		       us = US, jid = {U1, S1, <<"">>},
-		       name = get_user_name(U1, S1), subscription = both,
-		       ask = none, groups = GroupNames}
+    SRItems = [#roster_item{jid = jid:make(U1, S1),
+			    name = get_user_name(U1, S1), subscription = both,
+			    ask = undefined, groups = GroupNames}
 	       || {{U1, S1}, GroupNames} <- dict:to_list(SRUsersRest)],
     SRItems ++ NewItems1.