-
Notifications
You must be signed in to change notification settings - Fork 429
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
Use more granular c2s hooks #3955
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportBase: 83.52% // Head: 83.47% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3955 +/- ##
==========================================
- Coverage 83.52% 83.47% -0.05%
==========================================
Files 538 538
Lines 33953 33957 +4
==========================================
- Hits 28358 28347 -11
- Misses 5595 5610 +15
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. |
b2907f6
to
c851084
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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.
The two modules still subscribed to user_send_packet
:
- MAM: don't we want to move it to
user_send_message
as well? - event pusher: it is probably right, but, have we verified that down the line the event pusher only cares about messages? I'd be happy with a pointer to the line of code were we can know that :)
@@ -22,7 +22,7 @@ | |||
|
|||
-export([add_hooks/1, delete_hooks/1]). | |||
|
|||
-export([user_send_packet/3, | |||
-export([user_send_message/3, |
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'm not 100% sure if really want to change the hook type here.
What about other places, e.g. mam, inbox, mod_offline (actually all the places that use user_send_packet hook)? Even if we don't need to change anything there, that would be nice to see some analysis and conclusions. |
|
|
ping, stream management, and metrics, are the ones that actually make sense they should capture all packets. About the event_pusher, I also understood it cares only for messages: in the hook handler, it checks for
About MAM, this one's weird, because as far as I understand https://xmpp.org/extensions/xep-0313.html it seems to me to require only messages, so I don't know why MAM would be concerned about the other types of messages. If a test fails I find that suspicious 🙄 MAM is also used for fetching messages, but the fetch happens in the IQ handler, here the hook is only capturing the messages that will be stored 🤔 So I'd only ask, do we want to investigate and fix that MAM failure in this PR or in some other case? If leaving that for later I'd approve this one for now 👌🏽 |
c851084
to
3f042fc
Compare
small_tests_24 / small_tests / 3f042fc small_tests_25 / small_tests / 3f042fc ldap_mnesia_24 / ldap_mnesia / 3f042fc dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 3f042fc ldap_mnesia_25 / ldap_mnesia / 3f042fc dynamic_domains_mysql_redis_25 / mysql_redis / 3f042fc dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3f042fc pgsql_mnesia_24 / pgsql_mnesia / 3f042fc dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3f042fc muc_SUITE:register:user_unregisters_nick_twice{error,
{{assertion_failed,assert,is_iq_result,
[{xmlel,<<"iq">>,
[{<<"type">>,<<"set">>},
{<<"id">>,<<"df1b582a7e938deb17cfe739d2eee65d">>},
{<<"to">>,<<"groupchats.domain.example.com">>}],
[{xmlel,<<"query">>,
[{<<"xmlns">>,<<"jabber:iq:register">>}],
[{xmlel,<<"x">>,
[{<<"xmlns">>,<<"jabber:x:data">>},
{<<"type">>,<<"submit">>}],
[{xmlel,<<"field">>,
[{<<"type">>,<<"hidden">>},
{<<"var">>,<<"FORM_TYPE">>}],
[{xmlel,<<"value">>,[],
[{xmlcdata,<<"jabber:iq:register">>}]}]},
{xmlel,<<"field">>,
[{<<"type">>,<<"text-single">>},
{<<"var">>,<<"nick">>}],
[{xmlel,<<"value">>,[],
[{xmlcdata,
<<"thirdwitch1room-9daa74711a">>}]}]}]}]}]}],
{xmlel,<<"iq">>,
[{<<"from">>,<<"groupchats.domain.example.com">>},
{<<"to">>,
<<"alice_user_unregisters_nick_twice_2437@domain.example.com/res1">>},
{<<"type">>,<<"error">>},
{<<"id">>,<<"df1b582a7e938deb17cfe739d2eee65d">>}],
[{xmlel,<<"query">>,
[{<<"xmlns">>,<<"jabber:iq:register">>}],
[{xmlel,<<"x">>,
[{<<"xmlns">>,<<"jabber:x:data">>},
{<<"type">>,<<"submit">>}],
[{xmlel,<<"field">>,
[{<<"type">>,<<"hidden">>},
{<<"var">>,<<"FORM_TYPE">>}],
... elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 3f042fc internal_mnesia_25 / internal_mnesia / 3f042fc graphql_session_SUITE:admin_session:admin_session_cli:admin_list_users_with_status{error,
{{assertEqual,
[{module,graphql_session_SUITE},
{line,591},
{expression,"lists : sort ( ActualJIDs )"},
{expected,
[<<"alice_admin_list_users_with_status_912@localhost.bis/res1">>]},
{value,[]}]},
[{graphql_session_SUITE,check_users,2,
[{file,
"/home/circleci/project/big_tests/tests/graphql_session_SUITE.erl"},
{line,591}]},
{graphql_session_SUITE,admin_list_users_with_status_story,3,
[{file,
"/home/circleci/project/big_tests/tests/graphql_session_SUITE.erl"},
{line,422}]},
{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}]}]}} riak_mnesia_24 / riak_mnesia / 3f042fc pgsql_mnesia_25 / pgsql_mnesia / 3f042fc mssql_mnesia_25 / odbc_mssql_mnesia / 3f042fc mysql_redis_25 / mysql_redis / 3f042fc push_integration_SUITE:pubsub_less:pm_notifications_with_inbox:inbox_msg_reset_unread_count_fcm{error,
{{assertion_failed,assert_many,false,
[is_presence,is_message,is_message],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_inbox_msg_reset_unread_count_fcm_2794@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2794@localhost">>},
{<<"type">>,<<"chat">>},
{<<"id">>,<<"5a4e63a7f338821b1875c9a0c0293e47">>}],
[{xmlel,<<"body">>,[],[{xmlcdata,<<"FIRST MESSAGE">>}]},
{xmlel,<<"delay">>,
[{<<"xmlns">>,<<"urn:xmpp:delay">>},
{<<"stamp">>,<<"2023-02-17T14:37:05.201184Z">>},
{<<"from">>,<<"localhost">>}],
[{xmlcdata,<<"Offline Storage">>}]}]},
{xmlel,<<"presence">>,
[{<<"from">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2794@localhost/res1">>},
{<<"to">>,
<<"bob_inbox_msg_reset_unread_count_fcm_2794@localhost/res1">>}],
[]}],
" <message from='alice_inbox_msg_reset_unread_count_fcm_2794@localhost/res1' to='bob_inbox_msg_reset_unread_count_fcm_2794@localhost' type='chat' id='5a4e63a7f338821b1875c9a0c0293e47'><body>FIRST MESSAGE</body><delay xmlns='urn:xmpp:delay' stamp='2023-02-17T14:37:05.201184Z' from='localhost'>Offline Storage</delay></message> <presence from='bob_inbox_msg_reset_unread_count_fcm_2794@localhost/res1' to='bob_inbox_msg_reset_unread_count_fcm_2794@localhost/res1'/>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/project/big_tests/_buil... dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3f042fc internal_mnesia_25 / internal_mnesia / 3f042fc mysql_redis_25 / mysql_redis / 3f042fc |
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.
LGTM 👌🏽
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 ok
This PR changes the hooks used in the
mod_event_pusher
andmod_mam
to more granular ones.