Merge lp:~gary/launchpad/bug741684-2 into lp:launchpad

[Gary Poster]

[r=jcsackett][bug=741684,742230] Optimizations for the DB usage and performance of the bug notifications script.

This branch is the second of two that I have in mind to address the linked bugs. The other branch is the one on which this one depends, lp:~gary/launchpad/bug741684. The only additional change I know of at the moment may be to increase the Storm cache size, as discussed in the MP for the other branch.

This branch replaces canonical.launchpad.helpers.emailPeople with lib.lp.registry.model.person.get_recipients. The change in code is to make fewer passes through the transitive search, and to take advantage of the IPersonSet.getPrecachedPersonsFromIDs method to early-load email information that we need. The change in module is because the new code uses database tools that are not allowed to be imported from the scripts location. The change in name was because I already had to change all the call sites, and I thought it was an improved name, since I thought it more clearly described the function's purpose.

I briefly toyed with the idea of making this a method on the Person class. I really have wanted to see a move to tightly-focused data-based models, though, so I preferred to make this a function. That so, if you, the reviewer, request that it be a method, I will make it so with nary a complaint.

When migrating the tests of the old function to the new location, I expanded them. The test_get_recipients_complex_indirect in particular I used in experiments to show that even in a relatively simple case like that I was reducing database calls by half. I'm hopeful that in real-world usage, the change will be significantly more dramatic.

I incorporated the new version of the function everywhere the old one was used; I am running tests now to see if this causes any unpleasant hiccups. As you might expect, the usage I'm most interested in right now is the one in lib/lp/bugs/scripts/bugnotification.py.

Thank you for your time.

Gary

[j.c.sackett (jcsackett)]

This looks good to land.

review: Approve

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-03-29 03:35:23 +0000
+++ database/schema/security.cfg	2011-03-31 13:56:01 +0000
@@ -721,6 +721,7 @@
 [distributionmirror]
 type=user
 groups=script
+public.account                                  = SELECT
 public.archive                                  = SELECT
 public.archivearch                              = SELECT
 public.binarypackagefile                        = SELECT
@@ -1650,6 +1651,7 @@
 [translationstobranch]
 type=user
 groups=script
+public.account                          = SELECT
 public.branch                           = SELECT, UPDATE
 public.branchjob                        = SELECT, UPDATE, INSERT
 public.emailaddress                     = SELECT

=== modified file 'lib/canonical/launchpad/helpers.py'
--- lib/canonical/launchpad/helpers.py	2011-02-17 16:15:50 +0000
+++ lib/canonical/launchpad/helpers.py	2011-03-31 13:56:01 +0000
@@ -200,43 +200,20 @@
     return output
 
 
-def emailPeople(person):
-    """Return a set of people to who receive email for this Person.
-
-    If <person> has a preferred email, the set will contain only that
-    person.  If <person> doesn't have a preferred email but is a team,
-    the set will contain the preferred email address of each member of
-    <person>, including indirect members.
-
-    Finally, if <person> doesn't have a preferred email and is not a team,
-    the set will be empty.
-    """
-    pending_people = [person]
-    people = set()
-    seen = set()
-    while len(pending_people) > 0:
-        person = pending_people.pop()
-        if person in seen:
-            continue
-        seen.add(person)
-        if person.preferredemail is not None:
-            people.add(person)
-        elif person.isTeam():
-            pending_people.extend(person.activemembers)
-    return people
-
-
 def get_contact_email_addresses(person):
     """Return a set of email addresses to contact this Person.
 
-    In general, it is better to use emailPeople instead.
+    In general, it is better to use lp.registry.model.person.get_recipients
+    instead.
     """
     # Need to remove the security proxy of the email address because the
     # logged in user may not have permission to see it.
     from zope.security.proxy import removeSecurityProxy
+    # Circular imports force this import.
+    from lp.registry.model.person import get_recipients
     return set(
         str(removeSecurityProxy(mail_person.preferredemail).email)
-        for mail_person in emailPeople(person))
+        for mail_person in get_recipients(person))
 
 
 replacements = {0: {'.': ' |dot| ',

=== modified file 'lib/canonical/launchpad/tests/test_helpers.py'
--- lib/canonical/launchpad/tests/test_helpers.py	2010-10-21 04:19:36 +0000
+++ lib/canonical/launchpad/tests/test_helpers.py	2011-03-31 13:56:01 +0000
@@ -8,12 +8,9 @@
 from zope.publisher.interfaces.browser import IBrowserRequest
 
 from canonical.launchpad import helpers
-from canonical.launchpad.ftests import login
 from canonical.launchpad.webapp.interfaces import ILaunchBag
-from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.registry.interfaces.person import IPerson
 from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.testing.factory import LaunchpadObjectFactory
 from lp.translations.utilities.translation_export import LaunchpadWriteTarFile
 
 
@@ -298,41 +295,10 @@
         self.assertEqual(text, helpers.truncate_text(text, len(text)))
 
 
-class TestEmailPeople(unittest.TestCase):
-    """Tests for emailPeople"""
-
-    layer = LaunchpadFunctionalLayer
-
-    def setUp(self):
-        unittest.TestCase.setUp(self)
-        login('[e-mail]')
-        self.factory = LaunchpadObjectFactory()
-
-    def test_emailPeopleIndirect(self):
-        """Ensure emailPeople uses indirect memberships."""
-        owner = self.factory.makePerson(
-            displayname='Foo Bar', email='[e-mail]', password='password')
-        team = self.factory.makeTeam(owner)
-        super_team = self.factory.makeTeam(team)
-        recipients = helpers.emailPeople(super_team)
-        self.assertEqual(set([owner]), recipients)
-
-    def test_emailPeopleTeam(self):
-        """Ensure emailPeople uses teams with preferredemail."""
-        owner = self.factory.makePerson(
-            displayname='Foo Bar', email='[e-mail]', password='password')
-        team = self.factory.makeTeam(owner, email='[e-mail]')
-        super_team = self.factory.makeTeam(team)
-        recipients = helpers.emailPeople(super_team)
-        self.assertEqual(set([team]), recipients)
-
-
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(DocTestSuite())
     suite.addTest(DocTestSuite(helpers))
     suite.addTest(
         unittest.TestLoader().loadTestsFromTestCase(TruncateTextTest))
-    suite.addTest(
-        unittest.TestLoader().loadTestsFromTestCase(TestEmailPeople))
     return suite

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2011-03-31 13:55:39 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2011-03-31 13:56:01 +0000
@@ -18,10 +18,7 @@
 import transaction
 from zope.component import getUtility
 
-from canonical.launchpad.helpers import (
-    emailPeople,
-    get_email_template,
-    )
+from canonical.launchpad.helpers import get_email_template
 from canonical.launchpad.scripts.logger import log
 from canonical.launchpad.webapp import canonical_url
 from lp.bugs.mail.bugnotificationbuilder import (
@@ -30,6 +27,7 @@
     )
 from lp.bugs.mail.newbug import generate_bug_add_email
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
+from lp.registry.model.person import get_recipients
 from lp.services.mail.mailwrapper import MailWrapper
 
 
@@ -108,7 +106,7 @@
             # We will report this notification.
             filtered_notifications.append(notification)
             for recipient in notification.recipients:
-                email_people = emailPeople(recipient.person)
+                email_people = get_recipients(recipient.person)
                 for email_person in email_people:
                     recipients_for_person = recipients.get(email_person)
                     if recipients_for_person is None:
@@ -169,6 +167,7 @@
     cached_filters = {}
 
     bug_notification_set = getUtility(IBugNotificationSet)
+
     def get_filter_descriptions(recipients):
         "Given a list of recipients, return the filter descriptions for them."
         descriptions = set()

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-03-29 03:13:40 +0000
+++ lib/lp/registry/model/person.py	2011-03-31 13:56:01 +0000
@@ -7,6 +7,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'get_recipients',
     'generate_nick',
     'IrcID',
     'IrcIDSet',
@@ -57,6 +58,7 @@
     And,
     Desc,
     Exists,
+    In,
     Join,
     LeftJoin,
     Min,
@@ -4561,3 +4563,65 @@
     if person is None:
         raise ComponentLookupError()
     return person
+
+
+@ProxyFactory
+def get_recipients(person):
+    """Return a set of people who receive email for this Person (person/team).
+
+    If <person> has a preferred email, the set will contain only that
+    person.  If <person> doesn't have a preferred email but is a team,
+    the set will contain the preferred email address of each member of
+    <person>, including indirect members.
+
+    Finally, if <person> doesn't have a preferred email and is not a team,
+    the set will be empty.
+    """
+    if person.preferredemail:
+        return [person]
+    elif person.is_team:
+        # Get transitive members of team with a preferred email.
+        return _get_recipients_for_team(person)
+    else:
+        return []
+
+
+def _get_recipients_for_team(team):
+    """Given a team without a preferred email, return recipients.
+
+    Helper for get_recipients, divided out simply to make get_recipients
+    easier to understand in broad brush."""
+    store = IStore(Person)
+    source = store.using(TeamMembership,
+                         Join(Person,
+                              TeamMembership.personID==Person.id),
+                         LeftJoin(EmailAddress,
+                                  EmailAddress.person == Person.id))
+    pending_team_ids = [team.id]
+    recipient_ids = set()
+    seen = set()
+    while pending_team_ids:
+        intermediate_transitive_results = source.find(
+            (TeamMembership.personID, EmailAddress.personID),
+            In(TeamMembership.status,
+               [TeamMembershipStatus.ADMIN.value,
+                TeamMembershipStatus.APPROVED.value]),
+            In(TeamMembership.teamID, pending_team_ids),
+            Or(EmailAddress.status == EmailAddressStatus.PREFERRED,
+               And(Not(Person.teamownerID == None),
+                   EmailAddress.status == None))).config(distinct=True)
+        next_ids = []
+        for (person_id,
+             preferred_email_marker) in intermediate_transitive_results:
+            if preferred_email_marker is None:
+                # This is a team without a preferred email address.
+                if person_id not in seen:
+                    next_ids.append(person_id)
+                    seen.add(person_id)
+            else:
+                recipient_ids.add(person_id)
+        pending_team_ids = next_ids
+    return getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+        recipient_ids,
+        need_validity=True,
+        need_preferred_email=True)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-03-26 16:28:39 +0000
+++ lib/lp/registry/tests/test_person.py	2011-03-31 13:56:01 +0000
@@ -35,6 +35,7 @@
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
     reconnect_stores,
     )
 from lp.answers.model.answercontact import AnswerContact
@@ -61,7 +62,10 @@
     KarmaCategory,
     KarmaTotalCache,
     )
-from lp.registry.model.person import Person
+from lp.registry.model.person import (
+    get_recipients,
+    Person,
+    )
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -1293,3 +1297,67 @@
         # https://bugs.launchpad.net/storm/+bug/619017 which is adding 3
         # queries to the test.
         self.assertThat(collector, HasQueryCount(LessThan(14)))
+
+
+class TestGetRecipients(TestCaseWithFactory):
+    """Tests for get_recipients"""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGetRecipients, self).setUp()
+        login('[e-mail]')
+
+    def test_get_recipients_indirect(self):
+        """Ensure get_recipients uses indirect memberships."""
+        owner = self.factory.makePerson(
+            displayname='Foo Bar', email='[e-mail]', password='password')
+        team = self.factory.makeTeam(owner)
+        super_team = self.factory.makeTeam(team)
+        recipients = get_recipients(super_team)
+        self.assertEqual(set([owner]), set(recipients))
+
+    def test_get_recipients_team(self):
+        """Ensure get_recipients uses teams with preferredemail."""
+        owner = self.factory.makePerson(
+            displayname='Foo Bar', email='[e-mail]', password='password')
+        team = self.factory.makeTeam(owner, email='[e-mail]')
+        super_team = self.factory.makeTeam(team)
+        recipients = get_recipients(super_team)
+        self.assertEqual(set([team]), set(recipients))
+
+    def makePersonWithNoPreferredEmail(self, **kwargs):
+        kwargs['email_address_status'] = EmailAddressStatus.NEW
+        return self.factory.makePerson(**kwargs)
+
+    def get_test_recipients_person(self):
+        person = self.factory.makePerson()
+        recipients = get_recipients(person)
+        self.assertEqual(set([person]), set(recipients))
+
+    def test_get_recipients_empty(self):
+        """get_recipients returns empty set for person with no preferredemail.
+        """
+        recipients = get_recipients(self.makePersonWithNoPreferredEmail())
+        self.assertEqual(set(), set(recipients))
+
+    def test_get_recipients_complex_indirect(self):
+        """Ensure get_recipients uses indirect memberships."""
+        owner = self.factory.makePerson(
+            displayname='Foo Bar', email='[e-mail]', password='password')
+        team = self.factory.makeTeam(owner)
+        super_team_member_person = self.factory.makePerson(
+            displayname='Bing Bar', email='[e-mail]')
+        super_team_member_team = self.factory.makeTeam(
+            email='[e-mail]')
+        super_team = self.factory.makeTeam(
+            team, members=[super_team_member_person,
+                           super_team_member_team,
+                           self.makePersonWithNoPreferredEmail()])
+        super_team_member_team.acceptInvitationToBeMemberOf(
+            super_team, u'Go Team!')
+        recipients = list(get_recipients(super_team))
+        self.assertEqual(set([owner,
+                              super_team_member_person,
+                              super_team_member_team]),
+                         set(recipients))

=== modified file 'lib/lp/services/mail/notificationrecipientset.py'
--- lib/lp/services/mail/notificationrecipientset.py	2010-12-02 16:13:51 +0000
+++ lib/lp/services/mail/notificationrecipientset.py	2011-03-31 13:56:01 +0000
@@ -14,7 +14,6 @@
 from zope.interface import implements
 from zope.security.proxy import isinstance as zope_isinstance
 
-from canonical.launchpad.helpers import emailPeople
 from canonical.launchpad.interfaces.launchpad import (
     INotificationRecipientSet,
     UnknownRecipientError,
@@ -45,7 +44,7 @@
     def getRecipients(self):
         """See `INotificationRecipientSet`."""
         return sorted(
-            self._personToRationale.keys(),  key=attrgetter('displayname'))
+            self._personToRationale.keys(), key=attrgetter('displayname'))
 
     def getRecipientPersons(self):
         """See `INotificationRecipientSet`."""
@@ -88,6 +87,7 @@
     def add(self, persons, reason, header):
         """See `INotificationRecipientSet`."""
         from zope.security.proxy import removeSecurityProxy
+        from lp.registry.model.person import get_recipients
         if IPerson.providedBy(persons):
             persons = [persons]
 
@@ -98,7 +98,7 @@
             if person in self._personToRationale:
                 continue
             self._personToRationale[person] = reason, header
-            for receiving_person in emailPeople(person):
+            for receiving_person in get_recipients(person):
                 # Bypass zope's security because IEmailAddress.email is not
                 # public.
                 preferred_email = removeSecurityProxy(
@@ -116,6 +116,7 @@
     def remove(self, persons):
         """See `INotificationRecipientSet`."""
         from zope.security.proxy import removeSecurityProxy
+        from lp.registry.model.person import get_recipients
         if IPerson.providedBy(persons):
             persons = [persons]
         for person in persons:
@@ -123,7 +124,7 @@
                 'You can only remove() an IPerson: %r' % person)
             if person in self._personToRationale:
                 del self._personToRationale[person]
-            for removed_person in emailPeople(person):
+            for removed_person in get_recipients(person):
                 # Bypass zope's security because IEmailAddress.email is
                 # not public.
                 preferred_email = removeSecurityProxy(

=== modified file 'lib/lp/soyuz/scripts/ppareport.py'
--- lib/lp/soyuz/scripts/ppareport.py	2010-08-31 11:11:09 +0000
+++ lib/lp/soyuz/scripts/ppareport.py	2011-03-31 13:56:01 +0000
@@ -20,9 +20,9 @@
 from zope.component import getUtility
 
 from canonical.config import config
-from canonical.launchpad.helpers import emailPeople
 from canonical.launchpad.webapp import canonical_url
 from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.model.person import get_recipients
 from lp.services.propertycache import cachedproperty
 from lp.services.scripts.base import (
     LaunchpadScript,
@@ -181,7 +181,7 @@
         self.output.write('= PPA user emails =\n')
         people_to_email = set()
         for ppa in self.ppas:
-            people_to_email.update(emailPeople(ppa.owner))
+            people_to_email.update(get_recipients(ppa.owner))
         sorted_people_to_email = sorted(
             people_to_email, key=operator.attrgetter('name'))
         for user in sorted_people_to_email: