comments

inbox/mailsync/backends/imap/common.py

@@ -13,7 +13,6 @@
 """
 
 from collections import defaultdict
-from collections.abc import Iterable
 from datetime import datetime
 from typing import List, Set
 
@@ -244,7 +243,7 @@ def imapuids_for_message_query(
     return query
 
 
-def remove_deleted_uids(account_id: int, folder_id: int, uids: Iterable[int]) -> None:
+def remove_deleted_uids(account_id: int, folder_id: int, uids: set[int]) -> None:
     """
     Make sure you're holding a db write lock on the account. (We don't try
     to grab the lock in here in case the caller needs to put higher-level
@@ -254,8 +253,8 @@ def remove_deleted_uids(account_id: int, folder_id: int, uids: Iterable[int]) ->
     if not uids:
         return
 
-    deleted_uid_count = 0
-
+    # First, get the category type i.e. folder or label for the account
+    # and message_id corresponding to the uids.
     with session_scope(account_id) as db_session:
         category_type = Account.get(account_id, db_session).category_type
 
@@ -272,11 +271,15 @@ def remove_deleted_uids(account_id: int, folder_id: int, uids: Iterable[int]) ->
             .all()
         )
 
+    # Group uids by message_id.
     uids_by_message_id = defaultdict(list)
     for message_id, message_uid in message_ids_and_uid:
         uids_by_message_id[message_id].append(message_uid)
 
     for message_id, message_uids in uids_by_message_id.items():
+        # Delete the uids in batches of 1000.
+        # This eases the load on MySQL if we are processing a large number of uids.
+        # Some non-conforming IMAP servers can list the same message thousands of times.
         for message_uid_batch in chunk(message_uids, 1000):
             with session_scope(account_id) as db_session:
                 db_session.query(ImapUid).filter(
@@ -291,44 +294,67 @@ def remove_deleted_uids(account_id: int, folder_id: int, uids: Iterable[int]) ->
                 )
                 db_session.commit()
 
-            deleted_uid_count += len(message_uids)
-
-        with session_scope(account_id) as db_session:
-            message = db_session.query(Message).get(message_id)
-
-            if message is not None:
-                message_imapuids_exist = db_session.query(
-                    imapuids_for_message_query(
-                        account_id=account_id, message_id=message.id
-                    ).exists()
-                ).scalar()
-
-                if not message_imapuids_exist and message.is_draft:
-                    # Synchronously delete drafts.
-                    thread = message.thread
-                    if thread is not None:
-                        thread.messages.remove(message)
-                        # Thread.messages relationship is versioned i.e. extra
-                        # logic gets executed on remove call.
-                        # This early flush is needed so the configure_versioning logic
-                        # in inbox.model.sessions can work reliably on newer versions of
-                        # SQLAlchemy.
-                        db_session.flush()
-                    db_session.delete(message)
-                    if thread is not None and not thread.messages:
-                        db_session.delete(thread)
-                else:
-                    update_message_metadata(
-                        db_session, account_id, category_type, message, message.is_draft
-                    )
-                    if not message_imapuids_exist:
-                        # But don't outright delete messages. Just mark them as
-                        # 'deleted' and wait for the asynchronous
-                        # dangling-message-collector to delete them.
-                        message.mark_for_deletion()
-
-            db_session.commit()
-    log.info("Deleted expunged UIDs", count=deleted_uid_count)
+        update_message_after_uid_deletion(
+            account_id=account_id, message_id=message_id, category_type=category_type
+        )
+
+    log.info(
+        "Deleted expunged UIDs",
+        uid_count=len(uids),
+        message_count=len(uids_by_message_id),
+    )
+
+
+def update_message_after_uid_deletion(
+    *, account_id: int, message_id: int, category_type: CategoryType
+) -> None:
+    """
+    Update the message after deleting UIDs.
+
+    If the message has no more UIDs, and it's a draft, delete it immediately.
+    If the message has no more UIDs, and it's not a draft, mark it for deletion.
+
+    Args:
+        account_id: The account ID.
+        message_id: The message ID.
+        category_type: The category type for the account i.e. folder or label.
+    """
+    with session_scope(account_id) as db_session:
+        message = db_session.query(Message).get(message_id)
+        if message is None:
+            return
+
+        message_imapuids_exist = db_session.query(
+            imapuids_for_message_query(
+                account_id=account_id, message_id=message.id
+            ).exists()
+        ).scalar()
+
+        if not message_imapuids_exist and message.is_draft:
+            # Synchronously delete drafts.
+            thread = message.thread
+            if thread is not None:
+                thread.messages.remove(message)
+                # Thread.messages relationship is versioned i.e. extra
+                # logic gets executed on remove call.
+                # This early flush is needed so the configure_versioning logic
+                # in inbox.model.sessions can work reliably on newer versions of
+                # SQLAlchemy.
+                db_session.flush()
+            db_session.delete(message)
+            if thread is not None and not thread.messages:
+                db_session.delete(thread)
+        else:
+            update_message_metadata(
+                db_session, account_id, category_type, message, message.is_draft
+            )
+            if not message_imapuids_exist:
+                # But don't outright delete messages. Just mark them as
+                # 'deleted' and wait for the asynchronous
+                # dangling-message-collector to delete them.
+                message.mark_for_deletion()
+
+        db_session.commit()
 
 
 def get_folder_info(account_id, session, folder_name):

tests/imap/test_delete_handling.py

@@ -37,7 +37,7 @@ def test_messages_deleted_asynchronously(
         db.session,
     )
     assert "label" in [cat.display_name for cat in message.categories]
-    remove_deleted_uids(default_account.id, folder.id, [msg_uid])
+    remove_deleted_uids(default_account.id, folder.id, {msg_uid})
     db.session.expire_all()
     assert abs((message.deleted_at - datetime.utcnow()).total_seconds()) < 2
     # Check that message categories do get updated synchronously.
@@ -50,7 +50,7 @@ def test_drafts_deleted_synchronously(
     message.is_draft = True
     db.session.commit()
     msg_uid = imapuid.msg_uid
-    remove_deleted_uids(default_account.id, folder.id, [msg_uid])
+    remove_deleted_uids(default_account.id, folder.id, {msg_uid})
     db.session.expire_all()
     with pytest.raises(ObjectDeletedError):
         message.id
@@ -72,7 +72,7 @@ def test_deleting_from_a_message_with_multiple_uids(
 
     assert len(message.imapuids) == 2
 
-    remove_deleted_uids(default_account.id, inbox_folder.id, [2222])
+    remove_deleted_uids(default_account.id, inbox_folder.id, {2222})
     db.session.expire_all()
 
     assert (