Merge lp:~wallyworld/launchpad/person-index-timeout-931771 into lp:launchpad

[Ian Booth]

Improve archive permission checks so that visible ppas for a person can be found efficiently.

== Implementation ==

The root cause of the bug is that the page iterates over all person ppas checking for launchpad.View permission. Each permission check does several queries. I refactored the implementation as follows:

I looked at the permission checking code in the security adaptor and gathered all the rules used to formulate the final database query done for each archive. I coded these rules in a method called get_enabled_archive_filter(). There is a new method on IPerson called getVisiblePPAs() which uses this filter to load in one query all the PPAs visible to the caller. This single call replaces the individual calls to the security adaptor.

The TAL and view have been refactored to use the new API. To ensure property caching works, I eliminated the subview and converted it to a macro, thus allowing the visible ppas to be loaded once and passed via a variable declaration to the macro.

Finally, I converted other permission checks IArchive in the security adaptors to use the new filter. This was simply to ensure that a common code base was used for the permission checks.

I got wgrant to run the queries on DF to ensure performance was acceptable and a few tweaks were need due to Postgres being "special".

== Tests ==

This is an internal change, so rely on existing tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/security.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py
  lib/lp/soyuz/browser/archive.py
  lib/lp/soyuz/browser/configure.zcml
  lib/lp/soyuz/model/archive.py
  lib/lp/soyuz/templates/archive-activate.pt
  lib/lp/soyuz/templates/archive-macros.pt
  lib/lp/soyuz/templates/person-portlet-ppas.pt

[j.c.sackett (jcsackett)]

This looks good, Ian, just one minor quibble you can deal with before landing.

#248: If this is meant to get around circular imports, please comment it as
such. It's also cleaner to move imports to the top of the execution scope
they're brought into.

review: Approve

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-10-26 07:15:49 +0000
+++ lib/lp/registry/browser/person.py	2012-11-03 13:52:20 +0000
@@ -138,12 +138,8 @@
     LaunchpadRadioWidgetWithDescription,
     )
 from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
-from lp.bugs.interfaces.bugtask import (
-    BugTaskStatus,
-    IBugTaskSet,
-    )
+from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
-from lp.bugs.model.bugtask import BugTaskSet
 from lp.buildmaster.enums import BuildStatus
 from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
 from lp.code.errors import InvalidNamespace
@@ -1931,11 +1927,11 @@
             return True
 
         # If the current user can view any PPA, show the section.
-        for ppa in self.context.ppas:
-            if check_permission('launchpad.View', ppa):
-                return True
+        return self.visible_ppas.count() > 0
 
-        return False
+    @cachedproperty
+    def visible_ppas(self):
+        return self.context.getVisiblePPAs(self.user)
 
     @property
     def time_zone_offset(self):
@@ -3470,14 +3466,8 @@
         is_driver, is_bugsupervisor.
         """
         projects = []
-        user = getUtility(ILaunchBag).user
         max_projects = self.max_results_to_display
         pillarnames = self._related_projects[:max_projects]
-        products = [pillarname.pillar for pillarname in pillarnames
-                    if IProduct.providedBy(pillarname.pillar)]
-        bugtask_set = getUtility(IBugTaskSet)
-        product_bugtask_counts = bugtask_set.getOpenBugTasksPerProduct(
-            user, products)
         for pillarname in pillarnames:
             pillar = pillarname.pillar
             project = {}

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-10-26 13:29:52 +0000
+++ lib/lp/registry/interfaces/person.py	2012-11-03 13:52:20 +0000
@@ -1051,6 +1051,9 @@
         It will create a new IArchiveAuthToken if one doesn't already exist.
         """
 
+    def getVisiblePPAs(user):
+        """Return the PPAs for which user has launchpad.View permission."""
+
     def getInvitedMemberships():
         """Return all TeamMemberships of this team with the INVITED status.
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-11-01 20:33:27 +0000
+++ lib/lp/registry/model/person.py	2012-11-03 13:52:20 +0000
@@ -3055,6 +3055,20 @@
         return Archive.selectBy(
             owner=self, purpose=ArchivePurpose.PPA, orderBy='name')
 
+    def getVisiblePPAs(self, user):
+        """See `IPerson`."""
+
+        # Avoid circular imports.
+        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)
+        return Store.of(self).find(
+            Archive,
+            Archive.owner == self,
+            filter).order_by(Archive.name)
+
     def getPPAByName(self, name):
         """See `IPerson`."""
         return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-10-30 16:59:58 +0000
+++ lib/lp/security.py	2012-11-03 13:52:20 +0000
@@ -14,6 +14,7 @@
 from operator import methodcaller
 
 from storm.expr import (
+    And,
     Select,
     Union,
     )
@@ -219,6 +220,10 @@
     IPackageUploadQueue,
     )
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
+from lp.soyuz.model.archive import (
+    Archive,
+    get_enabled_archive_filter,
+    )
 from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
 from lp.translations.interfaces.languagepack import ILanguagePack
 from lp.translations.interfaces.pofile import IPOFile
@@ -2493,18 +2498,10 @@
         if user.inTeam(self.obj.owner):
             return True
 
-        # Uploaders can view private PPAs.
-        if self.obj.is_ppa and self.obj.checkArchivePermission(user.person):
-            return True
-
-        # Subscribers can view private PPAs.
-        if self.obj.is_ppa and self.obj.private:
-            archive_subs = getUtility(IArchiveSubscriberSet).getBySubscriber(
-                user.person, self.obj).any()
-            if archive_subs:
-                return True
-
-        return False
+        filter = get_enabled_archive_filter(
+            user.person, include_subscribed=True)
+        return not IStore(self.obj).find(
+            Archive.id, And(Archive.id == self.obj.id, filter)).is_empty()
 
     def checkUnauthenticated(self):
         """Unauthenticated users can see the PPA if it's not private."""

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-10-29 16:52:31 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-11-03 13:52:20 +0000
@@ -1955,6 +1955,10 @@
     def ubuntu(self):
         return getUtility(ILaunchpadCelebrities).ubuntu
 
+    @cachedproperty
+    def visible_ppas(self):
+        return self.context.getVisiblePPAs(self.user)
+
     @property
     def initial_values(self):
         """Set up default values for form fields."""

=== modified file 'lib/lp/soyuz/browser/configure.zcml'
--- lib/lp/soyuz/browser/configure.zcml	2012-07-09 12:32:23 +0000
+++ lib/lp/soyuz/browser/configure.zcml	2012-11-03 13:52:20 +0000
@@ -750,9 +750,6 @@
             <browser:page
                 name="+portlet-ppas"
                 template="../templates/person-portlet-ppas.pt"/>
-            <browser:page
-                name="+ppas-list"
-                template="../templates/person-ppas.pt"/>
         </browser:pages>
         <browser:pages
             for="lp.registry.interfaces.distribution.IDistribution"

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-10-25 11:02:37 +0000
+++ lib/lp/soyuz/model/archive.py	2012-11-03 13:52:20 +0000
@@ -9,6 +9,7 @@
     'Archive',
     'ArchiveSet',
     'get_archive_privacy_filter',
+    'get_enabled_archive_filter',
     'validate_ppa',
     ]
 
@@ -176,6 +177,7 @@
     )
 from lp.soyuz.model.archiveauthtoken import ArchiveAuthToken
 from lp.soyuz.model.archivedependency import ArchiveDependency
+from lp.soyuz.model.archivepermission import ArchivePermission
 from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import (
@@ -2597,3 +2599,64 @@
                     TeamParticipation.teamID,
                     where=(TeamParticipation.person == user))))
     return privacy_filter
+
+
+def get_enabled_archive_filter(user, purpose=None,
+                            include_public=False, include_subscribed=False):
+    """ Return a filter that can be used with a Storm query to filter Archives.
+
+    The archive must be enabled, plus satisfy the other specified conditions.
+    """
+    purpose_term = True
+    if purpose:
+        purpose_term = Archive.purpose == purpose
+    if user is None:
+        if include_public:
+            terms = [
+                purpose_term, Archive._private == False,
+                Archive._enabled == True]
+            return And(*terms)
+        else:
+            return False
+
+    # Administrator are allowed to view private archives.
+    roles = IPersonRoles(user)
+    if roles.in_admin or roles.in_commercial_admin:
+        return purpose_term
+
+    main = getUtility(IComponentSet)['main']
+    user_teams = Select(
+                TeamParticipation.teamID,
+                where=TeamParticipation.person == user)
+
+    is_owner = Archive.ownerID.is_in(user_teams)
+
+    from lp.soyuz.model.archivesubscriber import ArchiveSubscriber
+
+    is_allowed = Select(
+        ArchivePermission.archiveID, where=And(
+            ArchivePermission.permission == ArchivePermissionType.UPLOAD,
+            ArchivePermission.component == main,
+            ArchivePermission.personID.is_in(user_teams)),
+        tables=ArchivePermission, distinct=True)
+
+    is_subscribed = Select(
+        ArchiveSubscriber.archive_id, where=And(
+            ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT,
+            ArchiveSubscriber.subscriber_id.is_in(user_teams)),
+        tables=ArchiveSubscriber, distinct=True)
+
+    filter_terms = [
+        is_owner,
+        And(
+            Archive.purpose == ArchivePurpose.PPA,
+            Archive.id.is_in(is_allowed))]
+    if include_subscribed:
+        filter_terms.append(And(
+            Archive.purpose == ArchivePurpose.PPA,
+            Archive.id.is_in(is_subscribed)))
+
+    if include_public:
+        filter_terms.append(
+            And(Archive._enabled == True, Archive._private == False))
+    return And(purpose_term, Or(*filter_terms))

=== modified file 'lib/lp/soyuz/templates/archive-activate.pt'
--- lib/lp/soyuz/templates/archive-activate.pt	2011-01-17 18:55:38 +0000
+++ lib/lp/soyuz/templates/archive-activate.pt	2012-11-03 13:52:20 +0000
@@ -23,7 +23,9 @@
       <div tal:condition="context/ppas" id="ppas"
           style="padding-top: .3em; padding-bottom: .3em;">
         <h2>Existing PPAs</h2>
-        <div tal:replace="structure context/@@+ppas-list"/>
+        <tal:ppa tal:define="visible_ppas view/visible_ppas">
+          <metal:ppas-list use-macro="context/@@+macros/ppas-list"/>
+        </tal:ppa>
       </div>
       <p>
         Read the current version of

=== modified file 'lib/lp/soyuz/templates/archive-macros.pt'
--- lib/lp/soyuz/templates/archive-macros.pt	2012-02-01 15:31:32 +0000
+++ lib/lp/soyuz/templates/archive-macros.pt	2012-11-03 13:52:20 +0000
@@ -350,4 +350,21 @@
   </tal:latest_updates>
 </metal:latest_updates_portlet>
 
+<metal:ppas-list define-macro="ppas-list">
+  <tal:comment condition="nothing">
+    This macro requires the following defined variables:
+      visible_ppas - the ppas for which the user has view permission.
+  </tal:comment>
+
+  <div tal:define="ppas visible_ppas" tal:condition="ppas">
+    <table>
+      <tal:ppa_line tal:repeat="ppa ppas">
+        <tr>
+          <td tal:content="structure ppa/fmt:link"></td>
+        </tr>
+      </tal:ppa_line>
+    </table>
+  </div>
+</metal:ppas-list>
+
 </tal:root>

=== modified file 'lib/lp/soyuz/templates/person-portlet-ppas.pt'
--- lib/lp/soyuz/templates/person-portlet-ppas.pt	2012-06-16 13:12:41 +0000
+++ lib/lp/soyuz/templates/person-portlet-ppas.pt	2012-11-03 13:52:20 +0000
@@ -6,8 +6,9 @@
   <div id="ppas" class="portlet" tal:condition="view/should_show_ppa_section">
     <h2>Personal package archives</h2>
 
-    <div tal:replace="structure context/@@+ppas-list"/>
-
+    <tal:ppa tal:define="visible_ppas view/visible_ppas">
+      <metal:ppas-list use-macro="context/@@+macros/ppas-list"/>
+    </tal:ppa>
     <ul class="horizontal">
       <tal:can-create-ppa condition="context/canCreatePPA">
         <li tal:define="link context/menu:overview/activate_ppa"

=== removed file 'lib/lp/soyuz/templates/person-ppas.pt'
--- lib/lp/soyuz/templates/person-ppas.pt	2010-06-12 05:05:11 +0000
+++ lib/lp/soyuz/templates/person-ppas.pt	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-<tal:root
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  omit-tag="">
-  <div tal:define="ppas context/ppas" tal:condition="ppas">
-    <table>
-      <tal:ppa_line tal:repeat="ppa ppas">
-        <tr tal:condition="ppa/required:launchpad.View">
-          <td tal:content="structure ppa/fmt:link"></td>
-        </tr>
-      </tal:ppa_line>
-    </table>
-  </div>
-</tal:root>