launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00483
[Merge] lp:~lifeless/launchpad/registry into lp:launchpad/devel
Robert Collins has proposed merging lp:~lifeless/launchpad/registry into lp:launchpad/devel.
Requested reviews:
Curtis Hovey (sinzui)
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#615237 /participants API timing out
https://bugs.launchpad.net/bugs/615237
Make the /participation API make a constant number of DB calls, rather than scaling per-team-member found.
This change has a few key components.
First, the existing allmembers attribute is perserved unaltered; rather a new all_members_prepopulated one is created and exported, so that the API fix doesn't cause unnecessary DB work for other callers that don't need all the attributes.
A common helper, _all_members is created on Person, which can prepopulate various attributes.
Secondly, all the attributes that are exported as fields, which the API wants to examine, are prepopulated, bringing the query count down to 11 for a team with 3 members (from nearly 40 - and that 11 is now constant in tests).
The prepopulation is done via cachedproperty decorators, which should be totally fine for webservice and API requests but does introduce a risk that other scripts which acccess these properties, and read-them-back-after-changing-the-originating-data will break, so when QA'ing a test run of all the registry related cronscripts is probably wise, though we have no particular data to suggest that this is needed.
I haven't run a full test suite yet, I will be doing so to find out if there are any side effects which will need fixing.
All that said, I think this is a more robust approach than having a separate CachedPersonDecorator and returning that - any changes to person would need more work in that case, where here it is built in.
--
https://code.launchpad.net/~lifeless/launchpad/registry/+merge/32067
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/registry into lp:launchpad/devel.
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-08-02 01:37:09 +0000
+++ lib/lp/registry/interfaces/person.py 2010-08-09 07:06:07 +0000
@@ -1264,7 +1264,7 @@
all_member_count = Attribute(
"The total number of real people who are members of this team, "
"including subteams.")
- allmembers = exported(
+ all_members_prepopulated = exported(
doNotSnapshot(
CollectionField(
title=_("All participants of this team."),
@@ -1276,6 +1276,8 @@
"IPerson.inTeam()."),
value_type=Reference(schema=Interface))),
exported_as='participants')
+ allmembers = doNotSnapshot(
+ Attribute("List of all members, without checking karma etc."))
approvedmembers = doNotSnapshot(
Attribute("List of members with APPROVED status"))
deactivated_member_count = Attribute("Number of deactivated members")
@@ -1355,14 +1357,17 @@
center_lat, and center_lng
"""
- def getMembersWithPreferredEmails(include_teams=False):
+ def getMembersWithPreferredEmails():
"""Returns a result set of persons with precached addresses.
Persons or teams without preferred email addresses are not included.
"""
- def getMembersWithPreferredEmailsCount(include_teams=False):
- """Returns the count of persons/teams with preferred emails."""
+ def getMembersWithPreferredEmailsCount():
+ """Returns the count of persons/teams with preferred emails.
+
+ See also getMembersWithPreferredEmails.
+ """
def getDirectAdministrators():
"""Return this team's administrators.
@@ -2156,9 +2161,16 @@
# Fix value_type.schema of IPersonViewRestricted attributes.
-for name in ['allmembers', 'activemembers', 'adminmembers', 'proposedmembers',
- 'invited_members', 'deactivatedmembers', 'expiredmembers',
- 'unmapped_participants']:
+for name in [
+ 'all_members_prepopulated',
+ 'activemembers',
+ 'adminmembers',
+ 'proposedmembers',
+ 'invited_members',
+ 'deactivatedmembers',
+ 'expiredmembers',
+ 'unmapped_participants',
+ ]:
IPersonViewRestricted[name].value_type.schema = IPerson
IPersonPublic['sub_teams'].value_type.schema = ITeam
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-07-23 12:24:55 +0000
+++ lib/lp/registry/model/person.py 2010-08-09 07:06:07 +0000
@@ -48,7 +48,9 @@
StringCol)
from sqlobject.sqlbuilder import AND, OR, SQLConstant
from storm.store import EmptyResultSet, Store
-from storm.expr import And, In, Join, Lower, Not, Or, SQL
+from storm.expr import (
+ Alias, And, Exists, In, Join, LeftJoin, Lower, Min, Not, Or, Select, SQL,
+ )
from storm.info import ClassAlias
from canonical.config import config
@@ -63,6 +65,7 @@
from canonical.lazr.utils import get_current_browser_request, safe_hasattr
+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
from canonical.launchpad.database.account import Account, AccountPassword
from canonical.launchpad.interfaces.account import AccountSuspendedError
from lp.bugs.model.bugtarget import HasBugsBase
@@ -1028,9 +1031,10 @@
result = result.order_by(KarmaCategory.title)
return [karma_cache for (karma_cache, category) in result]
- @property
+ @cachedproperty('_karma_cached')
def karma(self):
"""See `IPerson`."""
+ # May also be loaded from _all_members
cache = KarmaTotalCache.selectOneBy(person=self)
if cache is None:
# Newly created accounts may not be in the cache yet, meaning the
@@ -1048,7 +1052,7 @@
return self.is_valid_person
- @property
+ @cachedproperty('_is_valid_person_cached')
def is_valid_person(self):
"""See `IPerson`."""
if self.is_team:
@@ -1447,14 +1451,130 @@
@property
def allmembers(self):
"""See `IPerson`."""
- query = """
- Person.id = TeamParticipation.person AND
- TeamParticipation.team = %s AND
- TeamParticipation.person != %s
- """ % sqlvalues(self.id, self.id)
- return Person.select(query, clauseTables=['TeamParticipation'])
-
- def _getMembersWithPreferredEmails(self, include_teams=False):
+ return self._all_members()
+
+ @property
+ def all_members_prepopulated(self):
+ """See `IPerson`."""
+ return self._all_members(need_karma=True, need_ubuntu_coc=True,
+ need_location=True, need_archive=True, need_preferred_email=True,
+ need_validity=True)
+
+ def _all_members(self, need_karma=False, need_ubuntu_coc=False,
+ need_location=False, need_archive=False, need_preferred_email=False,
+ need_validity=False):
+ """Lookup all members of the team with optional precaching.
+
+ :param need_karma: The karma attribute will be cached.
+ :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+ cached.
+ :param need_location: The location attribute will be cached.
+ :param need_archive: The archive attribute will be cached.
+ :param need_preferred_email: The preferred email attribute will be
+ cached.
+ :param need_validity: The is_valid attribute will be cached.
+ """
+ # TODO: consolidate this with getMembersWithPreferredEmails.
+ # The difference between the two is that
+ # getMembersWithPreferredEmails includes self, which is arguably
+ # wrong, but perhaps deliberate.
+ store = Store.of(self)
+ origin = [
+ Person,
+ Join(TeamParticipation, TeamParticipation.person == Person.id),
+ ]
+ conditions = And(
+ # Members of this team,
+ TeamParticipation.team == self.id,
+ # But not the team itself.
+ TeamParticipation.person != self.id)
+ columns = [Person]
+ if need_karma:
+ # New people have no karmatotalcache rows.
+ origin.append(
+ LeftJoin(KarmaTotalCache, KarmaTotalCache.person == Person.id))
+ columns.append(KarmaTotalCache)
+ if need_ubuntu_coc:
+ columns.append(Alias(Exists(Select(SignedCodeOfConduct,
+ Person._is_ubuntu_coc_signer_condition())),
+ name='is_ubuntu_coc_signer'))
+ if need_location:
+ # New people have no location rows
+ origin.append(
+ LeftJoin(PersonLocation, PersonLocation.person == Person.id))
+ columns.append(PersonLocation)
+ if need_archive:
+ # Not everyone has PPA's
+ # It would be nice to cleanly expose the soyuz rules for this to avoid
+ # duplicating the relationships.
+ origin.append(
+ LeftJoin(Archive, Archive.owner == Person.id))
+ columns.append(Archive)
+ conditions = And(conditions,
+ Or(Archive.id == None, And(
+ Archive.id == Select(Min(Archive.id),
+ Archive.owner == Person.id, Archive),
+ Archive.purpose == ArchivePurpose.PPA)))
+ if need_preferred_email:
+ # Teams don't have email.
+ origin.append(
+ LeftJoin(EmailAddress, EmailAddress.person == Person.id))
+ columns.append(EmailAddress)
+ conditions = And(conditions,
+ Or(EmailAddress.status == None,
+ EmailAddress.status == EmailAddressStatus.PREFERRED))
+ if need_validity:
+ # May find invalid persons
+ origin.append(
+ LeftJoin(ValidPersonCache, ValidPersonCache.id == Person.id))
+ columns.append(ValidPersonCache)
+ if len(columns) == 1:
+ columns = columns[0]
+ # Return a simple ResultSet
+ return store.using(*origin).find(columns, conditions)
+ # Adapt the result into a cached Person.
+ columns = tuple(columns)
+ raw_result = store.using(*origin).find(columns, conditions)
+ def prepopulate_person(row):
+ result = row[0]
+ index = 1
+ #-- karma caching
+ if need_karma:
+ karma = row[index]
+ index += 1
+ if karma is None:
+ karma_total = 0
+ else:
+ karma_total = karma.karma_total
+ result._karma_cached = karma_total
+ #-- ubuntu code of conduct signer status caching.
+ if need_ubuntu_coc:
+ signed = row[index]
+ index += 1
+ result._is_ubuntu_coc_signer_cached = signed
+ #-- location caching
+ if need_location:
+ location = row[index]
+ index += 1
+ result._location = location
+ #-- archive caching
+ if need_archive:
+ archive = row[index]
+ index += 1
+ result._archive_cached = archive
+ #-- preferred email caching
+ if need_preferred_email:
+ email = row[index]
+ index += 1
+ result._preferredemail_cached = email
+ if need_validity:
+ valid = row[index]
+ index += 1
+ result._is_valid_person_cached = valid is not None
+ return result
+ return DecoratedResultSet(raw_result, result_decorator=prepopulate_person)
+
+ def _getMembersWithPreferredEmails(self):
"""Helper method for public getMembersWithPreferredEmails.
We can't return the preferred email address directly to the
@@ -1472,20 +1592,18 @@
EmailAddress.status == EmailAddressStatus.PREFERRED)
return store.using(*origin).find((Person, EmailAddress), conditions)
- def getMembersWithPreferredEmails(self, include_teams=False):
+ def getMembersWithPreferredEmails(self):
"""See `IPerson`."""
- result = self._getMembersWithPreferredEmails(
- include_teams=include_teams)
+ result = self._getMembersWithPreferredEmails()
person_list = []
for person, email in result:
person._preferredemail_cached = email
person_list.append(person)
return person_list
- def getMembersWithPreferredEmailsCount(self, include_teams=False):
+ def getMembersWithPreferredEmailsCount(self):
"""See `IPerson`."""
- result = self._getMembersWithPreferredEmails(
- include_teams=include_teams)
+ result = self._getMembersWithPreferredEmails()
return result.count()
@property
@@ -2290,17 +2408,23 @@
distribution.main_archive, self)
return permissions.count() > 0
- @cachedproperty
+ @cachedproperty('_is_ubuntu_coc_signer_cached')
def is_ubuntu_coc_signer(self):
"""See `IPerson`."""
+ # Also assigned to by self._all_members.
+ store = Store.of(self)
+ query = Person._is_ubuntu_coc_signer_condition()
+ # TODO: Using exists would be faster than count().
+ return bool(store.find(SignedCodeOfConduct, query).count())
+
+ @staticmethod
+ def _is_ubuntu_coc_signer_condition():
+ """Generate a Storm Expr for determing the coc signing status."""
sigset = getUtility(ISignedCodeOfConductSet)
lastdate = sigset.getLastAcceptedDate()
-
- query = AND(SignedCodeOfConduct.q.active==True,
- SignedCodeOfConduct.q.ownerID==self.id,
- SignedCodeOfConduct.q.datecreated>=lastdate)
-
- return bool(SignedCodeOfConduct.select(query).count())
+ return AND(SignedCodeOfConduct.active == True,
+ SignedCodeOfConduct.ownerID == Person.id,
+ SignedCodeOfConduct.datecreated >= lastdate)
@property
def activesignatures(self):
@@ -2314,7 +2438,7 @@
sCoC_util = getUtility(ISignedCodeOfConductSet)
return sCoC_util.searchByUser(self.id, active=False)
- @property
+ @cachedproperty('_archive_cached')
def archive(self):
"""See `IPerson`."""
return getUtility(IArchiveSet).getPPAOwnedByPerson(self)
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-08-04 23:27:49 +0000
+++ lib/lp/registry/tests/test_person.py 2010-08-09 07:06:07 +0000
@@ -7,6 +7,8 @@
import pytz
import time
+from testtools.matchers import LessThan
+
import transaction
from zope.component import getUtility
@@ -26,6 +28,7 @@
IPersonSet, ImmutableVisibilityError, NameAlreadyTaken,
PersonCreationRationale, PersonVisibility)
from canonical.launchpad.database import Bug, BugTask, BugSubscription
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
from lp.registry.model.structuralsubscription import (
StructuralSubscription)
from lp.registry.model.karma import KarmaCategory
@@ -34,8 +37,12 @@
from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
from lp.answers.model.answercontact import AnswerContact
from lp.blueprints.model.specification import Specification
-from lp.testing import login_person, logout, TestCase, TestCaseWithFactory
+from lp.testing import (
+ login_person, logout, person_logged_in, TestCase, TestCaseWithFactory,
+ )
+from lp.testing.matchers import HasQueryCount
from lp.testing.views import create_initialized_view
+from lp.testing._webservice import QueryCollector
from lp.registry.interfaces.person import PrivatePersonLinkageError
from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
@@ -186,7 +193,8 @@
def test_person_snapshot(self):
omitted = (
- 'activemembers', 'adminmembers', 'allmembers', 'approvedmembers',
+ 'activemembers', 'adminmembers', 'allmembers',
+ 'all_members_prepopulated', 'approvedmembers',
'deactivatedmembers', 'expiredmembers', 'inactivemembers',
'invited_members', 'member_memberships', 'pendingmembers',
'proposedmembers', 'unmapped_participants',
@@ -548,3 +556,28 @@
names = [entry['project'].name for entry in results]
self.assertEqual(
['cc', 'bb', 'aa', 'dd', 'ee'], names)
+
+
+class TestAPIPartipication(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_participation_query_limit(self):
+ # A team with 3 members should only query once for all their
+ # attributes.
+ team = self.factory.makeTeam()
+ with person_logged_in(team.teamowner):
+ team.addMember(self.factory.makePerson(), team.teamowner)
+ team.addMember(self.factory.makePerson(), team.teamowner)
+ team.addMember(self.factory.makePerson(), team.teamowner)
+ webservice = LaunchpadWebServiceCaller()
+ collector = QueryCollector()
+ collector.register()
+ self.addCleanup(collector.unregister)
+ url = "/~%s/participants" % team.name
+ logout()
+ response = webservice.get(url, headers={'User-Agent':'AnonNeedsThis'})
+ self.assertEqual(response.status, 200,
+ "Got %d for url %r with response %r" % (
+ response.status, url, response.body))
+ self.assertThat(collector, HasQueryCount(LessThan(12)))
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2010-08-05 04:15:12 +0000
+++ lib/lp/soyuz/model/archive.py 2010-08-09 07:06:07 +0000
@@ -1804,6 +1804,7 @@
def getPPAOwnedByPerson(self, person, name=None):
"""See `IArchiveSet`."""
+ # See Person._all_members which also directly queries this.
store = Store.of(person)
clause = [
Archive.purpose == ArchivePurpose.PPA,
Follow ups
