launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:archive-subscriptions-query-count into launchpad:master.

Commit message:
Fix Archive:+subscriptions timeouts with many existing subscriptions

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1851834 in Launchpad itself: "Timeout error when managing access of the fips-updates  PPA"
  https://bugs.launchpad.net/launchpad/+bug/1851834

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/400914

Archive:+subscriptions could easily time out when the archive had many existing subscriptions, especially if there were subscriptions for private teams, because it did permission checks individually on each subscriber.  We know up-front that users viewing this page have sufficient privilege that they should be able to at least see the identity of all the existing subscribers, so preload the necessary permissions.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:archive-subscriptions-query-count into launchpad:master.

diff --git a/lib/lp/security.py b/lib/lp/security.py
index 03b35bd..412f497 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -2912,7 +2912,7 @@ class ViewPersonalArchiveSubscription(DelegatedAuthorization):
 class ViewArchiveSubscriber(DelegatedAuthorization):
     """Restrict viewing of archive subscribers.
 
-    The user should be the subscriber, have edit privilege to the
+    The user should be the subscriber, have append privilege to the
     archive or be an admin.
     """
     permission = "launchpad.View"
diff --git a/lib/lp/soyuz/browser/archivesubscription.py b/lib/lp/soyuz/browser/archivesubscription.py
index 47ab800..f05a506 100644
--- a/lib/lp/soyuz/browser/archivesubscription.py
+++ b/lib/lp/soyuz/browser/archivesubscription.py
@@ -14,7 +14,6 @@ __all__ = [
     ]
 
 import datetime
-from operator import attrgetter
 
 import pytz
 from zope.component import getUtility
@@ -44,6 +43,7 @@ from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
     )
+from lp.services.webapp.authorization import precache_permission_for_objects
 from lp.services.webapp.batching import (
     BatchNavigator,
     StormRangeFactory,
@@ -164,9 +164,18 @@ class ArchiveSubscribersView(LaunchpadFormView):
         Bulk loads the related Person records.
         """
         batch = list(self.batchnav.currentBatch())
+        # If the user can see this view, then they must have Append
+        # permission on the archive, which grants View permission on all its
+        # subscriptions.  Skip slow privacy checks.
+        precache_permission_for_objects(self.request, 'launchpad.View', batch)
         ids = [subscription.subscriber_id for subscription in batch]
-        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(ids,
-            need_validity=True))
+        subscribers = list(
+            getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                ids, need_validity=True))
+        # People who can manage subscriptions to this archive are entitled
+        # to at least limited visibility of its existing subscribers.
+        precache_permission_for_objects(
+            self.request, 'launchpad.LimitedView', subscribers)
         return batch
 
     @cachedproperty
diff --git a/lib/lp/soyuz/tests/test_archivesubscriptionview.py b/lib/lp/soyuz/browser/tests/test_archivesubscription.py
similarity index 79%
rename from lib/lp/soyuz/tests/test_archivesubscriptionview.py
rename to lib/lp/soyuz/browser/tests/test_archivesubscription.py
index 5634fc7..73d771b 100644
--- a/lib/lp/soyuz/tests/test_archivesubscriptionview.py
+++ b/lib/lp/soyuz/browser/tests/test_archivesubscription.py
@@ -13,12 +13,17 @@ from soupmatchers import (
     )
 from zope.component import getUtility
 
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.webapp import canonical_url
 from lp.testing import (
+    login_person,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     )
 from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
 
 
@@ -49,3 +54,21 @@ class TestArchiveSubscribersView(TestCaseWithFactory):
             Tag('batch navigation links', 'td',
                 attrs={'class': 'batch-navigation-links'}, count=2))
         self.assertThat(html, has_batch_navigation)
+
+    def test_constant_query_count(self):
+        def create_subscribers():
+            self.private_ppa.newSubscription(
+                self.factory.makePerson(), self.p3a_owner)
+            self.private_ppa.newSubscription(
+                self.factory.makeTeam(
+                    visibility=PersonVisibility.PRIVATE,
+                    members=[self.p3a_owner]),
+                self.p3a_owner)
+
+        self.pushConfig('launchpad', default_batch_size=75)
+        url = canonical_url(self.private_ppa, view_name='+subscriptions')
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getUserBrowser(url, user=self.p3a_owner),
+            create_subscribers, 2,
+            login_method=lambda: login_person(self.p3a_owner))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))