do_delete_messages: Archive the messages in bulk.

The test added in this commit shows 37 queries - compared to 181 without
the change to the function. That seems very much worth it.

zerver/lib/actions.py

@@ -4654,8 +4654,16 @@ def do_update_message(user_profile: UserProfile, message: Message, topic_name: O
     send_event(user_profile.realm, event, users_to_be_notified)
     return len(changed_messages)
 
-
 def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None:
+    message_ids = [message.id for message in messages]
+    usermessages = UserMessage.objects.filter(message_id__in=message_ids)
+    message_id_to_notifiable_users = {}  # type: Dict[int, List[Dict[str, int]]]
+    for um in usermessages:
+        if um.message_id not in message_id_to_notifiable_users:
+            message_id_to_notifiable_users[um.message_id] = []
+        message_id_to_notifiable_users[um.message_id].append({"id": um.user_profile_id})
+
+    events_and_users_to_notify = []
     for message in messages:
         message_type = "stream"
         if not message.is_stream_message():
@@ -4673,13 +4681,12 @@ def do_delete_messages(realm: Realm, messages: Iterable[Message]) -> None:
         else:
             event['recipient_id'] = message.recipient_id
 
-        # TODO: Each part of the following should be changed to bulk
+        events_and_users_to_notify.append((event, message_id_to_notifiable_users[message.id]))
-        # queries, since right now if you delete 1000 messages, you'll
+
-        # end up doing 1000 database queries in a loop and timing out.
+    move_messages_to_archive(message_ids)
-        ums = [{'id': um.user_profile_id} for um in
+    for event, users_to_notify in events_and_users_to_notify:
-               UserMessage.objects.filter(message=message.id)]
+        # TODO: Figure out some kind of bulk event that we could send just one of?
-        move_messages_to_archive([message.id])
+        send_event(realm, event, users_to_notify)
-        send_event(realm, event, ums)
 
 def do_delete_messages_by_sender(user: UserProfile) -> None:
     message_ids = Message.objects.filter(sender=user).values_list('id', flat=True).order_by('id')

zerver/tests/test_retention.py

@@ -6,8 +6,9 @@ from unittest import mock
 from django.conf import settings
 from django.utils.timezone import now as timezone_now
 
-from zerver.lib.actions import internal_send_private_message, do_add_submessage
+from zerver.lib.actions import internal_send_private_message, do_add_submessage, do_delete_messages
 from zerver.lib.test_classes import ZulipTestCase
+from zerver.lib.test_helpers import queries_captured
 from zerver.lib.upload import create_attachment
 from zerver.models import (Message, Realm, Stream, ArchivedUserMessage, SubMessage,
                            ArchivedMessage, Attachment, ArchivedAttachment, UserMessage,
@@ -743,3 +744,19 @@ class TestCleaningArchive(ArchiveMessagesTestingBase):
 
         for message in ArchivedMessage.objects.all():
             self.assertEqual(message.archive_transaction_id, remaining_transactions[0].id)
+
+class TestDoDeleteMessages(ZulipTestCase):
+    def test_do_delete_messages_multiple(self) -> None:
+        realm = get_realm("zulip")
+        cordelia_email = self.example_email('cordelia')
+        message_ids = [self.send_stream_message(cordelia_email, "Denmark", str(i)) for i in range(0, 10)]
+        messages = Message.objects.filter(id__in=message_ids)
+
+        with queries_captured() as queries:
+            do_delete_messages(realm, messages)
+        self.assertFalse(Message.objects.filter(id__in=message_ids).exists())
+        self.assert_length(queries, 37)
+
+        archived_messages = ArchivedMessage.objects.filter(id__in=message_ids)
+        self.assertEqual(archived_messages.count(), len(message_ids))
+        self.assertEqual(len({message.archive_transaction_id for message in archived_messages}), 1)