Inbox extensions #3067
Merged
Inbox extensions #3067
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
* Inbox documentation v2 With attended review
This applies to both source and test code. Also assert more consistent error messages in big_tests.
Sometimes an inbox verifier wants to access the outer message, and not the default inner one, so add more cases based on the arity of the function. This is a hack for the extension tests.
This suite just has every single scenario covered by the extension, it is very exhaustive and I tried to write the code in an organised and predictable way, full of comments describing the intention, spec annotations, and what not. I'm not reflecting here how did this came to happen, just the final state of the test-suite.
I'm making the new values just binaries, to be transparent in their meaning, mod_inbox should just put more childrens in the xml-stanza, without knowing what are they.
Note that I'm modifying only the pgsql schema, and I'm not touching mssql nor mysql schemas, as in bkpr we're using pgsql alone.
Note that I'm modifying only the pgsql query, and I'm not touching mod_inbox_rdbms_mssql nor mod_inbox_rdbms_mysql, as in bkpr we're using pgsql alone.
There's a bunch of hardcoded calls to mod_inbox_bkpr, for selecting more fields, filtering from more patterns, and returning more values, that I tried to keep out of the base mod_inbox_rdbms, that might suffer modifications in open source, keeping all bkpr logic just in mod_inbox_bkpr. The most challenging part is the tuple returned from the sql driver, there's no way to pattern-match a variable-length tuple and tell the extensions to parse all the suffix, I had to change the tuple size and hardcode the extended fields. I considered doing a tuple_to_list and pattern-match on that, but it would be a performance overkill, I don't think we need to go that far. Note that, as only the extension knows how the fields were requested, only the extension knows how to process them and in which order.
As before, mod_inbox tries not to care what are these extra values being processed, and simply takes them and forwards them to mod_inbox_bkpr for processing.
This way we can add more flags to the extensions without touching mod_inbox in the future. The only problem was in the type for the parameters, where it wasn't accepting the new flags without adding them there.
The extensions might send more messages, with certain attributes, that are not desired to be stored on inbox, so the extension needs to add more filtering for this.
Here it is, all of the actions requested by Beekeeper, implemented.
Also rework a bit how other tests were done
Sometimes things were declared as jid:user() when they were jid:luser(), some others were passing jid:jid() and others binaries... They were all modified to expect ready binaries of types jid:luser(), jid:lserver(), or jid:literal_jid().
If we want to support more backends, the interface between them must be pretty erlang idiomatic, so keep it integers and booleans on their interface. Also, note that the start function was using the wrong key number for backend, it's the first of a tuple, not the second. And one more thing, the accumulator contains the timestamp of the incoming message, so we can skip calculating more timestamps along the way.
Avoid calculating a new timestamp that can be off from the one when the original message was received, and instead reuse that which was already created when the message was received. Save computation and potentially save clock miss-syncs.
Codecov Report
@@ Coverage Diff @@
## master #3067 +/- ##
==========================================
+ Coverage 78.77% 78.85% +0.08%
==========================================
Files 379 380 +1
Lines 31137 31269 +132
==========================================
+ Hits 24527 24658 +131
- Misses 6610 6611 +1
Continue to review full report at Codecov.
|
chrzaszcz
reviewed
Apr 8, 2021
chrzaszcz
approved these changes
Apr 9, 2021
Comment on lines
+156
to
+157
| dynamic_modules:stop(Host, mod_inbox), | ||
| dynamic_modules:stop(Host, mod_muc_light), |
There was a problem hiding this comment.
Let's refactor this with save_modules and restore_modules in the future, together with inbox_SUITE.
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.
This PR contains a bunch of commits extracted as-is from beekeeper's fork. They implement extended functionality, best explained in the documentation prepared for them at Beekeeper inbox extensions.pdf (markdown made a pdf).
They implement extensions to set inbox entries as archived, muted (and until), or read, being able to set all of them as either value.
On top of their work, I've carried out a big merge of modules, and also reworked the internals on my own effort, to reduce stringprepping, timestamps, conventions with correctly treating jids, reworking all the docs, and what not.
What is missing:
prepared queries for the whole thingThis is to be done in a separate PR.Details on the commits donated:
Rebasing on esl/master from
these are the commits that were done on the bkpr fork.