Add host_type to hook calls in core modules #3097
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 #3097 +/- ##
==========================================
+ Coverage 79.06% 79.12% +0.06%
==========================================
Files 386 386
Lines 31830 31820 -10
==========================================
+ Hits 25165 25177 +12
+ Misses 6665 6643 -22
Continue to review full report at Codecov.
|
chrzaszcz
force-pushed
the
multi-tenancy-core-hooks
branch
5 times, most recently
from
7c57d27 to
438415b
Compare
chrzaszcz
force-pushed
the
multi-tenancy-core-hooks
branch
from
33f3c69 to
9cfb521
Compare
- For binary: run as before
- For 'undefined': log an error and return Acc (no-op)
This might happen if host type is taken from the Acc.
We need to eradicate this case from the code later.
They have been deprecated for a long time and they conflict with new functions that will be introduced for host_type.
This required adding it to open_session, what in turn required adding it to mongoose_client_api.
Take it from Acc as the hook is always run for a local user.
Reason: The stanza is handled locally and host type
can be used when running local hooks.
Anyway, this clause is used only for sending MAM messages.
It is safe to take it from the acc created for the local user.
The Acc is constructed in ejabberd_c2s from the local state, which contains the host type of the local user.
Hooks changed: - roster_in_subscription - sm_filter_offline_message - privacy_get_user_list - offline_groupchat_message_hook
Hooks changed: - offline_message_hook - failed_to_store_message
This change is in a separate commit for clarity. The host_type argument is removed as it is always calculated from the server part of the JID.
Motivation: - Simplicity. - They were unused - no handlers in the code base. - One was already global, now they are consistent. - Host type can be introduced anytime when a need arises.
chrzaszcz
force-pushed
the
multi-tenancy-core-hooks
branch
from
06ab31c to
f8a8d47
Compare
Also:
- Avoid duplicate calculation of host_type in ejabberd_auth:remove_user/1
- Provide host_type in sm_register_connection_hook
to pass it in a call to register_user from ejabberd_auth_anonymous
chrzaszcz
force-pushed
the
multi-tenancy-core-hooks
branch
from
08d6a6c to
b4b6fcf
Compare
chrzaszcz
force-pushed
the
multi-tenancy-core-hooks
branch
from
b4b6fcf to
c376aed
Compare
arcusfelis
approved these changes
Apr 29, 2021
| case lists:any(fun(El) -> El == ok end, RemoveResult) of | ||
| true -> | ||
| Acc = mongoose_acc:new(#{ location => ?LOCATION, | ||
| host_type => HostType, | ||
| lserver => LServer, | ||
| element => undefined }), | ||
| mongoose_hooks:remove_user(LServer, Acc, LUser), |
There was a problem hiding this comment.
maybe it could be remove_user(Acc, LServer, LUser). Though backward compatibility would suffer.
There was a problem hiding this comment.
I'd rather rearrange the hook arguments in a separate PR as these changes are quite tricky.
As I said in the PR description: Do not rearrange the arguments when not necessary.
| @@ -153,13 +153,14 @@ remove_connection(SID, LUser, LServer) -> | |||
|
|
|||
| %% @doc Register connection | |||
| -spec register_connection(Acc, | |||
| HostType :: binary(), | |||
There was a problem hiding this comment.
We have this type defined in mongooseim:host_type() since yesterday
There was a problem hiding this comment.
I've seen it. I don't think it makes sense to amend this PR though. I'd rather do it later.
| @@ -1098,7 +1098,7 @@ get_roster_old(DestServer, JID) -> | |||
| A = mongoose_acc:new(#{ location => ?LOCATION, | |||
| lserver => DestServer, | |||
| element => undefined }), | |||
| A2 = mongoose_hooks:roster_get(DestServer, A, JID), | |||
| A2 = mongoose_hooks:roster_get(A, JID), | |||
There was a problem hiding this comment.
Could be Acc instead of A, to be consistent.
There was a problem hiding this comment.
You're right, I just wanted to minimize changes in mod_* for now.
DenysGonchar
approved these changes
Apr 29, 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 all hook calls not related to any extension modules work with host types.
The following rules were applied:
Accis passed, make sure itshost_typeis consistent with theLServerargument that was passed before and get the host type form the Acc insidemongoose_hooks.LServerargument if possible.More details in commit messages.