Apply reviews comments

big_tests/tests/pubsub_tools.erl

@@ -642,7 +642,7 @@ decode_affiliations(IQResult) ->
 
 pubsub_node() -> pubsub_node(1).
 pubsub_node(Num) ->
-    {pubsub_tools:node_addr(), <<"node",
+    {pubsub_tools:node_addr(), <<"node_",
                                  (integer_to_binary(Num))/binary, "_",
                                  (base64:encode(crypto:strong_rand_bytes(6)))/binary>>}.
 

src/pubsub/gen_pubsub_node.erl

@@ -201,6 +201,8 @@
 
 -callback should_delete_when_owner_removed() -> boolean().
 
+-callback remove_user(LUser :: jid:luser(), LServer :: jid:lserver(), Backend :: module()) -> any().
+
 -optional_callbacks([create_node_permission/6,
                      create_node/2,
                      delete_node/1,
@@ -225,7 +227,8 @@
                      set_item/1,
                      get_item_name/3,
                      path_to_node/1,
-                     should_delete_when_owner_removed/0]).
+                     should_delete_when_owner_removed/0,
+                     remove_user/3]).
 
 %% --------------------------------------------------------
 %% API

src/pubsub/mod_pubsub.erl

@@ -86,7 +86,7 @@
 -export([subscription_to_string/1, affiliation_to_string/1,
          string_to_subscription/1, string_to_affiliation/1,
          extended_error/2, extended_error/3, service_jid/1,
-         tree/1, tree/2, plugin/2, plugins/1, config/3,
+         tree/1, tree/2, plugin/2, plugin/1, plugins/1, plugin_call/3, config/3,
          host/1, serverhost/1]).
 
 %% API and gen_server callbacks
@@ -881,13 +881,17 @@ remove_user(User, Server) ->
     LServer = jid:nameprep(Server),
 
     BackendModules = mongoose_lib:find_behaviour_implementations(mod_pubsub_db),
-    lists:foreach(fun(Backend) ->
-                          remove_user_per_backend_safe(LUser, LServer, Backend)
-                  end, BackendModules).
+    PluginModules = mongoose_lib:find_behaviour_implementations(gen_pubsub_node),
+    % The line below will currently lead to unnecessary DB selects,
+    % because plugins don't really filter DB data by type.
+    % TODO: This should be optimised during GDPR load tests.
+    [ remove_user_per_backend_and_plugin_safe(LUser, LServer, Plugin, Backend)
+      || Plugin <- PluginModules, Backend <- BackendModules ],
+    ok.
 
-remove_user_per_backend_safe(LUser, LServer, Backend) ->
+remove_user_per_backend_and_plugin_safe(LUser, LServer, Plugin, Backend) ->
     try
-        ok = Backend:dirty(fun() -> remove_user_per_backend(LUser, LServer, Backend) end, #{})
+        plugin_call(Plugin, remove_user, [LUser, LServer, Backend])
     catch
         Class:Reason ->
             StackTrace = erlang:get_stacktrace(),
@@ -896,29 +900,6 @@ remove_user_per_backend_safe(LUser, LServer, Backend) ->
                          [LUser, LServer, Backend, Class, Reason, StackTrace])
     end.
 
-remove_user_per_backend(LUser, LServer, Backend) ->
-    LJID = {LUser, LServer, <<>>},
-    Backend:delete_user_subscriptions(LJID),
-    Nodes = Backend:find_nodes_by_affiliated_user(LJID),
-    %% We don't broadcast any node deletion notifications to subscribers because:
-    %% * PEP nodes do not broadcast anything upon deletion
-    %% * Push nodes do not have (should not have) any subscribers
-    %% * Remaining nodes are not deleted when the owner leaves
-    lists:foreach(fun({#pubsub_node{ id = Nidx, type = Type } = Node, owner}) ->
-                          Plugin = plugin(Type),
-                          MaybeBasePlugin
-                          = maybe_default_node(Plugin, should_delete_when_owner_removed, []),
-                          case MaybeBasePlugin:should_delete_when_owner_removed() of
-                              true ->
-                                  % Oh my, we do have a mess in the API, don't we?
-                                  Backend:delete_node(Node),
-                                  Backend:del_node(Nidx);
-                              _ -> Backend:set_affiliation(Nidx, LJID, none)
-                          end;
-                     ({#pubsub_node{ id = Nidx }, _}) ->
-                          Backend:set_affiliation(Nidx, LJID, none)
-                  end, Nodes).
-
 handle_call(server_host, _From, State) ->
     {reply, State#state.server_host, State};
 handle_call(plugins, _From, State) ->
@@ -4295,6 +4276,9 @@ tree_action(Host, Function, Args) ->
 node_call(Host, Type, Function, Args) ->
     ?DEBUG("node_call ~p ~p ~p", [Type, Function, Args]),
     PluginModule = plugin(Host, Type),
+    plugin_call(PluginModule, Function, Args).
+
+plugin_call(PluginModule, Function, Args) ->
     CallModule = maybe_default_node(PluginModule, Function, Args),
     case apply(CallModule, Function, Args) of
         {result, Result} ->

src/pubsub/mod_pubsub_db.erl

@@ -195,7 +195,9 @@
 %% API
 %%====================================================================
 
--spec db_error(ReasonData :: map(), ErrorDebug :: map(), Event :: any()) ->
+% ReasonData may either be a debug map provided by mod_pubsub
+% or some other term if the crash is serious enough to lose the debug map somewhere.
+-spec db_error(ReasonData :: map() | any(), ErrorDebug :: map(), Event :: any()) ->
     {error, Details :: map()}.
 db_error(ReasonData, ErrorDebug, Event) ->
     {error, maps:merge(ErrorDebug#{ event => Event }, sanitize_reason(ReasonData))}.

src/pubsub/mod_pubsub_db_mnesia.erl

@@ -280,7 +280,7 @@ del_node(Nidx) ->
     {ok, States} = get_states(Nidx),
     lists:foreach(fun (#pubsub_state{stateid = {LJID, _}, items = Items}) ->
                           del_items(Nidx, Items),
-                          del_state(Nidx, LJID)
+                          del_state_by_idx_and_ljid(Nidx, LJID)
                   end, States),
     {ok, States}.
 
@@ -469,7 +469,7 @@ set_affiliation(Nidx, LJID, Affiliation) ->
     BareLJID = jid:to_bare(LJID),
     {ok, State} = get_state(Nidx, BareLJID, write),
     case {Affiliation, State#pubsub_state.subscriptions} of
-        {none, []} -> del_state(Nidx, BareLJID);
+        {none, []} -> del_state_by_idx_and_ljid(Nidx, BareLJID);
         _ ->  mnesia:write(State#pubsub_state{ affiliation = Affiliation })
     end.
 
@@ -528,7 +528,7 @@ delete_subscription(Nidx, LJID, SubId) ->
     NewSubs = lists:keydelete(SubId, 2, State#pubsub_state.subscriptions),
     mnesia:delete({pubsub_subscription, SubId}),
     case {State#pubsub_state.affiliation, NewSubs} of
-        {none, []} -> del_state(Nidx, LJID);
+        {none, []} -> del_state_by_idx_and_ljid(Nidx, LJID);
         _ -> mnesia:write(State#pubsub_state{subscriptions = NewSubs})
     end.
 
@@ -643,9 +643,9 @@ del_items(Nidx, ItemIds) ->
 %% Internal functions
 %%====================================================================
 
--spec del_state(Nidx :: mod_pubsub:nodeIdx(),
-                LJID :: jid:ljid()) -> ok.
-del_state(Nidx, LJID) ->
+-spec del_state_by_idx_and_ljid(Nidx :: mod_pubsub:nodeIdx(),
+                                LJID :: jid:ljid()) -> ok.
+del_state_by_idx_and_ljid(Nidx, LJID) ->
     {ok, State} = get_state(Nidx, LJID, write),
     del_state(State).
 

src/pubsub/node_flat.erl

@@ -50,7 +50,7 @@
          get_items_if_authorised/3, get_items/3, get_item/7,
          get_item/2, set_item/1, get_item_name/3, node_to_path/1,
          path_to_node/1, can_fetch_item/2, is_subscribed/1,
-         should_delete_when_owner_removed/0]).
+         should_delete_when_owner_removed/0, remove_user/3]).
 
 based_on() ->  none.
 
@@ -686,3 +686,38 @@ is_subscribed(Subscriptions) ->
 make_subid() ->
     mongoose_bin:gen_from_timestamp().
 
+remove_user(LUser, LServer, Backend) ->
+    Backend:dirty(fun() -> 
+                          LJID = {LUser, LServer, <<>>},
+                          Backend:delete_user_subscriptions(LJID),
+                          NodesAndAffs = Backend:find_nodes_by_affiliated_user(LJID),
+                          %% We don't broadcast node deletion notifications to subscribers because:
+                          %% * PEP nodes do not broadcast anything upon deletion
+                          %% * Push nodes do not have (should not have) any subscribers
+                          %% * Remaining nodes are not deleted when the owner leaves
+                          lists:foreach(
+                            fun(NodeAffPair) ->
+                                    remove_user_by_affiliation_and_node(NodeAffPair, LJID, Backend)
+                            end, NodesAndAffs)
+                  end, #{}).
+
+-spec remove_user_by_affiliation_and_node(NodeAffPair :: {mod_pubsub:pubsubNode(),
+                                                          mod_pubsub:affiliation()},
+                                          LJID :: jid:ljid(),
+                                          Backend :: module()) ->
+    any().
+remove_user_by_affiliation_and_node({#pubsub_node{ id = Nidx } = Node, owner}, LJID, Backend) ->
+    case mod_pubsub:plugin_call(mod_pubsub:plugin(Node#pubsub_node.type),
+                                should_delete_when_owner_removed, []) of
+        {result, true} ->
+            % Oh my, we do have a mess in the API, don't we?
+            % delete_node removes actual node structure
+            % del_node removes node's items and states (affs and subs)
+            Backend:delete_node(Node),
+            Backend:del_node(Nidx);
+        _ ->
+            Backend:set_affiliation(Nidx, LJID, none)
+    end;
+remove_user_by_affiliation_and_node({#pubsub_node{ id = Nidx }, _}, LJID, Backend) ->
+    Backend:set_affiliation(Nidx, LJID, none).
+