ejabberd - Comments for "New hooks for mod_pubsub"

I am seeing a curious

kailesmith — Sun, 19 Jun 2011 03:28:02 +0000

I am seeing a curious behaviour and wanted to check. I made the changes to mod_pubsub.erl to fire pubsub_create_node and pubsub_delete_node events and tried it out with a test module. For example the create event is handled as:

create_node(ServerHost, Host, Node, NodeOptions) ->
  ?DEBUG("mod_sequence: Node created at ~p, ~p is ~p with ~n~p",
         [ServerHost, Host, Node, NodeOptions]),
  ok.

I am seeing two entries in the log right after each other for each create event. Example:

=INFO REPORT==== 2011-06-15 09:56:18 ===
D(<0.357.0>:mod_sequence:67) : mod_sequence: Node created at <<"localhost">>, <<"pubsub.localhost">> is <<"test1">> with
[{deliver_payloads,true},
 {notify_config,false},
 {notify_delete,false},
 {notify_retract,true},
 {notify_sub,false},
 {purge_offline,false},
 {persist_items,true},
 {max_items,10},
 {subscribe,true},
 {access_model,open},
 {roster_groups_allowed,[]},
 {publish_model,publishers},
 {notification_type,headline},
 {max_payload_size,60000},
 {send_last_published_item,on_sub_and_presence},
 {deliver_notifications,true},
 {presence_based_delivery,false}]
=INFO REPORT==== 2011-06-15 09:56:18 ===
D(<0.357.0>:mod_sequence:67) : mod_sequence: Node created at <<"localhost">>, <<"pubsub.localhost">> is <<"test1">> with
[{deliver_payloads,true},
 {notify_config,false},
 {notify_delete,false},
 {notify_retract,true},
 {notify_sub,false},
 {purge_offline,false},
 {persist_items,true},
 {max_items,10},
 {subscribe,true},
 {access_model,open},
 {roster_groups_allowed,[]},
 {publish_model,publishers},
 {notification_type,headline},
 {max_payload_size,60000},
 {send_last_published_item,on_sub_and_presence},
 {deliver_notifications,true},
 {presence_based_delivery,false}]

Is this expected behaviour?

I finally figured out how to dump stacktrace and here are the results. It is being called twice, but I am still not sure why. Here is what I see in the log file:

=INFO REPORT==== 2011-06-18 20:23:32 ===
D(<0.357.0>:mod_sequence:73) : mod_sequence: stack trace 
[{mod_sequence,create_node,4},
 {ejabberd_hooks,run1,3},
 {ejabberd_hooks,run,3},
 {mod_pubsub,create_node,7},
 {mod_pubsub,do_route,7},
 {mod_pubsub,handle_info,2},
 {gen_server,handle_msg,5},
 {proc_lib,init_p_do_apply,3}]
=INFO REPORT==== 2011-06-18 20:23:32 ===
D(<0.357.0>:mod_sequence:75) : mod_sequence: Node created at <<"localhost">>, <<"pubsub.localhost">> is <<"test1">> with 
[{deliver_payloads,true},
 {notify_config,false},
 {notify_delete,false},
 {notify_retract,true},
 {notify_sub,false},
 {purge_offline,false},
 {persist_items,true},
 {max_items,10},
 {subscribe,true},
 {access_model,open},
 {roster_groups_allowed,[]},
 {publish_model,publishers},
 {notification_type,headline},
 {max_payload_size,60000},
 {send_last_published_item,on_sub_and_presence},
 {deliver_notifications,true},
 {presence_based_delivery,false}]
=INFO REPORT==== 2011-06-18 20:23:32 ===
D(<0.357.0>:mod_sequence:73) : mod_sequence: stack trace 
[{mod_sequence,create_node,4},
 {ejabberd_hooks,run1,3},
 {mod_pubsub,create_node,7},
 {mod_pubsub,do_route,7},
 {mod_pubsub,handle_info,2},
 {gen_server,handle_msg,5},
 {proc_lib,init_p_do_apply,3}]
=INFO REPORT==== 2011-06-18 20:23:32 ===
D(<0.357.0>:mod_sequence:75) : mod_sequence: Node created at <<"localhost">>, <<"pubsub.localhost">> is <<"test1">> with 
[{deliver_payloads,true},
 {notify_config,false},
 {notify_delete,false},
 {notify_retract,true},
 {notify_sub,false},
 {purge_offline,false},
 {persist_items,true},
 {max_items,10},
 {subscribe,true},
 {access_model,open},
 {roster_groups_allowed,[]},
 {publish_model,publishers},
 {notification_type,headline},
 {max_payload_size,60000},
 {send_last_published_item,on_sub_and_presence},
 {deliver_notifications,true},
 {presence_based_delivery,false}]

Great. Glad to see that you

kailesmith — Wed, 15 Jun 2011 15:02:14 +0000

Great. Glad to see that you agree. I will make the changes, test them and submit them via bug tracker.

Right, the best way would be

mfoss — Wed, 15 Jun 2011 10:55:27 +0000

Right, the best way would be to run two new hooks in mod_pubsub.

The improvement looks useful. Once you have a patch that you like, you can create a ticket in the bug tracker describing the problem and attaching your patch as a proposed solution.

Here's a preliminary implementation. I didn't test it.

--- mod_pubsub/mod_pubsub.erl
+++ mod_pubsub/mod_pubsub.erl
@@ -1783,13 +1783,16 @@ create_node(Host, ServerHost, Node, Owner, GivenType, Access, Configuration) ->
 	    case transaction(CreateNode, transaction) of
 		{result, {NodeId, SubsByDepth, {Result, broadcast}}} ->
 		    broadcast_created_node(Host, Node, NodeId, Type, NodeOptions, SubsByDepth),
+		    ejabberd_hooks:run(pubsub_create_node, ServerHost, [ServerHost, Host, NodeId, NodeOptions, SubsByDepth]),
 		    case Result of
 			default -> {result, Reply};
 			_ -> {result, Result}
 		    end;
-		{result, {_NodeId, _SubsByDepth, default}} ->
+		{result, {NodeId, SubsByDepth, default}} ->
+		    ejabberd_hooks:run(pubsub_create_node, ServerHost, [ServerHost, Host, NodeId, NodeOptions, SubsByDepth]),
 		    {result, Reply};
-		{result, {_NodeId, _SubsByDepth, Result}} ->
+		{result, {NodeId, SubsByDepth, Result}} ->
+		    ejabberd_hooks:run(pubsub_create_node, ServerHost, [ServerHost, Host, NodeId, NodeOptions, SubsByDepth]),
 		    {result, Result};
 		Error ->
 		    %% in case we change transaction to sync_dirty...
@@ -1834,6 +1837,7 @@ delete_node(Host, Node, Owner) ->
 		    end
 	     end,
     Reply = [],
+    ServerHost = dumb_get_serverhost(Host),
     case transaction(Host, Node, Action, transaction) of
 	{result, {_, {SubsByDepth, {Result, broadcast, Removed}}}} ->
 	    lists:foreach(fun({RNode, _RSubscriptions}) ->
@@ -1843,23 +1847,31 @@ delete_node(Host, Node, Owner) ->
 		Options = RNode#pubsub_node.options,
 		broadcast_removed_node(RH, RN, NodeId, Type, Options, SubsByDepth)
 	    end, Removed),
+	    ejabberd_hooks:run(pubsub_delete_node, ServerHost, [ServerHost, Host, Node]),
 	    case Result of
 		default -> {result, Reply};
 		_ -> {result, Result}
 	    end;
 	{result, {_, {_, {Result, _Removed}}}} ->
+	    ejabberd_hooks:run(pubsub_delete_node, ServerHost, [ServerHost, Host, Node]),
 	    case Result of
 		default -> {result, Reply};
 		_ -> {result, Result}
 	    end;
 	{result, {_, {_, default}}} ->
+	    ejabberd_hooks:run(pubsub_delete_node, ServerHost, [ServerHost, Host, Node]),
 	    {result, Reply};
 	{result, {_, {_, Result}}} ->
+	    ejabberd_hooks:run(pubsub_delete_node, ServerHost, [ServerHost, Host, Node]),
 	    {result, Result};
 	Error ->
 	    Error
     end.
 
+%% "pubsub.localhost.pubsub.example.org" --> "localhost.pubsub.example.org"
+dumb_get_serverhost(Host) ->
+    string:join(string:substr(string:tokens(Host, "."), 2), ".").
+
 %% @spec (Host, Node, From, JID, Configuration) ->
 %%		  {error, Reason::stanzaError()} |
 %%		  {result, []}