launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04841
[Merge] lp:~wallyworld/launchpad/affiliation-query-count into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/affiliation-query-count into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #838604 in Launchpad itself: "Pillar affiliation query count too high"
https://bugs.launchpad.net/launchpad/+bug/838604
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/affiliation-query-count/+merge/73630
When determining the pillar affiliation for a batch of people in the person picker, the affiliation adaptor checks for membership of various teams eg pillar owner, pillar drivers. It does this one person/one team at a time. This results in potentially one query per check, which could be 24 or greater depending on picker batch size and pillar.
This branch makes the affiliation check more efficient.
== Implementation ==
The basis of the problem is: we have a set of people and some teams and we need to find which of those teams the people belong to. A new method is provided for this purpose in module lp.registry.model.teammembership - find_team_participations(people, teams=None). This method will use (at most) 1 query to return a dict of person->set(teams).
The affiliation business logic has been refactored to use the above code to look up the team based affiliations.
== Tests ==
Tests are provided for find_team_participations in a new test case TestTeamParticipationQuery
The existing pillar affiliation tests are sufficient for the new affiliation logic because the external behaviour has not changed.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/model/pillaraffiliation.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/tests/test_teammembership.p
--
https://code.launchpad.net/~wallyworld/launchpad/affiliation-query-count/+merge/73630
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/affiliation-query-count into lp:launchpad.
=== modified file 'lib/lp/registry/model/pillaraffiliation.py'
--- lib/lp/registry/model/pillaraffiliation.py 2011-08-18 20:35:41 +0000
+++ lib/lp/registry/model/pillaraffiliation.py 2011-09-01 06:27:28 +0000
@@ -34,6 +34,7 @@
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
)
+from lp.registry.model.teammembership import find_team_participations
class IHasAffiliation(Interface):
@@ -61,36 +62,71 @@
implements(IHasAffiliation)
+ # We rank the affiliations from most important to least important.
+ # Unlisted roles are given a rank of 10.
+ affiliation_priorities = {
+ 'maintainer': 1,
+ 'driver': 2,
+ 'bug supervisor': 3,
+ 'security contact': 4,
+ }
+
def __init__(self, context):
self.context = context
def getPillar(self):
return self.context
- def _getAffiliationDetails(self, person, pillar):
+ def _getAffiliation(self, person, pillar):
""" Return the affiliation information for a person, if any.
+ Subclasses will override this method to perform specific affiliation
+ checks.
+ The return result is a list of tuples (pillar displayanme, role).
+ """
+ return []
+
+ def _getAffiliationTeamRoles(self, pillar):
+ """ Return teams for which a person needs to belong, if affiliated.
+
A person is affiliated with a pillar if they are in the list of
drivers or are the maintainer.
- The return result is a list of tuples (pillar displayanme, role).
"""
- result = []
- if person.inTeam(pillar.owner):
- result.append((pillar.displayname, 'maintainer'))
- for driver in pillar.drivers:
- if person.inTeam(driver):
- result.append((pillar.displayname, 'driver'))
- break
+ result = {
+ (pillar.displayname, 'maintainer'): [pillar.owner],
+ (pillar.displayname, 'driver'): pillar.drivers}
return result
def getAffiliationBadges(self, persons):
""" Return the affiliation badge details for people given a context.
+
+ There are 2 ways we check for affiliation:
+ 1. Generic membership checks of particular teams as returned by
+ _getAffiliationTeamRoles
+ 2. Specific affiliation checks as performed by _getAffiliation
"""
pillar = self.getPillar()
result = []
+
+ # We find the teams to check for participation..
+ affiliation_team_details = self._getAffiliationTeamRoles(pillar)
+ teams_to_check = set()
+ for teams in affiliation_team_details.values():
+ teams_to_check.update(teams)
+ # We gather the participation for the persons.
+ people_teams = find_team_participations(persons, teams_to_check)
+
for person in persons:
- affiliation_details = self._getAffiliationDetails(person, pillar)
- if not affiliation_details:
+ # Specific affiliations
+ affiliations = self._getAffiliation(person, pillar)
+ # Generic, team based affiliations
+ affiliated_teams = people_teams.get(person, [])
+ for affiliated_team in affiliated_teams:
+ for affiliation, teams in affiliation_team_details.items():
+ if affiliated_team in teams:
+ affiliations.append(affiliation)
+
+ if not affiliations:
result.append([])
continue
@@ -108,8 +144,14 @@
else:
default_icon_url = "/@@/product-badge"
icon_url = getIconUrl(self.context, pillar, default_icon_url)
+
+ # Sort the affiliation list according the the importance of each
+ # affiliation role.
+ affiliations.sort(
+ key=lambda affiliation_rec:
+ self.affiliation_priorities.get(affiliation_rec[1], 10))
badges = []
- for affiliation in affiliation_details:
+ for affiliation in affiliations:
alt_text = "%s %s" % affiliation
badges.append(BadgeDetails(icon_url, alt_text))
result.append(badges)
@@ -121,19 +163,19 @@
def getPillar(self):
return self.context.pillar
- def _getAffiliationDetails(self, person, pillar):
+ def _getAffiliationTeamRoles(self, pillar):
""" A person is affiliated with a bugtask based on (in order):
- owner of bugtask pillar
- driver of bugtask pillar
- bug supervisor of bugtask pillar
- security contact of bugtask pillar
"""
- result = super(BugTaskPillarAffiliation, self)._getAffiliationDetails(
- person, pillar)
- if person.inTeam(pillar.bug_supervisor):
- result.append((pillar.displayname, 'bug supervisor'))
- if person.inTeam(pillar.security_contact):
- result.append((pillar.displayname, 'security contact'))
+ super_instance = super(BugTaskPillarAffiliation, self)
+ result = super_instance._getAffiliationTeamRoles(pillar)
+ result[(pillar.displayname, 'bug supervisor')] = (
+ [pillar.bug_supervisor])
+ result[(pillar.displayname, 'security contact')] = (
+ [pillar.security_contact])
return result
@@ -146,9 +188,9 @@
def getBranch(self):
return self.context
- def _getAffiliationDetails(self, person, pillar):
+ def _getAffiliation(self, person, pillar):
super_instance = super(BranchPillarAffiliation, self)
- result = super_instance._getAffiliationDetails(person, pillar)
+ result = super_instance._getAffiliation(person, pillar)
if self.getBranch().isPersonTrustedReviewer(person):
result.append((pillar.displayname, 'trusted reviewer'))
return result
@@ -185,18 +227,20 @@
class QuestionPillarAffiliation(PillarAffiliation):
- """An affiliation adapter for questions."""
+ """An affiliation adapter for questions.
+
+ A person is affiliated with a question based on (in order):
+ - answer contact for question target
+ - owner of question target
+ - driver of question target
+ """
+
def getPillar(self):
return self.context.product or self.context.distribution
- def _getAffiliationDetails(self, person, pillar):
- """ A person is affiliated with a question based on (in order):
- - answer contact for question target
- - owner of question target
- - driver of question target
- """
+ def _getAffiliation(self, person, pillar):
result = (super(QuestionPillarAffiliation, self)
- ._getAffiliationDetails(person, pillar))
+ ._getAffiliation(person, pillar))
target = self.context.target
if IDistributionSourcePackage.providedBy(target):
question_targets = (target, target.distribution)
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2011-08-25 20:00:19 +0000
+++ lib/lp/registry/model/teammembership.py 2011-09-01 06:27:28 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
__all__ = [
+ 'find_team_participations',
'TeamMembership',
'TeamMembershipSet',
'TeamParticipation',
@@ -20,6 +21,7 @@
ForeignKey,
StringCol,
)
+from storm.info import ClassAlias
from storm.store import Store
from zope.component import getUtility
from zope.interface import implements
@@ -655,3 +657,64 @@
store = Store.of(member)
store.execute(query)
+
+
+def find_team_participations(people, teams=None):
+ """Find the teams the given people participate in.
+
+ :param people: The people for which to query team participation.
+ :param teams: Optionally, limit the participation check to these teams.
+
+ This method performs its work with at most a single database query.
+ It first does similar checks to those performed by IPerson.in_team() and
+ it may turn out that no database query is required at all.
+ """
+
+ teams_to_query = []
+ people_teams = {}
+
+ def add_team_to_result(person, team):
+ teams = people_teams.get(person)
+ if teams is None:
+ teams = set()
+ people_teams[person] = teams
+ teams.add(team)
+
+ # Check for the simple cases - self membership etc.
+ if teams:
+ for team in teams:
+ if team is None:
+ continue
+ for person in people:
+ if team.id == person.id:
+ add_team_to_result(person, team)
+ continue
+ if not team.is_team:
+ continue
+ teams_to_query.append(team)
+
+ # Avoid circular imports
+ from lp.registry.model.person import Person
+
+ # We are either checking for membership of any team or didn't eliminate
+ # all the specific team participation checks above.
+ if teams_to_query or not teams:
+ Team = ClassAlias(Person, 'Team')
+ person_ids = [person.id for person in people]
+ conditions = [
+ TeamParticipation.personID == Person.id,
+ TeamParticipation.teamID == Team.id,
+ Person.id.is_in(person_ids)
+ ]
+ team_ids = [team.id for team in teams_to_query]
+ if team_ids:
+ conditions.append(Team.id.is_in(team_ids))
+
+ store = IStore(Person)
+ rs = store.find(
+ (Person, Team),
+ *conditions)
+
+ for (person, team) in rs:
+ add_team_to_result(person, team)
+ return people_teams
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py 2011-08-26 20:00:37 +0000
+++ lib/lp/registry/tests/test_teammembership.py 2011-09-01 06:27:28 +0000
@@ -9,6 +9,7 @@
)
import re
import subprocess
+from testtools.matchers import Equals
from unittest import (
TestCase,
TestLoader,
@@ -52,7 +53,8 @@
ITeamMembershipSet,
TeamMembershipStatus,
)
-from lp.registry.model.teammembership import (
+from lp.registry.model.teammembership import (\
+ find_team_participations,
TeamMembership,
TeamParticipation,
)
@@ -60,8 +62,9 @@
login_celebrity,
person_logged_in,
TestCaseWithFactory,
- )
+ StormStatementRecorder)
from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import HasQueryCount
from lp.testing.storm import reload_object
@@ -261,6 +264,53 @@
return IStore(TeamParticipation).find(TeamParticipation).count()
+class TestTeamParticipationQuery(TeamParticipationTestCase):
+ """A test case for teammembership.test_find_team_participations."""
+
+ def test_find_team_participations(self):
+ # The correct team participations are found and the query count is 1.
+ self.team1.addMember(self.no_priv, self.foo_bar)
+ self.team2.addMember(self.no_priv, self.foo_bar)
+ self.team1.addMember(self.team2, self.foo_bar, force_team_add=True)
+
+ people = [self.team1, self.team2]
+ with StormStatementRecorder() as recorder:
+ people_teams = find_team_participations(people)
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+ self.assertContentEqual([self.team1, self.team2], people_teams.keys())
+ self.assertContentEqual([self.team1], people_teams[self.team1])
+ self.assertContentEqual(
+ [self.team1, self.team2], people_teams[self.team2])
+
+ def test_find_team_participations_limited_teams(self):
+ # The correct team participations are found and the query count is 1.
+ self.team1.addMember(self.no_priv, self.foo_bar)
+ self.team2.addMember(self.no_priv, self.foo_bar)
+ self.team1.addMember(self.team2, self.foo_bar, force_team_add=True)
+
+ people = [self.foo_bar, self.team2]
+ teams = [self.team1, self.team2]
+ with StormStatementRecorder() as recorder:
+ people_teams = find_team_participations(people, teams)
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+ self.assertContentEqual(
+ [self.foo_bar, self.team2], people_teams.keys())
+ self.assertContentEqual(
+ [self.team1, self.team2], people_teams[self.foo_bar])
+ self.assertContentEqual(
+ [self.team1, self.team2], people_teams[self.team2])
+
+ def test_find_team_participations_no_query(self):
+ # Check that no database query is made unless necessary.
+ people = [self.foo_bar, self.team2]
+ teams = [self.foo_bar]
+ with StormStatementRecorder() as recorder:
+ people_teams = find_team_participations(people, teams)
+ self.assertThat(recorder, HasQueryCount(Equals(0)))
+ self.assertContentEqual([self.foo_bar], people_teams.keys())
+ self.assertContentEqual([self.foo_bar], people_teams[self.foo_bar])
+
+
class TestTeamParticipationHierarchy(TeamParticipationTestCase):
"""Participation management tests using 5 nested teams.
