From 682baff1b9c044a2d996924e695fde1e61adae53 Mon Sep 17 00:00:00 2001 From: Hocuri Date: Mon, 27 Jul 2026 14:53:58 +0200 Subject: [PATCH] refactor: Remove FolderMeaning and target_folder (#8456) The `FolderMeaning` and the `target_folder()` function and related code were needed for the message moving logic. The message moving logic isn't needed anymore, so, this PR is a first step to remove it. We only ever look at one folder by now (usually the Inbox folder), so, any message moving logic isn't needed anymore because we will never notice a message that is supposed to be moved. The `target` column of the `imap` table is mostly the same as the `folder` column now. Except when the message is supposed to be deleted, then it's an empty string. --- src/imap.rs | 270 ++------------------------- src/imap/imap_tests.rs | 115 ------------ src/imap/session.rs | 6 - src/receive_imf.rs | 14 +- src/receive_imf/receive_imf_tests.rs | 1 + src/scheduler.rs | 4 +- 6 files changed, 16 insertions(+), 394 deletions(-) diff --git a/src/imap.rs b/src/imap.rs index 15e362725..7e397520a 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -16,29 +16,27 @@ use std::{ use anyhow::{Context as _, Result, bail, ensure, format_err}; use async_channel::{self, Receiver, Sender}; -use async_imap::types::{Fetch, Flag, Name, NameAttribute, UnsolicitedResponse}; +use async_imap::types::{Fetch, Flag, UnsolicitedResponse}; use futures::{FutureExt as _, TryStreamExt}; use futures_lite::FutureExt; use ratelimit::Ratelimit; use url::Url; -use crate::chat::{self, ChatIdBlocked, add_device_msg}; +use crate::chat::{self, add_device_msg}; use crate::config::Config; -use crate::constants::{Blocked, DC_VERSION_STR}; -use crate::contact::ContactId; +use crate::constants::DC_VERSION_STR; use crate::context::Context; use crate::ensure_and_debug_assert; use crate::events::EventType; use crate::headerdef::{HeaderDef, HeaderDefMap}; -use crate::log::{LogExt, warn}; +use crate::log::LogExt; +use crate::log::warn; use crate::message::{self, Message}; use crate::mimeparser; use crate::net::proxy::ProxyConfig; use crate::net::session::SessionStream; use crate::push::encrypt_device_token; -use crate::receive_imf::{ - ReceivedMsg, from_field_to_contact_id, get_prefetch_parent_message, receive_imf_inner, -}; +use crate::receive_imf::{ReceivedMsg, from_field_to_contact_id, receive_imf_inner}; use crate::scheduler::connectivity::ConnectivityStore; use crate::stock_str; use crate::tools::{self, create_id, duration_to_str, time}; @@ -140,24 +138,6 @@ pub(crate) struct ServerMetadata { pub ice_servers_expiration_timestamp: i64, } -#[derive(Debug, Display, PartialEq, Eq, Clone, Copy)] -pub enum FolderMeaning { - Unknown, - - /// Spam folder. - Spam, - Inbox, - Trash, - - /// Virtual folders. - /// - /// On Gmail there are virtual folders marked as \\All, \\Important and \\Flagged. - /// Delta Chat ignores these folders because the same messages can be fetched - /// from the real folder and the result of moving and deleting messages via - /// virtual folder is unclear. - Virtual, -} - struct UidGrouper> { inner: Peekable, } @@ -805,36 +785,16 @@ impl Imap { } impl Session { - /// Synchronizes UIDs for all folders. - pub(crate) async fn resync_folders(&mut self, context: &Context) -> Result<()> { - let all_folders = self - .list_folders() - .await - .context("listing folders for resync")?; - for folder in all_folders { - let folder_meaning = get_folder_meaning(&folder); - if !matches!( - folder_meaning, - FolderMeaning::Virtual | FolderMeaning::Unknown - ) { - self.resync_folder_uids(context, folder.name(), folder_meaning) - .await?; - } - } - Ok(()) - } - /// Synchronizes UIDs in the database with UIDs on the server. /// /// It is assumed that no operations are taking place on the same /// folder at the moment. Make sure to run it in the same /// thread/task as other network operations on this folder to /// avoid race conditions. - pub(crate) async fn resync_folder_uids( + pub(crate) async fn resync_uids_with_server( &mut self, context: &Context, folder: &str, - folder_meaning: FolderMeaning, ) -> Result<()> { let uid_validity; // Collect pairs of UID and Message-ID. @@ -858,25 +818,19 @@ impl Session { let message_id = prefetch_get_message_id(&headers); if let (Some(uid), Some(rfc724_mid)) = (fetch.uid, message_id) { - msgs.insert( - uid, - ( - rfc724_mid, - target_folder(context, folder, folder_meaning, &headers).await?, - ), - ); + msgs.insert(uid, rfc724_mid); } } info!( context, - "resync_folder_uids: Collected {} message IDs in {folder}.", + "resync_uids_with_server: Collected {} message IDs in {folder}.", msgs.len(), ); uid_validity = get_uidvalidity(context, transport_id, folder).await?; } else { - warn!(context, "resync_folder_uids: No folder {folder}."); + warn!(context, "resync_uids_with_server: No folder {folder}."); uid_validity = 0; } @@ -885,16 +839,14 @@ impl Session { .sql .transaction(move |transaction| { transaction.execute("DELETE FROM imap WHERE transport_id=? AND folder=?", (transport_id, folder,))?; - for (uid, (rfc724_mid, target)) in &msgs { - // This may detect previously undetected moved - // messages, so we update server_folder too. + for (uid, rfc724_mid) in &msgs { transaction.execute( "INSERT INTO imap (transport_id, rfc724_mid, folder, uid, uidvalidity, target) VALUES (?, ?, ?, ?, ?, ?) ON CONFLICT(transport_id, folder, uid, uidvalidity) DO UPDATE SET rfc724_mid=excluded.rfc724_mid, target=excluded.target", - (transport_id, rfc724_mid, folder, uid, uid_validity, target), + (transport_id, rfc724_mid, folder, uid, uid_validity, folder), )?; } Ok(()) @@ -1640,204 +1592,6 @@ impl Session { } } -async fn should_move_out_of_spam( - context: &Context, - headers: &[mailparse::MailHeader<'_>], -) -> Result { - if headers.get_header_value(HeaderDef::ChatVersion).is_some() { - // If this is a chat message (i.e. has a ChatVersion header), then this might be - // a securejoin message. We can't find out at this point as we didn't prefetch - // the SecureJoin header. So, we always move chat messages out of Spam. - // Two possibilities to change this would be: - // 1. Remove the `&& !context.is_spam_folder(folder).await?` check from - // `fetch_new_messages()`, and then let `receive_imf()` check - // if it's a spam message and should be hidden. - // 2. Or add a flag to the ChatVersion header that this is a securejoin - // request, and return `true` here only if the message has this flag. - // `receive_imf()` can then check if the securejoin request is valid. - return Ok(true); - } - - if let Some(msg) = get_prefetch_parent_message(context, headers).await? { - if msg.chat_blocked != Blocked::Not { - // Blocked or contact request message in the spam folder, leave it there. - return Ok(false); - } - } else { - let from = match mimeparser::get_from(headers) { - Some(f) => f, - None => return Ok(false), - }; - // No chat found. - let (from_id, blocked_contact, _origin) = - match from_field_to_contact_id(context, &from, None, true, true) - .await - .context("from_field_to_contact_id")? - { - Some(res) => res, - None => { - warn!( - context, - "Contact with From address {:?} cannot exist, not moving out of spam", from - ); - return Ok(false); - } - }; - if blocked_contact { - // Contact is blocked, leave the message in spam. - return Ok(false); - } - - if let Some(chat_id_blocked) = ChatIdBlocked::lookup_by_contact(context, from_id).await? { - if chat_id_blocked.blocked != Blocked::Not { - return Ok(false); - } - } else if from_id != ContactId::SELF { - // No chat with this contact found. - return Ok(false); - } - } - - Ok(true) -} - -/// Returns target folder for a message found in the Spam folder. -/// If this returns None, the message will not be moved out of the -/// Spam folder, and as `fetch_new_messages()` doesn't download -/// messages from the Spam folder, the message will be ignored. -async fn spam_target_folder_cfg( - context: &Context, - headers: &[mailparse::MailHeader<'_>], -) -> Result> { - if !should_move_out_of_spam(context, headers).await? { - return Ok(None); - } - - Ok(Some(Config::ConfiguredInboxFolder)) -} - -/// Returns `ConfiguredInboxFolder` or `ConfiguredMvboxFolder` if -/// the message needs to be moved from `folder`. Otherwise returns `None`. -pub async fn target_folder_cfg( - context: &Context, - folder: &str, - folder_meaning: FolderMeaning, - headers: &[mailparse::MailHeader<'_>], -) -> Result> { - if folder == "DeltaChat" { - return Ok(None); - } - - if folder_meaning == FolderMeaning::Spam { - spam_target_folder_cfg(context, headers).await - } else { - Ok(None) - } -} - -pub async fn target_folder( - context: &Context, - folder: &str, - folder_meaning: FolderMeaning, - headers: &[mailparse::MailHeader<'_>], -) -> Result { - match target_folder_cfg(context, folder, folder_meaning, headers).await? { - Some(config) => match context.get_config(config).await? { - Some(target) => Ok(target), - None => Ok(folder.to_string()), - }, - None => Ok(folder.to_string()), - } -} - -/// Try to get the folder meaning by the name of the folder only used if the server does not support XLIST. -// TODO: lots languages missing - maybe there is a list somewhere on other MUAs? -// however, if we fail to find out the sent-folder, -// only watching this folder is not working. at least, this is no show stopper. -// CAVE: if possible, take care not to add a name here that is "sent" in one language -// but sth. different in others - a hard job. -fn get_folder_meaning_by_name(folder_name: &str) -> FolderMeaning { - // source: - const SPAM_NAMES: &[&str] = &[ - "spam", - "junk", - "Correio electrónico não solicitado", - "Correo basura", - "Lixo", - "Nettsøppel", - "Nevyžádaná pošta", - "No solicitado", - "Ongewenst", - "Posta indesiderata", - "Skräp", - "Wiadomości-śmieci", - "Önemsiz", - "Ανεπιθύμητα", - "Спам", - "垃圾邮件", - "垃圾郵件", - "迷惑メール", - "스팸", - ]; - const TRASH_NAMES: &[&str] = &[ - "Trash", - "Bin", - "Caixote do lixo", - "Cestino", - "Corbeille", - "Papelera", - "Papierkorb", - "Papirkurv", - "Papperskorgen", - "Prullenbak", - "Rubujo", - "Κάδος απορριμμάτων", - "Корзина", - "Кошик", - "ゴミ箱", - "垃圾桶", - "已删除邮件", - "휴지통", - ]; - let lower = folder_name.to_lowercase(); - - if lower == "inbox" { - FolderMeaning::Inbox - } else if SPAM_NAMES.iter().any(|s| s.to_lowercase() == lower) { - FolderMeaning::Spam - } else if TRASH_NAMES.iter().any(|s| s.to_lowercase() == lower) { - FolderMeaning::Trash - } else { - FolderMeaning::Unknown - } -} - -fn get_folder_meaning_by_attrs(folder_attrs: &[NameAttribute]) -> FolderMeaning { - for attr in folder_attrs { - match attr { - NameAttribute::Trash => return FolderMeaning::Trash, - NameAttribute::Junk => return FolderMeaning::Spam, - NameAttribute::All | NameAttribute::Flagged => return FolderMeaning::Virtual, - NameAttribute::Extension(label) => { - match label.as_ref() { - "\\Spam" => return FolderMeaning::Spam, - "\\Important" => return FolderMeaning::Virtual, - _ => {} - }; - } - _ => {} - } - } - FolderMeaning::Unknown -} - -pub(crate) fn get_folder_meaning(folder: &Name) -> FolderMeaning { - match get_folder_meaning_by_attrs(folder.attributes()) { - FolderMeaning::Unknown => get_folder_meaning_by_name(folder.name()), - meaning => meaning, - } -} - /// Parses the headers from the FETCH result. fn get_fetch_headers(prefetch_msg: &Fetch) -> Result>> { match prefetch_msg.header() { diff --git a/src/imap/imap_tests.rs b/src/imap/imap_tests.rs index b492b0f8b..e606c4461 100644 --- a/src/imap/imap_tests.rs +++ b/src/imap/imap_tests.rs @@ -1,15 +1,6 @@ use super::*; -use crate::chat::ChatId; -use crate::contact::Contact; use crate::test_utils::TestContext; -#[test] -fn test_get_folder_meaning_by_name() { - assert_eq!(get_folder_meaning_by_name("xxx"), FolderMeaning::Unknown); - assert_eq!(get_folder_meaning_by_name("SPAM"), FolderMeaning::Spam); - assert_eq!(get_folder_meaning_by_name("Trash"), FolderMeaning::Trash); -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_set_uid_next_validity() { let t = TestContext::new_alice().await; @@ -99,112 +90,6 @@ fn test_build_sequence_sets() { } } -async fn check_target_folder_combination( - folder: &str, - chat_msg: bool, - expected_destination: &str, - accepted_chat: bool, - outgoing: bool, -) -> Result<()> { - println!( - "Testing: For folder {folder}, chat_msg {chat_msg}, accepted {accepted_chat}, outgoing {outgoing}" - ); - - let t = TestContext::new_alice().await; - - if accepted_chat { - let contact_id = Contact::create(&t.ctx, "", "bob@example.net").await?; - ChatId::create_for_contact(&t.ctx, contact_id).await?; - } - let temp; - - let bytes = { - temp = format!( - "Received: (Postfix, from userid 1000); Mon, 4 Dec 2006 14:51:39 +0100 (CET)\n\ - {}\ - Subject: foo\n\ - Message-ID: \n\ - {}\ - Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ - \n\ - hello\n", - if outgoing { - "From: alice@example.org\nTo: bob@example.net\n" - } else { - "From: bob@example.net\nTo: alice@example.org\n" - }, - if chat_msg { "Chat-Version: 1.0\n" } else { "" }, - ); - temp.as_bytes() - }; - - let (headers, _) = mailparse::parse_headers(bytes)?; - let actual = if let Some(config) = - target_folder_cfg(&t, folder, get_folder_meaning_by_name(folder), &headers).await? - { - t.get_config(config).await? - } else { - None - }; - - let expected = if expected_destination == folder { - None - } else { - Some(expected_destination) - }; - assert_eq!( - expected, - actual.as_deref(), - "For folder {folder}, chat_msg {chat_msg}, accepted {accepted_chat}, outgoing {outgoing}: expected {expected:?}, got {actual:?}" - ); - Ok(()) -} - -// chat_msg means that the message was sent by Delta Chat -// The tuples are (folder, chat_msg, expected_destination) -const COMBINATIONS_ACCEPTED_CHAT: &[(&str, bool, &str)] = &[ - ("INBOX", false, "INBOX"), - ("INBOX", true, "INBOX"), - ("Spam", false, "INBOX"), // Move classical emails in accepted chats from Spam to Inbox, not 100% sure on this, we could also just never move non-chat-msgs - ("Spam", true, "INBOX"), -]; - -// These are the same as above, but non-chat messages in Spam stay in Spam -const COMBINATIONS_REQUEST: &[(&str, bool, &str)] = &[ - ("INBOX", false, "INBOX"), - ("INBOX", true, "INBOX"), - ("Spam", false, "Spam"), - ("Spam", true, "INBOX"), -]; - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_target_folder_incoming_accepted() -> Result<()> { - for (folder, chat_msg, expected_destination) in COMBINATIONS_ACCEPTED_CHAT { - check_target_folder_combination(folder, *chat_msg, expected_destination, true, false) - .await?; - } - Ok(()) -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_target_folder_incoming_request() -> Result<()> { - for (folder, chat_msg, expected_destination) in COMBINATIONS_REQUEST { - check_target_folder_combination(folder, *chat_msg, expected_destination, false, false) - .await?; - } - Ok(()) -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_target_folder_outgoing() -> Result<()> { - // Test outgoing emails - for (folder, chat_msg, expected_destination) in COMBINATIONS_ACCEPTED_CHAT { - check_target_folder_combination(folder, *chat_msg, expected_destination, true, true) - .await?; - } - Ok(()) -} - #[test] fn test_uid_grouper() { // Input: sequence of (rowid: i64, uid: u32, target: String) diff --git a/src/imap/session.rs b/src/imap/session.rs index 65d90d851..17c7eee38 100644 --- a/src/imap/session.rs +++ b/src/imap/session.rs @@ -115,12 +115,6 @@ impl Session { self.capabilities.is_chatmail } - /// Returns the names of all folders on the IMAP server. - pub async fn list_folders(&mut self) -> Result> { - let list = self.list(Some(""), Some("*")).await?.try_collect().await?; - Ok(list) - } - /// Prefetch `n_uids` messages starting from `uid_next`. Returns a list of fetch results in the /// order of ascending UIDs. #[expect(clippy::arithmetic_side_effects)] diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 4b8a17652..cc76e504a 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -25,7 +25,7 @@ use crate::debug_logging::maybe_set_logging_xdc_inner; use crate::download::{DownloadState, msg_is_downloaded_for}; use crate::ephemeral::{Timer as EphemeralTimer, stock_ephemeral_timer_changed}; use crate::events::EventType; -use crate::headerdef::{HeaderDef, HeaderDefMap}; +use crate::headerdef::HeaderDef; use crate::imap::{GENERATED_PREFIX, markseen_on_imap_table}; use crate::key::{DcKey, Fingerprint}; use crate::key::{ @@ -4124,18 +4124,6 @@ async fn get_parent_message( message::get_by_rfc724_mids(context, &mids).await } -pub(crate) async fn get_prefetch_parent_message( - context: &Context, - headers: &[mailparse::MailHeader<'_>], -) -> Result> { - get_parent_message( - context, - headers.get_header_value(HeaderDef::References).as_deref(), - headers.get_header_value(HeaderDef::InReplyTo).as_deref(), - ) - .await -} - /// Looks up contact IDs from the database given the list of recipients. async fn add_or_lookup_contacts_by_address_list( context: &Context, diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 49c82da00..21d18b3ba 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -11,6 +11,7 @@ use crate::chat::{ use crate::chatlist::Chatlist; use crate::constants::DC_GCL_FOR_FORWARDING; use crate::contact; +use crate::headerdef::HeaderDefMap as _; use crate::imap::prefetch_should_download; use crate::imex::{ImexMode, imex}; use crate::key; diff --git a/src/scheduler.rs b/src/scheduler.rs index be7e4190e..f44a4021f 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -421,11 +421,11 @@ async fn inbox_fetch_idle(ctx: &Context, imap: &mut Imap, mut session: Session) } if let Ok(()) = imap.resync_request_receiver.try_recv() - && let Err(err) = session.resync_folders(ctx).await + && let Err(err) = session.resync_uids_with_server(ctx, &imap.folder).await { warn!( ctx, - "Transport {transport_id}: Failed to resync folders: {err:#}." + "Transport {transport_id}: Failed to resync UIDs: {err:#}." ); imap.resync_request_sender.try_send(()).ok(); }