Feature/xep 357 push notifications #1178

rslota · 2017-02-03T13:55:02Z

This PR features XEP-0357 implementation. All needed informations can be found in mod_push.md. Right now only PM and MUCLight messages are supported.
Also pleas note that this code requires a custom PubSub node that will handle notifications from this module and pass them to external Push Notifications Service.

Tis PR also adds worker_pool as pooler used in mod_push. This pooler is faster and more configurable than others like poolboy. There are several pending PRs that use this pooler, so maybe someday we are gonna switch fully to worker_pool.

gadgetci · 2017-02-03T13:55:06Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -130,9 +131,9 @@ route(From, To, Packet) ->
    case (catch do_route(From, To, Packet)) of
        {'EXIT', Reason} ->
            ?ERROR_MSG("error when routing from=~ts to=~ts in module=~p, reason=~p, packet=~ts, stack_trace=~p",


According to Elvis:

Line 133 is too long: ?ERROR_MSG("error when routing from=~ts to=~ts in module=~p, reason=~p, packet=~ts, stack_trace=~p",.

gadgetci · 2017-02-03T13:55:07Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -819,7 +820,7 @@ get_user_present_pids(LUser, LServer) ->

 -spec get_user_present_resources(LUser :: ejabberd:user(),
                                 LServer :: ejabberd:server()
-                                 ) -> [{priority(), binary()}].
+                                ) -> [{priority(), binary()}].
 get_user_present_resources(LUser, LServer) ->
    Ss = ?SM_BACKEND:get_sessions(LUser, LServer),


According to Elvis:

Don't use macros (like SM_BACKEND on line 825) as module names.

gadgetci · 2017-02-03T13:58:12Z

apps/ejabberd/src/mod_push_mnesia.erl

+              Node :: mod_push:pubsub_node()) -> ok | {error, Reason :: term()}.
+disable(User, undefined, undefined) ->
+    delete(key(User));
+disable(User, undefined, undefined) ->


According to Compiler:

Warning: this clause cannot match because a previous clause at line 61 always matches

gadgetci · 2017-02-03T13:58:13Z

apps/ejabberd/src/mod_push_plugin.erl

+%%% @end
+%%%-------------------------------------------------------------------
+-module(mod_push_plugin).
+-behavior(mod_push_plugin).


According to Compiler:

Warning: behaviour mod_push_plugin undefined

gadgetci · 2017-02-03T14:20:35Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -819,7 +821,7 @@ get_user_present_pids(LUser, LServer) ->

 -spec get_user_present_resources(LUser :: ejabberd:user(),
                                 LServer :: ejabberd:server()
-                                 ) -> [{priority(), binary()}].
+                                ) -> [{priority(), binary()}].
 get_user_present_resources(LUser, LServer) ->
    Ss = ?SM_BACKEND:get_sessions(LUser, LServer),


According to Elvis:

Don't use macros (like SM_BACKEND on line 826) as module names.

fenek · 2017-02-13T09:01:01Z

doc/modules/mod_push.md

+### Module Description
+This module implements [XEP-0357: Push Notifications](https://xmpp.org/extensions/xep-0357.html).
+ It enables a service that notifies `PubSub` of user's choice about every message that he could 
+ miss while being offline. There are two control stanzas that clinet may send to this module: 


Typo: clinet

fenek · 2017-02-13T09:25:01Z

apps/ejabberd/src/ejabberd_sm.erl

 do_route_no_resource(<<"broadcast">>, _, From, To, Packet) ->
-    % Backward compatibility
+                                                % Backward compatibility


Indent glitch? :)

fenek · 2017-02-13T09:25:31Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -744,7 +746,7 @@ route_message(From, To, Packet) ->
              %% Route messages to all priority that equals the max, if
              %% positive
              fun({Prio, Pid}) when Prio == Priority ->
-                      % we will lose message if PID is not alive
+                                                % we will lose message if PID is not alive


Indent glitch? :)

Yeah... My formatter (emacs) assumes that comments with single '%' have fixed indentation because of reasons. I'm gonna revert all those changes.

fenek · 2017-02-13T09:25:46Z

apps/ejabberd/src/ejabberd_sm.erl


-% Backward compatibility
+                                                % Backward compatibility


Indent glitch? :)

fenek · 2017-02-13T09:26:25Z

apps/ejabberd/src/ejabberd_sm.erl

@@ -976,19 +978,19 @@ user_resources(UserStr, ServerStr) ->
 sm_backend(Backend) ->
    lists:flatten(
      ["-module(ejabberd_sm_backend).
-        -export([backend/0]).
+-export([backend/0]).


Broken indent

fenek · 2017-02-13T09:26:45Z

apps/ejabberd/src/ejabberd_sm.erl

-        _ ->
-            0
-    end.
+case mongoose_metrics:get_metric_value(global, ?UNIQUE_COUNT_CACHE) of


Broken indent.

michalwski · 2017-02-13T10:13:12Z

apps/ejabberd/src/mod_push_plugin.erl

+        <<"chat">> ->
+            jid:to_binary(jid:to_bare(jid:to_lower(From)));
+        <<"groupchat">> ->
+            LResource


In this case LResource is the message sender. Important information in case of groupchat notifications is also the room name. How this will be delivered in the push notification?

You're right. Room name will be stripped and the user will get only sender JID. If you think it shouldn't, I'll add LUser to sender_id.

Yes, I think it's mandatory to deliver the room name for groupchat notifications.

I agree that room's luser should be included as well.

michalwski · 2017-02-13T10:16:33Z

doc/modules/mod_push.md

+  filter messages that shall not be published to `PubSub` node.
+
+
+[aws-virtual-host]: https://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html


It's not clear to me why do we need this and following links in this doc.

Just a copy-paste error. Removed.

michalwski · 2017-02-13T10:18:49Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+-define(NS_XDATA,               <<"jabber:x:data">>).
+-define(NS_PUBSUB_PUB_OPTIONS,  <<"http://jabber.org/protocol/pubsub#publish-options">>).
+-define(PUSH_FORM_TYPE,         <<"urn:xmpp:push:summary">>).
+-define(MUCHOST,                <<"muclight.localhost">>).


We should avoid hardcoding the main XMPP domain served by the target server. You can easily read it from tests configuration via:

ct:get_config({hosts, mim, domain})

michalwski · 2017-02-13T10:19:09Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+-define(PUSH_FORM_TYPE,         <<"urn:xmpp:push:summary">>).
+-define(MUCHOST,                <<"muclight.localhost">>).
+
+-define(MUC_HOST,               <<"muc.localhost">>).


Another hardcoded localhost.

michalwski · 2017-02-13T10:21:18Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+init_per_group(muclight_msg_notifications, Config) ->
+    Host = ct:get_config({hosts, mim, domain}),
+    dynamic_modules:start(Host, mod_push, ?PUSH_OPTS),
+    dynamic_modules:start(<<"localhost">>, mod_muc_light,


Hardcoded localhost while one line above domain read from config was used.

fenek · 2017-02-13T10:21:53Z

apps/ejabberd/src/mod_push.erl

+start(Host, Opts) ->
+    ?INFO_MSG("mod_push starting on host ~p", [Host]),
+
+    ok = application:ensure_started(worker_pool),


I'd avoid command that could start worker_pool here, as it should be done during VM boot. I think it would be better to check for wpool:start_sup_pool return value to not only verify that worker_pool is running but also that pool creation was indeed successful.

michalwski · 2017-02-13T10:27:02Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+        {group, muclight_msg_notifications}
+    ].
+
+groups() ->


This set of test cases looks impressive. I have two general comments:

This sute is not executed on travis as it is not added to default.spec

I encourage to run all groups in parallel. escalus:fresh_story is your friend here, also you can take a look at sm_SUITE or any other suite using fresh_story.

Ok, I'm gonna try enabling parallel groups.

fenek · 2017-02-13T10:28:27Z

apps/ejabberd/src/mod_push.erl

+    ok.
+
+-spec stop(Host :: ejabberd:server()) -> ok.
+stop(Host) ->


Lacks worker pool shutdown.

fenek · 2017-02-13T10:40:44Z

apps/ejabberd/src/mod_push_mnesia.erl

+       form = Form
+      }.
+
+-spec key(ejabberd:jid()) -> key() | no_return().


It can't have no_return(), can it?

Good question, indeed :)

Yeah, there was explicit throw in early implementation but its gone now, I just forgot to change spec.

fenek · 2017-02-13T10:56:16Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+
+-record(route, {from, to, packet}).
+
+-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).


Is it used anywhere actually?

Nope, sorry about that.

fenek · 2017-02-13T10:58:30Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+            Server = escalus_client:server(Alice),
+            escalus:send(Alice, escalus_stanza:disco_info(Server)),
+            Stanza = escalus:wait_for_stanza(Alice),
+            try


What is the reason for using these try..catches in test cases?

I guess for better error readability. I saw something like that in other test suite in disco checks so I thought thats a thing. If its not I'm just gonna remove it.

Those checks in other suite were probably implemented when escalus wasn't reporting full stanza log upon error yet. Now there is no need to manually log stanza. Which suite is it BTW?

There are several try..catch (11) like that in mam_SUITE and muc_SUITE.
Anyway, I'm gonna remove it from here.

fenek · 2017-02-13T11:00:53Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+         end]),
+    Config.
+
+-spec rpc(M :: atom(), F :: atom(), A :: [term()]) -> term().


Already implemented in https://github.com/esl/escalus/blob/master/src/escalus_ejabberd.erl#L55

Unless it's about the timeout value?

No its not, just forgot about that.

fenek · 2017-02-13T11:02:31Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+    Cookie = escalus_ct:get_config(ejabberd_cookie),
+    escalus_ct:rpc_call(Node, M, F, A, 10000, Cookie).
+
+create_room(Room, [Owner | Members]) ->


Maybe using https://github.com/esl/MongooseIM/blob/master/test.disabled/ejabberd_tests/tests/muc_light_SUITE.erl#L271 would be easier, faster and would require less imports?

I'll try this way. Somehow I prefer initialising via XMPP.

fenek · 2017-02-13T11:22:51Z

test.disabled/ejabberd_tests/tests/push_SUITE.erl

+start_publish_listener(Config) ->
+    TestCasePid = self(),
+    rpc(meck, new, [ejabberd_router, [no_link, passthrough]]),
+    rpc(meck, expect,


I don't like the tampering with ejabberd_router here. Have you considered declaring e.g. push.localhost as JID in enable request and registering CT process as domain handler with register_route?

To be honest I just wasn't aware there is such option. I'll try that, thanks.

fenek · 2017-02-13T11:29:15Z

apps/ejabberd/src/mod_push_mnesia.erl

+    transaction(F).
+
+-spec transaction(fun(() -> any())) -> Result :: any().
+transaction(F) ->


Do we need transactions in case of mod_push? I believe that conflicts or race conditions won't occur without them and it's always less work for Mnesia. :)

It just reduces the change of race condition with poorly written clients. If no one have an objection I will remove those transactions.

gadgetci · 2017-02-17T08:02:43Z

apps/ejabberd/src/mod_push_plugin.erl

+-spec should_publish(From :: ejabberd:jid(), To :: ejabberd:jid(), Packet :: jlib:xmlel()) ->
+                            boolean().
+should_publish(_From, To = #jid{luser = LUser, lserver = LServer}, _Packet) ->
+    ?WARNING_MSG("should_publish ~p", [{LUser, LServer,


According to Elvis:

Missing space right "," on line 53

gadgetci · 2017-02-17T08:02:44Z

apps/ejabberd/src/mod_push_plugin.erl

+should_publish(_From, To = #jid{luser = LUser, lserver = LServer}, _Packet) ->
+    ?WARNING_MSG("should_publish ~p", [{LUser, LServer,
+                                        ejabberd_users:does_user_exist(LUser, LServer),
+                                        catch lists:max(ejabberd_sm:get_user_present_pids(LUser, LServer))}]),


According to Elvis:

Line 55 is too long: catch lists:max(ejabberd_sm:get_user_present_pids(LUser, LServer))}]),
.

gadgetci · 2017-02-17T08:05:30Z

apps/ejabberd/src/mod_push_plugin.erl

+%%% @end
+%%%-------------------------------------------------------------------
+-module(mod_push_plugin).
+-behavior(mod_push_plugin).


According to Compiler:

Warning: behaviour mod_push_plugin undefined

michalwski · 2017-02-18T10:50:52Z

Thanks!

gadgetci reviewed Feb 3, 2017

View reviewed changes

rslota added to-discuss waiting-for-review labels Feb 3, 2017

gadgetci reviewed Feb 3, 2017

View reviewed changes

rslota force-pushed the feature/xep-357-push_notifications branch from da3e73c to 69b126a Compare February 3, 2017 14:20

gadgetci reviewed Feb 3, 2017

View reviewed changes

rslota force-pushed the feature/xep-357-push_notifications branch from 69b126a to b4e2c1b Compare February 10, 2017 11:06

fenek reviewed Feb 13, 2017

View reviewed changes

apps/ejabberd/src/ejabberd_sm.erl Outdated

% Backward compatibility

% Backward compatibility

Copy link

Member

fenek Feb 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent glitch? :)

fenek reviewed Feb 13, 2017

View reviewed changes

michalwski reviewed Feb 13, 2017

View reviewed changes

fenek reviewed Feb 13, 2017

View reviewed changes

michalwski reviewed Feb 13, 2017

View reviewed changes

fenek reviewed Feb 13, 2017

View reviewed changes

gadgetci reviewed Feb 17, 2017

View reviewed changes

rslota force-pushed the feature/xep-357-push_notifications branch 3 times, most recently from 76f5a03 to 3791c48 Compare February 17, 2017 13:37

rslota added 16 commits February 17, 2017 16:14

Initial code structure for XEP-357

380f7da

Initial code structure for XEP-357

88aa171

Full draft-implementation of XEP=357

32c7d66

Add several test cases and make sure that pm notifications work

484b40e

Add support for MUC Light's messages in push notifications module

ed6f738

Code formatting changes

ab5fefc

Refactor mod_push_plugin module

b7a9c1d

Fix xforms encoding

0432621

Fix pubsub JID decoding

77cc3dd

Fix Host on push plugin

0a280dc

Add documentation

1553009

Fix dialyzer

78d74f1

Code quality improvements

994a107

Run push_SUITE's cases run in parallel

4969c35

Remove ?FUNCTION_NAME macro usage

ed21adf

Split mod_push_plugin into two modules

a34a6ea

rslota force-pushed the feature/xep-357-push_notifications branch from 3791c48 to a34a6ea Compare February 17, 2017 15:15

rslota added ready and removed to-discuss waiting-for-review labels Feb 17, 2017

michalwski merged commit 67aeaf6 into master Feb 18, 2017

michalwski deleted the feature/xep-357-push_notifications branch February 18, 2017 10:50

michalwski mentioned this pull request Mar 28, 2017

MongooseIM 2.1.0beta1 #1244

Merged

		filter messages that shall not be published to `PubSub` node.


		[aws-virtual-host]: https://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html


		-record(route, {from, to, packet}).

		-define(assertReceivedMatch(Expect), ?assertReceivedMatch(Expect, 0)).

Feature/xep 357 push notifications #1178

Feature/xep 357 push notifications #1178

Conversation

rslota commented Feb 3, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenek Feb 13, 2017 • edited Loading

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

michalwski commented Feb 18, 2017

rslota commented Feb 3, 2017 •

edited

Loading

fenek Feb 13, 2017 •

edited

Loading