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))
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
