From 3e90ff37b5ce58670855d97967253b355fd91662 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Sun, 14 Jul 2024 21:58:41 -0300 Subject: [PATCH 1/2] refactor: Move group name calculation out of create_adhoc_group() --- src/receive_imf.rs | 85 ++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 48 deletions(-) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index c758a17862..a0335409c6 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1827,22 +1827,43 @@ async fn lookup_chat_or_create_adhoc_group( // Try to assign to a chat based on In-Reply-To/References. lookup_chat_by_reply(context, mime_parser, parent, to_ids, from_id).await? { - Ok(Some((new_chat_id, new_chat_id_blocked))) - } else if allow_creation { - // Try to create an ad hoc group. - create_adhoc_group( + return Ok(Some((new_chat_id, new_chat_id_blocked))); + } + if !allow_creation { + return Ok(None); + } + // Partial download may be an encrypted message with protected Subject header. We do not want to + // create a group with "..." or "Encrypted message" as a subject. The same is for undecipherable + // messages. Instead, assign the message to 1:1 chat with the sender. + if is_partial_download { + info!( context, - mime_parser, - create_blocked, - from_id, - to_ids, - is_partial_download, - ) - .await - .context("Could not create ad hoc group") - } else { - Ok(None) + "Ad-hoc group cannot be created from partial download." + ); + return Ok(None); } + if mime_parser.decrypting_failed { + warn!( + context, + "Not creating ad-hoc group for message that cannot be decrypted." + ); + return Ok(None); + } + + let grpname = mime_parser + .get_subject() + .map(|s| remove_subject_prefix(&s)) + .unwrap_or_else(|| "👥📧".to_string()); + create_adhoc_group( + context, + mime_parser, + create_blocked, + from_id, + to_ids, + &grpname, + ) + .await + .context("Could not create ad hoc group") } /// If this method returns true, the message shall be assigned to the 1:1 chat with the sender. @@ -2512,19 +2533,8 @@ async fn create_adhoc_group( create_blocked: Blocked, from_id: ContactId, to_ids: &[ContactId], - is_partial_download: bool, + grpname: &str, ) -> Result> { - if is_partial_download { - // Partial download may be an encrypted message with protected Subject header. - // - // We do not want to create a group with "..." or "Encrypted message" as a subject. - info!( - context, - "Ad-hoc group cannot be created from partial download." - ); - return Ok(None); - } - let mut member_ids: Vec = to_ids.to_vec(); if !member_ids.contains(&(from_id)) { member_ids.push(from_id); @@ -2536,22 +2546,6 @@ async fn create_adhoc_group( if mime_parser.is_mailinglist_message() { return Ok(None); } - - if mime_parser.decrypting_failed { - // Do not create a new ad-hoc group if the message cannot be - // decrypted. - // - // The subject may be encrypted and contain a placeholder such - // as "...". It can also be a COI group, with encrypted - // Chat-Group-ID and incompatible Message-ID format. - // - // Instead, assign the message to 1:1 chat with the sender. - warn!( - context, - "Not creating ad-hoc group for message that cannot be decrypted." - ); - return Ok(None); - } if mime_parser .get_header(HeaderDef::ChatGroupMemberRemoved) .is_some() @@ -2566,16 +2560,11 @@ async fn create_adhoc_group( return Ok(None); } - let grpname = mime_parser - .get_subject() - .map(|s| remove_subject_prefix(&s)) - .unwrap_or_else(|| "👥📧".to_string()); - let new_chat_id: ChatId = ChatId::create_multiuser_record( context, Chattype::Group, "", // Ad hoc groups have no ID. - &grpname, + grpname, create_blocked, ProtectionStatus::Unprotected, None, From e94af720ab13ec75677dd30d21907c1974360ea4 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Mon, 15 Jul 2024 11:25:33 -0300 Subject: [PATCH 2/2] feat: Assign message to ad-hoc group with matching name and members (#5385) This should fix ad-hoc groups splitting when messages are fetched out of order from different folders or otherwise reordered, or some messages are missing so that the messages reference chain is broken, or a member was removed from the thread and readded later, etc. Even if this way two different threads are merged, it looks acceptable, having many threads with the same name/subject and members isn't a common use case. --- src/receive_imf.rs | 47 ++++++++++++++++++++++++++--- src/receive_imf/tests.rs | 65 ++++++++++++++++++++++++++++++++++++++++ src/sql/migrations.rs | 9 ++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index a0335409c6..e19696a284 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -35,7 +35,7 @@ use crate::peerstate::Peerstate; use crate::reaction::{set_msg_reaction, Reaction}; use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on_other_device}; use crate::simplify; -use crate::sql; +use crate::sql::{self, params_iter}; use crate::stock_str; use crate::sync::Sync::*; use crate::tools::{self, buf_compress, remove_subject_prefix}; @@ -1829,9 +1829,6 @@ async fn lookup_chat_or_create_adhoc_group( { return Ok(Some((new_chat_id, new_chat_id_blocked))); } - if !allow_creation { - return Ok(None); - } // Partial download may be an encrypted message with protected Subject header. We do not want to // create a group with "..." or "Encrypted message" as a subject. The same is for undecipherable // messages. Instead, assign the message to 1:1 chat with the sender. @@ -1854,6 +1851,48 @@ async fn lookup_chat_or_create_adhoc_group( .get_subject() .map(|s| remove_subject_prefix(&s)) .unwrap_or_else(|| "👥📧".to_string()); + let mut contact_ids = Vec::with_capacity(to_ids.len() + 1); + contact_ids.extend(to_ids); + if !contact_ids.contains(&from_id) { + contact_ids.push(from_id); + } + if let Some((chat_id, blocked)) = context + .sql + .query_row_optional( + &format!( + "SELECT c.id, c.blocked + FROM chats c INNER JOIN msgs m ON c.id=m.chat_id + WHERE m.hidden=0 AND c.grpid='' AND c.name=? + AND (SELECT COUNT(*) FROM chats_contacts + WHERE chat_id=c.id)=? + AND (SELECT COUNT(*) FROM chats_contacts + WHERE chat_id=c.id + AND contact_id NOT IN ({}))=0 + ORDER BY m.timestamp DESC", + sql::repeat_vars(contact_ids.len()), + ), + rusqlite::params_from_iter( + params_iter(&[&grpname]) + .chain(params_iter(&[contact_ids.len()])) + .chain(params_iter(&contact_ids)), + ), + |row| { + let id: ChatId = row.get(0)?; + let blocked: Blocked = row.get(1)?; + Ok((id, blocked)) + }, + ) + .await? + { + info!( + context, + "Assigning message to ad-hoc group {chat_id} with matching name and members." + ); + return Ok(Some((chat_id, blocked))); + } + if !allow_creation { + return Ok(None); + } create_adhoc_group( context, mime_parser, diff --git a/src/receive_imf/tests.rs b/src/receive_imf/tests.rs index d3842a29a8..f71ad1cf20 100644 --- a/src/receive_imf/tests.rs +++ b/src/receive_imf/tests.rs @@ -204,6 +204,71 @@ async fn test_adhoc_group_show_all() { assert_eq!(chat::get_chat_contacts(&t, chat_id).await.unwrap().len(), 3); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_adhoc_groups_merge() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org, claire@example.com\n\ + Message-ID: <1111@example.net>\n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + Subject: New thread\n\ + \n\ + The first of us should create a thread as discussed\n", + false, + ) + .await?; + receive_imf( + alice, + b"From: alice@example.org\n\ + To: bob@example.net, claire@example.com\n\ + Message-ID: <2222@example.org>\n\ + Date: Sun, 22 Mar 2020 22:37:58 +0000\n\ + Subject: New thread\n\ + \n\ + The first of us should create a thread as discussed\n", + false, + ) + .await?; + let chats = Chatlist::try_load(alice, 0, None, None).await?; + assert_eq!(chats.len(), 1); + let chat_id = chats.get_chat_id(0)?; + assert_eq!(chat_id.get_msg_cnt(alice).await?, 2); + + // If member list doesn't match, threads aren't merged. + receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org, claire@example.com, fiona@example.net\n\ + Message-ID: <3333@example.net>\n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + Subject: New thread\n\ + \n\ + This is another thread, with Fiona\n", + false, + ) + .await?; + let chats = Chatlist::try_load(alice, 0, None, None).await?; + assert_eq!(chats.len(), 2); + receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org, fiona@example.net\n\ + Message-ID: <4444@example.net>\n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + Subject: New thread\n\ + \n\ + This is yet another thread, with Fiona and 0 Claires\n", + false, + ) + .await?; + let chats = Chatlist::try_load(alice, 0, None, None).await?; + assert_eq!(chats.len(), 3); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_read_receipt_and_unarchive() -> Result<()> { // create alice's account diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 1979b541ef..e9146ae2c4 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1028,6 +1028,15 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid); .await?; } + inc_and_check(&mut migration_version, 121)?; + if dbversion < migration_version { + sql.execute_migration( + "CREATE INDEX chats_index4 ON chats (name)", + migration_version, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await?