diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index 78b9d955f..fccec85ef 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -804,10 +804,13 @@ Response::ResponseCode Server_ProtocolHandler::cmdMessage(const Command_Message userInterface->sendProtocolItem(*se); rc.enqueuePreResponseItem(ServerMessage::SESSION_EVENT, se); - databaseInterface->logMessage(userInfo->id(), QString::fromStdString(userInfo->name()), - QString::fromStdString(userInfo->address()), QString::fromStdString(cmd.message()), - Server_DatabaseInterface::MessageTargetChat, userInterface->getUserInfo()->id(), - receiver); + databaseInterface->logMessage( + userInfo->id(), QString::fromStdString(userInfo->name()), QString::fromStdString(userInfo->address()), + QString::fromStdString(cmd.message()), Server_DatabaseInterface::MessageTargetChat, + // A federated user's id belongs to the remote server's + // database, not ours. Logging it would collide with a + // local id and corrupt local history lookups. + userInterface->getUserInfo()->server_id() == -1 ? userInterface->getUserInfo()->id() : 0, receiver); resetIdleTimer(); return Response::RespOk; } diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index 9d8dd46d2..0204b15a6 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -10,6 +10,7 @@ set(servatrice_SOURCES src/forwarded_address.cpp src/main.cpp src/metrics_registry.cpp + src/private_chat_paging.cpp src/ratelimiter.cpp src/servatrice.cpp src/servatrice_connection_pool.cpp diff --git a/servatrice/src/private_chat_paging.cpp b/servatrice/src/private_chat_paging.cpp new file mode 100644 index 000000000..872a1e733 --- /dev/null +++ b/servatrice/src/private_chat_paging.cpp @@ -0,0 +1,31 @@ +#include "private_chat_paging.h" + +#include + +PrivateChatPage assemblePrivateChatPage(const QList &newestFirst, int limit) +{ + PrivateChatPage page; + page.messages = newestFirst; + page.hasMore = newestFirst.size() > limit; + if (!page.hasMore) { + std::reverse(page.messages.begin(), page.messages.end()); + return page; + } + + // The (limit+1)th message is the first one that does not fit. If its second + // appears anywhere in the page, that whole second would be split, so drop + // every message in that second and leave it for the next page. + const quint64 boundary = newestFirst.at(limit).time(); + while (!page.messages.isEmpty() && page.messages.last().time() == boundary) { + page.messages.removeLast(); + } + + if (page.messages.isEmpty()) { + page.needsWholeSecond = true; + page.boundarySecond = boundary; + return page; + } + + std::reverse(page.messages.begin(), page.messages.end()); + return page; +} diff --git a/servatrice/src/private_chat_paging.h b/servatrice/src/private_chat_paging.h new file mode 100644 index 000000000..ee503f2d9 --- /dev/null +++ b/servatrice/src/private_chat_paging.h @@ -0,0 +1,30 @@ +#ifndef PRIVATE_CHAT_PAGING_H +#define PRIVATE_CHAT_PAGING_H + +#include +#include + +/** One page of private chat history, ready for the response. */ +struct PrivateChatPage +{ + /** Messages for the response, oldest first. */ + QList messages; + /** Whether messages older than this page may still exist. */ + bool hasMore = false; + /** + * Set when every fetched message belongs to the same second, so the caller + * has to fetch that whole second before it can answer. messages is empty in + * that case and boundarySecond names the second to fetch. + */ + bool needsWholeSecond = false; + quint64 boundarySecond = 0; +}; + +/** + * Assembles one newest-first page from up to limit + 1 messages ordered newest + * first. A second is never split across pages: when the limit would cut into a + * second, that whole second is deferred to the next page instead. + */ +PrivateChatPage assemblePrivateChatPage(const QList &newestFirst, int limit); + +#endif // PRIVATE_CHAT_PAGING_H diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index 1c7246919..c11f63363 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -8,6 +8,7 @@ #include "libcockatrice/protocol/pb/serverinfo_user_alt.pb.h" #include "libcockatrice/protocol/pb/serverinfo_user_session.pb.h" #include "libcockatrice/protocol/pb/serverinfo_warning.pb.h" +#include "private_chat_paging.h" #include "servatrice.h" #include "server.h" #include "server_protocolhandler.h" @@ -2639,7 +2640,9 @@ int Servatrice_DatabaseInterface::clearBlacklistedBanners() } bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId, + const QString &callerName, int peerId, + const QString &peerName, quint64 before, int limit, QList &messages, @@ -2647,30 +2650,35 @@ bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId, { messages.clear(); hasMore = false; - if (callerId < 1 || peerId < 1 || !checkSql()) { + if (callerId < 1 || peerId < 1 || callerName.isEmpty() || peerName.isEmpty() || !checkSql()) { return false; } - // Both participants are matched by account id, never by name: names can be - // reused by guests or re-registered after deletion, and matching by name - // would merge conversations from different people. + // Match both participants by account id, and verify their names as well. + // Names can be reused (guests, re-registration), so ids are what separate + // earlier holders; names are what reject a federated user's remote id that + // happens to collide with a local one in the shared log table. const QString conversationFilter = " FROM {prefix}_log WHERE target_type = 'chat'" - " AND ((sender_id = :caller_sender_id AND target_id = :peer_target_id)" - " OR (target_id = :caller_target_id AND sender_id = :peer_sender_id))"; + " AND ((sender_id = :caller_sender_id AND sender_name = :caller_name" + " AND target_id = :peer_target_id AND target_name = :peer_name)" + " OR (target_id = :caller_target_id AND target_name = :caller_name" + " AND sender_id = :peer_sender_id AND sender_name = :peer_name))"; const QString selectMessages = "SELECT UNIX_TIMESTAMP(log_time), sender_name, log_message"; const auto bindParticipants = [&](QSqlQuery *query) { query->bindValue(":caller_sender_id", callerId); + query->bindValue(":caller_name", callerName); query->bindValue(":peer_target_id", peerId); + query->bindValue(":peer_name", peerName); query->bindValue(":peer_sender_id", peerId); query->bindValue(":caller_target_id", callerId); }; - const auto readMessages = [&](QSqlQuery *query) { + const auto readMessages = [&](QSqlQuery *query, QList &out) { while (query->next()) { ServerInfo_PrivateChatMessage message; message.set_time(query->value(0).toULongLong()); message.set_sender_name(query->value(1).toString().toStdString()); message.set_message(query->value(2).toString().toStdString()); - messages.append(message); + out.append(message); } }; @@ -2688,34 +2696,32 @@ bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId, if (!execSqlQuery(query)) { return false; } - readMessages(query); - hasMore = messages.size() > limit; - if (hasMore) { - const quint64 boundary = messages.at(limit).time(); - while (!messages.isEmpty() && messages.last().time() == boundary) { - messages.removeLast(); - } - if (messages.isEmpty()) { - query = prepareQuery(selectMessages + conversationFilter + " AND log_time = FROM_UNIXTIME(:second)"); - bindParticipants(query); - query->bindValue(":second", boundary); - if (!execSqlQuery(query)) { - return false; - } - readMessages(query); + QList newestFirst; + readMessages(query, newestFirst); + PrivateChatPage page = assemblePrivateChatPage(newestFirst, limit); - query = prepareQuery("SELECT 1" + conversationFilter + " AND log_time < FROM_UNIXTIME(:before) LIMIT 1"); - bindParticipants(query); - query->bindValue(":before", boundary); - if (!execSqlQuery(query)) { - return false; - } - hasMore = query->next(); + if (page.needsWholeSecond) { + query = prepareQuery(selectMessages + conversationFilter + " AND log_time = FROM_UNIXTIME(:second)"); + bindParticipants(query); + query->bindValue(":second", page.boundarySecond); + if (!execSqlQuery(query)) { + return false; } + readMessages(query, page.messages); + std::reverse(page.messages.begin(), page.messages.end()); + + query = prepareQuery("SELECT 1" + conversationFilter + " AND log_time < FROM_UNIXTIME(:before) LIMIT 1"); + bindParticipants(query); + query->bindValue(":before", page.boundarySecond); + if (!execSqlQuery(query)) { + return false; + } + page.hasMore = query->next(); } - std::reverse(messages.begin(), messages.end()); + messages = page.messages; + hasMore = page.hasMore; return true; } diff --git a/servatrice/src/servatrice_database_interface.h b/servatrice/src/servatrice_database_interface.h index 548b3fbb2..13a581673 100644 --- a/servatrice/src/servatrice_database_interface.h +++ b/servatrice/src/servatrice_database_interface.h @@ -226,7 +226,9 @@ public: bool *hasMore = nullptr); QList getUserSessions(const QString &userName, int limit); bool getPrivateChatHistory(int callerId, + const QString &callerName, int peerId, + const QString &peerName, quint64 before, int limit, QList &messages, diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index d5dbb7266..49cc053b3 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -1735,7 +1735,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdGetPrivateChatHistory(c const int limit = qBound(1, cmd.limit(), 200); QList messages; bool hasMore = false; - if (!sqlInterface->getPrivateChatHistory(userInfo->id(), peerId, cmd.before(), limit, messages, hasMore)) { + if (!sqlInterface->getPrivateChatHistory(userInfo->id(), QString::fromStdString(userInfo->name()), peerId, peer, + cmd.before(), limit, messages, hasMore)) { return Response::RespInternalError; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ae90ee896..6eb59fc72 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -426,6 +426,19 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + private_chat_paging_test + SOURCES + ${CMAKE_SOURCE_DIR}/servatrice/src/private_chat_paging.cpp + private_chat_paging_test.cpp + LIBS + libcockatrice_protocol + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/servatrice/src + LABELS + server +) cockatrice_add_test( NAME latency_tracker_test diff --git a/tests/private_chat_paging_test.cpp b/tests/private_chat_paging_test.cpp new file mode 100644 index 000000000..4e5919517 --- /dev/null +++ b/tests/private_chat_paging_test.cpp @@ -0,0 +1,115 @@ +#include "private_chat_paging.h" + +#include + +namespace +{ +ServerInfo_PrivateChatMessage messageAt(quint64 time, const QString &text) +{ + ServerInfo_PrivateChatMessage message; + message.set_time(time); + message.set_sender_name("sender"); + message.set_message(text.toStdString()); + return message; +} + +QList newestFirst(std::initializer_list times) +{ + QList messages; + for (const quint64 time : times) { + messages.append(messageAt(time, QString::number(time))); + } + return messages; +} + +QList timesOf(const QList &messages) +{ + QList times; + for (const auto &message : messages) { + times.append(message.time()); + } + return times; +} +} // namespace + +TEST(PrivateChatPaging, EmptyInputProducesEmptyPage) +{ + const PrivateChatPage page = assemblePrivateChatPage({}, 50); + + EXPECT_TRUE(page.messages.isEmpty()); + EXPECT_FALSE(page.hasMore); + EXPECT_FALSE(page.needsWholeSecond); +} + +TEST(PrivateChatPaging, PageShorterThanLimitIsReturnedOldestFirst) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 50); + + EXPECT_EQ(timesOf(page.messages), QList({100, 200, 300})); + EXPECT_FALSE(page.hasMore); + EXPECT_FALSE(page.needsWholeSecond); +} + +TEST(PrivateChatPaging, PageExactlyAtLimitIsReturnedWithoutMore) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 3); + + EXPECT_EQ(timesOf(page.messages), QList({100, 200, 300})); + EXPECT_FALSE(page.hasMore); +} + +TEST(PrivateChatPaging, DistinctSecondsKeepWholePage) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 2); + + EXPECT_EQ(timesOf(page.messages), QList({200, 300})); + EXPECT_TRUE(page.hasMore); + EXPECT_FALSE(page.needsWholeSecond); +} + +TEST(PrivateChatPaging, BoundarySecondIsDeferredToNextPage) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 200}), 2); + + EXPECT_EQ(timesOf(page.messages), QList({300})); + EXPECT_TRUE(page.hasMore); + EXPECT_FALSE(page.needsWholeSecond); +} + +TEST(PrivateChatPaging, WholeSecondIsRequestedWhenNothingElseFits) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({200, 200, 200}), 2); + + EXPECT_TRUE(page.messages.isEmpty()); + EXPECT_TRUE(page.hasMore); + EXPECT_TRUE(page.needsWholeSecond); + EXPECT_EQ(page.boundarySecond, 200); +} + +TEST(PrivateChatPaging, WholeSecondIsRequestedForSingleMessageLimit) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({200, 200}), 1); + + EXPECT_TRUE(page.messages.isEmpty()); + EXPECT_TRUE(page.needsWholeSecond); + EXPECT_EQ(page.boundarySecond, 200); +} + +TEST(PrivateChatPaging, BoundarySecondAtPageEdgeIsDropped) +{ + const PrivateChatPage page = assemblePrivateChatPage(newestFirst({400, 400, 300, 300}), 3); + + EXPECT_EQ(timesOf(page.messages), QList({400, 400})); + EXPECT_TRUE(page.hasMore); + EXPECT_FALSE(page.needsWholeSecond); +} + +TEST(PrivateChatPaging, ReturnedMessagesKeepSenderAndText) +{ + const PrivateChatPage page = assemblePrivateChatPage({messageAt(200, "hello"), messageAt(100, "world")}, 50); + + ASSERT_EQ(page.messages.size(), 2); + EXPECT_EQ(page.messages.at(0).message(), "world"); + EXPECT_EQ(page.messages.at(1).message(), "hello"); + EXPECT_EQ(page.messages.at(0).sender_name(), "sender"); +}