launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05369
[Merge] lp:~allenap/launchpad/participation-keyerror-bug-810114 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/participation-keyerror-bug-810114 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #810114 in Launchpad itself: "KeyError in +participation page."
https://bugs.launchpad.net/launchpad/+bug/810114
For more details, see:
https://code.launchpad.net/~allenap/launchpad/participation-keyerror-bug-810114/+merge/81012
This refactors the check-teamparticipation script, and addresses
performance by slurping everything from TeamMembership and
TeamParticipation into memory and doing the checks there.
--
https://code.launchpad.net/~allenap/launchpad/participation-keyerror-bug-810114/+merge/81012
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/participation-keyerror-bug-810114 into lp:launchpad.
=== modified file 'cronscripts/check-teamparticipation.py'
--- cronscripts/check-teamparticipation.py 2011-09-18 03:43:46 +0000
+++ cronscripts/check-teamparticipation.py 2011-11-02 12:47:26 +0000
@@ -20,77 +20,8 @@
import _pythonpath
-import transaction
-
-from canonical.database.sqlbase import cursor
-from lp.services.scripts.base import LaunchpadScript, LaunchpadScriptFailure
-
-
-def check_teamparticipation(log):
- # Check self-participation.
- query = """
- SELECT id, name
- FROM Person WHERE id NOT IN (
- SELECT person FROM Teamparticipation WHERE person = team
- ) AND merged IS NULL
- """
- cur = cursor()
- cur.execute(query)
- non_self_participants = cur.fetchall()
- if len(non_self_participants) > 0:
- log.warn("Some people/teams are not members of themselves: %s"
- % non_self_participants)
-
- # Check if there are any circular references between teams.
- cur.execute("""
- SELECT tp.team, tp2.team
- FROM teamparticipation AS tp, teamparticipation AS tp2
- WHERE tp.team = tp2.person
- AND tp.person = tp2.team
- AND tp.id != tp2.id;
- """)
- circular_references = cur.fetchall()
- if len(circular_references) > 0:
- raise LaunchpadScriptFailure(
- "Circular references found: %s" % circular_references)
-
- # Check if there are any missing/spurious TeamParticipation entries.
- cur.execute("SELECT id FROM Person WHERE teamowner IS NOT NULL")
- team_ids = cur.fetchall()
- transaction.abort()
-
- def get_participants(team):
- """Recurse through the team's members to get all its participants."""
- participants = set()
- for member in team.activemembers:
- participants.add(member)
- if member.is_team:
- participants.update(get_participants(member))
- return participants
-
- from lp.registry.model.person import Person
- batch = team_ids[:50]
- team_ids = team_ids[50:]
- while batch:
- for [id] in batch:
- team = Person.get(id)
- expected = get_participants(team)
- found = set(team.allmembers)
- difference = expected.difference(found)
- if len(difference) > 0:
- people = ", ".join("%s (%s)" % (person.name, person.id)
- for person in difference)
- log.warn("%s (%s): missing TeamParticipation entries for %s."
- % (team.name, team.id, people))
- reverse_difference = found.difference(expected)
- if len(reverse_difference) > 0:
- people = ", ".join("%s (%s)" % (person.name, person.id)
- for person in reverse_difference)
- log.warn("%s (%s): spurious TeamParticipation entries for %s."
- % (team.name, team.id, people))
- transaction.abort()
- batch = team_ids[:50]
- team_ids = team_ids[50:]
+from lp.registry.scripts.teamparticipation import check_teamparticipation
+from lp.services.scripts.base import LaunchpadScript
class CheckTeamParticipationScript(LaunchpadScript):
@@ -99,5 +30,6 @@
def main(self):
check_teamparticipation(self.logger)
+
if __name__ == '__main__':
CheckTeamParticipationScript("check-teamparticipation").run()
=== added file 'lib/lp/registry/scripts/teamparticipation.py'
--- lib/lp/registry/scripts/teamparticipation.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/scripts/teamparticipation.py 2011-11-02 12:47:26 +0000
@@ -0,0 +1,146 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""XXX: Module docstring goes here."""
+
+__metaclass__ = type
+__all__ = [
+ "check_teamparticipation",
+ ]
+
+from collections import (
+ defaultdict,
+ namedtuple,
+ )
+from itertools import (
+ chain,
+ imap,
+ )
+
+import transaction
+from zope.component import getUtility
+
+from canonical.database.sqlbase import quote
+from canonical.launchpad.webapp.interfaces import (
+ IStoreSelector,
+ MAIN_STORE,
+ SLAVE_FLAVOR,
+ )
+from lp.registry.interfaces.teammembership import ACTIVE_STATES
+from lp.services.scripts.base import LaunchpadScriptFailure
+
+
+def get_store():
+ # Return a slave store.
+ return getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
+
+
+def check_teamparticipation_self(log):
+ # Check self-participation.
+ query = """
+ SELECT id, name
+ FROM Person WHERE id NOT IN (
+ SELECT person FROM Teamparticipation WHERE person = team
+ ) AND merged IS NULL
+ """
+ non_self_participants = list(get_store().execute(query))
+ if len(non_self_participants) > 0:
+ log.warn(
+ "Some people/teams are not members of themselves: %s",
+ non_self_participants)
+
+
+def check_teamparticipation_circular(log):
+ # Check if there are any circular references between teams.
+ query = """
+ SELECT tp.team, tp2.team
+ FROM teamparticipation AS tp, teamparticipation AS tp2
+ WHERE tp.team = tp2.person
+ AND tp.person = tp2.team
+ AND tp.id != tp2.id;
+ """
+ circular_references = list(get_store().execute(query))
+ if len(circular_references) > 0:
+ raise LaunchpadScriptFailure(
+ "Circular references found: %s" % circular_references)
+
+
+ConsistencyError = namedtuple(
+ "ConsistencyError", ("type", "team", "people"))
+
+
+def check_teamparticipation_consistency(log):
+ # Check if there are any missing/spurious TeamParticipation entries.
+ store = get_store()
+
+ # Slurp everything in.
+ people = dict(
+ store.execute(
+ "SELECT id, name FROM Person"
+ " WHERE teamowner IS NULL"
+ " AND merged IS NULL"))
+ teams = dict(
+ store.execute(
+ "SELECT id, name FROM Person"
+ " WHERE teamowner IS NOT NULL"
+ " AND merged IS NULL"))
+ team_memberships = defaultdict(set)
+ results = store.execute(
+ "SELECT team, person FROM TeamMembership"
+ " WHERE status in %s" % quote(ACTIVE_STATES))
+ for (team, person) in results:
+ team_memberships[team].add(person)
+ team_participations = defaultdict(set)
+ results = store.execute(
+ "SELECT team, person FROM TeamParticipation")
+ for (team, person) in results:
+ team_participations[team].add(person)
+
+ # Don't hold any locks.
+ transaction.abort()
+
+ # Check team memberships.
+ def get_participants(team):
+ """Recurse through membership records to get participants."""
+ member_people = team_memberships[team].intersection(people)
+ member_people.add(team) # Teams always participate in themselves.
+ member_teams = team_memberships[team].intersection(teams)
+ return member_people.union(
+ chain.from_iterable(imap(get_participants, member_teams)))
+
+ errors = []
+ for team in teams:
+ participants_observed = team_participations[team]
+ participants_expected = get_participants(team)
+ participants_spurious = participants_expected - participants_observed
+ participants_missing = participants_observed - participants_expected
+ if len(participants_spurious) > 0:
+ error = ConsistencyError("spurious", team, participants_spurious)
+ errors.append(error)
+ if len(participants_missing) > 0:
+ error = ConsistencyError("missing", team, participants_missing)
+ errors.append(error)
+
+ # TODO:
+ # - Check that the only participant of a *person* is the person.
+ # - Check that merged people and teams do not appear in TeamParticipation.
+
+ def get_repr(id):
+ return "%s (%d)" % (people[id] if id in people else teams[id], id)
+
+ for error in errors:
+ people_repr = ", ".join(imap(get_repr, error.people))
+ log.warn(
+ "%s: %s TeamParticipation entries for %s.",
+ get_repr(error.team), error.type, people_repr)
+
+ return errors
+
+
+def check_teamparticipation(log):
+ # Check self-participation.
+ check_teamparticipation_self(log)
+ # Check if there are any circular references between teams.
+ check_teamparticipation_circular(log)
+ # Check if there are any missing/spurious TeamParticipation entries.
+ check_teamparticipation_consistency(log)
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py 2011-09-01 06:18:57 +0000
+++ lib/lp/registry/tests/test_teammembership.py 2011-11-02 12:47:26 +0000
@@ -9,13 +9,11 @@
)
import re
import subprocess
+from unittest import TestLoader
+
+import pytz
+from testtools.content import text_content
from testtools.matchers import Equals
-from unittest import (
- TestCase,
- TestLoader,
- )
-
-import pytz
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -27,10 +25,6 @@
flush_database_updates,
sqlvalues,
)
-from canonical.launchpad.ftests import (
- login,
- login_person,
- )
from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.testing.systemdocs import (
default_optionflags,
@@ -53,16 +47,22 @@
ITeamMembershipSet,
TeamMembershipStatus,
)
-from lp.registry.model.teammembership import (\
+from lp.registry.model.teammembership import (
find_team_participations,
TeamMembership,
TeamParticipation,
)
+from lp.registry.scripts.teamparticipation import check_teamparticipation
+from lp.services.log.logger import BufferLogger
from lp.testing import (
+ login,
login_celebrity,
+ login_person,
person_logged_in,
+ StormStatementRecorder,
+ TestCase,
TestCaseWithFactory,
- StormStatementRecorder)
+ )
from lp.testing.mail_helpers import pop_notifications
from lp.testing.matchers import HasQueryCount
from lp.testing.storm import reload_object
@@ -1047,6 +1047,7 @@
class TestCheckTeamParticipationScript(TestCase):
+
layer = DatabaseFunctionalLayer
def _runScript(self, expected_returncode=0):
@@ -1054,14 +1055,19 @@
'cronscripts/check-teamparticipation.py', shell=True,
stdin=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
- (out, err) = process.communicate()
- self.assertEqual(process.returncode, expected_returncode, (out, err))
+ out, err = process.communicate()
+ if len(out) > 0:
+ self.addDetail("stdout", text_content(out))
+ if len(err) > 0:
+ self.addDetail("stderr", text_content(err))
+ self.assertEqual(process.returncode, expected_returncode)
return out, err
def test_no_output_if_no_invalid_entries(self):
"""No output if there's no invalid teamparticipation entries."""
out, err = self._runScript()
- self.assertEqual((out, err), ('', ''))
+ self.assertEqual(0, len(out))
+ self.assertEqual(0, len(err))
def test_report_invalid_teamparticipation_entries(self):
"""The script reports missing/spurious TeamParticipation entries.
@@ -1103,16 +1109,13 @@
transaction.commit()
out, err = self._runScript()
- self.assertEqual(out, '', (out, err))
- self.failUnless(
- re.search('missing TeamParticipation entries for zzzzz', err),
- (out, err))
- self.failUnless(
- re.search('spurious TeamParticipation entries for zzzzz', err),
- (out, err))
- self.failUnless(
- re.search('not members of themselves:.*zzzzz.*', err),
- (out, err))
+ self.assertEqual(0, len(out))
+ self.failUnless(
+ re.search('missing TeamParticipation entries for zzzzz', err))
+ self.failUnless(
+ re.search('spurious TeamParticipation entries for zzzzz', err))
+ self.failUnless(
+ re.search('not members of themselves:.*zzzzz.*', err))
def test_report_circular_team_references(self):
"""The script reports circular references between teams.
@@ -1145,9 +1148,34 @@
import transaction
transaction.commit()
out, err = self._runScript(expected_returncode=1)
- self.assertEqual(out, '', (out, err))
- self.failUnless(
- re.search('Circular references found', err), (out, err))
+ self.assertEqual(0, len(out))
+ self.failUnless(re.search('Circular references found', err))
+
+
+class TestCheckTeamParticipationScriptPerformance(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_queries(self):
+ """The script does not overly tax the database.
+
+ The whole check_teamparticipation() run executes a constant low number
+ of queries.
+ """
+ # Create a deeply nested team and member structure.
+ team = self.factory.makeTeam()
+ for num in xrange(10):
+ another_team = self.factory.makeTeam()
+ another_person = self.factory.makePerson()
+ with person_logged_in(team.teamowner):
+ team.addMember(another_team, team.teamowner)
+ team.addMember(another_person, team.teamowner)
+ team = another_team
+ transaction.commit()
+ with StormStatementRecorder() as recorder:
+ logger = BufferLogger()
+ check_teamparticipation(logger)
+ self.assertThat(recorder, HasQueryCount(Equals(6)))
def test_suite():
