Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inbox/pagination — part 1 #3827

Merged
merged 8 commits into from
Nov 8, 2022
Merged

Inbox/pagination — part 1 #3827

merged 8 commits into from
Nov 8, 2022

Conversation

NelsonVides
Copy link
Collaborator

Currently inbox introduces only the max key of the RSM XEP, and within the inbox form we also allow for setting something equivalent to the RSM’s before and after. But there’s no RSM payload in the iq-fin-result, and also, helper libraries might be used on the client side for more complex pagination, provided full support for RSM is available.

This PR introduces support for points:

Other points in the document might be implemented fully or partially but I haven't taken them into consideration for this PR and are therefore not verified nor tested.

Note that the count is not introduced, the document specifies that:

The responding entity SHOULD also include the number of items in the full result set (which MAY be approximate) encapsulated in a element. The element SHOULD include an 'index' attribute. This integer specifies the position within the full set (which MAY be approximate) of the first item in the page. If that item is the first in the full set, then the index SHOULD be '0'. If the last item in the page is the last item in the full set, then the value of the element's 'index' attribute SHOULD be the specified count minus the number of items in the last page.

Note: The element and 'index' attribute enable important functionality for requesting entities (for example, a scroll-bar user-interface component). They MAY be omitted, but only if it would be either impossible or exceptionally resource intensive to calculate reasonably accurate values.

Also not yet fully perfected is the fact that the UIDs as specified in the document are not unique, currently it is using the integer representation of the timestamp of the inbox row, which can potentially collide. Left is to ensure timestamps are unique, or use a hash of the jid which are known to be unique, though in that case it'd need to make more complex queries to find out about the order by timestamp.

This is for now experimental and therefore not yet fully documented, I plan to do so in a subsequent PR, but it is as of this PR pretty much the requirement I currently have.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 83.03% // Head: 83.04% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (a9d463e) compared to base (0704606).
Patch coverage: 85.29% of modified lines in pull request are covered.

❗ Current head a9d463e differs from pull request most recent head 5d3c85d. Consider uploading reports for the commit 5d3c85d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3827   +/-   ##
=======================================
  Coverage   83.03%   83.04%           
=======================================
  Files         529      529           
  Lines       33889    33902   +13     
=======================================
+ Hits        28141    28153   +12     
- Misses       5748     5749    +1     
Impacted Files Coverage Δ
src/inbox/mod_inbox.erl 87.50% <85.18%> (-0.19%) ⬇️
src/jlib.erl 91.66% <85.71%> (ø)
src/elasticsearch/mongoose_elasticsearch.erl 76.92% <0.00%> (-7.70%) ⬇️
src/inbox/mod_inbox_rdbms_async.erl 70.58% <0.00%> (-2.95%) ⬇️
src/mam/mod_mam_elasticsearch_arch.erl 86.60% <0.00%> (-1.79%) ⬇️
src/mam/mod_mam_utils.erl 89.72% <0.00%> (-0.31%) ⬇️
src/pubsub/mod_pubsub.erl 73.04% <0.00%> (-0.19%) ⬇️
src/ejabberd_c2s.erl 88.95% <0.00%> (-0.08%) ⬇️
src/mod_muc_log.erl 63.08% <0.00%> (+0.26%) ⬆️
src/mod_roster.erl 79.42% <0.00%> (+0.71%) ⬆️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@NelsonVides NelsonVides marked this pull request as draft November 2, 2022 02:22
@mongoose-im

This comment was marked as outdated.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Nov 3, 2022

small_tests_24 / small_tests / a9d463e
Reports root / small


small_tests_25 / small_tests / a9d463e
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / a9d463e
Reports root/ big
OK: 3995 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / a9d463e
Reports root/ big
OK: 2099 / Failed: 0 / User-skipped: 783 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / a9d463e
Reports root/ big
OK: 3969 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / a9d463e
Reports root/ big
OK: 2099 / Failed: 0 / User-skipped: 783 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / a9d463e
Reports root/ big
OK: 3995 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / a9d463e
Reports root/ big
OK: 2235 / Failed: 0 / User-skipped: 647 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / a9d463e
Reports root/ big
OK: 2577 / Failed: 0 / User-skipped: 640 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / a9d463e
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / a9d463e
Reports root/ big
OK: 4355 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / a9d463e
Reports root/ big
OK: 2422 / Failed: 1 / User-skipped: 626 / Auto-skipped: 0

graphql_roster_SUITE:user_roster:user_invite_accept_and_cancel_subscription
{error,
  {timeout_when_waiting_for_stanza,
    [{escalus_client,wait_for_stanza,
       [{client,
          <<"bob_user_invite_accept_and_cancel_subscription_871@localhost/res1">>,
          escalus_tcp,<0.22653.0>,
          [{event_manager,<0.22651.0>},
           {server,<<"localhost">>},
           {username,
             <<"bOb_user_invite_accept_and_cancel_subscription_871">>},
           {resource,<<"res1">>}],
          [{event_client,
             [{event_manager,<0.22651.0>},
            {server,<<"localhost">>},
            {username,
              <<"bOb_user_invite_accept_and_cancel_subscription_871">>},
            {resource,<<"res1">>}]},
           {resource,<<"res1">>},
           {username,
             <<"bob_user_invite_accept_and_cancel_subscription_871">>},
           {server,<<"localhost">>},
           {host,<<"localhost">>},
           {port,5222},
           {auth,{escalus_auth,auth_plain}},
           {wspath,undefined},
           {username,
             <<"bOb_user_invite_accept_and_cancel_subscription_871">>},
           {server,<<"localhost">>},
           {password,<<"makrolika">>},
           {stream_id,<<"f39077d764063c5c">>}]},
        1],
       [{file,
          "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
        {line,136}]},
     {graphql_roster_SUITE,
       user_invite_accept_and_cancel_subscription_story,3,
       [{file,
          "/home/circleci/project/big_tests/tests/graphql_roster_SUITE...

Report log


pgsql_mnesia_25 / pgsql_mnesia / a9d463e
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / a9d463e
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / a9d463e
Reports root/ big
OK: 2423 / Failed: 0 / User-skipped: 626 / Auto-skipped: 0

@NelsonVides NelsonVides marked this pull request as ready for review November 4, 2022 13:57
@NelsonVides NelsonVides requested a review from chrzaszcz November 7, 2022 16:17
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I added a few minor comments.

big_tests/tests/inbox_extensions_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/inbox_extensions_SUITE.erl Outdated Show resolved Hide resolved
src/inbox/mod_inbox.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Nov 8, 2022

small_tests_24 / small_tests / 5d3c85d
Reports root / small


small_tests_25 / small_tests / 5d3c85d
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 5d3c85d
Reports root/ big
OK: 3995 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 5d3c85d
Reports root/ big
OK: 2099 / Failed: 0 / User-skipped: 783 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 5d3c85d
Reports root/ big
OK: 2099 / Failed: 0 / User-skipped: 783 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 5d3c85d
Reports root/ big
OK: 3969 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 5d3c85d
Reports root/ big
OK: 2235 / Failed: 0 / User-skipped: 647 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 5d3c85d
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 5d3c85d
Reports root/ big
OK: 2577 / Failed: 0 / User-skipped: 640 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 5d3c85d
Reports root/ big
OK: 3995 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 5d3c85d
Reports root/ big
OK: 4355 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 5d3c85d
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 5d3c85d
Reports root/ big
OK: 2423 / Failed: 0 / User-skipped: 626 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 5d3c85d
Reports root/ big
OK: 4369 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@chrzaszcz chrzaszcz merged commit ef6adee into master Nov 8, 2022
@chrzaszcz chrzaszcz deleted the inbox/pagination branch November 8, 2022 14:05
@chrzaszcz chrzaszcz added this to the 6.0.0 milestone Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants