Use passive_delete to handle LabelItem cascade (#1077)

Author: mrhiggi-close

inbox/mailsync/backends/gmail.py

@@ -20,6 +20,7 @@
 
 """
 
+import itertools
 from collections import OrderedDict
 from datetime import datetime, timedelta
 from threading import Semaphore
@@ -173,7 +174,7 @@ def initial_sync_impl(self, crispin_client: "CrispinClient") -> None:
                 # schedule change_poller to die
                 change_poller.kill()
 
-    def resync_uids_impl(self) -> None:
+    def resync_uids_impl(self, chunk_size: int = 1000) -> None:
         with session_scope(self.namespace_id) as db_session:
             imap_folder_info_entry = (
                 db_session.query(ImapFolderInfo)
@@ -188,6 +189,7 @@ def resync_uids_impl(self) -> None:
                     self.folder_name, lambda *args: True
                 )
                 uidvalidity = crispin_client.selected_uidvalidity
+                remote_uidnext = crispin_client.selected_uidnext
                 if uidvalidity <= imap_folder_info_entry.uidvalidity:
                     # if the remote UIDVALIDITY is less than or equal to -
                     # from my (siro) understanding it should not be less than -
@@ -221,8 +223,7 @@ def resync_uids_impl(self) -> None:
                     "FORCE INDEX (ix_imapuid_account_id_folder_id_msg_uid_desc)",
                 )
             )
-
-            chunk_size = 1000
+            imap_uids_to_delete = []
             for entry in imap_uid_entries.yield_per(chunk_size):
                 if entry.message.g_msgid in mapping:
                     log.debug(
@@ -234,14 +235,22 @@ def resync_uids_impl(self) -> None:
                     )
                     entry.msg_uid = mapping[entry.message.g_msgid]
                 else:
-                    db_session.delete(entry)
+                    imap_uids_to_delete.append(entry.msg_uid)
             log.debug(
                 "UIDVALIDITY from {} to {}".format(  # noqa: G001
                     imap_folder_info_entry.uidvalidity, uidvalidity
                 )
             )
+            for uid_batch in itertools.batched(
+                imap_uids_to_delete, chunk_size
+            ):
+                for uid_to_delete in db_session.query(ImapUid).filter(
+                    ImapUid.msg_uid.in_(uid_batch)
+                ):
+                    db_session.delete(uid_to_delete)
             imap_folder_info_entry.uidvalidity = uidvalidity
             imap_folder_info_entry.highestmodseq = None
+            imap_folder_info_entry.uidnext = remote_uidnext
             db_session.commit()
 
     def __deduplicate_message_object_creation(  # type: ignore[no-untyped-def]
@@ -470,15 +479,15 @@ def g_msgids(namespace_id, session, in_):  # type: ignore[no-untyped-def]  # noq
             .filter(Message.namespace_id == namespace_id)
             .all()
         )
-        return sorted(g_msgid for g_msgid, in query if g_msgid in in_)
+        return sorted(g_msgid for (g_msgid,) in query if g_msgid in in_)
     # But in the normal case that in_ only has a few elements, it's way better
     # to not fetch a bunch of values from MySQL only to return a few of them.
     query = (
         session.query(Message.g_msgid)
         .filter(Message.namespace_id == namespace_id, Message.g_msgid.in_(in_))
         .all()
     )
-    return {g_msgid for g_msgid, in query}
+    return {g_msgid for (g_msgid,) in query}
 
 
 class GmailSyncMonitor(ImapSyncMonitor):

tests/imap/test_folder_sync.py

@@ -428,3 +428,69 @@ def test_imap_message_deduplication(
         .count()
         == 1
     )
+
+
+def test_gmail_handle_uidinvalid(
+    db, default_account, all_mail_folder, mock_imapclient, label
+) -> None:
+    """
+    Tests when a GMail account's UID validity changes. In this case we attempt
+    to resync existing messages by their google message ID. If this fails, we
+    delete the message and trust that we'll sync it back in if it still exists.
+
+    When deleting these messages, during the `yield_chunk`, it was possible for
+    a `sqlalchemy.exc.ProgrammingError: (MySQLdb.ProgrammingError)
+    (2014, "Commands out of sync; you can't run this command now")` to be raised.
+
+    This error is raised when an association attempts a cascade delete from
+    within the ORM. Adding label associations triggers the error. To resolve
+    this, we can use `passive_deletes=True` in the `relationship` call.
+    """
+    uid_dict = uids.example()
+    mock_imapclient.add_folder_data(all_mail_folder.name, uid_dict)
+    mock_imapclient.list_folders = lambda: [
+        ((b"\\All", b"\\HasNoChildren"), b"/", "[Gmail]/All Mail")
+    ]
+    mock_imapclient.idle = lambda: None
+    mock_imapclient.idle_check = raise_imap_error
+
+    folder_sync_engine = GmailFolderSyncEngine(
+        default_account.id,
+        default_account.namespace.id,
+        all_mail_folder.name,
+        default_account.email_address,
+        "gmail",
+        BoundedSemaphore(1),
+    )
+    folder_sync_engine.initial_sync()
+
+    # We need to add labels to our imap uids so that the cascade delete on
+    # LabelItems is required. This would trigger a deletion during the yield_chunk
+    # query if any ORM level cascades are triggered.
+    imap_uids = (
+        db.session.query(ImapUid)
+        .filter(ImapUid.folder_id == all_mail_folder.id)
+        .limit(5)
+        .all()
+    )
+
+    assert len(imap_uids) == 5
+    for uid in imap_uids:
+        uid.labels.add(label)
+
+    db.session.commit()
+
+    mock_imapclient.uidvalidity = 2
+    with pytest.raises(UidInvalid):
+        folder_sync_engine.poll_impl()
+
+    # chunk_size must be set small (default is 1000) so that we actually have
+    # more than one chunk, keeping the query open.
+    folder_sync_engine.resync_uids_impl(chunk_size=1)
+
+    assert (
+        db.session.query(ImapUid)
+        .filter(ImapUid.folder_id == all_mail_folder.id)
+        .all()
+        == []
+    )