Skip to content
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

Further mod_disco rework #3146

Merged
merged 10 commits into from
Jun 9, 2021
Merged

Further mod_disco rework #3146

merged 10 commits into from
Jun 9, 2021

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Jun 1, 2021

Further preparation and cleanup of disco-related code.

Some functionality was untested (and even broken). Tests added, bugs fixed. This will help assure that after introducing host types everything is still working.

Planned for the next PR: introduce host types to mod_disco and all the hooks to support dynamic domains.

Skipped for now: unit tests for mongoose_disco. It is a simple module covered by multiple functional tests.

Not sure: more pubsub tests. It's a deep rabbit hole, so I needed to stop at some point. A few lines I touched remain untested, just as before.

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #3146 (c325dd0) into master (85e657c) will increase coverage by 0.22%.
The diff coverage is 86.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
+ Coverage   79.51%   79.74%   +0.22%     
==========================================
  Files         397      397              
  Lines       32321    32305      -16     
==========================================
+ Hits        25701    25761      +60     
+ Misses       6620     6544      -76     
Impacted Files Coverage Δ
src/pubsub/mod_pubsub.erl 73.13% <67.85%> (+1.26%) ⬆️
src/mod_disco.erl 83.33% <89.47%> (+23.64%) ⬆️
src/http_upload/mod_http_upload.erl 94.38% <100.00%> (+3.79%) ⬆️
src/mod_adhoc.erl 79.24% <100.00%> (+30.06%) ⬆️
src/mongoose_disco.erl 100.00% <100.00%> (ø)
src/mongoose_hooks.erl 94.07% <100.00%> (+1.26%) ⬆️
src/offline/mod_offline.erl 77.77% <100.00%> (+0.30%) ⬆️
src/ejabberd_s2s_out.erl 58.99% <0.00%> (-1.83%) ⬇️
src/muc_light/mod_muc_light.erl 85.13% <0.00%> (-0.75%) ⬇️
...c/global_distrib/mod_global_distrib_server_mgr.erl 76.83% <0.00%> (-0.57%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85e657c...c325dd0. Read the comment docs.

Before:
  - mod_http_upload registered IQ handler, which called mod_disco
  - mod_disco called 3 hooks, which called handlers in mod_http_upload
  - these handlers returned identity, items and features
  - mod_disco constructed the response

Now:
  - mod_http_upload registers IQ handler, which returns the response
- Identities stored in a map
- No duplicate removal, just like before (not necessary now)
- Convert mod_disco
- Convert mod_adhoc
    - add tests
- Convert mod pubsub
    - rework local features as well (should be done already)
    - add tests for pubsub and pep identity
All modules using the hook are updated.

Other changes:
- mongoose_hooks: fix a bug (wrong hook name)
- mod_adhoc: test sm features, reorganize tests
- mod_offline: test sm features
- mod_pubsub: test sm features in pubsub (negative) and pep (positive)
All modules using the hook are updated.

Other changes:
- mod_adhoc: add tests
- pubsub: add PEP test, fix a bug discovered by the test
All modules using the hook are updated.

Other changes:
  - mod_adhoc: added test for the command NS
  - mod_pubsub: updated pep test to check the identity
@chrzaszcz chrzaszcz marked this pull request as ready for review June 2, 2021 13:39
Improve coverage of mod_disco:
- Cover SM discovery for online user resources
- Cover the case when a bad node name is specified
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some coomens

src/http_upload/mod_http_upload.erl Outdated Show resolved Hide resolved
src/http_upload/mod_http_upload.erl Outdated Show resolved Hide resolved
disco_iq_handler(_From, _To, Acc, #iq{type = set, sub_el = SubEl} = IQ) ->
{Acc, IQ#iq{type = error, sub_el = [SubEl, mongoose_xmpp_errors:not_allowed()]}};
disco_iq_handler(_From, To, Acc, #iq{type = get, lang = Lang, sub_el = SubEl} = IQ) ->
LServer = To#jid.lserver,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be matched in the function clause.

Copy link
Member Author

@chrzaszcz chrzaszcz Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather limit the clause head to be less than 100 characters.

src/http_upload/mod_http_upload.erl Show resolved Hide resolved
{<<"name">>, ItemId}]}
|| #pubsub_item{itemid = {ItemId, _}} <- Items]};
{result, [disco_item(Host, ItemId) ||
#pubsub_item{itemid = {ItemId, _}} <- Items]};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be nice to have [pubsub_to_disco_item(Host, Item) || Item Item <- Items]. So, we would start crashing, if itemid format changes.

Copy link
Member Author

@chrzaszcz chrzaszcz Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but I think I will limit the changes for now. That code might return items that are not matching the pattern and the filtering might be done on purpose - I don't want to dive deeper into pubsub code now, especially that there are virtually no tests or type specs.

Copy link
Collaborator

@DenysGonchar DenysGonchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

- Make IQ errors more descriptive
- Change IQ handler names
- Add tests for the IQ error cases
@DenysGonchar DenysGonchar merged commit 251305d into master Jun 9, 2021
@DenysGonchar DenysGonchar deleted the disco-rework branch June 9, 2021 07:48
@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants