Refactored disco_local_features hook to a gen_hook format #3758

pawlooss1 · 2022-09-13T11:03:22Z

This PR changes all handlers for disco_local_features hook to gen_hook format.

mongoose-im · 2022-09-13T11:11:33Z

small_tests_24 / small_tests / 3d55ec5
Reports root / small

small_tests_25 / small_tests / 3d55ec5
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 3d55ec5
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 3d55ec5
Reports root/ big
OK: 1948 / Failed: 0 / User-skipped: 551 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3d55ec5
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / 3d55ec5
Reports root/ big
OK: 1948 / Failed: 0 / User-skipped: 551 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / 3d55ec5
Reports root/ big
OK: 3566 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3d55ec5
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / 3d55ec5
Reports root/ big
OK: 2069 / Failed: 0 / User-skipped: 430 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 3d55ec5
Reports root/ big
OK: 3957 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 3d55ec5
Reports root/ big
OK: 2409 / Failed: 0 / User-skipped: 425 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / 3d55ec5
Reports root/ big
OK: 3957 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / 3d55ec5
Reports root/ big
OK: 3952 / Failed: 0 / User-skipped: 102 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 3d55ec5
Reports root/ big
OK: 2248 / Failed: 0 / User-skipped: 418 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / 3d55ec5
Reports root/ big
OK: 3968 / Failed: 1 / User-skipped: 97 / Auto-skipped: 0

pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription

{error,
  {{badmatch,
     [{xmlel,<<"message">>,
        [{<<"from">>,
        <<"alice_unsubscribe_after_presence_unsubscription_2388@localhost">>},
         {<<"to">>,
        <<"bob_unsubscribe_after_presence_unsubscription_2388@localhost/res1">>},
         {<<"type">>,<<"headline">>}],
        [{xmlel,<<"event">>,
           [{<<"xmlns">>,
           <<"http://jabber.org/protocol/pubsub#event">>}],
           [{xmlel,<<"items">>,
            [{<<"node">>,<<"PtsED89XiO+TZxl0nkreFg==">>}],
            [{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}]}]}}

Report log

codecov · 2022-09-13T11:12:10Z

Codecov Report

Base: 82.72% // Head: 82.72% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (ecd29ce) compared to base (7e19c4f).
Patch coverage: 94.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3758      +/-   ##
==========================================
- Coverage   82.72%   82.72%   -0.01%     
==========================================
  Files         529      529              
  Lines       33970    34006      +36     
==========================================
+ Hits        28102    28130      +28     
- Misses       5868     5876       +8

Impacted Files	Coverage Δ
src/ejabberd_local.erl	`77.04% <80.00%> (ø)`
src/inbox/mod_inbox.erl	`86.93% <85.71%> (+0.19%)`	⬆️
src/pubsub/mod_pubsub.erl	`73.04% <85.71%> (-0.14%)`	⬇️
src/mam/mod_mam_pm.erl	`89.64% <87.50%> (+0.12%)`	⬆️
src/mod_auth_token.erl	`83.91% <87.50%> (+0.34%)`	⬆️
src/mod_adhoc.erl	`81.66% <100.00%> (+0.96%)`	⬆️
src/mod_amp.erl	`95.45% <100.00%> (+0.21%)`	⬆️
src/mod_blocking.erl	`88.88% <100.00%> (+0.42%)`	⬆️
src/mod_caps.erl	`75.79% <100.00%> (+0.45%)`	⬆️
src/mod_carboncopy.erl	`88.65% <100.00%> (+0.33%)`	⬆️
... and 17 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

NelsonVides

I like the renaming of all hooks to legacy_hooks, make it clear who's leaving here 😛
Only one comment on xref and then this is ready, great job! 👌🏽

src/ejabberd_local.erl

DenysGonchar · 2022-09-13T17:27:00Z

Thanks for the PR, it looks fine.

I have one note for the future changes, taking into account that some of the hooks are quite generic and used in a lot of modules, you may want to have simultaneous support of the new gen_hook and the legacy ejabberd_hooks interfaces for a short period of time.

e.g. the disco_local_features hook execution function could look like this:

-spec disco_local_features(mongoose_disco:feature_acc()) -> mongoose_disco:feature_acc().
disco_local_features(Acc = #{host_type := HostType}) ->
    Parameters = #{},
    LegacyArgs = [Node],
    ParametersWithLegacyArgs = ejabberd_hooks:add_args(Params, LegacyArgs),
    run_hook_for_host_type(disco_local_features, HostType, Acc, ParametersWithLegacyArgs).

And it would work for both types (new and legacy) of interfaces, And it would work for both types (new and legacy) of interfaces, allowing changing the corresponding modules in the multiple PRs. And so, organize rework on a per-module basis rather than per-hook.

Once all the modules are converted, we can make a finalizing PR which removes ejabberd_hooks module and ejabberd_hooks:add_args/2 calls from all the hook execution wrappers in the mongoose_hooks modules.

mongoose-im · 2022-09-14T09:31:07Z

riak_mnesia_24 / riak_mnesia / ecd29ce
Reports root

small_tests_24 / small_tests / ecd29ce
Reports root / small

small_tests_25 / small_tests / ecd29ce
Reports root / small

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / ecd29ce
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / ecd29ce
Reports root/ big
OK: 1948 / Failed: 0 / User-skipped: 551 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / ecd29ce
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

dynamic_domains_mysql_redis_25 / mysql_redis / ecd29ce
Reports root/ big
OK: 3566 / Failed: 0 / User-skipped: 105 / Auto-skipped: 0

ldap_mnesia_25 / ldap_mnesia / ecd29ce
Reports root/ big
OK: 1948 / Failed: 0 / User-skipped: 551 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / ecd29ce
Reports root/ big
OK: 3583 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / ecd29ce
Reports root/ big
OK: 3957 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

internal_mnesia_25 / internal_mnesia / ecd29ce
Reports root/ big
OK: 2069 / Failed: 0 / User-skipped: 430 / Auto-skipped: 0

elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / ecd29ce
Reports root/ big
OK: 2409 / Failed: 0 / User-skipped: 425 / Auto-skipped: 0

pgsql_mnesia_25 / pgsql_mnesia / ecd29ce
Reports root/ big
OK: 3957 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

mysql_redis_25 / mysql_redis / ecd29ce
Reports root/ big
OK: 3952 / Failed: 0 / User-skipped: 102 / Auto-skipped: 0

mssql_mnesia_25 / odbc_mssql_mnesia / ecd29ce
Reports root/ big
OK: 3957 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / ecd29ce
Reports root/ big
OK: 2247 / Failed: 1 / User-skipped: 418 / Auto-skipped: 0

sm_SUITE:parallel_unacknowledged_message_hook:unacknowledged_message_hook_bounce

{error,
  {{assertion_failed,assert,is_chat_message,
     [<<"msg-1">>],
     {xmlel,<<"message">>,
       [{<<"from">>,
         <<"bob_unacknowledged_message_hook_bounce_2004@localhost/escalus-default-resource">>},
        {<<"to">>,
         <<"alice_unacknowledged_message_hook_bounce_2010@localhost">>},
        {<<"xml:lang">>,<<"en">>},
        {<<"type">>,<<"chat">>}],
       [{xmlel,<<"body">>,[],[{xmlcdata,<<"msg-2">>}]},
        {xmlel,<<"delay">>,
          [{<<"xmlns">>,<<"urn:xmpp:delay">>},
           {<<"stamp">>,<<"2022-09-15T08:35:50.634008Z">>},
           {<<"from">>,<<"localhost">>}],
          [{xmlcdata,<<"SM Storage">>}]}]},
     "<message from='bob_unacknowledged_message_hook_bounce_2004@localhost/escalus-default-resource' to='alice_unacknowledged_message_hook_bounce_2010@localhost' xml:lang='en' type='chat'><body>msg-2</body><delay xmlns='urn:xmpp:delay' stamp='2022-09-15T08:35:50.634008Z' from='localhost'>SM Storage</delay></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_SUITE,unacknowledged_message_hook_common,2,
      [{file,"/home/circleci/project/big_tests/tests/sm_SUITE.erl"},
       {line,857}]},
    {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,"tes...

Report log

riak_mnesia_24 / riak_mnesia / ecd29ce
Reports root/ big
OK: 2248 / Failed: 0 / User-skipped: 418 / Auto-skipped: 0

DenysGonchar

Looks fine to me

NelsonVides

LGTM 👌🏽

chrzaszcz

Looks good 👍

Refactored disco_local_features hook to a gen_hook format

3d55ec5

pawlooss1 requested review from DenysGonchar and NelsonVides September 13, 2022 11:03

NelsonVides reviewed Sep 13, 2022

View reviewed changes

src/ejabberd_local.erl Outdated Show resolved Hide resolved

Removed disco_local_features/3 from ignore_xref

ecd29ce

pawlooss1 requested a review from NelsonVides September 14, 2022 09:28

pawlooss1 self-assigned this Sep 14, 2022

DenysGonchar approved these changes Sep 15, 2022

View reviewed changes

DenysGonchar requested a review from chrzaszcz September 15, 2022 08:15

mongoose-im mentioned this pull request Sep 15, 2022

CI build failures summary - part 2 #3685

Closed

NelsonVides approved these changes Sep 15, 2022

View reviewed changes

chrzaszcz approved these changes Sep 15, 2022

View reviewed changes

chrzaszcz merged commit 0cd1de7 into master Sep 16, 2022

chrzaszcz deleted the hooks/disco_local_features branch September 16, 2022 07:41

chrzaszcz added this to the 6.0.0 milestone Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored disco_local_features hook to a gen_hook format #3758

Refactored disco_local_features hook to a gen_hook format #3758

pawlooss1 commented Sep 13, 2022

mongoose-im commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

NelsonVides left a comment

DenysGonchar commented Sep 13, 2022

mongoose-im commented Sep 14, 2022 •

edited

Loading

DenysGonchar left a comment

NelsonVides left a comment

chrzaszcz left a comment

Refactored disco_local_features hook to a gen_hook format #3758

Refactored disco_local_features hook to a gen_hook format #3758

Conversation

pawlooss1 commented Sep 13, 2022

mongoose-im commented Sep 13, 2022 • edited Loading

codecov bot commented Sep 13, 2022 • edited Loading

Codecov Report

NelsonVides left a comment

Choose a reason for hiding this comment

DenysGonchar commented Sep 13, 2022

mongoose-im commented Sep 14, 2022 • edited Loading

DenysGonchar left a comment

Choose a reason for hiding this comment

NelsonVides left a comment

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

mongoose-im commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

mongoose-im commented Sep 14, 2022 •

edited

Loading