Delete messages more thoroughly (and at all) (#2114)

- Make sure delete_expired_messages and housekeeping runs once a day
- delete more info about messages when putting them to trash (esp. also
delete txt_raw, from_id and to_id as we don't need those anymore, so
they are data that was unnecessarily kept)

fix #1926, fix #2090

Also:

* Nicer test_utils: add send_text() and print_chat()

* Adapt ephemeral messages for testing (make them accurate to the second)

* Add test for ephemeral messages

* Make pop_sent_msg() really pop the last sent message

src/chat.rs

@@ -1040,7 +1040,7 @@ impl Chat {
         };
         let ephemeral_timestamp = match ephemeral_timer {
             EphemeralTimer::Disabled => 0,
-            EphemeralTimer::Enabled { duration } => timestamp + i64::from(duration),
+            EphemeralTimer::Enabled { duration } => time() + i64::from(duration),
         };
 
         // add message to the database

src/config.rs

@@ -136,6 +136,9 @@ pub enum Config {
 
     /// address to webrtc instance to use for videochats
     WebrtcInstance,
+
+    /// Timestamp of the last time housekeeping was run
+    LastHousekeeping,
 }
 
 impl Context {
@@ -175,6 +178,13 @@ impl Context {
             .unwrap_or_default()
     }
 
+    pub async fn get_config_i64(&self, key: Config) -> i64 {
+        self.get_config(key)
+            .await
+            .and_then(|s| s.parse().ok())
+            .unwrap_or_default()
+    }
+
     pub async fn get_config_bool(&self, key: Config) -> bool {
         self.get_config_int(key).await != 0
     }

src/ephemeral.rs

@@ -278,10 +278,10 @@ pub(crate) async fn delete_expired_messages(context: &Context) -> Result<bool, E
         .sql
         .execute(
             "UPDATE msgs \
-             SET txt = 'DELETED', chat_id = ? \
+             SET chat_id=?, txt='', txt_raw='', from_id=0, to_id=0, param='' \
              WHERE \
              ephemeral_timestamp != 0 \
-             AND ephemeral_timestamp < ? \
+             AND ephemeral_timestamp <= ? \
              AND chat_id != ?",
             paramsv![DC_CHAT_ID_TRASH, time(), DC_CHAT_ID_TRASH],
         )
@@ -417,7 +417,7 @@ pub(crate) async fn load_imap_deletion_msgid(context: &Context) -> sql::Result<O
             "SELECT id FROM msgs \
          WHERE ( \
          timestamp < ? \
-         OR (ephemeral_timestamp != 0 AND ephemeral_timestamp < ?) \
+         OR (ephemeral_timestamp != 0 AND ephemeral_timestamp <= ?) \
          ) \
          AND server_uid != 0 \
          LIMIT 1",
@@ -459,9 +459,14 @@ pub(crate) async fn start_ephemeral_timers(context: &Context) -> sql::Result<()>
 
 #[cfg(test)]
 mod tests {
+    use async_std::task::sleep;
+
     use super::*;
-    use crate::chat;
     use crate::test_utils::*;
+    use crate::{
+        chat::{self, Chat, ChatItem},
+        dc_tools::IsNoneOrEmpty,
+    };
 
     #[async_std::test]
     async fn test_stock_ephemeral_messages() {
@@ -588,4 +593,51 @@ mod tests {
 
         Ok(())
     }
+
+    #[async_std::test]
+    async fn test_ephemeral_delete_msgs() {
+        let t = TestContext::new_alice().await;
+        let chat = t.get_self_chat().await;
+
+        t.send_text(chat.id, "Saved message, which we delete manually")
+            .await;
+        let msg = t.get_last_msg(chat.id).await;
+        msg.id.delete_from_db(&t).await.unwrap();
+        check_msg_was_deleted(&t, &chat, msg.id).await;
+
+        chat.id
+            .set_ephemeral_timer(&t, Timer::Enabled { duration: 1 })
+            .await
+            .unwrap();
+        let msg = t
+            .send_text(chat.id, "Saved message, disappearing after 1s")
+            .await;
+
+        sleep(Duration::from_millis(1100)).await;
+
+        check_msg_was_deleted(&t, &chat, msg.sender_msg_id).await;
+    }
+
+    async fn check_msg_was_deleted(t: &TestContext, chat: &Chat, msg_id: MsgId) {
+        let chat_items = chat::get_chat_msgs(&t, chat.id, 0, None).await;
+        // Check that the chat is empty except for possibly info messages:
+        for item in &chat_items {
+            if let ChatItem::Message { msg_id } = item {
+                let msg = Message::load_from_db(t, *msg_id).await.unwrap();
+                assert!(msg.is_info())
+            }
+        }
+
+        // Check that if there is a message left, the text and metadata are gone
+        if let Ok(msg) = Message::load_from_db(&t, msg_id).await {
+            assert_eq!(msg.from_id, 0);
+            assert_eq!(msg.to_id, 0);
+            assert!(msg.text.is_none_or_empty(), msg.text);
+            let rawtxt: Option<String> = t
+                .sql
+                .query_get_value(&t, "SELECT txt_raw FROM msgs WHERE id=?;", paramsv![msg_id])
+                .await;
+            assert!(rawtxt.is_none_or_empty(), rawtxt);
+        }
+    }
 }

src/job.rs

@@ -1215,6 +1215,18 @@ pub async fn add(context: &Context, job: Job) {
     }
 }
 
+async fn load_housekeeping_job(context: &Context) -> Option<Job> {
+    let last_time = context.get_config_i64(Config::LastHousekeeping).await;
+
+    let next_time = last_time + (60 * 60 * 24);
+    if next_time <= time() {
+        kill_action(context, Action::Housekeeping).await;
+        Some(Job::new(Action::Housekeeping, 0, Params::new(), 0))
+    } else {
+        None
+    }
+}
+
 /// Load jobs from the database.
 ///
 /// Load jobs for this "[Thread]", i.e. either load SMTP jobs or load
@@ -1331,8 +1343,10 @@ LIMIT 1;
                 } else {
                     Some(job)
                 }
+            } else if let Some(job) = load_imap_deletion_job(context).await.unwrap_or_default() {
+                Some(job)
             } else {
-                load_imap_deletion_job(context).await.unwrap_or_default()
+                load_housekeeping_job(context).await
             }
         }
         Thread::Smtp => job,
@@ -1379,7 +1393,8 @@ mod tests {
             &InterruptInfo::new(false, None),
         )
         .await;
-        assert!(jobs.is_none());
+        // The housekeeping job should be loaded as we didn't run housekeeping in the last day:
+        assert!(jobs.unwrap().action == Action::Housekeeping);
 
         insert_job(&t, 1).await;
         let jobs = load_next(

src/message.rs

@@ -110,7 +110,7 @@ impl MsgId {
         context
             .sql
             .execute(
-                "UPDATE msgs SET chat_id=?, txt='', txt_raw='' WHERE id=?",
+                "UPDATE msgs SET chat_id=?, txt='', txt_raw='', from_id=0, to_id=0, param='' WHERE id=?",
                 paramsv![chat_id, self],
             )
             .await?;

src/sql.rs

@@ -9,7 +9,6 @@ use std::time::Duration;
 
 use rusqlite::{Connection, Error as SqlError, OpenFlags};
 
-use crate::chat::{update_device_icon, update_saved_messages_icon};
 use crate::constants::{ShowEmails, DC_CHAT_ID_TRASH};
 use crate::context::Context;
 use crate::dc_tools::*;
@@ -17,6 +16,10 @@ use crate::ephemeral::start_ephemeral_timers;
 use crate::error::format_err;
 use crate::param::*;
 use crate::peerstate::*;
+use crate::{
+    chat::{update_device_icon, update_saved_messages_icon},
+    config::Config,
+};
 
 #[macro_export]
 macro_rules! paramsv {
@@ -465,6 +468,10 @@ pub fn get_rowid2(
 }
 
 pub async fn housekeeping(context: &Context) {
+    if let Err(err) = crate::ephemeral::delete_expired_messages(context).await {
+        warn!(context, "Failed to delete expired messages: {}", err);
+    }
+
     let mut files_in_use = HashSet::new();
     let mut unreferenced_count = 0;
 
@@ -595,7 +602,13 @@ pub async fn housekeeping(context: &Context) {
         );
     }
 
-    info!(context, "Housekeeping done.",);
+    if let Err(e) = context
+        .set_config(Config::LastHousekeeping, Some(&time().to_string()))
+        .await
+    {
+        warn!(context, "Can't set config: {}", e);
+    }
+    info!(context, "Housekeeping done.");
 }
 
 #[allow(clippy::indexing_slicing)]

src/test_utils.rs

@@ -9,9 +9,6 @@ use async_std::path::PathBuf;
 use async_std::sync::RwLock;
 use tempfile::{tempdir, TempDir};
 
-use crate::context::Context;
-use crate::dc_receive_imf::dc_receive_imf;
-use crate::dc_tools::EmailAddress;
 use crate::job::Action;
 use crate::key::{self, DcKey};
 use crate::message::{update_msg_state, Message, MessageState, MsgId};
@@ -23,6 +20,9 @@ use crate::{
     contact::Origin,
 };
 use crate::{config::Config, constants::DC_CONTACT_ID_SELF};
+use crate::{constants::Viewtype, context::Context};
+use crate::{constants::DC_MSG_ID_DAYMARKER, dc_tools::EmailAddress};
+use crate::{constants::DC_MSG_ID_MARKER1, dc_receive_imf::dc_receive_imf};
 
 /// A Context and temporary directory.
 ///
@@ -129,7 +129,7 @@ impl TestContext {
                     SELECT id, foreign_id, param
                       FROM jobs
                      WHERE action=?
-                  ORDER BY desired_timestamp;
+                  ORDER BY desired_timestamp DESC;
                 "#,
                     paramsv![Action::SendMsgToSmtp],
                     |row| {
@@ -163,7 +163,11 @@ impl TestContext {
             .await
             .expect("failed to remove job");
         update_msg_state(&self.ctx, id, MessageState::OutDelivered).await;
-        SentMessage { params, blob_path }
+        SentMessage {
+            params,
+            blob_path,
+            sender_msg_id: id,
+        }
     }
 
     /// Parse a message.
@@ -235,6 +239,55 @@ impl TestContext {
             .unwrap();
         Chat::load_from_db(self, chat_id).await.unwrap()
     }
+
+    /// Sends out the text message. If the other side shall receive it, you have to call `recv_msg()` with the returned `SentMessage`.
+    pub async fn send_text(&self, chat_id: ChatId, txt: &str) -> SentMessage {
+        let mut msg = Message::new(Viewtype::Text);
+        msg.set_text(Some(txt.to_string()));
+        chat::prepare_msg(&self, chat_id, &mut msg).await.unwrap();
+        chat::send_msg(&self, chat_id, &mut msg).await.unwrap();
+        self.pop_sent_msg().await
+    }
+
+    /// You can use this to debug your test by printing a chat structure
+    // This code is mainly the same as `log_msglist` in `cmdline.rs`, so one day, we could merge them to a public function in the `deltachat` crate.
+    #[allow(dead_code)]
+    pub async fn print_chat(&self, chat: &Chat) {
+        let msglist = chat::get_chat_msgs(&self, chat.get_id(), 0x1, None).await;
+        let msglist: Vec<MsgId> = msglist
+            .into_iter()
+            .map(|x| match x {
+                ChatItem::Message { msg_id } => msg_id,
+                ChatItem::Marker1 => MsgId::new(DC_MSG_ID_MARKER1),
+                ChatItem::DayMarker { .. } => MsgId::new(DC_MSG_ID_DAYMARKER),
+            })
+            .collect();
+
+        let mut lines_out = 0;
+        for msg_id in msglist {
+            if msg_id == MsgId::new(DC_MSG_ID_DAYMARKER) {
+                println!(
+                "--------------------------------------------------------------------------------"
+            );
+
+                lines_out += 1
+            } else if !msg_id.is_special() {
+                if lines_out == 0 {
+                    println!(
+                    "--------------------------------------------------------------------------------",
+                );
+                    lines_out += 1
+                }
+                let msg = Message::load_from_db(&self, msg_id).await.unwrap();
+                log_msg(self, "", &msg).await;
+            }
+        }
+        if lines_out > 0 {
+            println!(
+                "--------------------------------------------------------------------------------"
+            );
+        }
+    }
 }
 
 impl Deref for TestContext {
@@ -253,6 +306,7 @@ impl Deref for TestContext {
 pub struct SentMessage {
     params: Params,
     blob_path: PathBuf,
+    pub sender_msg_id: MsgId,
 }
 
 impl SentMessage {
@@ -309,3 +363,56 @@ pub(crate) fn bob_keypair() -> key::KeyPair {
         secret,
     }
 }
+
+async fn log_msg(context: &Context, prefix: impl AsRef<str>, msg: &Message) {
+    let contact = Contact::get_by_id(context, msg.get_from_id())
+        .await
+        .expect("invalid contact");
+
+    let contact_name = contact.get_name();
+    let contact_id = contact.get_id();
+
+    let statestr = match msg.get_state() {
+        MessageState::OutPending => " o",
+        MessageState::OutDelivered => " √",
+        MessageState::OutMdnRcvd => " √√",
+        MessageState::OutFailed => " !!",
+        _ => "",
+    };
+    let msgtext = msg.get_text();
+    println!(
+        "{}{}{}{}: {} (Contact#{}): {} {}{}{}{}{}",
+        prefix.as_ref(),
+        msg.get_id(),
+        if msg.get_showpadlock() { "🔒" } else { "" },
+        if msg.has_location() { "📍" } else { "" },
+        &contact_name,
+        contact_id,
+        msgtext.unwrap_or_default(),
+        if msg.get_from_id() == 1 as libc::c_uint {
+            ""
+        } else if msg.get_state() == MessageState::InSeen {
+            "[SEEN]"
+        } else if msg.get_state() == MessageState::InNoticed {
+            "[NOTICED]"
+        } else {
+            "[FRESH]"
+        },
+        if msg.is_info() { "[INFO]" } else { "" },
+        if msg.get_viewtype() == Viewtype::VideochatInvitation {
+            format!(
+                "[VIDEOCHAT-INVITATION: {}, type={}]",
+                msg.get_videochat_url().unwrap_or_default(),
+                msg.get_videochat_type().unwrap_or_default()
+            )
+        } else {
+            "".to_string()
+        },
+        if msg.is_forwarded() {
+            "[FORWARDED]"
+        } else {
+            ""
+        },
+        statestr,
+    );
+}