Merge pull request #3843 from esl/inbox/unique_pagination_ids

Make the IDs for the inbox pagination unique

big_tests/tests/inbox_extensions_SUITE.erl

@@ -99,12 +99,16 @@ groups() ->
         properties_full_entry_can_be_get,
         properties_many_can_be_set,
         properties_many_can_be_set_queryid,
-        inbox_pagination_overrides_form,
-        inbox_can_paginate_forwards,
-        inbox_can_paginate_backwards,
+        timestamp_is_not_reset_with_setting_properties,
+        {group, pagination}
+      ]},
+     {pagination, [parallel], [
+        bad_id_throws_error,
+        pagination_overrides_form,
+        can_paginate_forwards,
+        can_paginate_backwards,
         max_queries_can_be_limited,
-        max_queries_can_fetch_ahead,
-        timestamp_is_not_reset_with_setting_properties
+        max_queries_can_fetch_ahead
       ]},
      {muclight, [], [
         groupchat_setunread_stanza_sets_inbox
@@ -669,7 +673,18 @@ properties_many_can_be_set(Config, QueryId) ->
                                                 end}], #{box => archive})
     end).
 
-inbox_pagination_overrides_form(Config) ->
+bad_id_throws_error(Config) ->
+    escalus:fresh_story(Config, [{alice, 1}], fun(Alice) ->
+        % We set start and end to return Convs with Mike, but using RSM we override that
+        TS = erlang:system_time(millisecond),
+        AliceJid = escalus_utils:get_short_jid(Alice),
+        IdNotDividing = <<(integer_to_binary(TS))/binary, (base64:encode(AliceJid))/binary>>,
+        verify_returns_error(Alice, #{box => inbox, 'after' => IdNotDividing}),
+        BadInt = <<(integer_to_binary(-10))/binary, "/", (base64:encode(AliceJid))/binary>>,
+        verify_returns_error(Alice, #{box => inbox, 'after' => BadInt})
+    end).
+
+pagination_overrides_form(Config) ->
     escalus:fresh_story(Config, [{alice, 1}, {bob, 1}, {kate, 1}, {mike, 1}],
                         fun(Alice, Bob, Kate, Mike) ->
         % Several people write to Alice
@@ -687,7 +702,7 @@ inbox_pagination_overrides_form(Config) ->
         inbox_helper:check_inbox(Alice, AliceConvs, Params)
     end).
 
-inbox_can_paginate_forwards(Config) ->
+can_paginate_forwards(Config) ->
     escalus:fresh_story(Config, [{alice, 1}, {bob, 1}, {kate, 1}, {mike, 1}],
                         fun(Alice, Bob, Kate, Mike) ->
         % Several people write to Alice
@@ -699,11 +714,11 @@ inbox_can_paginate_forwards(Config) ->
         #{respond_iq := Iq} = inbox_helper:check_inbox(Alice, [ConvWithMike], Params1),
         AfterPrevious = exml_query:path(Iq, [{element_with_ns, <<"fin">>, inbox_helper:inbox_ns()},
                                              {element, <<"set">>}, {element, <<"last">>}, cdata]),
-        Params2 = #{box => inbox, 'after' => AfterPrevious},
+        Params2 = #{box => inbox, 'after' => AfterPrevious, limit => 2},
         inbox_helper:check_inbox(Alice, [ConvWithKate, ConvWithBob], Params2)
     end).
 
-inbox_can_paginate_backwards(Config) ->
+can_paginate_backwards(Config) ->
     escalus:fresh_story(Config, [{alice, 1}, {bob, 1}, {kate, 1}, {mike, 1}],
                         fun(Alice, Bob, Kate, Mike) ->
         % Several people write to Alice
@@ -955,3 +970,7 @@ muted_status(MutedOrUnmuted, Outer) ->
             Diff = erlang:convert_time_unit(MutedDiff, second, microsecond),
             ?assert(Now + Diff < GivenMutedUntil)
     end.
+
+verify_returns_error(User, Params) ->
+    Stanza = inbox_helper:make_inbox_stanza(Params),
+    assert_invalid_request(User, Stanza, <<"bad-request">>).

doc/open-extensions/inbox.md

@@ -148,6 +148,7 @@ It can happen that the amount of inbox entries is too big for a given user, even
 ```
 where `Max` is a non-negative integer.
 
+Inbox also has partial support for pagination as described in [XEP-0059](https://xmpp.org/extensions/xep-0059.html). If specifying `before` or `after`, the `start` and `end` form fields will be overridden.
 
 ## Properties of an entry
 Given an entry, certain properties are defined for such an entry:

src/inbox/mod_inbox.erl

@@ -47,7 +47,8 @@
         hidden_read => true | false,
         box => binary(),
         limit => undefined | pos_integer(),
-        rsm => jlib:rsm_in()
+        rsm => jlib:rsm_in(),
+        filter_on_jid => binary()
        }.
 
 -type count_res() :: ok | {ok, non_neg_integer()} | {error, term()}.
@@ -209,11 +210,26 @@ process_iq(Acc, From, _To, #iq{type = set, sub_el = QueryEl} = IQ, _Extra) ->
             {Acc, Res}
     end.
 
+-spec with_rsm([inbox_res()], get_inbox_params()) -> [inbox_res()].
+with_rsm(List, #{order := asc, start := TS, filter_on_jid := BinJid, rsm := #rsm_in{}}) ->
+    lists:reverse(drop_filter_on_jid(List, BinJid, TS, List));
 with_rsm(List, #{order := asc, rsm := #rsm_in{}}) ->
     lists:reverse(List);
+with_rsm(List, #{order := desc, 'end' := TS, filter_on_jid := BinJid}) ->
+    drop_filter_on_jid(List, BinJid, TS, List);
 with_rsm(List, _) ->
     List.
 
+%% As IDs must be unique but timestamps are not, and SQL queries and orders by timestamp alone,
+%% we query max+1 and then match to remove the entry that matches the ID given before.
+-spec drop_filter_on_jid([inbox_res()], binary(), integer(), [inbox_res()]) -> [inbox_res()].
+drop_filter_on_jid(_List, BinJid, TS, [#{remote_jid := BinJid, timestamp := TS} | Rest]) ->
+    Rest;
+drop_filter_on_jid(List, BinJid, TS, [_ | Rest]) ->
+    drop_filter_on_jid(List, BinJid, TS, Rest);
+drop_filter_on_jid(List, _, _, []) ->
+    List.
+
 -spec forward_messages(Acc :: mongoose_acc:t(),
                        List :: [inbox_res()],
                        QueryEl :: jlib:iq(),
@@ -408,10 +424,10 @@ build_result_iq(List) ->
 -spec result_set([inbox_res()]) -> exml:element().
 result_set([]) ->
     #xmlel{name = <<"set">>, attrs = [{<<"xmlns">>, ?NS_RSM}]};
-result_set([#{timestamp := First} | _] = List) ->
-    #{timestamp := Last} = lists:last(List),
-    BFirst = integer_to_binary(First),
-    BLast = integer_to_binary(Last),
+result_set([#{remote_jid := FirstBinJid, timestamp := FirstTS} | _] = List) ->
+    #{remote_jid := LastBinJid, timestamp := LastTS} = lists:last(List),
+    BFirst = mod_inbox_utils:encode_rsm_id(FirstTS, FirstBinJid),
+    BLast = mod_inbox_utils:encode_rsm_id(LastTS, LastBinJid),
     mod_mam_utils:result_set(BFirst, BLast, undefined, undefined).
 
 %%%%%%%%%%%%%%%%%%%
@@ -473,15 +489,26 @@ build_params_with_rsm(Params, #rsm_in{max = Max, id = <<>>, direction = before})
 build_params_with_rsm(Params, #rsm_in{max = Max, id = <<>>, direction = aft}) ->
     Params#{limit => Max};
 build_params_with_rsm(Params, #rsm_in{max = Max, id = Id, direction = Dir}) when is_binary(Id) ->
-    case {mod_inbox_utils:maybe_binary_to_positive_integer(Id), Dir} of
-        {{error, _}, _} -> {error, bad_request, <<"bad-request">>};
-        {Stamp, aft} -> Params#{limit => Max, 'end' => Stamp - 1};
-        {Stamp, undefined} -> Params#{limit => Max, 'end' => Stamp - 1};
-        {Stamp, before} -> Params#{limit => Max, order => asc, start => Stamp + 1}
+    case {mod_inbox_utils:decode_rsm_id(Id), Dir} of
+        {error, _} ->
+            {error, bad_request, <<"bad-request">>};
+        {{Stamp, Jid}, aft} ->
+            Params#{limit => expand_limit(Max), filter_on_jid => Jid, 'end' => Stamp};
+        {{Stamp, Jid}, undefined} ->
+            Params#{limit => expand_limit(Max), filter_on_jid => Jid, 'end' => Stamp};
+        {{Stamp, Jid}, before} ->
+            Params#{limit => expand_limit(Max), order => asc, filter_on_jid => Jid, start => Stamp}
     end;
 build_params_with_rsm(Params, _Rsm) ->
     Params.
 
+-spec expand_limit(undefined) -> undefined;
+                  (integer()) -> integer().
+expand_limit(undefined) ->
+    undefined;
+expand_limit(Max) ->
+    Max + 1.
+
 -spec form_to_params(mongooseim:host_type(), FormEl :: exml:element() | undefined) ->
     get_inbox_params() | {error, bad_request, Msg :: binary()}.
 form_to_params(_, undefined) ->

src/inbox/mod_inbox_utils.erl

@@ -31,6 +31,8 @@
          get_option_remove_on_kicked/1,
          extract_attr_jid/1,
          maybe_binary_to_positive_integer/1,
+         encode_rsm_id/2,
+         decode_rsm_id/1,
          binary_to_bool/1,
          bool_to_binary/1,
          build_inbox_entry_key/2,
@@ -197,6 +199,26 @@ maybe_binary_to_positive_integer(Bin) ->
     catch error:badarg -> {error, 'NaN'}
     end.
 
+-spec encode_rsm_id(integer(), binary()) -> binary().
+encode_rsm_id(Int, BinJid) ->
+    BinInt = integer_to_binary(Int),
+    EncodedJid = base64:encode(BinJid),
+    <<BinInt/binary, "/", EncodedJid/binary>>.
+
+-spec decode_rsm_id(binary()) -> {integer(), binary()} | error.
+decode_rsm_id(Bin) ->
+    case binary:split(Bin, <<"/">>) of
+        [BinInt, BinJid] ->
+            Int = maybe_binary_to_positive_integer(BinInt),
+            case Int of
+                Int when is_integer(Int) ->
+                    Jid = base64:decode(BinJid),
+                    {Int, Jid};
+                _ -> error
+            end;
+        _ -> error
+    end.
+
 -spec binary_to_bool(binary()) -> true | false | error.
 binary_to_bool(<<"true">>) -> true;
 binary_to_bool(<<"false">>) -> false;