From b0168fd45b3683c2d6f61ccae67dcd5b918a363d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?louiz=E2=80=99?= Date: Sun, 29 Apr 2018 22:18:26 +0200 Subject: =?UTF-8?q?mam:=20Send=20=E2=80=9Cfin=20complete=E2=80=9D=20only?= =?UTF-8?q?=20when=20appropriate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also simplify how we did the whole “limit + 1” And fix one bad interpretation of the XEP for the case where the query has no after or before restriction. fix #3349 --- src/bridge/bridge.cpp | 3 ++- src/database/database.cpp | 26 +++++++++++++++++--------- src/database/database.hpp | 4 ++-- src/xmpp/biboumi_component.cpp | 22 +++++++++------------- tests/end_to_end/__main__.py | 17 +++++++---------- 5 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/bridge/bridge.cpp b/src/bridge/bridge.cpp index 1c646fe..7a0157a 100644 --- a/src/bridge/bridge.cpp +++ b/src/bridge/bridge.cpp @@ -1005,7 +1005,8 @@ void Bridge::send_room_history(const std::string& hostname, std::string chan_nam auto limit = coptions.col(); if (history_limit.stanzas >= 0 && history_limit.stanzas < limit) limit = history_limit.stanzas; - const auto lines = Database::get_muc_logs(this->user_jid, chan_name, hostname, limit, history_limit.since, {}, Id::unset_value, Database::Paging::last); + const auto result = Database::get_muc_logs(this->user_jid, chan_name, hostname, limit, history_limit.since, {}, Id::unset_value, Database::Paging::last); + const auto& lines = std::get<1>(result); chan_name.append(utils::empty_if_fixed_server("%" + hostname)); for (const auto& line: lines) { diff --git a/src/database/database.cpp b/src/database/database.cpp index 3b3e890..6e08ee1 100644 --- a/src/database/database.cpp +++ b/src/database/database.cpp @@ -190,12 +190,9 @@ std::string Database::store_muc_message(const std::string& owner, const std::str return uuid; } -std::vector Database::get_muc_logs(const std::string& owner, const std::string& chan_name, const std::string& server, - int limit, const std::string& start, const std::string& end, const Id::real_type reference_record_id, Database::Paging paging) +std::tuple> Database::get_muc_logs(const std::string& owner, const std::string& chan_name, const std::string& server, + std::size_t limit, const std::string& start, const std::string& end, const Id::real_type reference_record_id, Database::Paging paging) { - if (limit == 0) - return {}; - auto request = select(Database::muc_log_lines); request.where() << Database::Owner{} << "=" << owner << \ " and " << Database::IrcChanName{} << "=" << chan_name << \ @@ -228,15 +225,26 @@ std::vector Database::get_muc_logs(const std::string& owne else request.order_by() << Id{} << " DESC "; - if (limit >= 0) - request.limit() << limit; + // Just a simple trick: to know whether we got the totality of the + // possible results matching this query (except for the limit), we just + // ask one more element. If we get that additional element, this means + // we don’t have everything. And then we just discard it. If we don’t + // have more, this means we have everything. + request.limit() << limit + 1; auto result = request.execute(*Database::db); + bool complete = true; + + if (result.size() == limit + 1) + { + complete = false; + result.erase(std::prev(result.end())); + } if (paging == Database::Paging::first) - return result; + return {complete, result}; else - return {result.crbegin(), result.crend()}; + return {complete, {result.crbegin(), result.crend()}}; } Database::MucLogLine Database::get_muc_log(const std::string& owner, const std::string& chan_name, const std::string& server, diff --git a/src/database/database.hpp b/src/database/database.hpp index 30ffcb4..3e25b30 100644 --- a/src/database/database.hpp +++ b/src/database/database.hpp @@ -131,8 +131,8 @@ class Database * Get all the lines between (optional) start and end dates, with a (optional) limit. * If after_id is set, only the records after it will be returned. */ - static std::vector get_muc_logs(const std::string& owner, const std::string& chan_name, const std::string& server, - int limit=-1, const std::string& start="", const std::string& end="", + static std::tuple> get_muc_logs(const std::string& owner, const std::string& chan_name, const std::string& server, + std::size_t limit, const std::string& start="", const std::string& end="", const Id::real_type reference_record_id=Id::unset_value, Paging=Paging::first); /** diff --git a/src/xmpp/biboumi_component.cpp b/src/xmpp/biboumi_component.cpp index 6dc5fc5..dbaf8a4 100644 --- a/src/xmpp/biboumi_component.cpp +++ b/src/xmpp/biboumi_component.cpp @@ -743,19 +743,15 @@ bool BiboumiComponent::handle_mam_request(const Stanza& stanza) } // Do not send more than 100 messages, even if the client asked for more, // or if it didn’t specify any limit. - // 101 is just a trick to know if there are more available messages. - // If our query returns 101 message, we know it’s incomplete, but we - // still send only 100 - if ((limit == -1 && start.empty() && end.empty()) - || limit > 100) - limit = 101; - auto lines = Database::get_muc_logs(from.bare(), iid.get_local(), iid.get_server(), limit, start, end, reference_record_id, paging_order); - bool complete = true; - if (lines.size() > 100) - { - complete = false; - lines.erase(lines.begin(), std::prev(lines.end(), 100)); - } + if (limit < 0 || limit > 100) + limit = 100; + auto result = Database::get_muc_logs(from.bare(), iid.get_local(), iid.get_server(), + limit, + start, end, + reference_record_id, paging_order); + bool complete = std::get(result); + auto& lines = std::get<1>(result); + for (const Database::MucLogLine& line: lines) { if (!line.col().empty()) diff --git a/tests/end_to_end/__main__.py b/tests/end_to_end/__main__.py index 2f6fc28..1ec5a74 100644 --- a/tests/end_to_end/__main__.py +++ b/tests/end_to_end/__main__.py @@ -1961,7 +1961,7 @@ if __name__ == '__main__': ("/iq[@type='result'][@id='id3'][@from='#foo%{irc_server_one}'][@to='{jid_one}/{resource_one}']", "/iq/mam:fin[@complete='true']/rsm:set")), - # Retrieve a limited archive + # Retrieve the whole archive, but limit the response to one elemet partial(send_stanza, "1"), partial(expect_stanza, @@ -1971,7 +1971,7 @@ if __name__ == '__main__': partial(expect_stanza, ("/iq[@type='result'][@id='id4'][@from='#foo%{irc_server_one}'][@to='{jid_one}/{resource_one}']", - "/iq/mam:fin[@complete='true']/rsm:set")), + "!/iq/mam:fin[@complete='true']/rsm:set")), ]), Scenario("mam_with_timestamps", @@ -2205,11 +2205,9 @@ if __name__ == '__main__': ] * 150 + [ # Retrieve the archive, without any restriction partial(send_stanza, ""), - # Since we should only receive the last 100 messages from the archive, - # it should start with message "1" partial(expect_stanza, ("/message/mam:result[@queryid='qid1']/forward:forwarded/delay:delay", - "/message/mam:result[@queryid='qid1']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='1']") + "/message/mam:result[@queryid='qid1']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='0']") ), ] + [ # followed by 98 more messages @@ -2221,7 +2219,7 @@ if __name__ == '__main__': # and finally the message "99" partial(expect_stanza, ("/message/mam:result[@queryid='qid1']/forward:forwarded/delay:delay", - "/message/mam:result[@queryid='qid1']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='100']"), + "/message/mam:result[@queryid='qid1']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='99']"), after = partial(save_value, "last_uuid", partial(extract_attribute, "/message/mam:result", "id")) ), # And it should not be marked as complete @@ -2236,9 +2234,9 @@ if __name__ == '__main__': partial(expect_stanza, ("/message/mam:result[@queryid='qid2']/forward:forwarded/delay:delay", - "/message/mam:result[@queryid='qid2']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='101']") + "/message/mam:result[@queryid='qid2']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body[text()='100']") ), - ] + 47 * [ + ] + 48 * [ partial(expect_stanza, ("/message/mam:result[@queryid='qid2']/forward:forwarded/delay:delay", "/message/mam:result[@queryid='qid2']/forward:forwarded/client:message[@from='#foo%{irc_server_one}/{nick_one}'][@type='groupchat']/client:body") @@ -2297,8 +2295,7 @@ if __name__ == '__main__': partial(expect_stanza, ("/iq[@type='result'][@id='id4'][@from='#foo%{irc_server_one}'][@to='{jid_one}/{resource_one}']", "/iq/mam:fin/rsm:set/rsm:last[text()='{last_uuid}']", - "/iq/mam:fin[@complete='true']", - "/iq/mam:fin")), + "!/iq/mam:fin[@complete='true']",)), ]), Scenario("channel_history_on_fixed_server", [ -- cgit v1.2.3