launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17430
[Merge] lp:~wgrant/launchpad/bug-1383401 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-1383401 into lp:launchpad.
Commit message:
Fix Person.*members to preload all fields required for the IPerson JSON representation, eliminating several timeouts.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1383401 in Launchpad itself: "Timeout when retrieving expired members"
https://bugs.launchpad.net/launchpad/+bug/1383401
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1383401/+merge/239241
Fix timeouts on several collections of people by precaching everything needed for the IPerson JSON representation.
I added a new need_api convenience parameter to the Person precaching helper, as most of the callsites need to load exactly the same set of things.
I added a query count test for deactivated_members.
--
https://code.launchpad.net/~wgrant/launchpad/bug-1383401/+merge/239241
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1383401 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py 2014-01-14 07:03:22 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py 2014-10-22 17:33:23 +0000
@@ -3,18 +3,27 @@
__metaclass__ = type
+from testtools.matchers import Equals
+from zope.component import getUtility
from zope.security.management import endInteraction
from zope.security.proxy import removeSecurityProxy
+from lp.registry.interfaces.person import TeamMembershipStatus
+from lp.registry.interfaces.teammembership import ITeamMembershipSet
from lp.testing import (
admin_logged_in,
launchpadlib_for,
login,
logout,
+ record_two_runs,
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.pages import LaunchpadWebServiceCaller
+from lp.testing.matchers import HasQueryCount
+from lp.testing.pages import (
+ LaunchpadWebServiceCaller,
+ webservice_for_person,
+ )
class TestPersonEmailSecurity(TestCaseWithFactory):
@@ -78,6 +87,35 @@
'<a href="/~test-person" class="sprite person">Test Person</a>')
+class PersonWebServiceTests(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_deactivated_members_query_count(self):
+ with admin_logged_in():
+ team = self.factory.makeTeam()
+ owner = team.teamowner
+ name = team.name
+ ws = webservice_for_person(owner)
+
+ def create_member():
+ with admin_logged_in():
+ person = self.factory.makePerson()
+ team.addMember(person, owner)
+ getUtility(ITeamMembershipSet).getByPersonAndTeam(
+ person, team).setStatus(
+ TeamMembershipStatus.DEACTIVATED, owner, u"Go away.")
+
+ def get_members():
+ ws.get('/~%s/deactivated_members' % name).jsonBody()
+
+ # Ensure that we're already in a stable cache state.
+ get_members()
+ recorder1, recorder2 = record_two_runs(
+ get_members, create_member, 2)
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+
class PersonSetWebServiceTests(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2014-08-01 08:47:28 +0000
+++ lib/lp/registry/interfaces/person.py 2014-10-22 17:33:23 +0000
@@ -1463,7 +1463,8 @@
"List of direct members with ADMIN or APPROVED status"),
value_type=Reference(schema=Interface))),
exported_as='members')
- adminmembers = exported(
+ adminmembers = Attribute("List of this team's admins.")
+ api_adminmembers = exported(
doNotSnapshot(
CollectionField(
title=_("List of this team's admins."),
@@ -1472,7 +1473,7 @@
all_member_count = Attribute(
"The total number of real people who are members of this team, "
"including subteams.")
- all_members_prepopulated = exported(
+ api_all_members = exported(
doNotSnapshot(
CollectionField(
title=_("All participants of this team."),
@@ -1489,32 +1490,36 @@
approvedmembers = doNotSnapshot(
Attribute("List of members with APPROVED status"))
deactivated_member_count = Attribute("Number of deactivated members")
- deactivatedmembers = exported(
+ deactivatedmembers = Attribute("Former members of the team.")
+ api_deactivatedmembers = exported(
doNotSnapshot(
CollectionField(
- title=_(
- "All members whose membership is in the "
- "DEACTIVATED state"),
+ title=_("Former members of the team."),
value_type=Reference(schema=Interface))),
exported_as='deactivated_members')
expired_member_count = Attribute("Number of EXPIRED members.")
- expiredmembers = exported(
+ expiredmembers = Attribute("Expired members of the team.")
+ api_expiredmembers = exported(
doNotSnapshot(
CollectionField(
- title=_("All members whose membership is in the "
- "EXPIRED state"),
+ title=_("Expired members of the team."),
value_type=Reference(schema=Interface))),
exported_as='expired_members')
inactivemembers = doNotSnapshot(
Attribute(
"List of members with EXPIRED or DEACTIVATED status"))
inactive_member_count = Attribute("Number of inactive members")
- invited_members = exported(
+ invited_members = Attribute(
+ "Other teams which have been invited to become members of this "
+ "team.")
+ api_invited_members = exported(
doNotSnapshot(
CollectionField(
- title=_("All members whose membership is "
- "in the INVITED state"),
- value_type=Reference(schema=Interface))))
+ title=_(
+ "Other teams which have been invited to become members "
+ "of this team."),
+ value_type=Reference(schema=Interface))),
+ exported_as="invited_members")
invited_member_count = Attribute("Number of members with INVITED status")
member_memberships = exported(
@@ -1531,11 +1536,11 @@
pendingmembers = doNotSnapshot(
Attribute(
"List of members with INVITED or PROPOSED status"))
- proposedmembers = exported(
+ proposedmembers = Attribute("People who have applied to join the team.")
+ api_proposedmembers = exported(
doNotSnapshot(
CollectionField(
- title=_("All members whose membership is in the "
- "PROPOSED state"),
+ title=_("People who have applied to join the team."),
value_type=Reference(schema=Interface))),
exported_as='proposed_members')
proposed_member_count = Attribute("Number of PROPOSED members")
@@ -2338,12 +2343,14 @@
"""Prefetch Librarian aliases and content for personal images."""
def getPrecachedPersonsFromIDs(
- person_ids, need_karma=False, need_ubuntu_coc=False,
- need_location=False, need_archive=False,
+ person_ids, need_api=False, need_karma=False,
+ need_ubuntu_coc=False, need_location=False, need_archive=False,
need_preferred_email=False, need_validity=False):
"""Lookup person objects from ids with optional precaching.
:param person_ids: List of person ids.
+ :param need_api: All attributes needed by the JSON
+ representation will be cached.
:param need_karma: The karma attribute will be cached.
:param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
cached.
@@ -2490,13 +2497,13 @@
# Fix value_type.schema of IPersonViewRestricted attributes.
for name in [
- 'all_members_prepopulated',
+ 'api_all_members',
'api_activemembers',
- 'adminmembers',
- 'proposedmembers',
- 'invited_members',
- 'deactivatedmembers',
- 'expiredmembers',
+ 'api_adminmembers',
+ 'api_proposedmembers',
+ 'api_invited_members',
+ 'api_deactivatedmembers',
+ 'api_expiredmembers',
]:
IPersonViewRestricted[name].value_type.schema = IPerson
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2014-08-01 08:47:00 +0000
+++ lib/lp/registry/model/person.py 2014-10-22 17:33:23 +0000
@@ -1893,11 +1893,9 @@
return self._members(direct=False)
@property
- def all_members_prepopulated(self):
+ def api_all_members(self):
"""See `IPerson`."""
- return self._members(direct=False, need_karma=True,
- need_ubuntu_coc=True, need_location=True, need_archive=True,
- need_preferred_email=True, need_validity=True)
+ return self._members(direct=False, preload_for_api=True)
@staticmethod
def _validity_queries(person_table=None):
@@ -1963,20 +1961,12 @@
tables=columns,
decorators=decorators)
- def _members(self, direct, need_karma=False, need_ubuntu_coc=False,
- need_location=False, need_archive=False, need_preferred_email=False,
- need_validity=False):
+ def _members(self, direct, status=None, preload_for_api=False):
"""Lookup all members of the team with optional precaching.
:param direct: If True only direct members are returned.
- :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.
+ :param preload_for_api: Preload attributes contained in the API
+ JSON representation.
"""
# TODO: consolidate this with getMembersWithPreferredEmails.
# The difference between the two is that
@@ -1984,6 +1974,7 @@
# wrong, but perhaps deliberate.
origin = [Person]
if not direct:
+ assert status is None
origin.append(Join(
TeamParticipation, TeamParticipation.person == Person.id))
conditions = And(
@@ -1992,26 +1983,20 @@
# But not the team itself.
TeamParticipation.person != self.id)
else:
+ if not isinstance(status, tuple):
+ status = (status,)
origin.append(Join(
TeamMembership, TeamMembership.personID == Person.id))
conditions = And(
# Membership in this team,
TeamMembership.team == self.id,
# And approved or admin status
- TeamMembership.status.is_in([
- TeamMembershipStatus.APPROVED,
- TeamMembershipStatus.ADMIN]))
+ TeamMembership.status.is_in(status))
# Use a PersonSet object that is not security proxied to allow
# manipulation of the object.
person_set = PersonSet()
return person_set._getPrecachedPersons(
- origin, conditions, store=Store.of(self),
- need_karma=need_karma,
- need_ubuntu_coc=need_ubuntu_coc,
- need_location=need_location,
- need_archive=need_archive,
- need_preferred_email=need_preferred_email,
- need_validity=need_validity)
+ origin, conditions, store=Store.of(self), need_api=preload_for_api)
def _getMembersWithPreferredEmails(self):
"""Helper method for public getMembersWithPreferredEmails.
@@ -2056,6 +2041,12 @@
return self.getMembersByStatus(TeamMembershipStatus.INVITED)
@property
+ def api_invited_members(self):
+ return self._members(
+ True, status=TeamMembershipStatus.INVITED,
+ preload_for_api=True)
+
+ @property
def invited_member_count(self):
"""See `IPerson`."""
return self.invited_members.count()
@@ -2066,6 +2057,12 @@
return self.getMembersByStatus(TeamMembershipStatus.DEACTIVATED)
@property
+ def api_deactivatedmembers(self):
+ return self._members(
+ True, status=TeamMembershipStatus.DEACTIVATED,
+ preload_for_api=True)
+
+ @property
def deactivated_member_count(self):
"""See `IPerson`."""
return self.deactivatedmembers.count()
@@ -2076,6 +2073,12 @@
return self.getMembersByStatus(TeamMembershipStatus.EXPIRED)
@property
+ def api_expiredmembers(self):
+ return self._members(
+ True, status=TeamMembershipStatus.EXPIRED,
+ preload_for_api=True)
+
+ @property
def expired_member_count(self):
"""See `IPerson`."""
return self.expiredmembers.count()
@@ -2086,6 +2089,12 @@
return self.getMembersByStatus(TeamMembershipStatus.PROPOSED)
@property
+ def api_proposedmembers(self):
+ return self._members(
+ True, status=TeamMembershipStatus.PROPOSED,
+ preload_for_api=True)
+
+ @property
def proposed_member_count(self):
"""See `IPerson`."""
return self.proposedmembers.count()
@@ -2096,6 +2105,12 @@
return self.getMembersByStatus(TeamMembershipStatus.ADMIN)
@property
+ def api_adminmembers(self):
+ return self._members(
+ True, status=TeamMembershipStatus.ADMIN,
+ preload_for_api=True)
+
+ @property
def approvedmembers(self):
"""See `IPerson`."""
return self.getMembersByStatus(TeamMembershipStatus.APPROVED)
@@ -2109,9 +2124,11 @@
@property
def api_activemembers(self):
"""See `IPerson`."""
- return self._members(direct=True, need_karma=True,
- need_ubuntu_coc=True, need_location=True, need_archive=True,
- need_preferred_email=True, need_validity=True)
+ return self._members(
+ direct=True,
+ status=(
+ TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN),
+ preload_for_api=True)
@property
def active_member_count(self):
@@ -3755,9 +3772,10 @@
% sqlvalues(aliases), prejoins=["content"]))
def getPrecachedPersonsFromIDs(
- self, person_ids, need_karma=False, need_ubuntu_coc=False,
- need_location=False, need_archive=False,
- need_preferred_email=False, need_validity=False, need_icon=False):
+ self, person_ids, need_api=False, need_karma=False,
+ need_ubuntu_coc=False, need_location=False, need_archive=False,
+ need_preferred_email=False, need_validity=False,
+ need_icon=False):
"""See `IPersonSet`."""
person_ids = set(person_ids)
person_ids.discard(None)
@@ -3767,16 +3785,16 @@
conditions = [
Person.id.is_in(person_ids)]
return self._getPrecachedPersons(
- origin, conditions,
+ origin, conditions, need_api=need_api,
need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
need_location=need_location, need_archive=need_archive,
need_preferred_email=need_preferred_email,
need_validity=need_validity, need_icon=need_icon)
def _getPrecachedPersons(
- self, origin, conditions, store=None,
- need_karma=False, need_ubuntu_coc=False,
- need_location=False, need_archive=False, need_preferred_email=False,
+ self, origin, conditions, store=None, need_api=False,
+ need_karma=False, need_ubuntu_coc=False, need_location=False,
+ need_archive=False, need_preferred_email=False,
need_validity=False, need_icon=False):
"""Lookup all members of the team with optional precaching.
@@ -3799,13 +3817,13 @@
store = IStore(Person)
columns = [Person]
decorators = []
- if need_karma:
+ if need_karma or need_api:
# New people have no karmatotalcache rows.
origin.append(
LeftJoin(KarmaTotalCache,
KarmaTotalCache.person == Person.id))
columns.append(KarmaTotalCache)
- if need_ubuntu_coc:
+ if need_ubuntu_coc or need_api:
columns.append(
Alias(
Exists(Select(
@@ -3815,13 +3833,13 @@
Person._is_ubuntu_coc_signer_condition(),
SignedCodeOfConduct.ownerID == Person.id))),
name='is_ubuntu_coc_signer'))
- if need_location:
+ if need_location or need_api:
# New people have no location rows
origin.append(
LeftJoin(PersonLocation,
PersonLocation.person == Person.id))
columns.append(PersonLocation)
- if need_archive:
+ if need_archive or need_api:
# Not everyone has PPAs.
# It would be nice to cleanly expose the soyuz rules for this to
# avoid duplicating the relationships.
@@ -3840,7 +3858,7 @@
columns.append(Archive)
# Checking validity requires having a preferred email.
- if need_preferred_email and not need_validity:
+ if not need_api and need_preferred_email and not need_validity:
# Teams don't have email, so a left join
origin.append(
LeftJoin(EmailAddress, EmailAddress.person == Person.id))
@@ -3848,7 +3866,7 @@
conditions = And(conditions,
Or(EmailAddress.status == None,
EmailAddress.status == EmailAddressStatus.PREFERRED))
- if need_validity:
+ if need_validity or need_api:
valid_stuff = Person._validity_queries()
origin.extend(valid_stuff["joins"])
columns.extend(valid_stuff["tables"])
@@ -3870,7 +3888,7 @@
cache = get_property_cache(result)
index = 1
#-- karma caching
- if need_karma:
+ if need_karma or need_api:
karma = row[index]
index += 1
if karma is None:
@@ -3879,22 +3897,22 @@
karma_total = karma.karma_total
cache.karma = karma_total
#-- ubuntu code of conduct signer status caching.
- if need_ubuntu_coc:
+ if need_ubuntu_coc or need_api:
signed = row[index]
index += 1
cache.is_ubuntu_coc_signer = signed
#-- location caching
- if need_location:
+ if need_location or need_api:
location = row[index]
index += 1
cache.location = location
#-- archive caching
- if need_archive:
+ if need_archive or need_api:
archive = row[index]
index += 1
cache.archive = archive
#-- preferred email caching
- if need_preferred_email and not need_validity:
+ if not need_api and need_preferred_email and not need_validity:
email = row[index]
index += 1
cache.preferredemail = email
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2014-06-19 06:38:53 +0000
+++ lib/lp/registry/tests/test_person.py 2014-10-22 17:33:23 +0000
@@ -244,27 +244,27 @@
def test_inTeam_person_incorrect_archive(self):
# If a person has an archive marked incorrectly that person should
- # still be retrieved by 'all_members_prepopulated'. See bug #680461.
+ # still be retrieved by 'api_all_members'. See bug #680461.
self.factory.makeArchive(
owner=self.user, purpose=ArchivePurpose.PARTNER)
expected_members = sorted([self.user, self.a_team.teamowner])
- retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+ retrieved_members = sorted(list(self.a_team.api_all_members))
self.assertEqual(expected_members, retrieved_members)
def test_inTeam_person_no_archive(self):
# If a person has no archive that person should still be retrieved by
- # 'all_members_prepopulated'.
+ # 'api_all_members'.
expected_members = sorted([self.user, self.a_team.teamowner])
- retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+ retrieved_members = sorted(list(self.a_team.api_all_members))
self.assertEqual(expected_members, retrieved_members)
def test_inTeam_person_ppa_archive(self):
# If a person has a PPA that person should still be retrieved by
- # 'all_members_prepopulated'.
+ # 'api_all_members'.
self.factory.makeArchive(
owner=self.user, purpose=ArchivePurpose.PPA)
expected_members = sorted([self.user, self.a_team.teamowner])
- retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+ retrieved_members = sorted(list(self.a_team.api_all_members))
self.assertEqual(expected_members, retrieved_members)
def test_getOwnedTeams(self):
@@ -959,7 +959,7 @@
def test_person_snapshot(self):
omitted = (
'activemembers', 'adminmembers', 'allmembers',
- 'all_members_prepopulated', 'approvedmembers',
+ 'api_all_members', 'approvedmembers',
'deactivatedmembers', 'expiredmembers', 'inactivemembers',
'invited_members', 'member_memberships', 'pendingmembers',
'proposedmembers', 'time_zone',
