Run hooks with a default (empty) accumulator #1156

bartekgorny · 2017-01-17T10:25:43Z

Simple modification resulting in plenty of code changes: all hook handers which were runned before (not run_folded) are now prepared to accept an accumulator, and return it, mostly unchanged. If called by 'run', the accumulator is an empty map, and return value is ignored.

By doing this, we make all handlers ready to switch to run_fold with a map-like accumulator, which will be subject of further PRs.

gadgetci · 2017-01-17T10:26:03Z

apps/ejabberd/src/ejabberd_hooks.erl

-run1([{_Seq, Module, Function} | Ls], Hook, Args) ->
+run1([], _Hook, Acc, _Args) ->
+    Acc;
+run1([{_Seq, Module, Function} | Ls], Hook, Acc, Args) ->
    Res = if is_function(Function) ->


According to Elvis:

Replace the 'if' expression on line 234 with a 'case' expression or function clauses.

gadgetci · 2017-01-17T10:26:04Z

apps/ejabberd/src/mod_last.erl

    LUser = jid:nodeprep(User),
    LServer = jid:nameprep(Server),
-    ?BACKEND:remove_user(LUser, LServer).
+    case ?BACKEND:remove_user(LUser, LServer) of


According to Elvis:

Don't use macros (like BACKEND on line 253) as module names.

gadgetci · 2017-01-17T10:26:05Z

apps/ejabberd/src/mod_caps.erl

                    #xmlel{name = <<"presence">>, attrs = Attrs,
-                           children = Els} = Pkt) ->
+                           children = Els}) ->
    Type = xml:get_attr_s(<<"type">>, Attrs),
    IsRemote = not lists:member(From#jid.lserver, ?MYHOSTS),
    if IsRemote and


According to Elvis:

Replace the 'if' expression on line 171 with a 'case' expression or function clauses.

gadgetci · 2017-01-17T10:26:06Z

apps/ejabberd/src/mod_offline.erl

+remove_user(Acc, User, Server) ->
+    remove_user(User, Server),
+    Acc.
+
 remove_user(User, Server) ->
    ?BACKEND:remove_user(User, Server).


According to Elvis:

Don't use macros (like BACKEND on line 544) as module names.

gadgetci · 2017-01-17T10:26:07Z

apps/ejabberd/src/mod_bosh.erl

-node_cleanup(Node) ->
-    ?BOSH_BACKEND:node_cleanup(Node).
+node_cleanup(Acc, Node) ->
+    Res = ?BOSH_BACKEND:node_cleanup(Node),


According to Elvis:

Don't use macros (like BOSH_BACKEND on line 139) as module names.

gadgetci · 2017-01-17T10:26:08Z

apps/ejabberd/src/mod_http_notification.erl

@@ -31,15 +31,16 @@ stop(Host) ->
    ejabberd_hooks:delete(user_send_packet, Host, ?MODULE, on_user_send_packet, 100),
    ok.

-on_user_send_packet(From, To, Packet) ->
+on_user_send_packet(Acc, From, To, Packet) ->
    Body = exml_query:path(Packet, [{element, <<"body">>}, cdata], <<>>),
    Mod = get_callback_module(),
    case Mod:should_make_req(Packet, From, To) of


According to Elvis:

Remove the dynamic function call on line 37. Only modules that define callbacks should make dynamic calls.

gadgetci · 2017-01-17T10:26:08Z

apps/ejabberd/src/mod_last.erl

@@ -223,11 +223,14 @@ get_last(LUser, LServer) ->
 count_active_users(LServer, Timestamp) ->
    ?BACKEND:count_active_users(LServer, Timestamp).


According to Elvis:

Don't use macros (like BACKEND on line 224) as module names.

gadgetci · 2017-01-17T10:26:09Z

apps/ejabberd/src/mod_caps.erl

                 #jid{luser = User, lserver = Server,
                      lresource = <<"">>},
                 #xmlel{name = <<"presence">>, attrs = Attrs,
-                        children = Els} = Pkt) ->
+                        children = Els}) ->
    Type = xml:get_attr_s(<<"type">>, Attrs),
    if Type == <<"">>; Type == <<"available">> ->


According to Elvis:

Replace the 'if' expression on line 154 with a 'case' expression or function clauses.

gadgetci · 2017-01-17T10:26:10Z

apps/ejabberd/src/mod_bosh.erl

-node_cleanup(Node) ->
-    ?BOSH_BACKEND:node_cleanup(Node).
+node_cleanup(Acc, Node) ->
+    Res = ?BOSH_BACKEND:node_cleanup(Node),


According to Elvis:

Remove the dynamic function call on line 139. Only modules that define callbacks should make dynamic calls.

gadgetci · 2017-01-17T10:26:11Z

apps/ejabberd/src/mod_offline.erl

@@ -533,6 +535,11 @@ remove_old_messages(Host, Days) ->
    Timestamp = fallback_timestamp(Days, os:timestamp()),
    ?BACKEND:remove_old_messages(Host, Timestamp).


According to Elvis:

Don't use macros (like BACKEND on line 536) as module names.

michalwski · 2017-01-17T10:40:42Z

As already agreed hook node_cleanup doesn't need special accumulator. I think It's enough if this hook's handler returns the value without putting it into a map.

bartekgorny · 2017-01-17T10:42:08Z

Actually, it was not on the list. But you're right, I'll rewrite it.

michalwski · 2017-01-17T10:52:17Z

apps/ejabberd/src/ejabberd_hooks.erl

@@ -111,7 +111,7 @@ run(Hook, Host, Args) ->
    case ets:lookup(hooks, {Hook, Host}) of
        [{_, Ls}] ->
            mongoose_metrics:increment_generic_hook_metric(Host, Hook),
-            run1(Ls, Hook, Args);
+            run1(Ls, Hook, #{}, Args); % run with empty map as accumulator


What stops us from calling run_fold1 here?

absolutely nothing :)

fenek · 2017-01-18T07:41:54Z

apps/ejabberd/src/ejabberd_gen_mam_archive.erl

@@ -19,8 +19,8 @@
        IsSimple :: boolean()) -> {ok, mod_mam:lookup_result()}
                                | {error, 'policy-violation'}.

-callback remove_archive(Host :: ejabberd:server(),
-    ArchiveID :: mod_mam:archive_id(), ArchiveJID :: ejabberd:jid()) -> 'ok'.
+-callback remove_archive(Accc :: map(), Host :: ejabberd:server(),


fenek · 2017-01-18T07:43:37Z

apps/ejabberd/src/ejabberd_hooks.erl

@@ -111,7 +111,7 @@ run(Hook, Host, Args) ->
    case ets:lookup(hooks, {Hook, Host}) of
        [{_, Ls}] ->
            mongoose_metrics:increment_generic_hook_metric(Host, Hook),
-            run1(Ls, Hook, Args);
+            run_fold1(Ls, Hook, #{}, Args); % run with empty map as accumulator


I think it would be better to replace whole function body with run_fold(Hook, Host, #{}, Args).

fenek · 2017-01-18T08:00:05Z

apps/ejabberd/src/mod_last.erl

-    on_presence_update(LUser, LServer, LResource, <<>>).
+session_cleanup(Acc, LUser, LServer, LResource, _SID) ->
+    case on_presence_update(Acc, LUser, LServer, LResource, <<>>) of
+        Acc when is_map(Acc) -> Acc;


Why is_map is needed here?

True, it is stupid. It went away in one of the next commits which did not end up in this PR.

fenek · 2017-01-18T08:10:55Z

apps/ejabberd/src/mod_offline_riak.erl

@@ -145,7 +145,7 @@ pop_msg(Key, LUser, LServer, To) ->
        MD = riakc_obj:get_update_metadata(Obj),
        [Timestamp] = riakc_obj:get_secondary_index(MD, ?TIMESTAMP_IDX),
        [Expire] = riakc_obj:get_secondary_index(MD, ?EXPIRE_IDX),
-        User = riakc_obj:get_secondary_index(MD, ?USER_IDX),
+%%        User = riakc_obj:get_secondary_index(MD, ?USER_IDX),


If it's not used, kill it with fire.

fenek · 2017-01-18T08:11:58Z

apps/ejabberd/src/mod_privacy.erl

@@ -122,7 +123,7 @@ start(Host, Opts) ->
                                                 remove_privacy_list, replace_privacy_list,
                                                 get_default_list]),
    ?BACKEND:init(Host, Opts),
-    IQDisc = gen_mod:get_opt(iqdisc, Opts, one_queue),
+%%    IQDisc = gen_mod:get_opt(iqdisc, Opts, one_queue),


Kill it with fire too. Here or in another PR, as it's not directly related to foldifying hooks.

fenek · 2017-01-18T08:41:51Z

test/ejabberd_tests/tests/login_SUITE.erl

    [AdminU, AdminS, AdminP] = escalus_users:get_usp(Config, AdminSpec),

-    ok = escalus_ejabberd:rpc(ejabberd_auth, try_register, [AdminU, AdminS, AdminP]),
+    escalus_ejabberd:rpc(ejabberd_auth, try_register, [AdminU, AdminS, AdminP]),


Why have you removed the match for success?

It was temporary - later commits introduced mongoose_stanza:to_map/1 which allows to check whether the return was correct, so those two lines because:

Res = escalus_ejabberd:rpc... #{} = mongoose_stanza:to_map(Res)

fenek · 2017-01-18T08:42:22Z

apps/ejabberd/src/ejabberd_admin.erl

        {error, exists} ->
            String = io_lib:format("User ~s@~s already registered at node ~p",
                                   [User, Host, node()]),
            {exists, String};
        {error, Reason} ->
            String = io_lib:format("Can't register user ~s@~s at node ~p: ~p",
                                   [User, Host, node(), Reason]),
-            {cannot_register, String}
+            {cannot_register, String};
+        _ ->


I would avoid matching on everything here.

fenek · 2017-01-18T08:42:36Z

apps/ejabberd/src/mod_commands.erl

        {error, exists} ->
            String = io_lib:format("User ~s@~s already registered at node ~p",
                                   [User, Host, node()]),
            throw({exists, String});
        {error, Reason} ->
            String = io_lib:format("Can't register user ~s@~s at node ~p: ~p",
                                   [User, Host, node(), Reason]),
-            throw({error, String})
+            throw({error, String});
+        _ ->


I would avoid matching on everything here.

fenek · 2017-01-18T08:42:54Z

apps/ejabberd/src/mod_register.erl

                                            {error, ?ERR_NOT_ACCEPTABLE}
-                                    end
+                                    end;
+                                _ ->


I would avoid matching on everything here.

fenek · 2017-01-18T08:44:32Z

test/ejabberd_tests/tests/mam_SUITE.erl

@@ -2068,7 +2068,7 @@ check_user_exist(Config) ->
  %% when
  [{_, AdminSpec}] = escalus_users:get_users([admin]),
  [AdminU, AdminS, AdminP] = escalus_users:get_usp(Config, AdminSpec),
-  ok = escalus_ejabberd:rpc(ejabberd_auth, try_register, [AdminU, AdminS, AdminP]),
+  #{} = escalus_ejabberd:rpc(ejabberd_auth, try_register, [AdminU, AdminS, AdminP]),


I think ejabberd_auth:try_register returning an empty map on success is not very intuitive, especially it's not a hook on the outside. Internal hook usage leaks outside the scope of the function now. :(

gadgetci · 2017-01-19T17:29:35Z

apps/ejabberd/src/mod_mam_cassandra_prefs.erl

@@ -200,13 +200,13 @@ get_prefs({GlobalDefaultMode, _, _}, _Host, _UserID, UserJID) ->

 -spec remove_archive(ejabberd:server(), mod_mam:archive_id(),


According to Compiler:

spec for undefined function remove_archive/3

gadgetci · 2017-01-19T17:29:36Z

compiler failed with exit status: 1

gadgetci · 2017-01-20T10:30:50Z

apps/ejabberd/src/mod_mam_cassandra_prefs.erl

@@ -200,13 +200,13 @@ get_prefs({GlobalDefaultMode, _, _}, _Host, _UserID, UserJID) ->

 -spec remove_archive(ejabberd:server(), mod_mam:archive_id(),


According to Compiler:

spec for undefined function remove_archive/3

michalwski · 2017-01-27T19:46:25Z

@fenek Could you make final review? You requested some changes, most of them were addressed by @bartekgorny

fenek · 2017-01-30T16:35:54Z

Well, now it requires rebasing. :( Sorry for the delay but sickness was merciless.

arg

gadgetci reviewed Jan 17, 2017

View reviewed changes

michalwski reviewed Jan 17, 2017

View reviewed changes

bartekgorny force-pushed the accum-run branch from 4d91bec to cac8bb9 Compare January 17, 2017 15:25

fenek requested changes Jan 18, 2017

View reviewed changes

bartekgorny force-pushed the accum-run branch from cac8bb9 to 21a1352 Compare January 18, 2017 17:19

gadgetci reviewed Jan 19, 2017

View reviewed changes

bartekgorny force-pushed the accum-run branch from de80cae to 9157b92 Compare January 20, 2017 10:27

gadgetci reviewed Jan 20, 2017

View reviewed changes

bartekgorny force-pushed the accum-run branch from 6a716af to 9f2fafa Compare January 25, 2017 13:21

bartekgorny added 5 commits January 31, 2017 11:35

Changed ejabberd_hooks:run and all its handlers to pass a map as first

66e4ff8

arg

Bugfixes, cleanup, applied reviewers' comments, rebased onto master

d5ac818

fixed cassandra mam backend bug

a9da6d5

fixed mam cassandra prefs handler arity

50ca079

fixed spec of a function

5a8a85a

bartekgorny force-pushed the accum-run branch from 9f2fafa to 5a8a85a Compare January 31, 2017 10:39

bartekgorny closed this Jan 31, 2017

bartekgorny reopened this Jan 31, 2017

fenek approved these changes Jan 31, 2017

View reviewed changes

fenek merged commit 57f0701 into master Jan 31, 2017

fenek deleted the accum-run branch January 31, 2017 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run hooks with a default (empty) accumulator #1156

Run hooks with a default (empty) accumulator #1156

bartekgorny commented Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

gadgetci Jan 17, 2017

michalwski commented Jan 17, 2017

bartekgorny commented Jan 17, 2017

michalwski Jan 17, 2017

bartekgorny Jan 17, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

bartekgorny Jan 18, 2017

fenek Jan 18, 2017

bartekgorny Jan 18, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

bartekgorny Jan 18, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

fenek Jan 18, 2017

gadgetci Jan 19, 2017

gadgetci commented Jan 19, 2017

gadgetci Jan 20, 2017

michalwski commented Jan 27, 2017

fenek commented Jan 30, 2017

		@@ -223,11 +223,14 @@ get_last(LUser, LServer) ->
		count_active_users(LServer, Timestamp) ->
		?BACKEND:count_active_users(LServer, Timestamp).

		@@ -533,6 +535,11 @@ remove_old_messages(Host, Days) ->
		Timestamp = fallback_timestamp(Days, os:timestamp()),
		?BACKEND:remove_old_messages(Host, Timestamp).

		@@ -200,13 +200,13 @@ get_prefs({GlobalDefaultMode, _, _}, _Host, _UserID, UserJID) ->

		-spec remove_archive(ejabberd:server(), mod_mam:archive_id(),

Run hooks with a default (empty) accumulator #1156

Run hooks with a default (empty) accumulator #1156

Conversation

bartekgorny commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Jan 17, 2017

bartekgorny commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadgetci commented Jan 19, 2017

Choose a reason for hiding this comment

michalwski commented Jan 27, 2017

fenek commented Jan 30, 2017