launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/participation-keyerror-bug-810114-2 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-2/+merge/81151

Add an additional check to check-teamparticipation to see if there are
any participants of people (i.e. not a team). I think this is fairly
unlikely to ever happen, but the data is there so we might as well
check it.

It also adds some logging around the queries in
check_teamparticipation_consistency. These pull in a lot of data, so
now progress is reported every 10000 rows if -v is passed to the
script.

-- 
https://code.launchpad.net/~allenap/launchpad/participation-keyerror-bug-810114-2/+merge/81151
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/participation-keyerror-bug-810114-2 into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/teamparticipation.py'
--- lib/lp/registry/scripts/teamparticipation.py	2011-11-02 15:38:23 +0000
+++ lib/lp/registry/scripts/teamparticipation.py	2011-11-03 14:18:40 +0000
@@ -12,9 +12,12 @@
     defaultdict,
     namedtuple,
     )
+from functools import partial
 from itertools import (
     chain,
+    count,
     imap,
+    izip,
     )
 
 import transaction
@@ -81,33 +84,46 @@
     "ConsistencyError", ("type", "team", "people"))
 
 
+def query(store, log, interval, query):
+    """Execute the given query, reporting as results are fetched.
+
+    The query is logged, then every `interval` rows a message is logged with
+    the total number of rows fetched thus far.
+    """
+    log.debug(query)
+    for rows, result in izip(count(1), store.execute(query)):
+        if rows % interval == 0:
+            log.debug("%d rows", rows)
+        yield result
+
+
 def check_teamparticipation_consistency(log):
     """Check for missing or spurious participations.
 
     For example, participations for people who are not members, or missing
     participations for people who are members.
     """
-    store = get_store()
+    slurp = partial(query, get_store(), log, 10000)
 
     # Slurp everything in.
     people = dict(
-        store.execute(
+        slurp(
             "SELECT id, name FROM Person"
             " WHERE teamowner IS NULL"
             "   AND merged IS NULL"))
     teams = dict(
-        store.execute(
+        slurp(
             "SELECT id, name FROM Person"
             " WHERE teamowner IS NOT NULL"
             "   AND merged IS NULL"))
     team_memberships = defaultdict(set)
-    results = store.execute(
+    results = slurp(
         "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(
+    results = slurp(
         "SELECT team, person FROM TeamParticipation")
     for (team, person) in results:
         team_participations[team].add(person)
@@ -115,7 +131,6 @@
     # Don't hold any locks.
     transaction.commit()
 
-    # Check team memberships.
     def get_participants(team):
         """Recurse through membership records to get participants."""
         member_people = team_memberships[team].intersection(people)
@@ -124,25 +139,36 @@
         return member_people.union(
             chain.from_iterable(imap(get_participants, member_teams)))
 
+    def check_participants(expected, observed):
+        spurious = observed - expected
+        missing = expected - observed
+        if len(spurious) > 0:
+            yield ConsistencyError("spurious", team, sorted(spurious))
+        if len(missing) > 0:
+            yield ConsistencyError("missing", team, sorted(missing))
+
     errors = []
+
+    for person in people:
+        participants_expected = set((person,))
+        participants_observed = team_participations[person]
+        errors.extend(
+            check_participants(participants_expected, participants_observed))
+
     for team in teams:
+        participants_expected = get_participants(team)
         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.
+        errors.extend(
+            check_participants(participants_expected, participants_observed))
 
     def get_repr(id):
-        return "%s (%d)" % (people[id] if id in people else teams[id], id)
+        if id in people:
+            name = people[id]
+        elif id in teams:
+            name = teams[id]
+        else:
+            name = "<unknown>"
+        return "%s (%d)" % (name, id)
 
     for error in errors:
         people_repr = ", ".join(imap(get_repr, error.people))

=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2011-11-02 15:34:11 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2011-11-03 14:18:40 +0000
@@ -52,7 +52,11 @@
     TeamMembership,
     TeamParticipation,
     )
-from lp.registry.scripts.teamparticipation import check_teamparticipation
+from lp.registry.scripts.teamparticipation import (
+    check_teamparticipation,
+    check_teamparticipation_consistency,
+    ConsistencyError,
+    )
 from lp.services.log.logger import BufferLogger
 from lp.testing import (
     login,
@@ -1105,7 +1109,6 @@
                         LIMIT 1),
                     %s);
             """ % sqlvalues(TeamMembershipStatus.APPROVED))
-        import transaction
         transaction.commit()
 
         out, err = self._runScript()
@@ -1145,12 +1148,38 @@
                 TeamParticipation (person, team)
                 VALUES (9997, 9998);
             """ % sqlvalues(approved=TeamMembershipStatus.APPROVED))
-        import transaction
         transaction.commit()
         out, err = self._runScript(expected_returncode=1)
         self.assertEqual(0, len(out))
         self.failUnless(re.search('Circular references found', err))
 
+    def test_report_spurious_participants_of_people(self):
+        """The script reports spurious participants of people.
+
+        Teams can have multiple participants, but only the person should be a
+        paricipant of him/herself.
+        """
+        # Create two new people and make both participate in the first.
+        cursor().execute("""
+            INSERT INTO
+                Person (id, name, displayname, creation_rationale)
+                VALUES (6969, 'bobby', 'Dazzler', 1);
+            INSERT INTO
+                Person (id, name, displayname, creation_rationale)
+                VALUES (6970, 'nobby', 'Jazzler', 1);
+            INSERT INTO
+                TeamParticipation (person, team)
+                VALUES (6970, 6969);
+            """ % sqlvalues(approved=TeamMembershipStatus.APPROVED))
+        transaction.commit()
+        logger = BufferLogger()
+        errors = check_teamparticipation_consistency(logger)
+        if logger.getLogBuffer() != "":
+            self.addDetail("log", text_content(logger.getLogBuffer()))
+        self.assertEqual(
+            [ConsistencyError("spurious", 6969, [6970])],
+            errors)
+
 
 class TestCheckTeamParticipationScriptPerformance(TestCaseWithFactory):