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

Define more granular overriding between inbox form and rms #3974

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

NelsonVides
Copy link
Collaborator

It happens that, if a user wants to paginate through inbox but only through what has changed since a given timestamp, once the second page is requested using the given <after> returned on the first page, the pagination would overrun any limit timestamp given and would therefore continue returning the entire inbox set.

Example:

<!-- We ask for all changes since the given `start` date -->
<iq type="set" id="8D07E14F-9A2D-442A-A996-81F4E47E6CC6">
    <inbox xmlns="erlang-solutions.com:xmpp:inbox:0">
        <x xmlns="jabber:x:data" type="form">
            <field var="box" type="list-single"><value>inbox</value></field>
            <field var="start"><value>2023-02-28T15:27:31.481Z</value></field>
        </x>
        <set xmlns="http://jabber.org/protocol/rsm">
            <max>30</max>
        </set>
    </inbox>
</iq>

<!-- we get all the 30 entries, and then the iq-result with the given page details -->
<iq xmlns="jabber:client" from="alice@localhost" to="alice@localhost/res1" id="8D07E14F-9A2D-442A-A996-81F4E47E6CC6" type="result">
    <fin xmlns="erlang-solutions.com:xmpp:inbox:0">
        <set xmlns="http://jabber.org/protocol/rsm">
            <first>1673349807109070/oaeusnth</first>
            <last>1673349791932211/qjkzlvwm</last>
        </set>
        <active-conversations>0</active-conversations>
        <count>30</count>
        <unread-messages>0</unread-messages>
    </fin>
</iq>

<!-- We decide to continue fetching from the previously given end of page, -->
<!-- but we still want to end on the same timestamp -->
<iq type="set" id="F2DF7FCE-4802-43E9-9C0B-6D0A333D7E8A">
    <inbox xmlns="erlang-solutions.com:xmpp:inbox:0">
        <x xmlns="jabber:x:data" type="form">
            <field var="box" type="list-single"><value>inbox</value></field>
            <field var="start"><value>2023-02-28T15:27:31.481Z</value></field>
        </x>
        <set xmlns="http://jabber.org/protocol/rsm">
            <max>30</max>
            <after>1673349791932211/qjkzlvwm</after>
        </set>
    </inbox>
</iq>

The old implementation would overrun the start part of the form and not limit the page size. The new implementation takes care of that, by instead of having RSM overriding the entire form, having after override only end, and before override only start.

It happens that, if a user wants to paginate through inbox but only through what
has changed since a given timestamp, once the second page is requested using the
given `<after>` returned on the first page, the pagination would overrun any
limit timestamp given and would therefore continue returning the entire inbox set.

Example:
```xml
<!-- We ask for all changes since the given `start` date -->
<iq type="set" id="8D07E14F-9A2D-442A-A996-81F4E47E6CC6">
    <inbox xmlns="erlang-solutions.com:xmpp:inbox:0">
        <x xmlns="jabber:x:data" type="form">
            <field var="box" type="list-single"><value>inbox</value></field>
            <field var="start"><value>2023-02-28T15:27:31.481Z</value></field>
        </x>
        <set xmlns="http://jabber.org/protocol/rsm">
            <max>30</max>
        </set>
    </inbox>
</iq>

<!-- we get all the 30 entries, and then the iq-result with the given page details -->
<iq xmlns="jabber:client" from="alice@localhost" to="alice@localhost/res1" id="8D07E14F-9A2D-442A-A996-81F4E47E6CC6" type="result">
    <fin xmlns="erlang-solutions.com:xmpp:inbox:0">
        <set xmlns="http://jabber.org/protocol/rsm">
            <first>1673349807109070/oaeusnth</first>
            <last>1673349791932211/qjkzlvwm</last>
        </set>
        <active-conversations>0</active-conversations>
        <count>30</count>
        <unread-messages>0</unread-messages>
    </fin>
</iq>

<!-- We decide to continue fetching from the previously given end of page, -->
<!-- but we still want to end on the same timestamp -->
<iq type="set" id="F2DF7FCE-4802-43E9-9C0B-6D0A333D7E8A">
    <inbox xmlns="erlang-solutions.com:xmpp:inbox:0">
        <x xmlns="jabber:x:data" type="form">
            <field var="box" type="list-single"><value>inbox</value></field>
            <field var="start"><value>2023-02-28T15:27:31.481Z</value></field>
        </x>
        <set xmlns="http://jabber.org/protocol/rsm">
            <max>30</max>
            <after>1673349791932211/qjkzlvwm</after>
        </set>
    </inbox>
</iq>
```

The old implementation would overrun the `start` part of the form and not limit
the page size. The new implementation takes care of that, by instead of having
RSM overriding the entire form, having `after` override _only_ `end`, and
`before` override _only_ `start`.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Mar 1, 2023

small_tests_24 / small_tests / 69a1753
Reports root / small


small_tests_25 / small_tests / 69a1753
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 69a1753
Reports root/ big
OK: 2225 / Failed: 0 / User-skipped: 825 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 69a1753
Reports root/ big
OK: 4177 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 69a1753
Reports root/ big
OK: 2225 / Failed: 0 / User-skipped: 825 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 69a1753
Reports root/ big
OK: 4150 / Failed: 1 / User-skipped: 114 / Auto-skipped: 0

muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive
{error,{{assertion_failed,assert,is_groupchat_message,
              [<<"Restorable message">>],
              undefined,"undefined"},
    [{escalus_new_assert,assert_true,2,
               [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
                {line,84}]},
     {muc_SUITE,wait_for_mam_result,3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4394}]},
     {muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4130}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
          [{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
           {line,4126}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1291}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1223}]}]}}

Report log


pgsql_mnesia_24 / pgsql_mnesia / 69a1753
Reports root/ big
OK: 4557 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 69a1753
Reports root/ big
OK: 4177 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 69a1753
Reports root/ big
OK: 2367 / Failed: 0 / User-skipped: 683 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 69a1753
Reports root/ big
OK: 4173 / Failed: 1 / User-skipped: 91 / Auto-skipped: 0

inbox_extensions_SUITE:async_pools:one_to_one:mute_muted_conv_restarts_timestamp
{error,
  {{assert,
     [{module,inbox_extensions_SUITE},
      {line,899},
      {expression,"escalus_pred : is_message ( Message )"},
      {expected,true},
      {value,false}]},
   [{inbox_extensions_SUITE,check_message_with_properties,4,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,899}]},
    {inbox_extensions_SUITE,set_inbox_properties,4,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,893}]},
    {inbox_extensions_SUITE,'-mute_muted_conv_restarts_timestamp/1-fun-2-',
      2,
      [{file,
         "/home/circleci/project/big_tests/tests/inbox_extensions_SUITE.erl"},
       {line,616}]},
    {escalus_story,story,4,
      [{file,
         "/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
       {line,72}]},
    {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
    {test_server,run_test_case_eval1,6,
      [{file,"test_server.erl"},{line,1291}]},
    {test_server,run_test_case_eval,9,
      [{file,"test_server.erl"},{line,1223}]}]}}

Report log


riak_mnesia_24 / riak_mnesia / 69a1753
Reports root/ big
OK: 2565 / Failed: 0 / User-skipped: 654 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 69a1753
Reports root/ big
OK: 2727 / Failed: 0 / User-skipped: 662 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 69a1753
Reports root/ big
OK: 4557 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 69a1753
Reports root/ big
OK: 4554 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 69a1753
Reports root/ big
OK: 4543 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 69a1753
Reports root/ big
OK: 4151 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 69a1753
Reports root/ big
OK: 4173 / Failed: 1 / User-skipped: 91 / Auto-skipped: 0

mam_SUITE:rdbms_async_cache_prefs_cases:messages_filtered_when_prefs_default_policy_is_always
{error,
  {{assertion_failed,assert,is_iq_result,
     [{xmlel,<<"iq">>,
        [{<<"type">>,<<"set">>},
         {<<"id">>,<<"d0a8cfe9eb56e3a55195e1b0b64e3657">>}],
        [{xmlel,<<"prefs">>,
           [{<<"xmlns">>,<<"urn:xmpp:mam:1">>},
          {<<"default">>,<<"always">>}],
           [{xmlel,<<"always">>,[],[]},{xmlel,<<"never">>,[],[]}]}]}],
     {xmlel,<<"iq">>,
       [{<<"from">>,
         <<"alice_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com">>},
        {<<"to">>,
         <<"alice_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com/res1">>},
        {<<"id">>,<<"pushc993e81a92fbe499">>},
        {<<"type">>,<<"set">>}],
       [{xmlel,<<"query">>,
          [{<<"xmlns">>,<<"jabber:iq:roster">>}],
          [{xmlel,<<"item">>,
             [{<<"subscription">>,<<"both">>},
            {<<"jid">>,
             <<"bob_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com">>}],
             []}]}]},
     "<iq from='alice_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com' to='alice_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com/res1' id='pushc993e81a92fbe499' type='set'><query xmlns='jabber:iq:roster'><item subscription='both' jid='bob_messages_filtered_when_prefs_default_policy_is_always_2092@domain.example.com'/></query></iq>"},
   [{escalus_new_assert,assert_true,2,
      [{file,
         "/home/circleci/project/big_test...

Report log


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 69a1753
Reports root/ big
OK: 4174 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.53 🎉

Comparison is base (4a97962) 81.08% compared to head (69a1753) 83.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3974      +/-   ##
==========================================
+ Coverage   81.08%   83.62%   +2.53%     
==========================================
  Files         538      538              
  Lines       34031    34030       -1     
==========================================
+ Hits        27594    28457     +863     
+ Misses       6437     5573     -864     
Impacted Files Coverage Δ
src/inbox/mod_inbox.erl 87.67% <100.00%> (+0.39%) ⬆️
src/domain/service_domain_db.erl 83.33% <0.00%> (-2.09%) ⬇️
src/mod_muc_log.erl 62.82% <0.00%> (ø)
src/pubsub/mod_pubsub.erl 73.48% <0.00%> (+0.18%) ⬆️
src/domain/mongoose_domain_loader.erl 90.17% <0.00%> (+0.89%) ⬆️
src/mam/mod_mam_utils.erl 89.96% <0.00%> (+0.91%) ⬆️
src/inbox/mod_inbox_rdbms_async.erl 72.46% <0.00%> (+1.44%) ⬆️
src/pubsub/node_pep.erl 79.62% <0.00%> (+1.85%) ⬆️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 68.88% <0.00%> (+2.22%) ⬆️
src/mam/mod_mam_muc_rdbms_arch_async.erl 85.71% <0.00%> (+2.85%) ⬆️
... and 16 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 ready for review March 1, 2023 14:56
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

ok

@arcusfelis arcusfelis merged commit 8b5d730 into master Mar 1, 2023
@arcusfelis arcusfelis deleted the inbox_pagination branch March 1, 2023 16:49
@jacekwegr jacekwegr added this to the 6.1.0 milestone Apr 26, 2023
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.

4 participants