Dynamic domains support in mod_disco #3151
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
Codecov Report
@@ Coverage Diff @@
## master #3151 +/- ##
==========================================
- Coverage 79.83% 79.80% -0.03%
==========================================
Files 396 396
Lines 32305 32340 +35
==========================================
+ Hits 25790 25809 +19
- Misses 6515 6531 +16
Continue to review full report at Codecov.
|
chrzaszcz
force-pushed
the
disco-with-dynamic-domains
branch
5 times, most recently
from
ceb9b75
to
9276d08
Compare
Use exported functions for easier debugging.
- Argument list got long, converted item_acc to a map
- These hooks are executed only for domains,
so there is no need to check if the IQ was addressed to a subdomain
- Unify handler names to match hook hames
Features are already added by ejabberd_local because of the iq handler.
Features are already added by ejabberd_local because of the iq handler.
- Rework feature_acc like item_acc
- Share common parts of item_acc and feature_acc (types and logic)
- Run the hooks for host types
NOTE: This commit breaks tests as all modules using disco features
need to be changed.
Run the hooks for host types.
Motivation: instead of overloading one hook (disco_local_features) and running it both for the local domain and for the MUC domain, the logic can be split. This eliminates the need to pattern-match on domains for all handlers. The code is simpler and it is harder to make a mistake, e.g. by forgetting to pattern-match. This way MUC features would never leak to local features and vice versa. NOTE: This commit fixes the tests, they should pass from now on.
Make identity hooks return the acc like the feature and item hooks. Update the handlers in modules.
Make the hook handler return the info as a map (like for other disco hooks). Add a helper for calling the hook and converting the results to XML. Also: - use the helpers from mongoose_disco in mod_muc and mod_muc_room - do not convert server info from binaries to strings and back
This way the accumulator is used only inside the handlers and XML is returned to the caller, which is more convenient. No module other than mongoose_disco should call disco hooks. Also: unify acc handling for all disco calls. NOTE: This change breaks tests as modules need to be updated next.
Before: the handlers were running the hooks for an empty node and discarded previous results. Now: the handlers are running first and they just set the node to empty and remaining hanflers are using that node. There is no need to nest hook calls in handlers anymore.
Also: - reorganize the handlers to follow the order from mongoose_disco - do not preprocess the config, this extra step was unnecessary
chrzaszcz
force-pushed
the
disco-with-dynamic-domains
branch
from
7360d09
to
80ab8b7
Compare
NOTE: This commit fixes the tests, are modules are consistent now.
The old code is still used for external components. It is not a performance issue as there is no entry per domain. The final solution would be to make external components use the domain API to regisyter components of type 'subdomain'. This should be done in a separate PR.
chrzaszcz
force-pushed
the
disco-with-dynamic-domains
branch
from
80ab8b7
to
ec0caff
Compare
Regarding mod_mam backends, the configuration for some options is read per domain, not per host type. This is a bug and needs to be addressed in the near future.
arcusfelis
reviewed
Jun 15, 2021
| {result, Items} -> | ||
| case mongoose_disco:get_local_items(HostType, From, To, Node, Lang) of | ||
| empty -> | ||
| Error = mongoose_xmpp_errors:item_not_found(), |
There was a problem hiding this comment.
maybe adding a text like mongoose_xmpp_errors:item_not_found(<<"en">>, <<"Local disco items not found">>).
There was a problem hiding this comment.
I am not sure if we need en as we have Lang here... anyway, there are more such errors in mod_disco, let's address it in a separate PR.
| -spec process_sm_iq_items(mongoose_acc:t(), jid:jid(), jid:jid(), jlib:iq(), map()) -> | ||
| {mongoose_acc:t(), jlib:iq()}. | ||
| process_sm_iq_items(Acc, _From, _To, #iq{type = set, sub_el = SubEl} = IQ, _Extra) -> | ||
| {Acc, IQ#iq{type = error, sub_el = [SubEl, mongoose_xmpp_errors:not_allowed()]}}; |
There was a problem hiding this comment.
nice to have some text for the error.
| children = ItemsXML}]}} | ||
| end; | ||
| false -> | ||
| {Acc, IQ#iq{type = error, sub_el = [SubEl, mongoose_xmpp_errors:service_unavailable()]}} |
There was a problem hiding this comment.
here text for sure, 503 errors are evil :D
| children = IdentityXML ++ FeaturesXML}]}} | ||
| end; | ||
| false -> | ||
| {Acc, IQ#iq{type = error, sub_el = [SubEl, mongoose_xmpp_errors:service_unavailable()]}} |
arcusfelis
approved these changes
Jun 15, 2021
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.
Make
mod_discoand all disco hooks/handlers compatible with dynamic domains.The new
mongoose_discomodule is responsible for:This way all the code handling the discovered features is more consistent across the modules and easier to maintain.
All disco hooks are run for host types and for MUC (Light) features there is simply a new hook introduced. Disco hooks are now run for specific functional entity (e.g.
disco_local_*for the local server,disco_sm_*for a local client,disco_muc_*for the MUC service) rather than for specific XMPP (sub)domains.