launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug811447 into lp:launchpad with lp:~gary/launchpad/bug838869 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #811447 in Launchpad itself: "BugTask:+index timeout - death by sql in PersonSubscriptions(user, bug)"
  https://bugs.launchpad.net/launchpad/+bug/811447

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug811447/+merge/73704

This branch fixes bug 811447 by drastically reducing the number of SQL needed for users who are team administrators and view a bug with many duplicates to which one or more of their teams is subscribed.  It now takes a single query.  This is done by converting the resultset of administrated teams to a list of administrated team ids, and then that list is consulted, rather than the result set.

Lint is happy.

This depends on lp:~gary/launchpad/bug838869 for a tool I used as test fixture.

Thank you.
-- 
https://code.launchpad.net/~gary/launchpad/bug811447/+merge/73704
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug811447 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/personsubscriptioninfo.py'
--- lib/lp/bugs/model/personsubscriptioninfo.py	2011-05-17 12:49:03 +0000
+++ lib/lp/bugs/model/personsubscriptioninfo.py	2011-09-01 17:23:38 +0000
@@ -59,9 +59,9 @@
 
     implements(IAbstractSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         self.person = person
-        self.administrated_teams = administrated_teams
+        self.administrated_team_ids = administrated_team_ids
         self.personal = []
         self.as_team_member = []
         self.as_team_admin = []
@@ -72,7 +72,7 @@
             collection = self.personal
         else:
             assert principal.isTeam(), (principal, self.person)
-            if principal in self.administrated_teams:
+            if principal.id in self.administrated_team_ids:
                 collection = self.as_team_admin
             else:
                 collection = self.as_team_member
@@ -88,9 +88,9 @@
 
     implements(IVirtualSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         super(VirtualSubscriptionInfoCollection, self).__init__(
-            person, administrated_teams)
+            person, administrated_team_ids)
         self._principal_pillar_to_info = {}
 
     def _add_item_to_collection(self,
@@ -110,9 +110,9 @@
 
     implements(IRealSubscriptionInfoCollection)
 
-    def __init__(self, person, administrated_teams):
+    def __init__(self, person, administrated_team_ids):
         super(RealSubscriptionInfoCollection, self).__init__(
-            person, administrated_teams)
+            person, administrated_team_ids)
         self._principal_bug_to_infos = {}
 
     def _add_item_to_collection(self, collection, principal,
@@ -189,13 +189,13 @@
             TeamParticipation.teamID == Person.id)
 
         direct = RealSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         duplicates = RealSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         bugs = set()
         for subscription, subscribed_bug, subscriber in info:
             bugs.add(subscribed_bug)
-            if subscribed_bug != bug:
+            if subscribed_bug.id != bug.id:
                 # This is a subscription through a duplicate.
                 collection = duplicates
             else:
@@ -232,7 +232,8 @@
 
     def loadSubscriptionsFor(self, person, bug):
         self.person = person
-        self.administrated_teams = person.getAdministratedTeams()
+        self.administrated_team_ids = [
+            team.id for team in person.getAdministratedTeams()]
         self.bug = bug
 
         # First get direct and duplicate real subscriptions.
@@ -244,9 +245,9 @@
 
         # Then get owner and assignee virtual subscriptions.
         as_owner = VirtualSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         as_assignee = VirtualSubscriptionInfoCollection(
-            self.person, self.administrated_teams)
+            self.person, self.administrated_team_ids)
         for bugtask in bug.bugtasks:
             pillar = self._getTaskPillar(bugtask)
             owner = pillar.owner
@@ -301,7 +302,7 @@
                 }
         direct = {}
         from_duplicate = {}
-        as_owner = {} # This is an owner of a pillar with no bug supervisor.
+        as_owner = {}  # This is an owner of a pillar with no bug supervisor.
         as_assignee = {}
         subscription_data = {
             'direct': direct,

=== modified file 'lib/lp/bugs/model/tests/test_personsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-05-17 12:49:03 +0000
+++ lib/lp/bugs/model/tests/test_personsubscriptioninfo.py	2011-09-01 17:23:38 +0000
@@ -5,8 +5,10 @@
 
 __metaclass__ = type
 
+from testtools.matchers import LessThan
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.adapter import SQLLogger
 from canonical.testing import DatabaseFunctionalLayer
 from lp.bugs.interfaces.personsubscriptioninfo import (
     IRealSubscriptionInfo,
@@ -33,6 +35,20 @@
         self.bug = self.factory.makeBug()
         self.subscriptions = PersonSubscriptions(self.subscriber, self.bug)
 
+    def makeDuplicates(self, count=1, subscriber=None):
+        if subscriber is None:
+            subscriber = self.subscriber
+        if subscriber.isTeam():
+            subscribed_by = subscriber.teamowner
+        else:
+            subscribed_by = subscriber
+        duplicates = [self.factory.makeBug() for i in range(count)]
+        with person_logged_in(subscribed_by):
+            for duplicate in duplicates:
+                duplicate.markAsDuplicate(self.bug)
+                duplicate.subscribe(subscriber, subscribed_by)
+        return duplicates
+
     def assertCollectionsAreEmpty(self, except_=None):
         names = ('direct', 'from_duplicate', 'as_owner', 'as_assignee')
         assert except_ is None or except_ in names
@@ -284,10 +300,7 @@
 
     def test_duplicate_direct(self):
         # Subscribed directly to the duplicate bug.
-        duplicate = self.factory.makeBug()
-        with person_logged_in(self.subscriber):
-            duplicate.markAsDuplicate(self.bug)
-            duplicate.subscribe(self.subscriber, self.subscriber)
+        [duplicate] = self.makeDuplicates(count=1)
         # Load a `PersonSubscriptionInfo`s for subscriber and a bug.
         self.subscriptions.reload()
 
@@ -487,3 +500,36 @@
         self.subscriptions.reload()
 
         self.failUnless(self.subscriptions.muted)
+
+    def test_many_duplicate_team_admin_subscriptions_few_queries(self):
+        # This is related to bug 811447. The user is subscribed to a
+        # duplicate bug through team membership in which the user is an admin.
+        team = self.factory.makeTeam()
+        with person_logged_in(team.teamowner):
+            team.addMember(self.subscriber, team.teamowner,
+                           status=TeamMembershipStatus.ADMIN)
+        self.makeDuplicates(count=1, subscriber=team)
+        logger = SQLLogger()
+        with logger:
+            self.subscriptions.reload()
+        # This should produce a very small number of queries.
+        count_with_one_subscribed_duplicate = len(logger.queries)
+        self.assertThat(count_with_one_subscribed_duplicate, LessThan(5))
+        # It should have the correct result.
+        self.assertCollectionsAreEmpty(except_='from_duplicate')
+        self.assertCollectionContents(
+            self.subscriptions.from_duplicate, as_team_admin=1)
+        # If we increase the number of duplicates subscribed via the team that
+        # the user administers...
+        self.makeDuplicates(count=4, subscriber=team)
+        with logger:
+            self.subscriptions.reload()
+        # ...then the query count should remain the same.
+        count_with_five_subscribed_duplicates = len(logger.queries)
+        self.assertEqual(
+            count_with_one_subscribed_duplicate,
+            count_with_five_subscribed_duplicates)
+        # We should still have the correct result.
+        self.assertCollectionsAreEmpty(except_='from_duplicate')
+        self.assertCollectionContents(
+            self.subscriptions.from_duplicate, as_team_admin=5)