launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/person-ppas-timeout into lp:launchpad.

Commit message:
Precache permissions for archives returned by Person.getVisiblePPAs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1685202 in Launchpad itself: "Time out error when accessing https://launchpad.net/~oem-archive";
  https://bugs.launchpad.net/launchpad/+bug/1685202

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/person-ppas-timeout/+merge/322995

Person.getVisiblePPAs uselessly included subscribed PPAs, on which the requesting user wouldn't have launchpad.View unless the subscribed PPAs happened to also match one of the other criteria.

Once the return value of Person.getVisiblePPAs is actually guaranteed to be launchpad.View-safe, precaching permissions then fixes large query counts on Person:+index in cases where the requesting user has upload permission on many PPAs owned by the context person.

I made the same change to Person:+activate-ppa for completeness, although that can't currently cause a problem in practice because it can only be used by the context person or by admins, thereby bypassing most of the slow bits of the security adapter.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/person-ppas-timeout into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2017-02-08 02:44:59 +0000
+++ lib/lp/registry/browser/person.py	2017-04-22 15:21:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Person-related view classes."""
@@ -245,7 +245,10 @@
     stepto,
     structured,
     )
-from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.breadcrumb import DisplaynameBreadcrumb
 from lp.services.webapp.interfaces import (
@@ -2017,7 +2020,9 @@
 
     @cachedproperty
     def visible_ppas(self):
-        return self.context.getVisiblePPAs(self.user)
+        ppas = self.context.getVisiblePPAs(self.user)
+        precache_permission_for_objects(self.request, 'launchpad.View', ppas)
+        return ppas
 
     @property
     def time_zone_offset(self):

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2017-02-08 02:44:59 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2017-04-22 15:21:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -349,6 +349,21 @@
         view = create_initialized_view(person, '+index')
         self.assertTrue(view.should_show_gpgkeys_section)
 
+    def test_ppas_query_count(self):
+        owner = self.factory.makePerson()
+
+        def create_ppa_and_permission():
+            ppa = self.factory.makeArchive(
+                owner=owner, purpose=ArchivePurpose.PPA, private=True)
+            ppa.newComponentUploader(self.user, 'main')
+
+        recorder1, recorder2 = record_two_runs(
+            lambda: self.getMainText(owner, '+index'),
+            create_ppa_and_permission, 5,
+            login_method=lambda: login_person(owner),
+            record_request=True)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
 
 class TestPersonViewKarma(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2017-04-11 06:36:15 +0000
+++ lib/lp/registry/model/person.py	2017-04-22 15:21:55 +0000
@@ -3050,8 +3050,7 @@
         from lp.soyuz.model.archive import get_enabled_archive_filter
 
         filter = get_enabled_archive_filter(
-            user, purpose=ArchivePurpose.PPA,
-            include_public=True, include_subscribed=True)
+            user, purpose=ArchivePurpose.PPA, include_public=True)
         return Store.of(self).find(
             Archive,
             Archive.owner == self,

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2016-01-26 15:47:37 +0000
+++ lib/lp/soyuz/browser/archive.py	2017-04-22 15:21:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for archive."""
@@ -109,7 +109,10 @@
     Navigation,
     stepthrough,
     )
-from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.authorization import (
+    check_permission,
+    precache_permission_for_objects,
+    )
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import (
@@ -1885,7 +1888,9 @@
 
     @cachedproperty
     def visible_ppas(self):
-        return self.context.getVisiblePPAs(self.user)
+        ppas = self.context.getVisiblePPAs(self.user)
+        precache_permission_for_objects(self.request, 'launchpad.View', ppas)
+        return ppas
 
     @property
     def initial_values(self):