api!: Remove msg_id (last message ID) from Chatlist::get_summary2() (#3071)

`Chatlist::get_summary2()` is used in the jsonrpc API. CFFI uses `Chatlist::get_summary()` which
logic is preserved (it uses message ids cached in the `Chatlist`).

The motivation for this change is that jsonrpc API uses `get_last_message_for_chat()` returning the
message id + `Chatlist::get_summary2()` taking the message id when building a chatlist, but if the
message is ephemeral, the id can expire and building the chatlist would fail. The solution is to
return a summary for the last message (if any) in the chat from `Chatlist::get_summary2()` so the
call to `get_last_message_for_chat()` goes away (and two SQL queries are just merged into one) and
overall the API is easier to use.

Also this change extends `struct Summary` with the message viewtype (so an extra call to
`Message::load_from_db()` goes away) and the message id in case if it's needed for any purposes
beyond building the chatlist.

deltachat-jsonrpc/src/api/types/chat_list.rs

@@ -1,12 +1,9 @@
 use anyhow::{Context, Result};
+use deltachat::chat::{get_chat_contacts, ChatVisibility};
 use deltachat::chat::{Chat, ChatId};
-use deltachat::chatlist::get_last_message_for_chat;
+use deltachat::chatlist::Chatlist;
 use deltachat::constants::*;
 use deltachat::contact::{Contact, ContactId};
-use deltachat::{
-    chat::{get_chat_contacts, ChatVisibility},
-    chatlist::Chatlist,
-};
 use num_traits::cast::ToPrimitive;
 use serde::Serialize;
 use typescript_type_def::TypeDef;
@@ -67,10 +64,8 @@ pub(crate) async fn get_chat_list_item_by_id(
         });
     }
 
-    let last_msgid = get_last_message_for_chat(ctx, chat_id).await?;
-
     let chat = Chat::load_from_db(ctx, chat_id).await.context("chat")?;
-    let summary = Chatlist::get_summary2(ctx, chat_id, last_msgid, Some(&chat))
+    let summary = Chatlist::get_summary2(ctx, chat_id, Some(&chat))
         .await
         .context("summary")?;
 
@@ -86,15 +81,12 @@ pub(crate) async fn get_chat_list_item_by_id(
         .await?
         .map(|path| path.to_str().unwrap_or("invalid/path").to_owned());
 
-    let (last_updated, message_type) = match last_msgid {
+    let (last_updated, message_type) = match summary.id {
-        Some(id) => {
-            let last_message = deltachat::message::Message::load_from_db(ctx, id).await?;
-            (
-                Some(last_message.get_timestamp() * 1000),
-                Some(last_message.get_viewtype().into()),
-            )
-        }
         None => (None, None),
+        Some(_) => (
+            Some(summary.timestamp * 1000),
+            Some(summary.viewtype.into()),
+        ),
     };
 
     let chat_contacts = get_chat_contacts(ctx, chat_id).await?;
@@ -145,6 +137,6 @@ pub(crate) async fn get_chat_list_item_by_id(
         dm_chat_contact,
         was_seen_recently,
         last_message_type: message_type,
-        last_message_id: last_msgid.map(|id| id.to_u32()),
+        last_message_id: summary.id.map(|id| id.to_u32()),
     })
 }

src/chat.rs

@@ -39,6 +39,7 @@ use crate::receive_imf::ReceivedMsg;
 use crate::smtp::send_msg_to_smtp;
 use crate::sql;
 use crate::stock_str;
+use crate::summary::Summary;
 use crate::sync::{self, Sync::*, SyncData};
 use crate::tools::{
     buf_compress, create_id, create_outgoing_rfc724_mid, create_smeared_timestamp,
@@ -1329,6 +1330,52 @@ impl ChatId {
         Ok(())
     }
 
+    /// Returns a summary for a given chat.
+    pub async fn get_summary(
+        self,
+        context: &Context,
+        chat: Option<&Chat>,
+        msg: Option<&Message>,
+    ) -> Result<Summary> {
+        let chat_loaded: Chat;
+        let chat = if let Some(chat) = chat {
+            chat
+        } else {
+            let chat = Chat::load_from_db(context, self).await?;
+            chat_loaded = chat;
+            &chat_loaded
+        };
+
+        let lastcontact = if let Some(msg) = msg {
+            if msg.from_id == ContactId::SELF {
+                None
+            } else {
+                match chat.typ {
+                    Chattype::Group | Chattype::Broadcast | Chattype::Mailinglist => {
+                        let lastcontact = Contact::get_by_id(context, msg.from_id)
+                            .await
+                            .context("Loading contact failed")?;
+                        Some(lastcontact)
+                    }
+                    Chattype::Single => None,
+                }
+            }
+        } else {
+            None
+        };
+
+        if chat.id.is_archived_link() {
+            Ok(Default::default())
+        } else if let Some(msg) = msg.filter(|msg| msg.from_id != ContactId::UNDEFINED) {
+            Summary::new(context, msg, chat, lastcontact.as_ref()).await
+        } else {
+            Ok(Summary {
+                text: stock_str::no_messages(context).await,
+                ..Default::default()
+            })
+        }
+    }
+
     /// Returns true if the chat is protected.
     pub async fn is_protected(self, context: &Context) -> Result<ProtectionStatus> {
         let protection_status = context

src/chatlist.rs

@@ -8,11 +8,10 @@ use crate::constants::{
     Blocked, Chattype, DC_CHAT_ID_ALLDONE_HINT, DC_CHAT_ID_ARCHIVED_LINK, DC_GCL_ADD_ALLDONE_HINT,
     DC_GCL_ARCHIVED_ONLY, DC_GCL_FOR_FORWARDING, DC_GCL_NO_SPECIALS,
 };
-use crate::contact::{Contact, ContactId};
+use crate::contact::ContactId;
 use crate::context::Context;
 use crate::message::{Message, MessageState, MsgId};
 use crate::param::{Param, Params};
-use crate::stock_str;
 use crate::summary::Summary;
 use crate::tools::IsNoneOrEmpty;
 
@@ -22,7 +21,8 @@ pub static IS_UNREAD_FILTER: Lazy<regex::Regex> =
 
 /// An object representing a single chatlist in memory.
 ///
-/// Chatlist objects contain chat IDs and, if possible, message IDs belonging to them.
+/// Chatlist objects contain IDs of chats and, optionally, their last messages, recently updated
+/// come first.
 /// The chatlist object is not updated; if you want an update, you have to recreate the object.
 ///
 /// For a **typical chat overview**, the idea is to get the list of all chats via dc_get_chatlist()
@@ -347,9 +347,11 @@ impl Chatlist {
         Ok(*chat_id)
     }
 
-    /// Get a single message ID of a chatlist.
+    /// Get the last message ID of a chat with index `index`.
     ///
-    /// To get the message object from the message ID, use dc_get_msg().
+    /// If you want to obtain the [Message] object further, it's not recommended to use as the
+    /// message may expire (if it's ephemeral f.e.). Better use [`Self::get_chat_id()`] +
+    /// [`Message::load_from_db_last_for_chat()`].
     pub fn get_msg_id(&self, index: usize) -> Result<Option<MsgId>> {
         let (_chat_id, msg_id) = self
             .ids
@@ -373,56 +375,28 @@ impl Chatlist {
             .ids
             .get(index)
             .context("chatlist index is out of range")?;
-        Chatlist::get_summary2(context, *chat_id, *lastmsg_id, chat).await
+        let lastmsg = if let Some(lastmsg_id) = lastmsg_id {
+            Some(
+                Message::load_from_db(context, *lastmsg_id)
+                    .await
+                    .with_context(|| format!("Loading message {lastmsg_id} failed"))?,
+            )
+        } else {
+            None
+        };
+        chat_id.get_summary(context, chat, lastmsg.as_ref()).await
     }
 
-    /// Returns a summary for a given chatlist item.
+    /// Returns a summary for a given chat.
     pub async fn get_summary2(
         context: &Context,
         chat_id: ChatId,
-        lastmsg_id: Option<MsgId>,
         chat: Option<&Chat>,
     ) -> Result<Summary> {
-        let chat_loaded: Chat;
+        let lastmsg = Message::load_from_db_last_for_chat(context, chat_id)
-        let chat = if let Some(chat) = chat {
-            chat
-        } else {
-            let chat = Chat::load_from_db(context, chat_id).await?;
-            chat_loaded = chat;
-            &chat_loaded
-        };
-
-        let (lastmsg, lastcontact) = if let Some(lastmsg_id) = lastmsg_id {
-            let lastmsg = Message::load_from_db(context, lastmsg_id)
             .await
-                .context("loading message failed")?;
+            .context("Loading message failed")?;
-            if lastmsg.from_id == ContactId::SELF {
+        chat_id.get_summary(context, chat, lastmsg.as_ref()).await
-                (Some(lastmsg), None)
-            } else {
-                match chat.typ {
-                    Chattype::Group | Chattype::Broadcast | Chattype::Mailinglist => {
-                        let lastcontact = Contact::get_by_id(context, lastmsg.from_id)
-                            .await
-                            .context("loading contact failed")?;
-                        (Some(lastmsg), Some(lastcontact))
-                    }
-                    Chattype::Single => (Some(lastmsg), None),
-                }
-            }
-        } else {
-            (None, None)
-        };
-
-        if chat.id.is_archived_link() {
-            Ok(Default::default())
-        } else if let Some(lastmsg) = lastmsg.filter(|msg| msg.from_id != ContactId::UNDEFINED) {
-            Summary::new(context, &lastmsg, chat, lastcontact.as_ref()).await
-        } else {
-            Ok(Summary {
-                text: stock_str::no_messages(context).await,
-                ..Default::default()
-            })
-        }
     }
 
     /// Returns chatlist item position for the given chat ID.
@@ -431,6 +405,9 @@ impl Chatlist {
     }
 
     /// An iterator visiting all chatlist items.
+    ///
+    /// See [`Self::get_msg_id()`] why it's not recommended to use returned [MsgId]-s to obtain
+    /// [Message] objects and what to use instead.
     pub fn iter(&self) -> impl Iterator<Item = &(ChatId, Option<MsgId>)> {
         self.ids.iter()
     }
@@ -448,8 +425,10 @@ pub async fn get_archived_cnt(context: &Context) -> Result<usize> {
     Ok(count)
 }
 
-/// Gets the last message of a chat, the message that would also be displayed in the ChatList
+/// Gets the id of the last user-visible message of a chat.
-/// Used for passing to `deltachat::chatlist::Chatlist::get_summary2`
+///
+/// See [`Chatlist::get_msg_id()`] why it's not recommended to use returned [MsgId] to obtain the
+/// [Message] object and what to use instead.
 pub async fn get_last_message_for_chat(
     context: &Context,
     chat_id: ChatId,
@@ -474,7 +453,8 @@ mod tests {
         add_contact_to_chat, create_group_chat, get_chat_contacts, remove_contact_from_chat,
         send_text_msg, ProtectionStatus,
     };
-    use crate::message::Viewtype;
+    use crate::contact::Contact;
+    use crate::message::{Message, Viewtype};
     use crate::receive_imf::receive_imf;
     use crate::stock_str::StockMessage;
     use crate::test_utils::TestContext;

src/message.rs

@@ -480,53 +480,83 @@ impl Message {
         let msg = context
             .sql
             .query_row_optional(
-                concat!(
+                &Self::select_query_with_filter("WHERE m.id=?"),
-                    "SELECT",
-                    "    m.id AS id,",
-                    "    rfc724_mid AS rfc724mid,",
-                    "    m.mime_in_reply_to AS mime_in_reply_to,",
-                    "    m.chat_id AS chat_id,",
-                    "    m.from_id AS from_id,",
-                    "    m.to_id AS to_id,",
-                    "    m.timestamp AS timestamp,",
-                    "    m.timestamp_sent AS timestamp_sent,",
-                    "    m.timestamp_rcvd AS timestamp_rcvd,",
-                    "    m.ephemeral_timer AS ephemeral_timer,",
-                    "    m.ephemeral_timestamp AS ephemeral_timestamp,",
-                    "    m.type AS type,",
-                    "    m.state AS state,",
-                    "    m.download_state AS download_state,",
-                    "    m.error AS error,",
-                    "    m.msgrmsg AS msgrmsg,",
-                    "    m.mime_modified AS mime_modified,",
-                    "    m.txt AS txt,",
-                    "    m.subject AS subject,",
-                    "    m.param AS param,",
-                    "    m.hidden AS hidden,",
-                    "    m.location_id AS location,",
-                    "    c.blocked AS blocked",
-                    " FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id",
-                    " WHERE m.id=?;"
-                ),
                 (id,),
-                |row| {
+                |row| Self::from_row(context, row),
+            )
+            .await
+            .with_context(|| format!("failed to load message {id} from the database"))?;
+
+        Ok(msg)
+    }
+
+    /// Loads the last user-visible message of the chat with given `id` from the database.
+    pub async fn load_from_db_last_for_chat(
+        context: &Context,
+        chat_id: ChatId,
+    ) -> Result<Option<Message>> {
+        let msg = context
+            .sql
+            .query_row_optional(
+                &Self::select_query_with_filter(concat!(
+                    "WHERE m.chat_id=? ",
+                    "AND (m.hidden=0 OR m.state=?) ",
+                    "ORDER BY m.timestamp DESC, m.id DESC LIMIT 1",
+                )),
+                (chat_id, MessageState::OutDraft),
+                |row| Self::from_row(context, row),
+            )
+            .await
+            .with_context(|| {
+                format!("Failed to load last message for chat {chat_id} from the database")
+            })?;
+
+        Ok(msg)
+    }
+
+    fn select_query_with_filter(filter: &str) -> String {
+        "\
+        SELECT \
+        m.id AS id, \
+        rfc724_mid AS rfc724mid, \
+        m.mime_in_reply_to AS mime_in_reply_to, \
+        m.chat_id AS chat_id, \
+        m.from_id AS from_id, \
+        m.to_id AS to_id, \
+        m.timestamp AS timestamp, \
+        m.timestamp_sent AS timestamp_sent, \
+        m.timestamp_rcvd AS timestamp_rcvd, \
+        m.ephemeral_timer AS ephemeral_timer, \
+        m.ephemeral_timestamp AS ephemeral_timestamp, \
+        m.type AS type, \
+        m.state AS state, \
+        m.download_state AS download_state, \
+        m.error AS error, \
+        m.msgrmsg AS msgrmsg, \
+        m.mime_modified AS mime_modified, \
+        m.txt AS txt, \
+        m.subject AS subject, \
+        m.param AS param, \
+        m.hidden AS hidden, \
+        m.location_id AS location, \
+        c.blocked AS blocked \
+        FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id "
+            .to_string()
+            + filter
+    }
+
+    fn from_row(context: &Context, row: &rusqlite::Row) -> rusqlite::Result<Message> {
         let text = match row.get_ref("txt")? {
-                        rusqlite::types::ValueRef::Text(buf) => {
+            rusqlite::types::ValueRef::Text(buf) => match String::from_utf8(buf.to_vec()) {
-                            match String::from_utf8(buf.to_vec()) {
                 Ok(t) => t,
                 Err(_) => {
                     warn!(
                         context,
-                                        concat!(
+                        "Message::from_row: could not get text column as a non-lossy UTF-8",
-                                            "dc_msg_load_from_db: could not get ",
-                                            "text column as non-lossy utf8 id {}"
-                                        ),
-                                        id
                     );
                     String::from_utf8_lossy(buf).into_owned()
                 }
-                            }
+            },
-                        }
             _ => String::new(),
         };
         let msg = Message {
@@ -546,8 +576,7 @@ impl Message {
             viewtype: row.get("type")?,
             state: row.get("state")?,
             download_state: row.get("download_state")?,
-                        error: Some(row.get::<_, String>("error")?)
+            error: Some(row.get::<_, String>("error")?).filter(|error| !error.is_empty()),
-                            .filter(|error| !error.is_empty()),
             is_dc_message: row.get("msgrmsg")?,
             mime_modified: row.get("mime_modified")?,
             text,
@@ -560,12 +589,6 @@ impl Message {
                 .unwrap_or_default(),
         };
         Ok(msg)
-                },
-            )
-            .await
-            .with_context(|| format!("failed to load message {id} from the database"))?;
-
-        Ok(msg)
     }
 
     /// Returns the MIME type of an attached file if it exists.

src/mimeparser.rs

@@ -3447,8 +3447,11 @@ On 2020-10-25, Bob wrote:
         .unwrap();
 
         let chats = Chatlist::try_load(&t.ctx, 0, None, None).await.unwrap();
-        let msg_id = chats.get_msg_id(0).unwrap().unwrap();
+        let chat_id = chats.get_chat_id(0).unwrap();
-        let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
+        let msg = Message::load_from_db_last_for_chat(&t.ctx, chat_id)
+            .await
+            .unwrap()
+            .unwrap();
 
         assert_eq!(msg.text, "subj with important info – body text");
         assert_eq!(msg.viewtype, Viewtype::Image);

src/peerstate.rs

@@ -650,9 +650,9 @@ impl Peerstate {
                 .await
             }
         };
-        for (chat_id, msg_id) in chats.iter() {
+        for (chat_id, _) in chats.iter() {
-            let timestamp_sort = if let Some(msg_id) = msg_id {
+            let lastmsg = Message::load_from_db_last_for_chat(context, *chat_id).await?;
-                let lastmsg = Message::load_from_db(context, *msg_id).await?;
+            let timestamp_sort = if let Some(lastmsg) = lastmsg {
                 lastmsg.timestamp_sort
             } else {
                 context

src/receive_imf/tests.rs

@@ -413,6 +413,7 @@ async fn test_no_from() {
     let context = &t;
 
     let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap();
+    assert!(chats.get_chat_id(0).is_err());
     assert!(chats.get_msg_id(0).is_err());
 
     let received = receive_imf(
@@ -455,6 +456,7 @@ async fn test_no_message_id_header() {
     let t = TestContext::new_alice().await;
 
     let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap();
+    assert!(chats.get_chat_id(0).is_err());
     assert!(chats.get_msg_id(0).is_err());
 
     let received = receive_imf(
@@ -557,8 +559,9 @@ async fn test_escaped_recipients() {
     assert_eq!(contact.get_display_name(), "h2");
 
     let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap();
-    let msg = Message::load_from_db(&t, chats.get_msg_id(0).unwrap().unwrap())
+    let msg = Message::load_from_db_last_for_chat(&t, chats.get_chat_id(0).unwrap())
         .await
+        .unwrap()
         .unwrap();
     assert_eq!(msg.is_dc_message, MessengerMessage::Yes);
     assert_eq!(msg.text, "hello");
@@ -745,7 +748,7 @@ async fn test_parse_ndn(
     .unwrap();
 
     let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap();
-    let msg_id = chats.get_msg_id(0).unwrap().unwrap();
+    let chat_id = chats.get_chat_id(0).unwrap();
 
     // Check that the ndn would be downloaded:
     let headers = mailparse::parse_mail(raw_ndn).unwrap().headers;
@@ -756,7 +759,10 @@ async fn test_parse_ndn(
     );
 
     receive_imf(&t, raw_ndn, false).await.unwrap();
-    let msg = Message::load_from_db(&t, msg_id).await.unwrap();
+    let msg = Message::load_from_db_last_for_chat(&t, chat_id)
+        .await
+        .unwrap()
+        .unwrap();
 
     assert_eq!(
         msg.state,
@@ -768,7 +774,7 @@ async fn test_parse_ndn(
     );
 
     assert_eq!(msg.error(), error_msg.map(|error| error.to_string()));
-    (t, msg_id)
+    (t, msg.id)
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -860,8 +866,11 @@ async fn load_imf_email(context: &Context, imf_raw: &[u8]) -> Message {
         .unwrap();
     receive_imf(context, imf_raw, false).await.unwrap();
     let chats = Chatlist::try_load(context, 0, None, None).await.unwrap();
-    let msg_id = chats.get_msg_id(0).unwrap().unwrap();
+    let chat_id = chats.get_chat_id(0).unwrap();
-    Message::load_from_db(context, msg_id).await.unwrap()
+    Message::load_from_db_last_for_chat(context, chat_id)
+        .await
+        .unwrap()
+        .unwrap()
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]

src/summary.rs

@@ -7,7 +7,7 @@ use crate::chat::Chat;
 use crate::constants::Chattype;
 use crate::contact::{Contact, ContactId};
 use crate::context::Context;
-use crate::message::{Message, MessageState, Viewtype};
+use crate::message::{Message, MessageState, MsgId, Viewtype};
 use crate::mimeparser::SystemMessage;
 use crate::stock_str;
 use crate::stock_str::msg_reacted;
@@ -54,6 +54,12 @@ pub struct Summary {
 
     /// Message preview image path
     pub thumbnail_path: Option<String>,
+
+    /// Message viewtype.
+    pub viewtype: Viewtype,
+
+    /// Message ID.
+    pub id: Option<MsgId>,
 }
 
 impl Summary {
@@ -79,6 +85,8 @@ impl Summary {
                 timestamp: msg.get_timestamp(), // message timestamp (not reaction) to make timestamps more consistent with chats ordering
                 state: msg.state, // message state (not reaction) - indicating if it was me sending the last message
                 thumbnail_path: None,
+                viewtype: msg.viewtype,
+                id: Some(msg.id),
             });
         }
 
@@ -127,6 +135,8 @@ impl Summary {
             timestamp: msg.get_timestamp(),
             state: msg.state,
             thumbnail_path,
+            viewtype: msg.viewtype,
+            id: Some(msg.id),
         })
     }
 

src/test_utils.rs

@@ -561,10 +561,11 @@ impl TestContext {
         // The chatlist describes what you see when you open DC, a list of chats and in each of them
         // the first words of the last message. To get the last message overall, we look at the chat at the top of the
         // list, which has the index 0.
-        let msg_id = chats.get_msg_id(0).unwrap().unwrap();
+        let chat_id = chats.get_chat_id(0).unwrap();
-        Message::load_from_db(&self.ctx, msg_id)
+        Message::load_from_db_last_for_chat(&self.ctx, chat_id)
             .await
             .expect("failed to load msg")
+            .unwrap()
     }
 
     /// Returns the [`Contact`] for the other [`TestContext`], creating it if necessary.