launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/preload-subscriptions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1148353 in Launchpad itself: "Person:+archivesubscriptions causes O(n) queries per subscription"
  https://bugs.launchpad.net/launchpad/+bug/1148353

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/preload-subscriptions/+merge/152073

Preload archives and their owners, and pre-cache permissions for Person:+archivesubscriptions.

It turns out the bulk of the queries were trying to work out if the PPA links could be links or just text. Since subscribers are able to see Archive:+index I dropped the permission required to SubscriberView, and made the code make use of check_permission.
-- 
https://code.launchpad.net/~stevenk/launchpad/preload-subscriptions/+merge/152073
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/preload-subscriptions into lp:launchpad.

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2013-02-18 05:42:23 +0000
+++ lib/lp/app/browser/tales.py	2013-03-07 01:18:22 +0000
@@ -5,7 +5,7 @@
 
 __metaclass__ = type
 
-import bisect
+from bisect import bisect
 from datetime import (
     datetime,
     timedelta,
@@ -70,10 +70,6 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.services.utils import total_seconds
-from lp.services.webapp import (
-    canonical_url,
-    urlappend,
-    )
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.canonicalurl import nearest_adapter
 from lp.services.webapp.error import SystemErrorView
@@ -95,13 +91,14 @@
     get_facet,
     )
 from lp.services.webapp.publisher import (
+    canonical_url,
     LaunchpadView,
     nearest,
     )
 from lp.services.webapp.session import get_cookie_domain
+from lp.services.webapp.url import urlappend
 from lp.soyuz.enums import ArchivePurpose
 from lp.soyuz.interfaces.archive import IPPA
-from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.soyuz.interfaces.binarypackagename import IBinaryAndSourcePackageName
 
 
@@ -1862,20 +1859,16 @@
     """Adapter providing fmt support for `IPPA` objects."""
 
     _link_summary_template = '%(display_name)s'
-    _link_permission = 'launchpad.View'
+    _link_permission = 'launchpad.SubscriberView'
     _reference_template = "ppa:%(owner_name)s/%(ppa_name)s"
 
-    final_traversable_names = {
-        'reference': 'reference',
-        }
+    final_traversable_names = {'reference': 'reference'}
     final_traversable_names.update(
         CustomizableFormatter.final_traversable_names)
 
     def _link_summary_values(self):
         """See CustomizableFormatter._link_summary_values."""
-        return {
-            'display_name': self._context.displayname,
-            }
+        return {'display_name': self._context.displayname}
 
     def link(self, view_name):
         """Return html including a link for the context PPA.
@@ -1901,21 +1894,11 @@
 
     def reference(self, view_name=None, rootsite=None):
         """Return the text PPA reference for a PPA."""
-        # XXX: noodles 2010-02-11 bug=336779: This following check
-        # should be replaced with the normal check_permission once
-        # permissions for archive subscribers has been resolved.
-        if self._context.private:
-            request = get_current_browser_request()
-            person = IPerson(request.principal)
-            subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(
-                person, self._context)
-            if subscriptions.is_empty():
-                return ''
-
+        if not check_permission(self._link_permission, self._context):
+            return ''
         return self._reference_template % {
             'owner_name': self._context.owner.name,
-            'ppa_name': self._context.name,
-            }
+            'ppa_name': self._context.name}
 
 
 class SpecificationBranchFormatterAPI(CustomizableFormatter):
@@ -2353,7 +2336,7 @@
         if seconds < second_boundaries[-1]:
             # Use the built-in bisection algorithm to locate the index
             # of the item which "seconds" sorts after.
-            matching_element_index = bisect.bisect(second_boundaries, seconds)
+            matching_element_index = bisect(second_boundaries, seconds)
 
             # Return the corresponding display value.
             return display_values[matching_element_index]
@@ -2510,8 +2493,7 @@
 
     def __init__(self, context):
         here = os.path.dirname(os.path.realpath(__file__))
-        self.path = os.path.join(
-            here, '../templates/base-layout.pt')
+        self.path = os.path.join(here, '../templates/base-layout.pt')
 
 
 class PageMacroDispatcher:

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2013-02-28 01:02:50 +0000
+++ lib/lp/security.py	2013-03-07 01:18:22 +0000
@@ -2487,6 +2487,8 @@
     usedfor = IArchive
 
     def checkAuthenticated(self, user):
+        if user.person in self.obj._known_subscribers:
+            return True
         if super(SubscriberViewArchive, self).checkAuthenticated(user):
             return True
         filter = get_enabled_archive_filter(

=== modified file 'lib/lp/soyuz/browser/archivesubscription.py'
--- lib/lp/soyuz/browser/archivesubscription.py	2012-12-12 05:27:55 +0000
+++ lib/lp/soyuz/browser/archivesubscription.py	2013-03-07 01:18:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views related to archive subscriptions."""
@@ -14,7 +14,10 @@
     ]
 
 import datetime
-from operator import attrgetter
+from operator import (
+    attrgetter,
+    itemgetter,
+    )
 
 import pytz
 from zope.app.form import CustomWidgetFactory
@@ -40,8 +43,12 @@
 from lp.app.widgets.date import DateWidget
 from lp.app.widgets.popup import PersonPickerWidget
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.database.bulk import load_related
 from lp.services.fields import PersonChoice
-from lp.services.propertycache import cachedproperty
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 from lp.services.webapp.batching import (
     BatchNavigator,
     StormRangeFactory,
@@ -56,6 +63,7 @@
     IArchiveSubscriberSet,
     IPersonalArchiveSubscription,
     )
+from lp.soyuz.model.archive import Archive
 
 
 def archive_subscription_ui_adapter(archive_subscription):
@@ -321,6 +329,13 @@
         subs_with_tokens = subscriber_set.getBySubscriberWithActiveToken(
             self.context)
 
+        archives = load_related(
+            Archive, map(itemgetter(0), subs_with_tokens), ['archive_id'])
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            [archive.ownerID for archive in archives], need_validity=True))
+        for archive in archives:
+            get_property_cache(archive)._known_subscribers = [self.user]
+
         # Turn the result set into a list of dicts so it can be easily
         # accessed in TAL. Note that we need to ensure that only one
         # PersonalArchiveSubscription is included for each archive,
@@ -331,15 +346,12 @@
         for subscription, token in subs_with_tokens:
             if subscription.archive in unique_archives:
                 continue
-
             unique_archives.add(subscription.archive)
 
             personal_subscription = PersonalArchiveSubscription(
                 self.context, subscription.archive)
             personal_subscription_tokens.append({
-                'subscription': personal_subscription,
-                'token': token
-                })
+                'subscription': personal_subscription, 'token': token})
 
         return personal_subscription_tokens
 

=== modified file 'lib/lp/soyuz/interfaces/archivesubscriber.py'
--- lib/lp/soyuz/interfaces/archivesubscriber.py	2013-01-07 02:40:55 +0000
+++ lib/lp/soyuz/interfaces/archivesubscriber.py	2013-03-07 01:18:22 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """ArchiveSubscriber interface."""
@@ -47,6 +47,7 @@
 
     id = Int(title=_('ID'), required=True, readonly=True)
 
+    archive_id = Int(title=_('Archive ID'), required=True, readonly=True)
     archive = exported(Reference(
         IArchive, title=_("Archive"), required=True, readonly=True,
         description=_("The archive for this subscription.")))

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2013-02-20 04:02:11 +0000
+++ lib/lp/soyuz/model/archive.py	2013-03-07 01:18:22 +0000
@@ -442,6 +442,10 @@
         else:
             return None
 
+    @cachedproperty
+    def _known_subscribers(self):
+        return set()
+
     @property
     def archive_url(self):
         """See `IArchive`."""

=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py	2013-03-06 02:48:35 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py	2013-03-07 01:18:22 +0000
@@ -5,6 +5,8 @@
 
 from urlparse import urljoin
 
+from storm.store import Store
+from testtools.matchers import Equals
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
@@ -17,10 +19,12 @@
     BrowserTestCase,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import (
     find_tag_by_id,
     setupBrowserForUser,
@@ -102,8 +106,7 @@
         notifications = pop_notifications()
         self.assertEqual(1, len(notifications))
         self.assertEqual(
-            self.subscriber.preferredemail.email,
-            notifications[0]['to'])
+            self.subscriber.preferredemail.email, notifications[0]['to'])
 
     def test_new_commercial_subscription_no_email(self):
         # As per bug 611568, an email is not sent for
@@ -167,3 +170,23 @@
             url = urljoin(canonical_url(self.archive), '+packages')
         browser = setupBrowserForUser(self.subscriber)
         self.assertRaises(Unauthorized, browser.open, url)
+
+
+class PersonArchiveSubscriptions(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_query_count(self):
+        subscriber = self.factory.makePerson()
+        for x in range(10):
+            archive = self.factory.makeArchive(private=True)
+            with person_logged_in(archive.owner):
+                archive.newSubscription(subscriber, archive.owner)
+        Store.of(subscriber).flush()
+        Store.of(subscriber).invalidate()
+        with person_logged_in(subscriber):
+            with StormStatementRecorder() as recorder:
+                view = create_initialized_view(
+                    subscriber, '+archivesubscriptions', principal=subscriber)
+                view.render()
+        self.assertThat(recorder, HasQueryCount(Equals(9)))