Merge pull request #466 from mremond/hooks_refactor

Hooks module refactor

Makefile.in

@@ -312,6 +312,9 @@ test:
 	@echo "*************************************************************************"
 	$(REBAR) skip_deps=true ct
 
+quicktest:
+	$(REBAR) skip_deps=true ct suites=elixir
+
 .PHONY: src doc edoc dialyzer Makefile TAGS clean clean-rel distclean rel \
 	install uninstall uninstall-binary uninstall-all translations deps test spec \
-	erlang_plt deps_plt ejabberd_plt
+	quicktest erlang_plt deps_plt ejabberd_plt

src/ejabberd_hooks.erl

@@ -32,18 +32,21 @@
 -export([start_link/0,
 	 add/3,
 	 add/4,
+	 add/5,
 	 add_dist/5,
+	 add_dist/6,
 	 delete/3,
 	 delete/4,
-	 delete_dist/5,
-	 run/2,
-	 run_fold/3,
-	 add/5,
-	 add_dist/6,
 	 delete/5,
+	 delete_dist/5,
 	 delete_dist/6,
+	 run/2,
 	 run/3,
-	 run_fold/4]).
+	 run_fold/3,
+	 run_fold/4,
+	 get_handlers/2]).
+
+-export([delete_all_hooks/0]).
 
 %% gen_server callbacks
 -export([init/1,
@@ -53,13 +56,14 @@
 	 handle_info/2,
 	 terminate/2]).
 
--include("ejabberd.hrl").
 -include("logger.hrl").
 
 %% Timeout of 5 seconds in calls to distributed hooks
 -define(TIMEOUT_DISTRIBUTED_HOOK, 5000).
 
 -record(state, {}).
+-type local_hook() :: { Seq :: integer(), Module :: atom(), Function :: atom()}.
+-type distributed_hook() :: { Seq :: integer(), Node :: atom(), Module :: atom(), Function :: atom()}.
 
 %%%----------------------------------------------------------------------
 %%% API
@@ -67,13 +71,13 @@
 start_link() ->
     gen_server:start_link({local, ejabberd_hooks}, ejabberd_hooks, [], []).
 
--spec add(atom(), fun(), number()) -> any().
+-spec add(atom(), fun(), number()) -> ok.
 
 %% @doc See add/4.
 add(Hook, Function, Seq) when is_function(Function) ->
     add(Hook, global, undefined, Function, Seq).
 
--spec add(atom(), binary() | atom(), fun() | atom() , number()) -> any().
+-spec add(atom(), HostOrModule :: binary() | atom(), fun() | atom() , number()) -> ok.
 add(Hook, Host, Function, Seq) when is_function(Function) ->
     add(Hook, Host, undefined, Function, Seq);
 
@@ -82,17 +86,17 @@ add(Hook, Host, Function, Seq) when is_function(Function) ->
 add(Hook, Module, Function, Seq) ->
     add(Hook, global, Module, Function, Seq).
 
--spec add(atom(), binary() | global, atom(), atom() | fun(), number()) -> any().
+-spec add(atom(), binary() | global, atom(), atom() | fun(), number()) -> ok.
 
 add(Hook, Host, Module, Function, Seq) ->
     gen_server:call(ejabberd_hooks, {add, Hook, Host, Module, Function, Seq}).
 
--spec add_dist(atom(), atom(), atom(), atom() | fun(), number()) -> any().
+-spec add_dist(atom(), atom(), atom(), atom() | fun(), number()) -> ok.
 
 add_dist(Hook, Node, Module, Function, Seq) ->
     gen_server:call(ejabberd_hooks, {add, Hook, global, Node, Module, Function, Seq}).
 
--spec add_dist(atom(), binary() | global, atom(), atom(), atom() | fun(), number()) -> any().
+-spec add_dist(atom(), binary() | global, atom(), atom(), atom() | fun(), number()) -> ok.
 
 add_dist(Hook, Host, Node, Module, Function, Seq) ->
     gen_server:call(ejabberd_hooks, {add, Hook, Host, Node, Module, Function, Seq}).
@@ -128,6 +132,17 @@ delete_dist(Hook, Node, Module, Function, Seq) ->
 delete_dist(Hook, Host, Node, Module, Function, Seq) ->
     gen_server:call(ejabberd_hooks, {delete, Hook, Host, Node, Module, Function, Seq}).
 
+-spec delete_all_hooks() -> true. 
+
+%% @doc Primarily for testing / instrumentation
+delete_all_hooks() ->
+    gen_server:call(ejabberd_hooks, {delete_all}).
+
+-spec get_handlers(atom(), binary() | global) -> [local_hook() | distributed_hook()].
+%% @doc Returns currently set handler for hook name 
+get_handlers(Hookname, Host) ->
+    gen_server:call(ejabberd_hooks, {get_handlers, Hookname, Host}).
+
 -spec run(atom(), list()) -> ok.
 
 %% @doc Run the calls of this hook in order, don't care about function results.
@@ -190,65 +205,70 @@ init([]) ->
 %%          {stop, Reason, State}            (terminate/2 is called)
 %%----------------------------------------------------------------------
 handle_call({add, Hook, Host, Module, Function, Seq}, _From, State) ->
-    Reply = case ets:lookup(hooks, {Hook, Host}) of
+    HookFormat = {Seq, Module, Function},
-		[{_, Ls}] ->
+    Reply = handle_add(Hook, Host, HookFormat),
-		    El = {Seq, Module, Function},
-		    case lists:member(El, Ls) of
-			true ->
-			    ok;
-			false ->
-			    NewLs = lists:merge(Ls, [El]),
-			    ets:insert(hooks, {{Hook, Host}, NewLs}),
-			    ok
-		    end;
-		[] ->
-		    NewLs = [{Seq, Module, Function}],
-		    ets:insert(hooks, {{Hook, Host}, NewLs}),
-		    ok
-	    end,
     {reply, Reply, State};
 handle_call({add, Hook, Host, Node, Module, Function, Seq}, _From, State) ->
-    Reply = case ets:lookup(hooks, {Hook, Host}) of
+    HookFormat = {Seq, Node, Module, Function},
-		[{_, Ls}] ->
+    Reply = handle_add(Hook, Host, HookFormat),
-		    El = {Seq, Node, Module, Function},
-		    case lists:member(El, Ls) of
-			true ->
-			    ok;
-			false ->
-			    NewLs = lists:merge(Ls, [El]),
-			    ets:insert(hooks, {{Hook, Host}, NewLs}),
-			    ok
-		    end;
-		[] ->
-		    NewLs = [{Seq, Node, Module, Function}],
-		    ets:insert(hooks, {{Hook, Host}, NewLs}),
-		    ok
-	    end,
     {reply, Reply, State};
+
 handle_call({delete, Hook, Host, Module, Function, Seq}, _From, State) ->
-    Reply = case ets:lookup(hooks, {Hook, Host}) of
+    HookFormat = {Seq, Module, Function},
-		[{_, Ls}] ->
+    Reply = handle_delete(Hook, Host, HookFormat),
-		    NewLs = lists:delete({Seq, Module, Function}, Ls),
-		    ets:insert(hooks, {{Hook, Host}, NewLs}),
-		    ok;
-		[] ->
-		    ok
-	    end,
     {reply, Reply, State};
 handle_call({delete, Hook, Host, Node, Module, Function, Seq}, _From, State) ->
-    Reply = case ets:lookup(hooks, {Hook, Host}) of
+    HookFormat = {Seq, Node, Module, Function},
-		[{_, Ls}] ->
+    Reply = handle_delete(Hook, Host, HookFormat),
-		    NewLs = lists:delete({Seq, Node, Module, Function}, Ls),
-		    ets:insert(hooks, {{Hook, Host}, NewLs}),
-		    ok;
-		[] ->
-		    ok
-	    end,
     {reply, Reply, State};
+
+handle_call({get_handlers, Hook, Host}, _From, State) ->
+    Reply = case ets:lookup(hooks, {Hook, Host}) of
+                [{_, Handlers}] -> Handlers;
+                []              -> []
+            end,
+    {reply, Reply, State};
+
+handle_call({delete_all}, _From, State) ->
+    Reply = ets:delete_all_objects(hooks),
+    {reply, Reply, State};
+
 handle_call(_Request, _From, State) ->
     Reply = ok,
     {reply, Reply, State}.
 
+-spec handle_add(atom(), atom(), local_hook() | distributed_hook()) -> ok.
+%% in-memory storage operation: Handle adding hook in ETS table
+handle_add(Hook, Host, El) ->
+    case ets:lookup(hooks, {Hook, Host}) of
+        [{_, Ls}] ->
+            case lists:member(El, Ls) of
+                true ->
+                    ok;
+                false ->
+                    NewLs = lists:merge(Ls, [El]),
+                    ets:insert(hooks, {{Hook, Host}, NewLs}),
+                    ok
+            end;
+        [] ->
+            NewLs = [El],
+            ets:insert(hooks, {{Hook, Host}, NewLs}),
+            ok
+    end.
+
+
+-spec handle_delete(atom(), atom(), local_hook() | distributed_hook()) -> ok.
+%% in-memory storage operation: Handle deleting hook from ETS table
+handle_delete(Hook, Host, El) ->
+    case ets:lookup(hooks, {Hook, Host}) of
+        [{_, Ls}] ->
+            NewLs = lists:delete(El, Ls),
+            ets:insert(hooks, {{Hook, Host}, NewLs}),
+            ok;
+        [] ->
+            ok
+    end. 
+
 %%----------------------------------------------------------------------
 %% Func: handle_cast/2
 %% Returns: {noreply, State}          |
@@ -275,7 +295,6 @@ handle_info(_Info, State) ->
 terminate(_Reason, _State) ->
     ok.
 
-
 code_change(_OldVsn, State, _Extra) ->
     {ok, State}.
 
@@ -283,9 +302,14 @@ code_change(_OldVsn, State, _Extra) ->
 %%% Internal functions
 %%%----------------------------------------------------------------------
 
+-spec run1([local_hook()|distributed_hook()], atom(), list()) -> ok.
+
 run1([], _Hook, _Args) ->
     ok;
+%% Run distributed hook on target node.
+%% It is not attempted again in case of failure. Next hook will be executed
 run1([{_Seq, Node, Module, Function} | Ls], Hook, Args) ->
+    %% MR: Should we have a safe rpc, like we have a safe apply or is bad_rpc enough ?
     case rpc:call(Node, Module, Function, Args, ?TIMEOUT_DISTRIBUTED_HOOK) of
 	timeout ->
 	    ?ERROR_MSG("Timeout on RPC to ~p~nrunning hook: ~p",
@@ -305,15 +329,10 @@ run1([{_Seq, Node, Module, Function} | Ls], Hook, Args) ->
 	    run1(Ls, Hook, Args)
     end;
 run1([{_Seq, Module, Function} | Ls], Hook, Args) ->
-    Res = if is_function(Function) ->
+    Res = safe_apply(Module, Function, Args),
-		  catch apply(Function, Args);
-	     true ->
-		  catch apply(Module, Function, Args)
-	  end,
     case Res of
 	{'EXIT', Reason} ->
-	    ?ERROR_MSG("~p~nrunning hook: ~p",
+	    ?ERROR_MSG("~p~nrunning hook: ~p", [Reason, {Hook, Args}]),
-		       [Reason, {Hook, Args}]),
 	    run1(Ls, Hook, Args);
 	stop ->
 	    ok;
@@ -346,15 +365,10 @@ run_fold1([{_Seq, Node, Module, Function} | Ls], Hook, Val, Args) ->
 	    run_fold1(Ls, Hook, NewVal, Args)
     end;
 run_fold1([{_Seq, Module, Function} | Ls], Hook, Val, Args) ->
-    Res = if is_function(Function) ->
+    Res = safe_apply(Module, Function, [Val | Args]),
-		  catch apply(Function, [Val | Args]);
-	     true ->
-		  catch apply(Module, Function, [Val | Args])
-	  end,
     case Res of
 	{'EXIT', Reason} ->
-	    ?ERROR_MSG("~p~nrunning hook: ~p",
+	    ?ERROR_MSG("~p~nrunning hook: ~p", [Reason, {Hook, Args}]),
-		       [Reason, {Hook, Args}]),
 	    run_fold1(Ls, Hook, Val, Args);
 	stop ->
 	    stopped;
@@ -363,3 +377,10 @@ run_fold1([{_Seq, Module, Function} | Ls], Hook, Val, Args) ->
 	NewVal ->
 	    run_fold1(Ls, Hook, NewVal, Args)
     end.
+
+safe_apply(Module, Function, Args) ->
+    if is_function(Function) ->
+            catch apply(Function, Args);
+       true ->
+            catch apply(Module, Function, Args)
+    end.

test/ejabberd_hooks_test.exs

@@ -0,0 +1,192 @@
+# ----------------------------------------------------------------------
+#
+# ejabberd, Copyright (C) 2002-2015   ProcessOne
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# ----------------------------------------------------------------------
+
+defmodule EjabberdHooksTest do
+  use ExUnit.Case, async: true
+  
+  @author "mremond@process-one.net"
+  @host <<"domain.net">>
+  @self __MODULE__
+  
+  setup_all do
+    {:ok, _Pid} = :ejabberd_hooks.start_link
+    :ok
+  end
+  
+  setup do
+    :meck.unload
+    :true = :ejabberd_hooks.delete_all_hooks
+    :ok
+  end  
+  
+  test "An anonymous function can be added as a hook" do
+    hookname = :test_fun_hook
+    :ok = :ejabberd_hooks.add(hookname, @host, fn _ -> :ok end, 50)
+    [{50, :undefined, _}] = :ejabberd_hooks.get_handlers(hookname, @host)
+  end
+  
+  test "A module function can be added as a hook" do
+    hookname = :test_mod_hook
+    callback = :hook_callback
+    :ok = :ejabberd_hooks.add(hookname, @host, @self, callback, 40)
+    [{40, @self, _callback}] = :ejabberd_hooks.get_handlers(hookname, @host)
+  end
+
+  test "An anonymous function can be removed from hook handlers" do
+    hookname = :test_fun_hook
+    anon_fun = fn _ -> :ok end                                              
+    :ok = :ejabberd_hooks.add(hookname, @host, anon_fun, 50)
+    :ok = :ejabberd_hooks.delete(hookname, @host, anon_fun, 50)
+    [] = :ejabberd_hooks.get_handlers(hookname, @host)
+  end
+
+  test "An module function can be removed from hook handlers" do
+    hookname = :test_mod_hook
+    callback = :hook_callback
+    :ok = :ejabberd_hooks.add(hookname, @host, @self, callback, 40)
+    :ok = :ejabberd_hooks.delete(hookname, @host, @self, callback, 40)
+    [] = :ejabberd_hooks.get_handlers(hookname, @host)
+    # TODO: Check that removed function is not call anymore
+  end
+
+  test "'Run hook' call registered handler once" do
+    test_result = :hook_result
+    run_hook([], fn -> test_result end, test_result)
+  end
+
+  test "'Run hook' can call registered handler with parameters" do
+    test_result = :hook_result_with_params
+    run_hook([:hook_params], fn _ -> test_result end, test_result)
+  end
+
+  # TODO test "Several handlers are run in order by hook"
+  
+  test "Hook run chain is stopped when handler return 'stop'" do
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    mock(modulename, :hook_callback1, fn _ -> :stop end)
+    mock(modulename, :hook_callback2, fn _ -> :end_result end)
+    
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 40)
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 50)
+
+    :ok = :ejabberd_hooks.run(hookname, @host, [:hook_params])
+    # callback2 is never run:
+    [{_pid, {^modulename, _callback, [:hook_params]}, :stop}] = :meck.history(modulename)   
+  end
+
+  test "Run fold hooks accumulate state in correct order through handlers" do
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    mock(modulename, :hook_callback1, fn(list, user) -> [user|list] end)
+    mock(modulename, :hook_callback2, fn(list, _user) -> ["jid2"|list] end)
+    
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 40)
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback2, 50)
+    
+    ["jid2", "jid1"] = :ejabberd_hooks.run_fold(hookname, @host, [], ["jid1"])
+  end
+
+  test "Hook run_fold are executed based on priority order, not registration order" do
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    mock(modulename, :hook_callback1, fn(_acc) -> :first end)
+    mock(modulename, :hook_callback2, fn(_acc) -> :second end)
+
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback2, 50)
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 40)
+    
+    :second = :ejabberd_hooks.run_fold(hookname, @host, :started, [])
+    # Both module have been called:
+    2 = length(:meck.history(modulename))
+  end
+  
+  # TODO: Test with ability to stop and return a value
+  test "Hook run_fold chain is stopped when handler return 'stop'" do
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    mock(modulename, :hook_callback1, fn(_acc) -> :stop end)
+    mock(modulename, :hook_callback2, fn(_acc) -> :executed end)
+
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 40)
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback2, 50)
+
+    :stopped = :ejabberd_hooks.run_fold(hookname, @host, :started, [])
+    # Only one module has been called
+    [{_pid, {^modulename, :hook_callback1, [:started]}, :stop}] = :meck.history(modulename)
+  end
+
+  test "Error in run_fold is ignored" do
+    run_fold_crash(fn(_acc) -> raise "crashed" end)
+  end
+
+  test "Throw in run_fold is ignored" do
+    run_fold_crash(fn(_acc) -> throw :crashed end)
+  end
+
+  test "Exit in run_fold is ignored" do
+    run_fold_crash(fn(_acc) -> exit :crashed end)
+  end
+  
+  # test for run hook with various number of params
+  def run_hook(params, fun, result) do 
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    callback = :hook_callback
+    mock(modulename, callback, fun)
+    
+    # Then check
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, callback, 40)
+    :ok = :ejabberd_hooks.run(hookname, @host, params)
+    [{_pid, {^modulename, ^callback, ^params}, ^result}] = :meck.history(modulename)   
+  end
+
+  def run_fold_crash(crash_fun) do
+    # setup test
+    hookname = :test_mod_hook
+    modulename = :hook_module
+    mock(modulename, :hook_callback1, crash_fun)
+    mock(modulename, :hook_callback2, fn(_acc) -> :final end)
+
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback1, 40)
+    :ok = :ejabberd_hooks.add(hookname, @host, modulename, :hook_callback2, 50)
+
+    :final = :ejabberd_hooks.run_fold(hookname, @host, :started, [])
+    # Both handlers were called
+    2 = length(:meck.history(modulename))    
+  end
+
+  # TODO refactor: Move to ejabberd_test_mock
+  def mock(module, function, fun) do
+    try do
+      :meck.new(module, [:non_strict])
+    catch
+      :error, {:already_started, _pid} -> :ok
+    end
+
+    :meck.expect(module, function, fun)
+  end
+  
+end