Merge lp:~thumper/launchpad/fix-mailman-excessive-queries into lp:launchpad

[Tim Penhey]

Make getMessageDispositions much more efficient in the number of DB queries.

The current implementation of getMessageDispositions on the MailingList xmlrpc api is excessively simplistic in its implementation. It gets an unlimited number of MessageApproval objects and then iterates over them both updating a status and querying for the team name.

This causes OOPSes like OOPS-1758XMLP341.

I changed the implementation of the IMessageApprovalSet to do the joins and return just the message_id and team names as needed. I also implemented a new method that does a set based update.

tests:
  message-holds
  mailinglists

[Brad Crittenden (bac)]

Really nice branch Tim.

I think you have a typo in your new method name that got propagated.
acknowledgeMessagesWithStats ->
acknowledgeMessagesWithStatus

review: Approve (code)

=== modified file 'lib/lp/registry/doc/message-holds.txt'
--- lib/lp/registry/doc/message-holds.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/registry/doc/message-holds.txt	2010-10-26 03:53:45 +0000
@@ -292,10 +292,10 @@
     ...     print message_id, list_name, subject
 
     >>> def print_messages(status):
-    ...     held_messages = sorted(hold_set.getHeldMessagesWithStatus(status),
-    ...                            key=attrgetter('message_id'))
-    ...     for message_hold in held_messages:
-    ...         print_hold(message_hold)
+    ...     held_messages = sorted(hold_set.getHeldMessagesWithStatus(status))
+    ...     for message_id, team_name in held_messages:
+    ...         held_message = hold_set.getMessageByMessageID(message_id)
+    ...         print_hold(held_message)
 
 Here are all the messages pending approval...
 

=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py	2010-09-17 03:58:05 +0000
+++ lib/lp/registry/interfaces/mailinglist.py	2010-10-26 03:53:45 +0000
@@ -808,6 +808,17 @@
         :rtype: sequence of MessageApproval
         """
 
+    def acknowledgeMessagesWithStatus(status):
+        """Acknowledge all the MessageApprovals with the matching status.
+
+        This changes the statuses APPROVAL_PENDING to APPROVED,
+        REJECTION_PENDING to REJECTED and DISCARD_PENDING to DISCARD.  It is
+        illegal to call this function when the status is not one of these
+        states.
+
+        :param status: A PostedMessageStatus enum value.
+        """
+
 
 class IHeldMessageDetails(Interface):
     """Details on a held message.

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2010-09-17 03:58:05 +0000
+++ lib/lp/registry/model/mailinglist.py	2010-10-26 03:53:45 +0000
@@ -73,6 +73,7 @@
     EmailAddressStatus,
     IEmailAddressSet,
     )
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector,
     MAIN_STORE,
@@ -887,7 +888,34 @@
 
     def getHeldMessagesWithStatus(self, status):
         """See `IMessageApprovalSet`."""
-        return MessageApproval.selectBy(status=status)
+        # Use the master store as the messages will also be acknowledged and
+        # we want to make sure we are acknowledging the same messages that we
+        # iterate over.
+        return IMasterStore(MessageApproval).find(
+            (Message.rfc822msgid, Person.name),
+            MessageApproval.status == status,
+            MessageApproval.message == Message.id,
+            MessageApproval.mailing_list == MailingList.id,
+            MailingList.team == Person.id)
+
+    def acknowledgeMessagesWithStatus(self, status):
+        """See `IMessageApprovalSet`."""
+        transitions = {
+            PostedMessageStatus.APPROVAL_PENDING:
+                PostedMessageStatus.APPROVED,
+            PostedMessageStatus.REJECTION_PENDING:
+                PostedMessageStatus.REJECTED,
+            PostedMessageStatus.DISCARD_PENDING:
+                PostedMessageStatus.DISCARDED,
+            }
+        try:
+            next_state = transitions[status]
+        except KeyError:
+            raise AssertionError(
+                'Not an acknowledgeable state: %s' % status)
+        approvals = IMasterStore(MessageApproval).find(
+            MessageApproval, MessageApproval.status == status)
+        approvals.set(status=next_state)
 
 
 class HeldMessageDetails:

=== modified file 'lib/lp/registry/tests/mailinglists_helper.py'
--- lib/lp/registry/tests/mailinglists_helper.py	2010-09-20 19:21:57 +0000
+++ lib/lp/registry/tests/mailinglists_helper.py	2010-10-26 03:53:45 +0000
@@ -263,15 +263,10 @@
             mailing_list.transitionToStatus(MailingListStatus.ACTIVE)
         # Simulate acknowledging held messages.
         message_set = getUtility(IMessageApprovalSet)
-        message_ids = set()
         for status in (PostedMessageStatus.APPROVAL_PENDING,
                        PostedMessageStatus.REJECTION_PENDING,
                        PostedMessageStatus.DISCARD_PENDING):
-            for message in message_set.getHeldMessagesWithStatus(status):
-                message_ids.add(message.message_id)
-        for message_id in message_ids:
-            message = message_set.getMessageByMessageID(message_id)
-            message.acknowledge()
+            message_set.acknowledgeMessagesWithStatus(status)
 
 
 mailman = MailmanStub()

=== modified file 'lib/lp/registry/xmlrpc/mailinglist.py'
--- lib/lp/registry/xmlrpc/mailinglist.py	2010-10-03 15:30:06 +0000
+++ lib/lp/registry/xmlrpc/mailinglist.py	2010-10-26 03:53:45 +0000
@@ -275,9 +275,8 @@
             (PostedMessageStatus.DISCARD_PENDING, 'discard'),
             )
         for status, disposition in status_dispositions:
-            for held_message in message_set.getHeldMessagesWithStatus(status):
-                held_message.acknowledge()
-                response[held_message.message_id] = (
-                    removeSecurityProxy(held_message.mailing_list.team).name,
-                    disposition)
+            held_messages = message_set.getHeldMessagesWithStatus(status)
+            for message_id, team_name in held_messages:
+                response[message_id] = (team_name, disposition)
+            message_set.acknowledgeMessagesWithStatus(status)
         return response