Route accumulator #3240
Route accumulator #3240
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3240 +/- ##
=======================================
Coverage 80.33% 80.34%
=======================================
Files 398 398
Lines 32540 32548 +8
=======================================
+ Hits 26142 26150 +8
Misses 6398 6398
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NelsonVides
force-pushed
the
route_accumulator
branch
from
878f465
to
af49b60
Compare
NelsonVides
force-pushed
the
route_accumulator
branch
from
af49b60
to
adb2522
Compare
|
small_tests_24 / small_tests / adb2522 internal_mnesia_24 / internal_mnesia / adb2522 dynamic_domains_24 / pgsql_mnesia / adb2522 small_tests_22 / small_tests / adb2522 dynamic_domains_23 / pgsql_mnesia / adb2522 small_tests_23 / small_tests / adb2522 ldap_mnesia_23 / ldap_mnesia / adb2522 ldap_mnesia_22 / ldap_mnesia / adb2522 ldap_mnesia_24 / ldap_mnesia / adb2522 pgsql_mnesia_24 / pgsql_mnesia / adb2522 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / adb2522 pgsql_mnesia_22 / pgsql_mnesia / adb2522 pgsql_mnesia_23 / pgsql_mnesia / adb2522 mysql_redis_24 / mysql_redis / adb2522 mssql_mnesia_24 / odbc_mssql_mnesia / adb2522 riak_mnesia_24 / riak_mnesia / adb2522 |
There was a problem hiding this comment.
The code looks good, I only see one possible issue: if a host type and server is set in Acc, the call to ejabberd_router:route(From, To, Acc, Packet) will result (in case of a locally routed packet) in an Acc that has a different host type and server (the ones for the recipient). As a result, further processing of such Acc might e.g. trigger hooks for a wrong host type.
To make sure it's not an issue, one would have to check all calls to ejabberd_router:route/4 for which the result is not ignored and ensure that the resulting Acc is never used to run hooks for a host type etc.
| @@ -41,6 +46,6 @@ do_route(OrigFrom, OrigTo, OrigAcc, OrigPacket, Handler) -> | |||
| drop -> | |||
| mongoose_hooks:xmpp_stanza_dropped(Acc0, OrigFrom, | |||
| OrigTo, OrigPacket), | |||
| ok | |||
| Acc0 | |||
There was a problem hiding this comment.
Nice! It was against the spec.
|
small_tests_24 / small_tests / 79dcea6 internal_mnesia_24 / internal_mnesia / 79dcea6 dynamic_domains_24 / pgsql_mnesia / 79dcea6 small_tests_22 / small_tests / 79dcea6 dynamic_domains_23 / pgsql_mnesia / 79dcea6 small_tests_23 / small_tests / 79dcea6 ldap_mnesia_24 / ldap_mnesia / 79dcea6 ldap_mnesia_22 / ldap_mnesia / 79dcea6 ldap_mnesia_23 / ldap_mnesia / 79dcea6 pgsql_mnesia_24 / pgsql_mnesia / 79dcea6 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 79dcea6 jingle_SUITE:all:jingle_session_is_established_with_a_conference_room{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"*[email protected]">>},
{<<"to">>,
<<"alice_jingle_session_is_established_with_a_conference_room_56.543391@localhost/res1">>},
{<<"id">>,<<"7ad62f5a-ca6b-43e9-ac87-22c1c9e0a287">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-info">>},
{<<"sid">>,<<"7c5f1353-426a-449c-8433-52dcf31582d6">>}],
[{xmlel,<<"ringing">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:apps:rtp:info:1">>}],
[]}]}]},
"<iq from='*[email protected]' to='alice_jingle_session_is_established_with_a_conference_room_56.543391@localhost/res1' id='7ad62f5a-ca6b-43e9-ac87-22c1c9e0a287' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-info' sid='7c5f1353-426a-449c-8433-52dcf31582d6'><ringing xmlns='urn:xmpp:jingle:apps:rtp:info:1'/></jingle></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{jingle_SUITE,send_initiate_and_wait_for_first_iq_set,2,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{line,393}]},
{jingle_SUITE,
'-jingle_session_is_established_with_a_conference_room/1-fun-0-',1,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{line,1...pgsql_mnesia_22 / pgsql_mnesia / 79dcea6 mysql_redis_24 / mysql_redis / 79dcea6 pgsql_mnesia_23 / pgsql_mnesia / 79dcea6 mssql_mnesia_24 / odbc_mssql_mnesia / 79dcea6 mod_event_pusher_rabbit_SUITE:group_chat_message_publish:group_chat_message_received_event_properly_formatted{error,
{{assertMatch,
[{module,mod_event_pusher_rabbit_SUITE},
{line,435},
{expression,
"get_decoded_message_from_rabbit ( AliceGroupChatMsgRecvRK )"},
{pattern,
"# { << \"from_user_id\" >> := BobRoomJID , << \"to_user_id\" >> := AliceFullJID , << \"message\" >> := Message }"},
{value,
#{<<"from_user_id">> => <<"[email protected]">>,
<<"message">> => <<>>,
<<"to_user_id">> =>
<<"alice_unnamed_73.417057@localhost/res1">>}}]},
[{mod_event_pusher_rabbit_SUITE,
'-group_chat_message_received_event_properly_formatted/1-fun-1-',3,
[{file,
"/home/circleci/app/big_tests/tests/mod_event_pusher_rabbit_SUITE.erl"},
{line,435}]},
{escalus_story,story,4,
[{file,
"/home/circleci/app/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}]}]}}riak_mnesia_24 / riak_mnesia / 79dcea6 |
I'd like all routing to pass the accumulator along, with whatever modifications are needed like stripping it or updating the routed element. This will help with tracing the routing mechanism from start to end, if at all times the accumulator is preserved. It will also for example give me at the caller site, information about the packet handler's handling, like what muclight or pubsub did with the package. Then it is up to the caller to inspect the accumulator.