Prepare mod_disco for dynamic domains #3128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrzaszcz
changed the title
Codecov Report
@@ Coverage Diff @@
## master #3128 +/- ##
==========================================
+ Coverage 79.47% 79.52% +0.05%
==========================================
Files 396 397 +1
Lines 32419 32325 -94
==========================================
- Hits 25764 25708 -56
+ Misses 6655 6617 -38
Continue to review full report at Codecov.
|
chrzaszcz
force-pushed
the
stateless-disco
branch
13 times, most recently
from
80160eb to
1bb13e5
Compare
It is added in a separate module to avoid calling mod_disco from other extension modules (module decoupling).
Also: add missing 'drop' condition to the AMP features.
They are already registered by ejabberd_local with the IQ handler.
It was already registered with the IQ handler. There was a double bug as well that caused the tests to pass: - Wrong group name caused extdisco NOT to start - Wrong IQ handler module caused extdisco NOT to stop correctly
It was already registered with the IQ handler.
It was already registered with the IQ handler.
It will be used by modules that need to get registered features and are not returning IQ errors. Error handling can be changed in the future if needed.
Rework feature discovery to use the hooks for disco. Additional features are added in place, as adding hooks brings no benefits in custom handlers. Also: - Use the disco hook in mod_muc (required for mod_mam_muc features) - Move 'features_to_xml' to a helper module Note: This commit breaks MUC Light tests because of the change in mod_mam_muc.
This commit fixes big tests.
Instead: register only the necessary IQ handler directly This makes the module responsible for its IQ handlers, as it should be.
Use the hooks instead. New ETS table is used for efficient lookup of the NS list.
They are unused, hooks are used instead.
- Remove unnecessary error handling
(no errors are returned form handlers anymore)
- Remove unnecessary support for feature tuples
(no tuples are returned from handlers anymore)
- Add type specs
Update the interface and the helperes in mongoose_disco. A map is used to guarantee unique JID's in the result. This commit breaks tests as the modules need to be updated.
Also: - Fix a bug in get_module_opt - Fix the tests to detect the bug
Also: - use add_features as well - add missing disco tests - remove nested hook with only one handler %adhoc fix
Change the test to check that the name is included.
This commit fixes the tests as all modules are reworked now.
chrzaszcz
force-pushed
the
stateless-disco
branch
from
1bb13e5 to
612fa3d
Compare
DenysGonchar
approved these changes
Jun 1, 2021
arcusfelis
reviewed
Jun 1, 2021
| @@ -688,7 +695,8 @@ config_metrics(HostType) -> | |||
|
|
|||
| -spec hooks(jid:lserver()) -> [ejabberd_hooks:hook()]. | |||
| hooks(HostType) -> | |||
| [{user_send_packet, HostType, ?MODULE, user_send_packet, 60}, | |||
| [{disco_local_features, HostType, ?MODULE, add_local_features, 99}, | |||
There was a problem hiding this comment.
maybe {disco_local_features, HostType, ?MODULE, disco_local_features, 99}
There was a problem hiding this comment.
This name would be consistent with the hook, but on the other hand add_local_features is more descriptive as the features are added to the list... Anyway, I can make the handler names more consistent in part 2 (there is more of them and they are mostly called add_local_features).
|
still LGTM |
arcusfelis
approved these changes
Jun 1, 2021
NelsonVides
approved these changes
Jun 1, 2021
| gen_iq_handler:add_iq_handler(ejabberd_local, Host, | ||
| ?NS_EXTDISCO, ?MODULE, process_iq, IQDisc). | ||
|
|
||
| -spec stop(jid:server()) -> ok. | ||
| stop(Host) -> | ||
| mod_disco:unregister_feature(Host, ?NS_EXTDISCO), | ||
| gen_iq_handler:remove_iq_handler(ejabberd_sm, Host, ?NS_EXTDISCO). | ||
| gen_iq_handler:remove_iq_handler(ejabberd_local, Host, ?NS_EXTDISCO). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Untangle the service discovery logic according to the following rules:
mod_disco. In general, a module shouldn't call other extension modules unless they are connected withdeps.register_feature.mod_discoshouldn't keep its state in ETS tables. There is no need for state when the hooks are used.Common logic for handling the hooks is extracted to
mongoose_disco. It has helpers fordisco_local_featuresanddisco_local_itemsright now.A lot of new tests for service discovery are added as well.
To do:
disco_local_identityanddisco_sm_*consistent with the functions already changed.mod_discoandmongoose_disco.Both changes can be done in one PR.