launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13922
[Merge] lp:~wallyworld/launchpad/person-index-timeout-931771 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/person-index-timeout-931771 into lp:launchpad.
Commit message:
Improve archive permission checks so that visible ppas for a person can be found efficiently.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #931771 in Launchpad itself: "Person:+index timeout on large commercial PPA owner"
https://bugs.launchpad.net/launchpad/+bug/931771
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/person-index-timeout-931771/+merge/132446
== 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.
== 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
--
https://code.launchpad.net/~wallyworld/launchpad/person-index-timeout-931771/+merge/132446
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/person-index-timeout-931771 into lp:launchpad.
=== 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-01 03:46:19 +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-01 03:46:19 +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-10-26 18:23:39 +0000
+++ lib/lp/registry/model/person.py 2012-11-01 03:46:19 +0000
@@ -3054,6 +3054,16 @@
return Archive.selectBy(
owner=self, purpose=ArchivePurpose.PPA, orderBy='name')
+ def getVisiblePPAs(self, user):
+ """See `IPerson`."""
+ from lp.soyuz.model.archive import get_enabled_archive_filter
+
+ filter = get_enabled_archive_filter(user, True, 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-01 03:46:19 +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
@@ -1770,7 +1775,9 @@
def checkAuthenticated(self, user):
archive = self.obj.target_archive
if archive.is_ppa:
- return archive.checkArchivePermission(user.person)
+ filter = get_enabled_archive_filter(user.person)
+ return not IStore(self.obj).find(
+ Archive, And(Archive.id == self.obj.id, filter)).is_empty()
permission_set = getUtility(IArchivePermissionSet)
permissions = permission_set.componentsForQueueAdmin(
@@ -2485,26 +2492,9 @@
if not self.obj.private and self.obj.enabled:
return True
- # Administrator are allowed to view private archives.
- if user.in_admin or user.in_commercial_admin:
- return True
-
- # Owners can view the PPA.
- 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, True, True)
+ return not IStore(self.obj).find(
+ Archive, And(Archive.id == self.obj.id, filter)).is_empty()
def checkUnauthenticated(self):
"""Unauthenticated users can see the PPA if it's not private."""
@@ -2532,7 +2522,7 @@
No one can upload to disabled archives.
- PPA upload rights are managed via `IArchive.checkArchivePermission`;
+ PPA upload rights are managed via `get_enabled_archive_filter`;
Appending to PRIMARY, PARTNER or COPY archives is restricted to owners.
"""
@@ -2546,8 +2536,10 @@
if user.inTeam(self.obj.owner):
return True
- if self.obj.is_ppa and self.obj.checkArchivePermission(user.person):
- return True
+ if self.obj.is_ppa:
+ filter = get_enabled_archive_filter(user.person)
+ return not IStore(self.obj).find(
+ Archive, And(Archive.id == self.obj.id, filter)).is_empty()
return False
=== 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-01 03:46:19 +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-01 03:46:19 +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-01 03:46:19 +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,54 @@
TeamParticipation.teamID,
where=(TeamParticipation.person == user))))
return privacy_filter
+
+
+def get_enabled_archive_filter(user, purpose=ArchivePurpose.PPA,
+ 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.
+ """
+ if user is None:
+ return And(
+ Archive.purpose == purpose, Archive._private == False,
+ Archive._enabled == True)
+
+ # Administrator are allowed to view private archives.
+ roles = IPersonRoles(user)
+ if roles.in_admin or roles.in_commercial_admin:
+ return True
+
+ 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)
+
+ is_subscribed = Select(
+ ArchiveSubscriber.archive_id, where=And(
+ ArchiveSubscriber.status == ArchiveSubscriberStatus.CURRENT,
+ ArchiveSubscriber.subscriber_id.is_in(user_teams)),
+ tables=ArchiveSubscriber)
+
+ access_terms = [is_allowed]
+ if include_subscribed:
+ access_terms.append(is_subscribed)
+
+ is_allowed_or_subscribed = Archive.id.is_in(Or(*access_terms))
+
+ enabled = [Archive.purpose == purpose, Archive._enabled == True]
+ filter_terms = [is_owner, is_allowed_or_subscribed]
+ if include_public:
+ filter_terms.append(Archive._private == False)
+ return And(enabled, 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-01 03:46:19 +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-01 03:46:19 +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-01 03:46:19 +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>
Follow ups
