launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-768443 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #768443 in Launchpad itself: "ProductSeries:+index timeout with many releases"
  https://bugs.launchpad.net/launchpad/+bug/768443

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-768443/+merge/59782

This branch fixes bug 768443: ProductSeries:+index timeout with many releases

The reason for the timeout are many calls of Person.getAdministratedTeams() in lp.bugs.browser.structuralsubscription.expose_user_administered_teams_to_js() and lp.bugs.browser.structuralsubscription.expose_user_subscriptions_to_js()

These functions are called in lp.registry.browser.productseries.ProductSeriesView.initiakize() and in lp.registry.browser.milestone.MilestoneView.initialize()

One OOPS had more than 90 calls of getAdministratedTeams(); with an "SQL time" of 80..200msec per call this is an obvious cause for the timeouts.

At first I intended to cache the query result inside Person.getAdministratedTeams() but got scared when I noticed that this method is also used in security.py -- if the affected checkAuthenticated() method is called in a method where team memberships are changed, we might end with an invalid permission check, so I added the somewhat redundant cached property Person.adminstrated_teams which is now used in the affected functions expose_.*_to_js()

tests:

./bin/test bugs -vvt test_expose
./bin/test registry -vvt test_person.TestPersonTeams

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-768443/+merge/59782
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-768443 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py	2011-04-26 17:28:37 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py	2011-05-03 14:11:18 +0000
@@ -427,7 +427,7 @@
     api_request = IWebServiceClientRequest(request)
     is_distro = IDistribution.providedBy(context)
     if user is not None:
-        administrated_teams = list(user.getAdministratedTeams())
+        administrated_teams = user.administrated_teams
         if administrated_teams:
             # Get this only if we need to.
             membership = list(user.teams_participated_in)
@@ -460,7 +460,7 @@
     if user is None:
         administered_teams = []
     else:
-        administered_teams = user.getAdministratedTeams()
+        administered_teams = user.administrated_teams
 
     if target is not None:
         try:

=== modified file 'lib/lp/bugs/browser/tests/test_expose.py'
--- lib/lp/bugs/browser/tests/test_expose.py	2011-04-22 05:18:40 +0000
+++ lib/lp/bugs/browser/tests/test_expose.py	2011-05-03 14:11:18 +0000
@@ -32,6 +32,7 @@
 
 from lp.testing import (
     person_logged_in,
+    StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     )
@@ -56,8 +57,10 @@
 class FakeUser:
     """A faux user that has a hard-coded set of administered teams."""
 
+    administrated_teams = [FakeTeam('Team One'), FakeTeam('Team Two')]
+
     def getAdministratedTeams(self):
-        return [FakeTeam('Team One'), FakeTeam('Team Two')]
+        return self.administrated_teams
 
 
 def fake_absoluteURL(ob, request):
@@ -144,6 +147,34 @@
         self.assertThat(team_info[0]['link'],
             Equals('http://example.com/BugSupervisorSubTeam'))
 
+    def test_expose_user_administered_teams_to_js__uses_cached_teams(self):
+        # The function expose_user_administered_teams_to_js uses a
+        # cached list of administrated teams.
+        context = self.factory.makeProduct(owner=self.user)
+        self._setup_teams(self.user)
+
+        # The first call requires one query to retrieve the administrated
+        # teams.
+        with StormStatementRecorder() as recorder:
+            expose_user_administered_teams_to_js(
+                self.request, self.user, context,
+                absoluteURL=fake_absoluteURL)
+        statements_for_admininstrated_teams = [
+            statement for statement in recorder.statements
+            if statement.startswith("'SELECT *")]
+        self.assertEqual(1, len(statements_for_admininstrated_teams))
+
+        # Calling the function a second time does not require an
+        # SQL call to retrieve the administrated teams.
+        with StormStatementRecorder() as recorder:
+            expose_user_administered_teams_to_js(
+                self.request, self.user, context,
+                absoluteURL=fake_absoluteURL)
+        statements_for_admininstrated_teams = [
+            statement for statement in recorder.statements
+            if statement.startswith("'SELECT *")]
+        self.assertEqual(0, len(statements_for_admininstrated_teams))
+
     def test_teams_owned_but_not_joined_are_not_included(self):
         context = self.factory.makeProduct(owner=self.user)
         team = self.factory.makeTeam(
@@ -333,3 +364,31 @@
         self.assertEqual(
             filter_info['subscriber_url'],
             canonical_url(user, rootsite='mainsite'))
+
+    def test_expose_user_subscriptions_to_js__uses_cached_teams(self):
+        # The function expose_user_subscriptions_to_js() uses a
+        # cached list of administrated teams.
+        user = self.factory.makePerson()
+        target = self.factory.makeProduct()
+        request = LaunchpadTestRequest()
+        with person_logged_in(user):
+            sub = target.addBugSubscription(user, user)
+
+        # The first call requires one query to retrieve the administrated
+        # teams.
+        with StormStatementRecorder() as recorder:
+            expose_user_subscriptions_to_js(user, [sub], request)
+        statements_for_admininstrated_teams = [
+            statement for statement in recorder.statements
+            if statement.startswith("'SELECT *")]
+        self.assertEqual(1, len(statements_for_admininstrated_teams))
+
+        # Calling the function a second time does not require an
+        # SQL call to retrieve the administrated teams.
+        with person_logged_in(user):
+            with StormStatementRecorder() as recorder:
+                expose_user_subscriptions_to_js(user, [sub], request)
+        statements_for_admininstrated_teams = [
+            statement for statement in recorder.statements
+            if statement.startswith("'SELECT *")]
+        self.assertEqual(0, len(statements_for_admininstrated_teams))

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-04-26 13:57:15 +0000
+++ lib/lp/registry/interfaces/person.py	2011-05-03 14:11:18 +0000
@@ -979,6 +979,9 @@
         title=_("Is this person due to be merged with another?"),
         required=False, default=False))
 
+    administrated_teams = Attribute(
+        u"the teams that this person/team is an administrator of.")
+
     @invariant
     def personCannotHaveIcon(person):
         """Only Persons can have icons."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-04-27 14:51:54 +0000
+++ lib/lp/registry/model/person.py	2011-05-03 14:11:18 +0000
@@ -1562,6 +1562,10 @@
         tm.setExpirationDate(expires, reviewer)
         tm.setStatus(status, reviewer, comment=comment)
 
+    @cachedproperty
+    def administrated_teams(self):
+        return list(self.getAdministratedTeams())
+
     def getAdministratedTeams(self):
         """See `IPerson`."""
         owner_of_teams = Person.select('''

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-04-27 15:51:17 +0000
+++ lib/lp/registry/tests/test_person.py	2011-05-03 14:11:18 +0000
@@ -36,7 +36,6 @@
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
-    LaunchpadFunctionalLayer,
     reconnect_stores,
     )
 from lp.answers.model.answercontact import AnswerContact
@@ -269,6 +268,18 @@
         retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
         self.assertEqual(expected_members, retrieved_members)
 
+    def test_administrated_teams(self):
+        # The property Person.administrated_teams is a cached copy of
+        # the result of Person.getAdministratedTeams().
+        expected = [self.b_team, self.c_team]
+        self.assertEqual(expected, list(self.user.getAdministratedTeams()))
+        with StormStatementRecorder() as recorder:
+            self.assertEqual(expected, self.user.administrated_teams)
+            self.user.administrated_teams
+        # The second access of administrated_teams did not require an
+        # SQL query, hence the total number of SQL queries is 1.
+        self.assertEqual(1, len(recorder.queries))
+
 
 class TestPerson(TestCaseWithFactory):