launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/preload-archive-subscriptions into lp:launchpad.

Commit message:
Preload ArchiveSubscriber.archive to make the query count of PublicOrPrivateTeamsExistence.checkAuthenticated more reasonable.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1428742 in Launchpad itself: "Time out which seems to be due to too many private ppas"
  https://bugs.launchpad.net/launchpad/+bug/1428742

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/preload-archive-subscriptions/+merge/252011

Preload ArchiveSubscriber.archive to make the query count of PublicOrPrivateTeamsExistence.checkAuthenticated more reasonable.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/preload-archive-subscriptions into lp:launchpad.

=== modified file 'lib/lp/app/tests/test_security.py'
--- lib/lp/app/tests/test_security.py	2015-02-23 03:22:37 +0000
+++ lib/lp/app/tests/test_security.py	2015-03-05 19:46:21 +0000
@@ -1,7 +1,9 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
+
+from testtools.matchers import Equals
 from zope.component import (
     getSiteManager,
     getUtility,
@@ -28,6 +30,7 @@
 from lp.testing import (
     admin_logged_in,
     person_logged_in,
+    record_two_runs,
     TestCase,
     TestCaseWithFactory,
     )
@@ -36,6 +39,7 @@
     DatabaseFunctionalLayer,
     ZopelessDatabaseLayer,
     )
+from lp.testing.matchers import HasQueryCount
 
 
 def registerFakeSecurityAdapter(interface, permission, adapter=None):
@@ -253,3 +257,30 @@
         self.assertTeamOwnerCanListPrivateTeamWithTeamStatus(
             TeamMembershipStatus.EXPIRED)
 
+    def test_private_team_query_count(self):
+        # Testing visibility of a private team involves checking for
+        # subscriptions to any private PPAs owned by that team.  Make sure
+        # that this doesn't involve a query for every archive subscription
+        # the user has.
+        person = self.factory.makePerson()
+        team_owner = self.factory.makePerson()
+        private_team = self.factory.makeTeam(
+            owner=team_owner, visibility=PersonVisibility.PRIVATE)
+        checker = PublicOrPrivateTeamsExistence(
+            removeSecurityProxy(private_team))
+
+        def create_subscribed_archive():
+            with person_logged_in(team_owner):
+                archive = self.factory.makeArchive(
+                    owner=private_team, private=True)
+                archive.newSubscription(person, team_owner)
+
+        def check_team_limited_view():
+            person.clearInTeamCache()
+            with person_logged_in(person):
+                self.assertTrue(
+                    checker.checkAuthenticated(IPersonRoles(person)))
+
+        recorder1, recorder2 = record_two_runs(
+            check_team_limited_view, create_subscribed_archive, 5)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2015-02-26 17:32:56 +0000
+++ lib/lp/security.py	2015-03-05 19:46:21 +0000
@@ -992,8 +992,8 @@
             and self.obj.visibility == PersonVisibility.PRIVATE):
             # Grant visibility to people with subscriptions on a private
             # team's private PPA.
-            subscriptions = getUtility(
-                IArchiveSubscriberSet).getBySubscriber(user.person)
+            subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(
+                user.person, need_archive=True)
             subscriber_archive_ids = set(
                 sub.archive.id for sub in subscriptions)
             team_ppa_ids = set(

=== modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
--- lib/lp/soyuz/interfaces/archivesubscriber.py	2014-03-07 01:35:46 +0000
+++ lib/lp/soyuz/interfaces/archivesubscriber.py	2015-03-05 19:46:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """ArchiveSubscriber interface."""
@@ -130,7 +130,8 @@
 class IArchiveSubscriberSet(Interface):
     """An interface for the set of all archive subscribers."""
 
-    def getBySubscriber(subscriber, archive=None, current_only=True):
+    def getBySubscriber(subscriber, archive=None, current_only=True,
+                        need_archive=False):
         """Return all the subscriptions for a person.
 
         :param subscriber: An `IPerson` for whom to return all
@@ -139,6 +140,7 @@
             the results to that particular archive.
         :param current_only: Whether the result should only include current
             subscriptions (which is the default).
+        :param need_archive: Whether the archive attribute should be cached.
         """
 
     def getBySubscriberWithActiveToken(subscriber, archive=None):

=== modified file 'lib/lp/soyuz/model/archivesubscriber.py'
--- lib/lp/soyuz/model/archivesubscriber.py	2014-03-07 00:44:37 +0000
+++ lib/lp/soyuz/model/archivesubscriber.py	2015-03-05 19:46:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database class for table ArchiveSubscriber."""
@@ -33,6 +33,7 @@
 from lp.registry.interfaces.person import validate_person
 from lp.registry.model.person import Person
 from lp.registry.model.teammembership import TeamParticipation
+from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import DBEnum
@@ -147,7 +148,7 @@
     """See `IArchiveSubscriberSet`."""
 
     def _getBySubscriber(self, subscriber, archive, current_only,
-                         with_active_tokens):
+                         with_active_tokens, need_archive=False):
         """Return all the subscriptions for a person.
 
         :param subscriber: An `IPerson` for whom to return all
@@ -159,6 +160,7 @@
         :param with_active_tokens: Indicates whether the tokens for the given
             subscribers subscriptions should be included in the resultset.
             By default the tokens are not included in the resultset.
+        :param need_archive: Whether the archive attribute should be cached.
         """
         # Grab the extra Storm expressions, for this query,
         # depending on the params:
@@ -185,6 +187,12 @@
         else:
             result_row = ArchiveSubscriber
 
+        def eager_load(rows):
+            # Circular import.
+            from lp.soyuz.model.archive import Archive
+            if need_archive:
+                load_related(Archive, rows, ["archive_id"])
+
         # Set the main expression to find all the subscriptions for
         # which the subscriber is a direct subscriber OR is a member
         # of a subscribed team.
@@ -193,14 +201,18 @@
         # showing that each person is a member of the "team" that
         # consists of themselves.
         store = Store.of(subscriber)
-        return store.using(*origin).find(
+        result_set = store.using(*origin).find(
             result_row,
             TeamParticipation.personID == subscriber.id,
             *extra_exprs).order_by(Desc(ArchiveSubscriber.date_created))
+        return DecoratedResultSet(result_set, pre_iter_hook=eager_load)
 
-    def getBySubscriber(self, subscriber, archive=None, current_only=True):
+    def getBySubscriber(self, subscriber, archive=None, current_only=True,
+                        need_archive=False):
         """See `IArchiveSubscriberSet`."""
-        return self._getBySubscriber(subscriber, archive, current_only, False)
+        return self._getBySubscriber(
+            subscriber, archive, current_only, False,
+            need_archive=need_archive)
 
     def getBySubscriberWithActiveToken(self, subscriber, archive=None):
         """See `IArchiveSubscriberSet`."""