- create and use a ContactIds type as an ordered set instead of "Vec<u32>".

- recreate the group list more carefully, fixes #985

- resultify a few functions in the dc_receive pipeline

- don't quote displaynames in email-addresses, use utf8, preliminrarily addresses #976

Cargo.lock

@@ -662,6 +662,7 @@ dependencies = [
  "failure_derive 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "hex 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "image-meta 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "indexmap 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "itertools 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "lettre_email 0.9.2 (git+https://github.com/deltachat/lettre?branch=feat/mail)",

Cargo.toml

@@ -30,6 +30,7 @@ serde_json = "1.0"
 chrono = "0.4.6"
 failure = "0.1.5"
 failure_derive = "0.1.5"
+indexmap = "1.3.0"
 # TODO: make optional
 rustyline = "4.1.0"
 lazy_static = "1.4.0"

examples/repl/cmdline.rs

@@ -94,7 +94,9 @@ pub fn dc_reset_tables(context: &Context, bits: i32) -> i32 {
 fn dc_poke_eml_file(context: &Context, filename: impl AsRef<Path>) -> Result<(), Error> {
     let data = dc_read_file(context, filename)?;
 
-    dc_receive_imf(context, &data, "import", 0, 0);
+    if let Err(err) = dc_receive_imf(context, &data, "import", 0, 0) {
+        println!("dc_receive_imf errored: {:?}", err);
+    }
     Ok(())
 }
 

python/tests/test_account.py

@@ -399,6 +399,7 @@ class TestOfflineChat:
         chat.remove_contact(contacts[3])
         assert len(chat.get_contacts()) == 9
 
+
 class TestOnlineAccount:
     def get_chat(self, ac1, ac2, both_created=False):
         c2 = ac1.create_contact(email=ac2.get_config("addr"))
@@ -1111,8 +1112,12 @@ class TestGroupStressTests:
         assert len(msg.chat.get_contacts()) == 4
 
         lp.sec("ac1: removing one contacts and checking things are right")
-        msg.chat.remove_contact(msg.chat.get_contacts()[-1])
+        to_remove = msg.chat.get_contacts()[-1]
-        assert 0
+        msg.chat.remove_contact(to_remove)
+
+        sysmsg = ac1.wait_next_incoming_message()
+        assert to_remove.addr in sysmsg.text
+        assert len(sysmsg.chat.get_contacts()) == 3
 
 
 class TestOnlineConfigureFails:

src/contact.rs

@@ -10,7 +10,7 @@ use crate::constants::*;
 use crate::context::Context;
 use crate::dc_tools::*;
 use crate::e2ee;
-use crate::error::Result;
+use crate::error::{Error, Result};
 use crate::events::Event;
 use crate::key::*;
 use crate::login_param::LoginParam;
@@ -1021,6 +1021,18 @@ fn cat_fingerprint(
     }
 }
 
+impl Context {
+    /// determine whether the specified addr maps to the/a self addr
+    pub fn is_self_addr(&self, addr: &str) -> Result<bool> {
+        let self_addr = match self.get_config(Config::ConfiguredAddr) {
+            Some(s) => s,
+            None => return Err(Error::NotConfigured),
+        };
+
+        Ok(addr_cmp(self_addr, addr))
+    }
+}
+
 pub fn addr_cmp(addr1: impl AsRef<str>, addr2: impl AsRef<str>) -> bool {
     let norm1 = addr_normalize(addr1.as_ref()).to_lowercase();
     let norm2 = addr_normalize(addr2.as_ref()).to_lowercase();
@@ -1028,15 +1040,6 @@ pub fn addr_cmp(addr1: impl AsRef<str>, addr2: impl AsRef<str>) -> bool {
     norm1 == norm2
 }
 
-pub fn addr_equals_self(context: &Context, addr: impl AsRef<str>) -> bool {
-    if !addr.as_ref().is_empty() {
-        if let Some(self_addr) = context.get_config(Config::ConfiguredAddr) {
-            return addr_cmp(addr, self_addr);
-        }
-    }
-    false
-}
-
 fn split_address_book(book: &str) -> Vec<(&str, &str)> {
     book.lines()
         .chunks(2)
@@ -1120,6 +1123,18 @@ mod tests {
         assert_eq!(contacts.len(), 0);
     }
 
+    #[test]
+    fn test_is_self_addr() -> Result<()> {
+        let t = test_context(None);
+        assert!(t.ctx.is_self_addr("me@me.org").is_err());
+
+        let addr = configure_alice_keypair(&t.ctx);
+        assert_eq!(t.ctx.is_self_addr("me@me.org")?, false);
+        assert_eq!(t.ctx.is_self_addr(&addr)?, true);
+
+        Ok(())
+    }
+
     #[test]
     fn test_add_or_lookup() {
         // add some contacts, this also tests add_address_book()

src/dc_receive_imf.rs

@@ -23,6 +23,9 @@ use crate::securejoin::handle_securejoin_handshake;
 use crate::sql;
 use crate::stock::StockMessage;
 
+// IndexSet is like HashSet but maintains order of insertion
+type ContactIds = indexmap::IndexSet<u32>;
+
 #[derive(Debug, PartialEq, Eq)]
 enum CreateEvent {
     MsgsChanged,
@@ -36,7 +39,7 @@ pub fn dc_receive_imf(
     server_folder: impl AsRef<str>,
     server_uid: u32,
     flags: u32,
-) {
+) -> Result<()> {
     info!(
         context,
         "Receiving message {}/{}...",
@@ -53,19 +56,10 @@ pub fn dc_receive_imf(
         println!("{}", String::from_utf8_lossy(imf_raw));
     }
 
-    let mime_parser = MimeParser::from_bytes(context, imf_raw);
+    let mut mime_parser = MimeParser::from_bytes(context, imf_raw)?;
-    let mut mime_parser = if let Err(err) = mime_parser {
-        warn!(context, "dc_receive_imf parse error: {}", err);
-        return;
-    } else {
-        mime_parser.unwrap()
-    };
 
-    if mime_parser.get(HeaderDef::From_).is_none() {
+    // we can not add even an empty record if we have no info whatsoever
-        // Error - even adding an empty record won't help as we do not know the sender
+    ensure!(mime_parser.has_headers(), "No Headers Found");
-        warn!(context, "No From header.");
-        return;
-    }
 
     // the function returns the number of created messages in the database
     let mut incoming = true;
@@ -85,7 +79,7 @@ pub fn dc_receive_imf(
     let mut create_event_to_send = Some(CreateEvent::MsgsChanged);
     let mut rr_event_to_send = Vec::new();
 
-    let mut to_ids = Vec::with_capacity(16);
+    let mut to_ids = ContactIds::new();
 
     // helper method to handle early exit and memory cleanup
     let cleanup = |context: &Context,
@@ -125,25 +119,26 @@ pub fn dc_receive_imf(
     // we do not check Return-Path any more as this is unreliable, see issue #150
     if let Some(field_from) = mime_parser.get(HeaderDef::From_) {
         let mut check_self = false;
-        let mut from_list = Vec::with_capacity(16);
+        let mut from_contact_ids = ContactIds::new();
         dc_add_or_lookup_contacts_by_address_list(
             context,
             &field_from,
             Origin::IncomingUnknownFrom,
-            &mut from_list,
+            &mut from_contact_ids,
             &mut check_self,
-        );
+        )?;
         if check_self {
             incoming = false;
             if mime_parser.sender_equals_recipient() {
                 from_id = DC_CONTACT_ID_SELF;
             }
-        } else if !from_list.is_empty() {
+        } else if !from_contact_ids.is_empty() {
+            from_id = from_contact_ids.get_index(0).cloned().unwrap_or_default();
+            incoming_origin = Contact::get_origin_by_id(context, from_id, &mut from_id_blocked)
+        } else {
             // if there is no from given, from_id stays 0 which is just fine. These messages
             // are very rare, however, we have to add them to the database (they go to the
             // "deaddrop" chat) to avoid a re-download from the server. See also [**]
-            from_id = from_list[0];
-            incoming_origin = Contact::get_origin_by_id(context, from_id, &mut from_id_blocked)
         }
     }
 
@@ -161,7 +156,7 @@ pub fn dc_receive_imf(
             },
             &mut to_ids,
             &mut to_self,
-        );
+        )?;
     }
 
     // Add parts
@@ -175,14 +170,13 @@ pub fn dc_receive_imf(
             match dc_create_incoming_rfc724_mid(sent_timestamp, from_id, &to_ids) {
                 Some(x) => x,
                 None => {
-                    error!(context, "can not create incoming rfc724_mid");
                     cleanup(
                         context,
                         &create_event_to_send,
                         &created_db_entries,
                         &rr_event_to_send,
                     );
-                    return;
+                    bail!("could not create incoming rfc724_mid");
                 }
             }
         }
@@ -211,15 +205,13 @@ pub fn dc_receive_imf(
             &mut created_db_entries,
             &mut create_event_to_send,
         ) {
-            warn!(context, "add_parts error: {:?}", err);
-
             cleanup(
                 context,
                 &create_event_to_send,
                 &created_db_entries,
                 &rr_event_to_send,
             );
-            return;
+            bail!("add_parts error: {:?}", err);
         }
     } else {
         // there are no non-meta data in message, do some basic calculations so that the varaiables
@@ -272,6 +264,8 @@ pub fn dc_receive_imf(
         &created_db_entries,
         &rr_event_to_send,
     );
+
+    Ok(())
 }
 
 fn add_parts(
@@ -282,7 +276,7 @@ fn add_parts(
     incoming_origin: &mut Origin,
     server_folder: impl AsRef<str>,
     server_uid: u32,
-    to_ids: &mut Vec<u32>,
+    to_ids: &mut ContactIds,
     rfc724_mid: &str,
     sent_timestamp: &mut i64,
     from_id: &mut u32,
@@ -321,7 +315,7 @@ fn add_parts(
             },
             to_ids,
             &mut false,
-        );
+        )?;
     }
 
     // check, if the mail is already in our database - if so, just update the folder/uid
@@ -506,7 +500,7 @@ fn add_parts(
         state = MessageState::OutDelivered;
         *from_id = DC_CONTACT_ID_SELF;
         if !to_ids.is_empty() {
-            *to_id = to_ids[0];
+            *to_id = to_ids.get_index(0).cloned().unwrap_or_default();
             if *chat_id == 0 {
                 let (new_chat_id, new_chat_id_blocked) = create_or_lookup_group(
                     context,
@@ -795,7 +789,7 @@ fn create_or_lookup_group(
     allow_creation: bool,
     create_blocked: Blocked,
     from_id: u32,
-    to_ids: &[u32],
+    to_ids: &ContactIds,
 ) -> Result<(u32, Blocked)> {
     let mut chat_id_blocked = Blocked::Not;
     let to_ids_cnt = to_ids.len();
@@ -1147,7 +1141,7 @@ fn create_or_lookup_adhoc_group(
     allow_creation: bool,
     create_blocked: Blocked,
     from_id: u32,
-    to_ids: &[u32],
+    to_ids: &ContactIds,
 ) -> Result<(u32, Blocked)> {
     // if we're here, no grpid was found, check if there is an existing
     // ad-hoc group matching the to-list or if we should and can create one
@@ -1163,7 +1157,7 @@ fn create_or_lookup_adhoc_group(
         return Ok((0, Blocked::Not));
     }
 
-    let mut member_ids = to_ids.to_vec();
+    let mut member_ids: Vec<u32> = to_ids.iter().copied().collect();
     if !member_ids.contains(&from_id) {
         member_ids.push(from_id);
     }
@@ -1382,7 +1376,7 @@ fn check_verified_properties(
     context: &Context,
     mimeparser: &MimeParser,
     from_id: u32,
-    to_ids: &[u32],
+    to_ids: &ContactIds,
 ) -> Result<()> {
     let contact = Contact::load_from_db(context, from_id)?;
 
@@ -1584,41 +1578,48 @@ fn dc_add_or_lookup_contacts_by_address_list(
     context: &Context,
     addr_list_raw: &str,
     origin: Origin,
-    ids: &mut Vec<u32>,
+    to_ids: &mut ContactIds,
     check_self: &mut bool,
-) {
+) -> Result<()> {
-    let addrs = mailparse::addrparse(addr_list_raw);
+    let addrs = match mailparse::addrparse(addr_list_raw) {
-    if addrs.is_err() {
+        Ok(addrs) => addrs,
-        return;
+        Err(err) => {
-    }
+            bail!("could not parse {:?}: {:?}", addr_list_raw, err);
-    info!(context, "dc_add_or_lookup_contacts_by_address-list={:?}", addr_list_raw);
+        }
-    info!(context, "addrs={:?}", addrs);
+    };
-    for addr in addrs.unwrap().iter() {
+
+    info!(
+        context,
+        "dc_add_or_lookup_contacts_by_address raw={:?} addrs={:?}", addr_list_raw, addrs
+    );
+    for addr in addrs.iter() {
         match addr {
             mailparse::MailAddr::Single(info) => {
-                add_or_lookup_contact_by_addr(
+                let contact_id =
-                    context,
+                    add_or_lookup_contact_by_addr(context, &info.display_name, &info.addr, origin)?;
-                    &info.display_name,
+                to_ids.insert(contact_id);
-                    &info.addr,
+                if contact_id == DC_CONTACT_ID_SELF {
-                    origin,
+                    *check_self = true;
-                    ids,
+                }
-                    check_self,
-                );
             }
             mailparse::MailAddr::Group(infos) => {
                 for info in &infos.addrs {
-                    add_or_lookup_contact_by_addr(
+                    let contact_id = add_or_lookup_contact_by_addr(
                         context,
                         &info.display_name,
                         &info.addr,
                         origin,
-                        ids,
+                    )?;
-                        check_self,
+                    to_ids.insert(contact_id);
-                    );
+                    if contact_id == DC_CONTACT_ID_SELF {
+                        *check_self = true;
+                    }
                 }
             }
         }
     }
+
+    Ok(())
 }
 
 /// Add contacts to database on receiving messages.
@@ -1627,24 +1628,11 @@ fn add_or_lookup_contact_by_addr(
     display_name: &Option<String>,
     addr: &str,
     origin: Origin,
-    ids: &mut Vec<u32>,
+) -> Result<u32> {
-    check_self: &mut bool,
+    if context.is_self_addr(addr)? {
-) {
+        return Ok(DC_CONTACT_ID_SELF);
-    // is addr_spec equal to SELF?
-    let self_addr = context
-        .get_config(Config::ConfiguredAddr)
-        .unwrap_or_default();
-
-    if addr_cmp(self_addr, addr) {
-        *check_self = true;
     }
 
-    /*
-    if *check_self {
-        return;
-    }
-    */
-
     // add addr_spec if missing, update otherwise
     let display_name_normalized = display_name
         .as_ref()
@@ -1652,14 +1640,31 @@ fn add_or_lookup_contact_by_addr(
         .unwrap_or_default();
 
     // can be NULL
-    info!(context, "looking up addr={:?} display_name={:?}", addr, display_name_normalized);
+    info!(
-    let row_id = Contact::add_or_lookup(context, display_name_normalized, addr, origin)
+        context,
-        .map(|(id, _)| id)
+        "looking up addr={:?} display_name={:?}", addr, display_name_normalized
-        .unwrap_or_default();
+    );
+    let (row_id, _modified) =
+        Contact::add_or_lookup(context, display_name_normalized, addr, origin)?;
+    ensure!(row_id > 0, "could not add contact: {:?}", addr);
 
-    if 0 != row_id && !ids.contains(&row_id) {
+    Ok(row_id)
-        ids.push(row_id);
+}
-    };
+
+fn dc_create_incoming_rfc724_mid(
+    message_timestamp: i64,
+    contact_id_from: u32,
+    contact_ids_to: &ContactIds,
+) -> Option<String> {
+    /* create a deterministic rfc724_mid from input such that
+    repeatedly calling it with the same input results in the same Message-id */
+
+    let largest_id_to = contact_ids_to.iter().max().copied().unwrap_or_default();
+    let result = format!(
+        "{}-{}-{}@stub",
+        message_timestamp, contact_id_from, largest_id_to
+    );
+    Some(result)
 }
 
 #[cfg(test)]
@@ -1704,4 +1709,24 @@ mod tests {
         assert_eq!(extract_grpid(&mimeparser, HeaderDef::InReplyTo), grpid);
         assert_eq!(extract_grpid(&mimeparser, HeaderDef::References), grpid);
     }
+
+    #[test]
+    fn test_dc_create_incoming_rfc724_mid() {
+        let mut members = ContactIds::new();
+        assert_eq!(
+            dc_create_incoming_rfc724_mid(123, 45, &members),
+            Some("123-45-0@stub".into())
+        );
+        members.insert(7);
+        members.insert(3);
+        assert_eq!(
+            dc_create_incoming_rfc724_mid(123, 45, &members),
+            Some("123-45-7@stub".into())
+        );
+        members.insert(9);
+        assert_eq!(
+            dc_create_incoming_rfc724_mid(123, 45, &members),
+            Some("123-45-9@stub".into())
+        );
+    }
 }

src/dc_tools.rs

@@ -182,22 +182,6 @@ fn encode_66bits_as_base64(v1: u32, v2: u32, fill: u32) -> String {
     String::from_utf8(wrapped_writer).unwrap()
 }
 
-pub(crate) fn dc_create_incoming_rfc724_mid(
-    message_timestamp: i64,
-    contact_id_from: u32,
-    contact_ids_to: &[u32],
-) -> Option<String> {
-    /* create a deterministic rfc724_mid from input such that
-    repeatedly calling it with the same input results in the same Message-id */
-
-    let largest_id_to = contact_ids_to.iter().max().copied().unwrap_or_default();
-    let result = format!(
-        "{}-{}-{}@stub",
-        message_timestamp, contact_id_from, largest_id_to
-    );
-    Some(result)
-}
-
 /// Function generates a Message-ID that can be used for a new outgoing message.
 /// - this function is called for all outgoing messages.
 /// - the message ID should be globally unique
@@ -780,14 +764,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn test_dc_create_incoming_rfc724_mid() {
-        let res = dc_create_incoming_rfc724_mid(123, 45, &[6, 7]);
-        assert_eq!(res, Some("123-45-7@stub".into()));
-        let res = dc_create_incoming_rfc724_mid(123, 45, &[]);
-        assert_eq!(res, Some("123-45-0@stub".into()));
-    }
-
     #[test]
     fn test_file_get_safe_basename() {
         assert_eq!(get_safe_basename("12312/hello"), "hello");

src/error.rs

@@ -28,12 +28,14 @@ pub enum Error {
     InvalidMsgId,
     #[fail(display = "Watch folder not found {:?}", _0)]
     WatchFolderNotFound(String),
-    #[fail(display = "Inalid Email: {:?}", _0)]
+    #[fail(display = "Invalid Email: {:?}", _0)]
     MailParseError(#[cause] mailparse::MailParseError),
     #[fail(display = "Building invalid Email: {:?}", _0)]
     LettreError(#[cause] lettre_email::error::Error),
     #[fail(display = "FromStr error: {:?}", _0)]
     FromStr(#[cause] mime::FromStrError),
+    #[fail(display = "Not Configured")]
+    NotConfigured,
 }
 
 pub type Result<T> = std::result::Result<T, Error>;

src/imap/mod.rs

@@ -713,7 +713,17 @@ impl Imap {
 
             if !is_deleted && msg.body().is_some() {
                 let body = msg.body().unwrap_or_default();
-                dc_receive_imf(context, &body, folder.as_ref(), server_uid, flags as u32);
+                if let Err(err) =
+                    dc_receive_imf(context, &body, folder.as_ref(), server_uid, flags as u32)
+                {
+                    warn!(
+                        context,
+                        "dc_receive_imf failed for imap-message {}/{}: {:?}",
+                        folder.as_ref(),
+                        server_uid,
+                        err
+                    );
+                }
             }
         }
 

src/mimefactory.rs

@@ -394,7 +394,7 @@ impl<'a, 'b> MimeFactory<'a, 'b> {
                 to.push(Address::new_mailbox(addr.clone()));
             } else {
                 to.push(Address::new_mailbox_with_name(
-                    name.to_string(), 
+                    name.to_string(),
                     addr.clone(),
                 ));
             }

src/mimeparser.rs

@@ -338,6 +338,10 @@ impl<'a> MimeParser<'a> {
         self.header.contains_key("chat-version")
     }
 
+    pub(crate) fn has_headers(&self) -> bool {
+        !self.header.is_empty()
+    }
+
     pub(crate) fn get_subject(&self) -> Option<String> {
         if let Some(s) = self.get(HeaderDef::Subject) {
             if s.is_empty() {

src/securejoin.rs

@@ -604,7 +604,7 @@ pub(crate) fn handle_securejoin_handshake(
                 .get(HeaderDef::ChatGroupMemberAdded)
                 .map(|s| s.as_str())
                 .unwrap_or_else(|| "");
-            if join_vg && !addr_equals_self(context, cg_member_added) {
+            if join_vg && !context.is_self_addr(cg_member_added)? {
                 info!(context, "Message belongs to a different handshake (scaled up contact anyway to allow creation of group).");
                 return Ok(ret);
             }