From 1f1cdcadda70444917e9e77c1e0b9ee12ba8d511 Mon Sep 17 00:00:00 2001 From: Pawel Wasowski Date: Fri, 24 Aug 2018 18:12:09 +0200 Subject: [PATCH] [Messaging] Fix sorting and filtering issues in findConversations The commit fixes the following problems of email service's findConversations: - conversations sorted with regard to timestamp, subject, to, from attributes were ordered incorrectly - an attempt to sort results with regard to unreadMessages, isRead, cc, bcc, lastMessageId attributes resulted in throwing a JS exception - unreadMessages attribute value depended on the offset argument value - filtering by unreadMessages, did not work The commit changes also the method of creation of MessageConversation from database records. It may be quicker than the previous one, that unnecessarily retrieved numerous messages from the database several times. [Verification] TCT Pass rate: 100% Several filters and sort modes were tested manually, with Chrome DevTools. Change-Id: I6196e316ff2b1477c909d528859ef980237454b6 Signed-off-by: Pawel Wasowski --- src/messaging/MsgCommon/AbstractFilter.cpp | 12 +- src/messaging/email_manager.cc | 18 +- src/messaging/message_conversation.cc | 195 +++++++++++++++++++- src/messaging/message_conversation.h | 11 +- src/messaging/messaging_database_manager.cc | 180 ++++++++++-------- src/messaging/messaging_database_manager.h | 9 +- 6 files changed, 315 insertions(+), 110 deletions(-) diff --git a/src/messaging/MsgCommon/AbstractFilter.cpp b/src/messaging/MsgCommon/AbstractFilter.cpp index 3f30d4d1..1037576d 100644 --- a/src/messaging/MsgCommon/AbstractFilter.cpp +++ b/src/messaging/MsgCommon/AbstractFilter.cpp @@ -150,26 +150,20 @@ bool FilterUtils::isTimeStampInRange(const time_t& time_stamp, tizen::AnyPtr& in tizen::AnyPtr& end_value) { ScopeLogger(); time_t from_time = 0; - time_t to_time = 0; - bool initial_is_valid_time_value = false; if (initial_value && !initial_value->isNullOrUndefined()) { struct tm ftime = *initial_value->toDateTm(); from_time = mktime(&ftime); - initial_is_valid_time_value = true; - } - if (!initial_is_valid_time_value) { + } else { LoggerE("initialValue is not Time!"); return false; } - bool end_is_valid_time_value = false; + time_t to_time = 0; if (end_value && !end_value->isNullOrUndefined()) { struct tm ttime = *end_value->toDateTm(); to_time = mktime(&ttime); - end_is_valid_time_value = true; - } - if (end_is_valid_time_value) { + } else { LoggerE("endValue is not Time!"); return false; } diff --git a/src/messaging/email_manager.cc b/src/messaging/email_manager.cc index 256fbb35..85608c12 100644 --- a/src/messaging/email_manager.cc +++ b/src/messaging/email_manager.cc @@ -1072,26 +1072,16 @@ void EmailManager::findMessages(FindMsgCallbackUserData* callback) { PlatformResult EmailManager::FindConversationsPlatform(ConversationCallbackData* callback) { ScopeLogger(); - int convListCount = 0; std::lock_guard lock(m_mutex); - std::vector conversationsInfo; + std::vector> conversationsInfo; PlatformResult ret = - MessagingDatabaseManager::getInstance().findEmailConversations(callback, &conversationsInfo); + MessagingDatabaseManager::getInstance().findEmailConversations(*callback, &conversationsInfo); if (ret.IsError()) return ret; - convListCount = conversationsInfo.size(); - LoggerD("Found %d conversations", convListCount); + LoggerD("Found %zu conversations", conversationsInfo.size()); - for (int i = 0; i < convListCount; ++i) { - std::shared_ptr conversation; - PlatformResult ret = MessageConversation::convertEmailConversationToObject( - conversationsInfo.at(i).id, &conversation); - if (ret.IsError()) { - LoggerE("%d (%s)", ret.error_code(), (ret.message()).c_str()); - return ret; - } - conversation->setUnreadMessages(conversationsInfo.at(i).unreadMessages); + for (const auto& conversation : conversationsInfo) { callback->addConversation(conversation); } diff --git a/src/messaging/message_conversation.cc b/src/messaging/message_conversation.cc index 5d1ea2cd..d320a3a8 100644 --- a/src/messaging/message_conversation.cc +++ b/src/messaging/message_conversation.cc @@ -43,6 +43,85 @@ MessageConversation::MessageConversation() ScopeLogger("Message Conversation constructor."); } +namespace { + +std::string SanitizeUtf8String(const std::string& input) { + ScopeLogger(); + + std::string result = input; + const gchar* end = nullptr; + + while (FALSE == g_utf8_validate(result.c_str(), -1, &end)) { + result = result.substr(0, end - result.c_str()); + } + + return result; +} + +std::vector SanitizeUtf8StringVector(const std::vector& string_vector) { + ScopeLogger(); + + std::vector sanitized_strings; + for (const auto& string : string_vector) { + sanitized_strings.push_back(SanitizeUtf8String(string)); + } + + return sanitized_strings; +} + +std::vector GetSanitizedEmailRecipientsFromCString(const char* recipients_c_string) { + ScopeLogger(); + + if (!recipients_c_string) { + return {}; + } + + return SanitizeUtf8StringVector(Message::getEmailRecipientsFromStruct(recipients_c_string)); +} + +std::string GetSanitizedStringFromCString(const char* c_string) { + ScopeLogger(); + + if (!c_string) { + return ""; + } + + return SanitizeUtf8String(c_string); +} + +std::string GetSingleSanitizedEmailAddressFromCString(const char* address_c_string) { + ScopeLogger(); + + if (!address_c_string) { + return ""; + } + + return SanitizeUtf8String(MessagingUtil::extractSingleEmailAddress(address_c_string)); +} + +} // namespace + +MessageConversation::MessageConversation(const email_mail_data_t& mail_data) + : m_conversation_id(mail_data.thread_id), + m_conversation_type(MessageType::EMAIL), + m_timestamp(mail_data.date_time), + m_count(mail_data.thread_item_count), + m_unread_messages(0), + m_preview(GetSanitizedStringFromCString(mail_data.preview_text)), + m_conversation_subject(GetSanitizedStringFromCString(mail_data.subject)), + m_is_read(mail_data.flags_seen_field), + m_from(GetSingleSanitizedEmailAddressFromCString(mail_data.full_address_from)), + m_to(GetSanitizedEmailRecipientsFromCString(mail_data.full_address_to)), + m_cc(GetSanitizedEmailRecipientsFromCString(mail_data.full_address_cc)), + m_bcc(GetSanitizedEmailRecipientsFromCString(mail_data.full_address_bcc)), + m_last_message_id(mail_data.mail_id) { + ScopeLogger(); + auto result = initializeUnreadMessagesCount(); + if (result.IsError()) { + LoggerE("Could not initialize unreadMessages. conversation id: %d", m_conversation_id); + } +} + MessageConversation::~MessageConversation() { ScopeLogger("Message Conversation destructor."); } @@ -327,19 +406,19 @@ PlatformResult MessageConversation::convertEmailConversationToObject( if (resultMail->full_address_from[0] != '\0') { conversation->m_from = - MessagingUtil::extractSingleEmailAddress(resultMail->full_address_from); + GetSingleSanitizedEmailAddressFromCString(resultMail->full_address_from); } if (mailData->full_address_to != NULL) { - conversation->m_to = Message::getEmailRecipientsFromStruct(mailData->full_address_to); + conversation->m_to = GetSanitizedEmailRecipientsFromCString(mailData->full_address_to); } if (mailData->full_address_cc != NULL) { - conversation->m_cc = Message::getEmailRecipientsFromStruct(mailData->full_address_cc); + conversation->m_cc = GetSanitizedEmailRecipientsFromCString(mailData->full_address_cc); } if (mailData->full_address_bcc != NULL) { - conversation->m_bcc = Message::getEmailRecipientsFromStruct(mailData->full_address_bcc); + conversation->m_bcc = GetSanitizedEmailRecipientsFromCString(mailData->full_address_bcc); } conversation->m_last_message_id = resultMail->mail_id; @@ -485,17 +564,113 @@ bool MessageConversation::isMatchingAttributeRange(const std::string& attribute_ return false; } -std::string MessageConversation::SanitizeUtf8String(const std::string& input) { +common::PlatformResult MessageConversation::initializeUnreadMessagesCount() { ScopeLogger(); - std::string result = input; - const gchar* end = nullptr; + email_list_filter_t filter_list = {}; + memset(&filter_list, 0, sizeof(email_list_filter_t)); - while (FALSE == g_utf8_validate(result.c_str(), -1, &end)) { - result = result.substr(0, end - result.c_str()); + filter_list.list_filter_item_type = EMAIL_LIST_FILTER_ITEM_RULE; + filter_list.list_filter_item.rule.rule_type = EMAIL_LIST_FILTER_RULE_EQUAL; + filter_list.list_filter_item.rule.target_attribute = EMAIL_MAIL_ATTRIBUTE_THREAD_ID; + filter_list.list_filter_item.rule.key_value.integer_type_value = m_conversation_id; + + int total_messages = 0; + int unread_messages = 0; + auto result = email_count_mail(&filter_list, 1, &total_messages, &unread_messages); + if (EMAIL_ERROR_NONE != result) { + return LogAndCreateResult( + ErrorCode::UNKNOWN_ERR, "Error while getting data from database.", + ("email_count_mail error: %d (%s)", result, get_error_message(result))); } - return result; + m_unread_messages = unread_messages; + + return PlatformResult{ErrorCode::NO_ERROR}; +} + +const MessageConversation::MessageConversationComparatorMap + MessageConversation::m_message_conversation_comparators = { + { + "id", [] (const ConversationPtr& a, const ConversationPtr& b) { + return std::to_string(a->m_conversation_id) < std::to_string(b->m_conversation_id); + } + }, + { + "timestamp", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_timestamp < b->m_timestamp; + } + }, + { + "messageCount", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_count < b->m_count; + } + }, + { + "unreadMessages", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_unread_messages < b->m_unread_messages; + } + }, + { + "preview", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_preview < b->m_preview; + } + }, + { + "subject", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_conversation_subject < b->m_conversation_subject; + } + }, + { + "isRead", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_is_read < b->m_is_read; + } + }, + { + "from", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_from < b->m_from; + } + }, + { + "to", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_to < b->m_to; + } + }, + { + "cc", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_cc < b->m_cc; + } + }, + { + "bcc", [] (const ConversationPtr& a, const ConversationPtr& b) { + return a->m_bcc < b->m_bcc; + } + }, + { + "lastMessageId", [] (const ConversationPtr& a, const ConversationPtr& b) { + return std::to_string(a->m_last_message_id) < std::to_string(b->m_last_message_id); + } + }, + { + /* Every record has EMAIL type - no element is "greater than" any other. */ + "type", [] (const ConversationPtr& a, const ConversationPtr& b) { + return false; + } + } +}; + +MessageConversation::MessageConversationComparator MessageConversation::getComparator( + const std::string& attribute) { + auto comparator_it = m_message_conversation_comparators.find(attribute); + if (std::end(m_message_conversation_comparators) != comparator_it) { + return comparator_it->second; + } + + auto default_comparator = [](const ConversationPtr& a, const ConversationPtr& b) { + return false; + }; + + return default_comparator; } } // messaging diff --git a/src/messaging/message_conversation.h b/src/messaging/message_conversation.h index 1d01ff5e..273f526c 100644 --- a/src/messaging/message_conversation.h +++ b/src/messaging/message_conversation.h @@ -25,6 +25,7 @@ #include #include #include "MsgCommon/AbstractFilter.h" +#include "common/platform_result.h" #include "messaging_util.h" namespace extension { @@ -43,6 +44,7 @@ typedef std::vector ConversationPtrVector; class MessageConversation : public tizen::FilterableObject { public: MessageConversation(); + MessageConversation(const email_mail_data_t& mail_data); ~MessageConversation(); // attributes getters @@ -94,8 +96,12 @@ class MessageConversation : public tizen::FilterableObject { virtual bool isMatchingAttributeRange(const std::string& attribute_name, tizen::AnyPtr initial_value, tizen::AnyPtr end_value) const; + using MessageConversationComparator = + std::function; + static MessageConversationComparator getComparator(const std::string& attribute); + private: - std::string SanitizeUtf8String(const std::string& input); + common::PlatformResult initializeUnreadMessagesCount(); int m_conversation_id; MessageType m_conversation_type; @@ -110,6 +116,9 @@ class MessageConversation : public tizen::FilterableObject { std::vector m_cc; std::vector m_bcc; int m_last_message_id; + + using MessageConversationComparatorMap = std::map; + static const MessageConversationComparatorMap m_message_conversation_comparators; }; } // messaging diff --git a/src/messaging/messaging_database_manager.cc b/src/messaging/messaging_database_manager.cc index 4e4edffc..03d190da 100644 --- a/src/messaging/messaging_database_manager.cc +++ b/src/messaging/messaging_database_manager.cc @@ -28,8 +28,10 @@ #include "common/logger.h" #include "common/platform_exception.h" +#include "common/scope_exit.h" #include "conversation_callback_data.h" +#include "message_conversation.h" #include "messaging_database_manager.h" #include "messaging_manager.h" @@ -143,31 +145,6 @@ MessagingDatabaseManager::MessagingDatabaseManager() { std::make_pair("msgId", AttributeInfo("B.MSG_ID", INTEGER, PrimitiveType_String))); m_msg_conv_attr_map.insert( std::make_pair("direction", AttributeInfo("B.MSG_DIRECTION", INTEGER, PrimitiveType_String))); - - // Attributes map for email conversations ===================================== - m_email_conv_attr_map.insert( - std::make_pair("id", AttributeInfo("thread_id", INTEGER, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("serviceId", AttributeInfo("account_id", INTEGER, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("type", AttributeInfo("account_id", INTEGER, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("timestamp", AttributeInfo("date_time", DATETIME, PrimitiveType_Time))); - m_email_conv_attr_map.insert(std::make_pair( - "messageCount", AttributeInfo("thread_item_count", INTEGER, PrimitiveType_ULong))); - m_email_conv_attr_map.insert(std::make_pair( - "unreadMessages", AttributeInfo(std::string("thread_id IN (SELECT thread_id ") + - std::string("FROM mail_tbl WHERE flags_seen_field = 0 ") + - std::string("GROUP BY thread_id HAVING COUNT(thread_id)"), - INTEGER, PrimitiveType_ULong))); - m_email_conv_attr_map.insert( - std::make_pair("preview", AttributeInfo("preview_text", TEXT, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("subject", AttributeInfo("subject", TEXT, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("from", AttributeInfo("full_address_from", TEXT, PrimitiveType_String))); - m_email_conv_attr_map.insert( - std::make_pair("to", AttributeInfo("full_address_to", TEXT, PrimitiveType_String))); } MessagingDatabaseManager::~MessagingDatabaseManager() { @@ -891,69 +868,126 @@ PlatformResult MessagingDatabaseManager::findShortMessageConversations( return PlatformResult(ErrorCode::NO_ERROR); } -PlatformResult MessagingDatabaseManager::findEmailConversations( - ConversationCallbackData* callback, std::vector* result) { +namespace { + +void convertToMessageConversations(const email_mail_data_t* mailData, int mailDataCount, + ConversationPtrVector* conversations) { ScopeLogger(); - std::ostringstream sqlWhereClause; - int resultsCount; - email_mail_data_t* results; - std::map conversationsBag; - std::vector conversationsInfo; - // Adding filters query - AbstractFilterPtr filter = callback->getFilter(); - SortModePtr sortMode = callback->getSortMode(); - long limit = callback->getLimit(); - long offset = callback->getOffset(); - MessageType msgType = callback->getMessageServiceType(); - int accountId = callback->getAccountId(); + conversations->reserve(static_cast(mailDataCount)); + for (int i = 0; i < mailDataCount; ++i) { + conversations->push_back(std::make_shared(mailData[i])); + } +} - std::string filters_query; - PlatformResult ret = - addFilters(filter, sortMode, limit, offset, m_email_conv_attr_map, msgType, &filters_query); - if (ret.IsError()) { - LoggerE("Add filters failed (%s)", ret.message().c_str()); - return ret; +void filterMessageConversations(const AbstractFilter& filter, + ConversationPtrVector* conversations) { + ScopeLogger(); + + auto toBeFilteredOut = [&filter](const ConversationPtr& conversation) { + return !filter.isMatching(conversation.get()); + }; + + conversations->erase( + std::remove_if(std::begin(*conversations), std::end(*conversations), toBeFilteredOut), + std::end(*conversations)); +} + +void sortConversations(const SortMode& sortMode, ConversationPtrVector* conversations) { + ScopeLogger(); + + const std::string attribute = sortMode.getAttributeName(); + auto comparator = MessageConversation::getComparator(attribute); + + if (SortModeOrder::ASC == sortMode.getOrder()) { + std::sort(conversations->begin(), conversations->end(), comparator); + } else { + std::sort(conversations->rbegin(), conversations->rend(), comparator); } - sqlWhereClause << "WHERE " << m_email_conv_attr_map["serviceId"].sql_name << " = " << accountId - << " AND " << filters_query; - LoggerD("%s", sqlWhereClause.str().c_str()); +} - // Getting results from database - msg_error_t err = - email_query_mails(const_cast(sqlWhereClause.str().c_str()), &results, &resultsCount); - if (EMAIL_ERROR_NONE != err) { - LoggerE("Getting mail list fail [%d]", err); +} // namespace - if (EMAIL_ERROR_MAIL_NOT_FOUND == err) { - resultsCount = 0; +PlatformResult MessagingDatabaseManager::retrieveEmailThreadsFromDatabase( + const ConversationCallbackData& findEmailConversationsData, + ConversationPtrVector* conversations) { + ScopeLogger(); + + std::string limitAndOffset; + PlatformResult result = PlatformResult{ErrorCode::UNKNOWN_ERR}; + AttributeInfoMap emptyAttributeMap; + /* + * Sorting and filtering is done programmatically, after data retrieval from the database, + * because some parameters cannot be used in ORDER BY/WHERE or both clauses, + * passed to the email_query_mails() - e.g. there is no "unreadMessages" column in the. + * + * For that reason both filter and sort mode arguments of addFilters are null pointers. + */ + result = addFilters(AbstractFilterPtr{nullptr}, SortModePtr{nullptr}, + findEmailConversationsData.getLimit(), findEmailConversationsData.getOffset(), + emptyAttributeMap, findEmailConversationsData.getMessageServiceType(), + &limitAndOffset); + + if (result.IsError()) { + LoggerE("addFilters failed (%s)", result.message().c_str()); + return result; + } + + const std::string isLastMessageInThread = + "(MAIL_ID IN " + "(SELECT MAIL_ID " + "FROM " + "(SELECT MAIL_ID, MAX(DATE_TIME) " + "FROM MAIL_TBL " + "GROUP BY THREAD_ID)))"; + + const std::string isServiceAccountId = + "account_id = " + std::to_string(findEmailConversationsData.getAccountId()); + std::string sqlWhereClause = + "WHERE " + isLastMessageInThread + " AND " + isServiceAccountId + " " + limitAndOffset; + + LoggerD("%s", sqlWhereClause.c_str()); + + int threadsCount = 0; + email_mail_data_t* threadsList = nullptr; + + SCOPE_EXIT { + if (threadsList) { + email_free_mail_data(&threadsList, threadsCount); + } + }; + + msg_error_t errorCode = email_query_mails(&sqlWhereClause[0], &threadsList, &threadsCount); + if (EMAIL_ERROR_NONE != errorCode) { + if (EMAIL_ERROR_MAIL_NOT_FOUND == errorCode) { + threadsCount = 0; } else { - return LogAndCreateResult(ErrorCode::UNKNOWN_ERR, "Error while getting data from database.", - ("email_query_mails error: %d (%s)", err, get_error_message(err))); + return LogAndCreateResult( + ErrorCode::UNKNOWN_ERR, "Error while getting data from database.", + ("email_query_mails error: %d (%s)", errorCode, get_error_message(errorCode))); } } - // Assigning found emails to conversation - for (int i = 0; i < resultsCount; ++i) { - if (conversationsBag.find(results[i].thread_id) == conversationsBag.end()) { - EmailConversationInfo info; - info.id = results[i].thread_id; - conversationsInfo.push_back(info); - conversationsBag.insert(std::make_pair(results[i].thread_id, 0)); - } + convertToMessageConversations(threadsList, threadsCount, conversations); + return PlatformResult{ErrorCode::NO_ERROR}; +} - if (!(static_cast(results[i].flags_seen_field))) { - ++conversationsBag[results[i].thread_id]; - } +PlatformResult MessagingDatabaseManager::findEmailConversations( + const ConversationCallbackData& findEmailConversationsData, + ConversationPtrVector* conversations) { + ScopeLogger(); + + auto result = retrieveEmailThreadsFromDatabase(findEmailConversationsData, conversations); + if (result.IsError()) { + return result; } - for (std::vector::iterator it = conversationsInfo.begin(); - it != conversationsInfo.end(); ++it) { - (*it).unreadMessages = conversationsBag[(*it).id]; + filterMessageConversations(*findEmailConversationsData.getFilter(), conversations); + + if (findEmailConversationsData.getSortMode()) { + sortConversations(*findEmailConversationsData.getSortMode(), conversations); } - email_free_mail_data(&results, resultsCount); - *result = conversationsInfo; return PlatformResult(ErrorCode::NO_ERROR); } diff --git a/src/messaging/messaging_database_manager.h b/src/messaging/messaging_database_manager.h index a2414925..c10c2ff3 100644 --- a/src/messaging/messaging_database_manager.h +++ b/src/messaging/messaging_database_manager.h @@ -26,6 +26,7 @@ #include "common/platform_result.h" #include "find_msg_callback_user_data.h" +#include "message_conversation.h" //#include "ConversationCallbackData.h" namespace extension { @@ -82,8 +83,8 @@ class MessagingDatabaseManager { std::pair* result); common::PlatformResult findShortMessageConversations(ConversationCallbackData* callback, std::vector* result); - common::PlatformResult findEmailConversations(ConversationCallbackData* callback, - std::vector* result); + common::PlatformResult findEmailConversations(const ConversationCallbackData& callback, + ConversationPtrVector* conversations); private: MessagingDatabaseManager(); @@ -109,12 +110,14 @@ class MessagingDatabaseManager { common::PlatformResult addFilters(tizen::AbstractFilterPtr filter, tizen::SortModePtr sortMode, long limit, long offset, AttributeInfoMap& attributeMap, MessageType msgType, std::string* result); + common::PlatformResult retrieveEmailThreadsFromDatabase( + const ConversationCallbackData& findEmailConversationsData, + ConversationPtrVector* conversations); AttributeInfoMap m_msg_attr_map; AttributeInfoMap m_email_attr_map; AttributeInfoMap m_msg_conv_attr_map; - AttributeInfoMap m_email_conv_attr_map; }; } // Messaging -- 2.34.1