rework of the gen_mod module, part #1 #3104
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3104 +/- ##
=======================================
Coverage 79.18% 79.19%
=======================================
Files 387 388 +1
Lines 31853 31850 -3
=======================================
- Hits 25224 25223 -1
+ Misses 6629 6627 -2
Continue to review full report at Codecov.
|
DenysGonchar
force-pushed
the
multi-tenancy-gen_mod
branch
3 times, most recently
from
1b85347
to
fbcd01e
Compare
some notes to the changes: * mod_event_pusher_push:maybe_enable_node/5: - first of all, previous implementation had problems with federation pub-sub servers. - also, having dynamic domains enabled, it might be too expensive to get all the domains. - and that dynamic domains are added in the lazy manner to the routing table * rework of the virtual_pubsub_hosts option processing/validation: - @hosts@ template was a bit confusing, people were asking about the difference to @host@ template all the time. - it is still possible to configure manually all the sub-hosts, if there's such a need. - now processing of the virtual_pubsub_hosts is done in the same way as other host templates - there is no need in storing separate normalized_virtual_pubsub_hosts option, so we can remove gen_mod:set_module_opt/4 call
DenysGonchar
force-pushed
the
multi-tenancy-gen_mod
branch
from
fbcd01e
to
a4a6480
Compare
DenysGonchar
force-pushed
the
multi-tenancy-gen_mod
branch
from
a4a6480
to
88c1b5a
Compare
DenysGonchar
changed the title
There was a problem hiding this comment.
I like the introduction of domain name patterns, good work!
One general comment: We need to keep the types consistent, they are being introduced ad-hoc now and I am not liking it very much as it's becoming more and more random. I'd rather introduce them once and fix the whole codebase in one PR. Using binary(), mongoose:host_type() and locally defined host_type() interchangeably makes the code less readable. I am afraid that domain_name() will become another randomly used type.
| @@ -16,7 +16,8 @@ | |||
| -module(mongooseim). | |||
|
|
|||
| -type host_type() :: binary(). | |||
| -export_type([host_type/0]). | |||
| -type domain_name() :: jid:lserver(). | |||
There was a problem hiding this comment.
While I like this type (as well as host_type), I think there will be many places in the code where we will spend extra time trying to decide whether to put jid:lserver() or domain_name() in a type spec. It might seem clear to you now, but I already tried to do this and I know there are many doubtful cases.
My opinion is that such types should be introduced in a separate PR and with a clear rule when to use them.
There was a problem hiding this comment.
it's hard to judge, when exactly we should introduce this type, I would say a soon as possible. I really need it for gen_mod.
There was a problem hiding this comment.
But why not jid:lserver()? The domain part of a JID is an XMPP domain name, why reinvent it? I see no real benefit here.
Anyway, it's not a blocker for sure, but it introduces yet another inconsistency to the code base. For me it's not a matter of when to introduce it, but how to do it in a consistent way.
There was a problem hiding this comment.
The main reason is that an old Host concept uses jid:lserver() and I want to make the wrong code look wrong. I don't want to guess if some code that uses jid:lserver() is already checked, and it's correct. Or no one did the check yet, and it must be performed. Ideally we should get rid of the host term in codebase at all, and it should be replaced with either host type or domain (or domain name). It significantly simplifies formal verification.
There was a problem hiding this comment.
The main reason is that an old Host concept uses
jid:lserver()and I want to make the wrong code look wrong.
jid:lserver() is still the best option in many places. How would you differentiate between something not changed yet and not needing to be changed at all? If your answer is "change jid:lserver to domain_name everywhere in the code", this doesn't make sense to me.
There was a problem hiding this comment.
why everywhere, functions that process stanzas/JIDs/etc. doesn't have to change anything. But every Host/jid:lserver() input parameter must be replaced with HostType/mongooseim:host_type() or Domain/mongooseim:domain_name() depending on the logic behind the code. It must be clear if that parameter is about configuration or routing entity.
There was a problem hiding this comment.
Let's say function a takes #jid{lserver = LServer} as an argument and passes LServer to function b.
You would have code like this:
a(#jid{lserver = LServer}) ->
b(LServer).
b(LServer) ->
do_something(...).
Now if you change the argument of function b to Domain with the type domain_name(), how would you address that in function a? Neithera(#jid{lserver = Domain}) nor calling b(LServer) look good to me.
The functions in this example were made-up, but there are many real cases like this in the code. If we agree on a rule which we both accept and understand, I would agree to proceed with the change.
The situation for host types is completely different and I agree with you on that one. We really need to cooperate when it comes to such changes, otherwise it becomes a mess.
| validate_subdomain_template(SubdomainTemplate) -> | ||
| Pattern = mongoose_subdomain_utils:make_subdomain_pattern(SubdomainTemplate), | ||
| Domain = mongoose_subdomain_utils:get_fqdn(Pattern, <<"example.com">>), | ||
| %% TODO: do we want warning printed by validate_domain_res, especially when |
There was a problem hiding this comment.
We need to change this, because example.com is misleading as an error message - users get confused by it, I've seen it in MongooseIM issues. For fqdn, validate as before, but for prefix just check that it is a valid domain string.
There was a problem hiding this comment.
I'm not sure that we need DNS validation at all
There was a problem hiding this comment.
Anyway, I would rather want to do this in another PR.
There was a problem hiding this comment.
I fully agree, it should be done in a separate PR for sure. @arcusfelis, you introduced the DNS check, are you OK to remove it from config validators? For me only syntax should be checked here.
There was a problem hiding this comment.
maybe. I still think dns check is important for many cases. at least as a warning.
this PR contains these changes:
virtual_pubsub_hostsconfiguration (documentation should be updated in a separate PR).gen_modmodule rework (to be continued in the following PRs).mongoose_domain_core:for_each_domain/2interface (to be used in the following PR).