-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored hook handlers in mod_event_pusher_hook_translator module #3769
Conversation
Codecov ReportBase: 82.82% // Head: 82.79% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3769 +/- ##
==========================================
- Coverage 82.82% 82.79% -0.03%
==========================================
Files 531 531
Lines 34138 34152 +14
==========================================
+ Hits 28274 28277 +3
- Misses 5864 5875 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
small_tests_24 / small_tests / a19916a small_tests_25 / small_tests / a19916a ldap_mnesia_24 / ldap_mnesia / a19916a dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a19916a sm_SUITE:parallel_manual_ack_freq_1:resume_session_state_send_message_with_ack{error,
{{assertion_failed,assert,is_presence,
{xmlel,<<"message">>,
[{<<"from">>,
<<"bob_resume_session_state_send_message_with_ack_2750@domain.example.com/escalus-default-resource">>},
{<<"to">>,
<<"alice_resume_session_state_send_message_with_ack_2757@domain.example.com">>},
{<<"xml:lang">>,<<"en">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-3">>}]}]},
"<message from='bob_resume_session_state_send_message_with_ack_2750@domain.example.com/escalus-default-resource' to='alice_resume_session_state_send_message_with_ack_2757@domain.example.com' xml:lang='en' type='chat'><body>msg-3</body></message>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{sm_helper,initial_presence_step,2,
[{file,"/home/circleci/project/big_tests/tests/sm_helper.erl"},
{line,133}]},
{escalus_connection,connection_step,2,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
{line,162}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_connection,start,2,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
{line,144}]},
{sm_helper,connect_spec,3,
[{file,"/home/circleci/project/big_tests/tests/sm_helper.er... dynamic_domains_mysql_redis_25 / mysql_redis / a19916a metrics_api_SUITE:global:session_counters{error,{{assertEqual,[{module,metrics_api_SUITE},
{line,428},
{comment,[{counter,totalSessionCount}]},
{expression,"ValueList"},
{expected,2},
{value,3}]},
[{metrics_api_SUITE,fetch_global_gauge_value,2,
[{file,"/home/circleci/project/big_tests/tests/metrics_api_SUITE.erl"},
{line,428}]},
{metrics_api_SUITE,'-session_counters/1-fun-3-',4,
[{file,"/home/circleci/project/big_tests/tests/metrics_api_SUITE.erl"},
{line,217}]},
{escalus_story,story,4,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1291}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}} ldap_mnesia_25 / ldap_mnesia / a19916a dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a19916a internal_mnesia_25 / internal_mnesia / a19916a pgsql_mnesia_24 / pgsql_mnesia / a19916a dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a19916a elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a19916a pgsql_mnesia_25 / pgsql_mnesia / a19916a mysql_redis_25 / mysql_redis / a19916a riak_mnesia_24 / riak_mnesia / a19916a mssql_mnesia_25 / odbc_mssql_mnesia / a19916a disco_and_caps_SUITE:disco_with_caps:user_can_query_friend_resources{error,{{assertion_failed,assert_many,false,[is_roster_set],[],[]},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{escalus_story,'-make_all_clients_friends/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,108}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl_1,3,[{file,"lists.erl"},{line,1355}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1350}]},
{escalus_utils,distinct_pairs,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,60}]},
{escalus_story,make_all_clients_friends,1,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,106}]}]}} dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a19916a dynamic_domains_mysql_redis_25 / mysql_redis / a19916a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, though I just have one thought here, see comment :)
src/mongoose_hooks.erl
Outdated
Params = #{from => From, to => To, packet => Packet}, | ||
Args = [From, To, Packet], | ||
ParamsWithLegacyArgs = ejabberd_hooks:add_args(Params, Args), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this one is one that I'll change again during my c2s rework, because from
, to
, and packet
are already part of the mongoose_acc
, and so it always felt redundant to have all those parameters. So when they're needed, the handler can do {From, To, Packet} = mongoose_acc:packet(Acc)
and will get them. So many handlers actually ignore one or another or both... So I don't know if we want to require a migration for the handlers in the next release, and yet another migration for the same handler in the following one, after my c2s work is merged 😕
@DenysGonchar what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting here some notes after conversation with @NelsonVides:
It's proposed to follow this rules to reduce the number of merge conflicts in the future, and prevent collisions between the modules:
Acc
should be the source of such data, because some of the handlers may extend thePacket
with extra fields/attributes, we must ensure that one handler cannot not accidentally override the changes done in another handler.- please don't put anything in
Params
except the legacyArgs
. - in the reworked hook handler functions get
{From, To, Packet}
parameters from theAcc
.
I believe the same comment is relevant to rest_user_send_packet
and probably some other send*/filter* hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied changes to follow these rules. Please check if it is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
c41b995
to
3d083b2
Compare
small_tests_24 / small_tests / c41b995 small_tests_25 / small_tests / c41b995 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c41b995 ldap_mnesia_24 / ldap_mnesia / c41b995 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / c41b995 ldap_mnesia_25 / ldap_mnesia / c41b995 dynamic_domains_mysql_redis_25 / mysql_redis / c41b995 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / c41b995 internal_mnesia_25 / internal_mnesia / c41b995 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / c41b995 pgsql_mnesia_24 / pgsql_mnesia / c41b995 mysql_redis_25 / mysql_redis / c41b995 pgsql_mnesia_25 / pgsql_mnesia / c41b995 riak_mnesia_24 / riak_mnesia / c41b995 mssql_mnesia_25 / odbc_mssql_mnesia / c41b995 pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_unsubscribe_after_presence_unsubscription_2458@localhost">>},
{<<"to">>,
<<"bob_unsubscribe_after_presence_unsubscription_2458@localhost/res1">>},
{<<"type">>,<<"headline">>}],
[{xmlel,<<"event">>,
[{<<"xmlns">>,
<<"http://jabber.org/protocol/pubsub#event">>}],
[{xmlel,<<"items">>,
[{<<"node">>,<<"jGUo2MOp83iPSLy+azvOpQ==">>}],
[{xmlel,<<"item">>,
[{<<"id">>,<<"salmon">>}],
[{xmlel,<<"entry">>,
[{<<"xmlns">>,
<<"http://www.w3.org/2005/Atom">>}],
[]}]}]}]},
{xmlel,<<"headers">>,
[{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
[]}]}]},
[{pep_SUITE,'-unsubscribe_after_presence_unsubscription/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
{line,384}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1291}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}} |
small_tests_24 / small_tests / 3d083b2 small_tests_25 / small_tests / 3d083b2 ldap_mnesia_24 / ldap_mnesia / 3d083b2 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 3d083b2 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3d083b2 ldap_mnesia_25 / ldap_mnesia / 3d083b2 dynamic_domains_mysql_redis_25 / mysql_redis / 3d083b2 internal_mnesia_25 / internal_mnesia / 3d083b2 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 3d083b2 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3d083b2 pgsql_mnesia_24 / pgsql_mnesia / 3d083b2 pgsql_mnesia_25 / pgsql_mnesia / 3d083b2 mysql_redis_25 / mysql_redis / 3d083b2 mssql_mnesia_25 / odbc_mssql_mnesia / 3d083b2 riak_mnesia_24 / riak_mnesia / 3d083b2 |
-spec filter_local_packet(drop, _, _) -> {ok, drop}; | ||
(routing_data(), _, _) -> {ok, routing_data()}. | ||
filter_local_packet(drop, _, _) -> | ||
{ok, drop}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, it's fine to leave it like this, but please collect the list of all the hook handlers that use the trick like this (special Acc value to stop processing). gen_hook allows handler to stop hook processing in a nicer way. We should get rid of this practice once the full conversion is done.
user_present(Acc, #jid{} = UserJID) -> | ||
end, | ||
{ok, ResultAcc}; | ||
user_send_packet(Acc, _, _) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function clause is unreachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do a little refactor. Please check if it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes required, see the review comments.
a1098ae
to
054ecf2
Compare
small_tests_24 / small_tests / a1098ae small_tests_25 / small_tests / a1098ae ldap_mnesia_24 / ldap_mnesia / a1098ae dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a1098ae dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / a1098ae ldap_mnesia_25 / ldap_mnesia / a1098ae dynamic_domains_mysql_redis_25 / mysql_redis / a1098ae internal_mnesia_25 / internal_mnesia / a1098ae dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a1098ae elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a1098ae pgsql_mnesia_24 / pgsql_mnesia / a1098ae pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_unsubscribe_after_presence_unsubscription_2485@localhost">>},
{<<"to">>,
<<"bob_unsubscribe_after_presence_unsubscription_2485@localhost/res1">>},
{<<"type">>,<<"headline">>}],
[{xmlel,<<"event">>,
[{<<"xmlns">>,
<<"http://jabber.org/protocol/pubsub#event">>}],
[{xmlel,<<"items">>,
[{<<"node">>,<<"mdT11Wklx5iFP4ygkpUBdw==">>}],
[{xmlel,<<"item">>,
[{<<"id">>,<<"salmon">>}],
[{xmlel,<<"entry">>,
[{<<"xmlns">>,
<<"http://www.w3.org/2005/Atom">>}],
[]}]}]}]},
{xmlel,<<"headers">>,
[{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
[]}]}]},
[{pep_SUITE,'-unsubscribe_after_presence_unsubscription/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
{line,384}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_unsubscribe_after_presence_unsubscription_2498@localhost">>},
{<<"to">>,
<<"bob_unsubscribe_after_presence_unsubscription_2498@localhost/res1">>},
{<<"type">>,<<"headline">>}],
[{xmlel,<<"event">>,
[{<<"xmlns">>,
<<"http://jabber.org/protocol/pubsub#event">>}],
[{xmlel,<<"items">>,
[{<<"node">>,<<"UD3+MCvC/GceToojUfa5MA==">>}],
[{xmlel,<<"item">>,
[{<<"id">>,<<"salmon">>}],
[{xmlel,<<"entry">>,
[{<<"xmlns">>,
<<"http://www.w3.org/2005/Atom">>}],
[]}]}]}]},
{xmlel,<<"headers">>,
[{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
[]}]}]},
[{pep_SUITE,'-unsubscribe_after_presence_unsubscription/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
{line,384}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} pgsql_mnesia_25 / pgsql_mnesia / a1098ae mysql_redis_25 / mysql_redis / a1098ae riak_mnesia_24 / riak_mnesia / a1098ae mssql_mnesia_25 / odbc_mssql_mnesia / a1098ae |
small_tests_24 / small_tests / 054ecf2 small_tests_25 / small_tests / 054ecf2 ldap_mnesia_24 / ldap_mnesia / 054ecf2 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 054ecf2 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 054ecf2 ldap_mnesia_25 / ldap_mnesia / 054ecf2 dynamic_domains_mysql_redis_25 / mysql_redis / 054ecf2 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 054ecf2 internal_mnesia_25 / internal_mnesia / 054ecf2 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 054ecf2 pgsql_mnesia_24 / pgsql_mnesia / 054ecf2 pgsql_mnesia_25 / pgsql_mnesia / 054ecf2 mysql_redis_25 / mysql_redis / 054ecf2 mssql_mnesia_25 / odbc_mssql_mnesia / 054ecf2 riak_mnesia_24 / riak_mnesia / 054ecf2 |
user_send_packet(Acc, _, _) -> | ||
Packet = mongoose_acc:packet(Acc), | ||
ChatType = chat_type(Acc), | ||
ResultAcc = if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for me it's fine, but don't be surprised if other devs ask you to replace if
construction with case
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
This PR changes all hook handlers in
mod_event_pusher_hook_translator
module togen_hook
format.