launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/person-invalidate-team-cache into lp:launchpad.

Commit message:
When a Person object is invalidated, clear its TeamParticipation cache.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/person-invalidate-team-cache/+merge/278704

When a Person object is invalidated, clear its TeamParticipation cache.

I noticed this while hunting down a mysterious test failure, which turned out to be because this bug had previously been cancelling out extra queries elsewhere.  I think it's probably been a bug for as long as Person._inTeam_cache has existed: certainly when aborting a transaction we should not retain the cache of team membership information which might depend on changes made in that transaction.

Test fallout is non-zero, but not too bad.  A few places need to commit a transaction that involved team membership changes before testing something that is expected to fail and abort the transaction, and a few query count tests need to be made more precise since they will now see extra TeamMembership queries on second and subsequent runs.  TestP3APackagesQueryCount.test_ppa_index_queries_count needs to make sure that all the BPPHs it creates are in the same distroseries, because it makes one extra query per unique distroseries, previously cancelled out by the missing TeamMembership queries on the second run; I think that kind of scaling is acceptable.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/person-invalidate-team-cache into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/nameblacklist-views.txt'
--- lib/lp/registry/browser/tests/nameblacklist-views.txt	2012-12-10 13:43:47 +0000
+++ lib/lp/registry/browser/tests/nameblacklist-views.txt	2015-11-26 13:41:51 +0000
@@ -1,6 +1,7 @@
 NameBlacklist pages
 ===================
 
+    >>> import transaction
     >>> from zope.component import getUtility
     >>> from lp.testing.sampledata import ADMIN_EMAIL
     >>> from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -12,6 +13,7 @@
     >>> registry_expert = factory.makePerson()
     >>> login(ADMIN_EMAIL)
     >>> ignore = registry_experts.addMember(registry_expert, registry_expert)
+    >>> transaction.commit()
 
 
 View all
@@ -61,7 +63,6 @@
     ...     print notification.message
     Regular expression "foo" has been added to the name blacklist.
 
-    >>> import transaction
     >>> transaction.commit()
     >>> foo_exp = name_blacklist_set.getByRegExp(u'foo')
     >>> print foo_exp.regexp

=== modified file 'lib/lp/registry/browser/tests/product-views.txt'
--- lib/lp/registry/browser/tests/product-views.txt	2012-10-08 06:13:17 +0000
+++ lib/lp/registry/browser/tests/product-views.txt	2015-11-26 13:41:51 +0000
@@ -144,6 +144,7 @@
     ...     'commercial-member@xxxxxxxxxxxxx')
     >>> registry_experts = getUtility(IPersonSet).getByName('registry')
     >>> ignored = registry_experts.addMember(commercial_member, reviewer=mark)
+    >>> transaction.commit()
     >>> login('commercial-member@xxxxxxxxxxxxx')
     >>> view = create_initialized_view(firefox, name='+review-license')
     >>> print view.label

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2015-05-14 13:57:51 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2015-11-26 13:41:51 +0000
@@ -28,7 +28,6 @@
 from lp.registry.interfaces.accesspolicy import IAccessPolicyGrantFlatSource
 from lp.registry.model.pillar import PillarPerson
 from lp.services.config import config
-from lp.services.database.interfaces import IStore
 from lp.services.webapp.interfaces import StormRangeFactoryError
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
@@ -36,6 +35,7 @@
     logout,
     normalize_whitespace,
     person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -226,15 +226,12 @@
     def test_view_query_count(self):
         # Test that the view bulk loads artifacts.
         person = self.factory.makePerson()
-        for x in range(0, 15):
-            self.makeArtifactGrantee(person, True, True, True, False)
         pillarperson = PillarPerson(self.pillar, person)
-
-        # Invalidate the Storm cache and check the query count.
-        IStore(self.pillar).invalidate()
-        with StormStatementRecorder() as recorder:
-            create_initialized_view(pillarperson, '+index')
-        self.assertThat(recorder, HasQueryCount(LessThan(14)))
+        recorder1, recorder2 = record_two_runs(
+            lambda: create_initialized_view(pillarperson, '+index'),
+            lambda: self.makeArtifactGrantee(person, True, True, True, False),
+            5, login_method=lambda: login_person(self.owner))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
 
 class TestProductSharingDetailsView(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2015-10-01 17:32:41 +0000
+++ lib/lp/registry/model/person.py	2015-11-26 13:41:51 +0000
@@ -1411,6 +1411,10 @@
         """See `IPerson`."""
         self._inTeam_cache = {}
 
+    def __storm_invalidated__(self):
+        super(Person, self).__storm_invalidated__()
+        self.clearInTeamCache()
+
     @cachedproperty
     def participant_ids(self):
         """See `IPerson`."""

=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
--- lib/lp/soyuz/browser/tests/archive-views.txt	2015-05-12 23:14:16 +0000
+++ lib/lp/soyuz/browser/tests/archive-views.txt	2015-11-26 13:41:51 +0000
@@ -1011,6 +1011,7 @@
 
     >>> login('foo.bar@xxxxxxxxxxxxx')
     >>> ignored = a_team.addMember(cprov, mark)
+    >>> transaction.commit()
     >>> login('celso.providelo@xxxxxxxxxxxxx')
 
     >>> view = create_initialized_view(

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py	2015-10-13 16:58:20 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py	2015-11-26 13:41:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for TestP3APackages."""
@@ -430,10 +430,14 @@
             owner=self.team, private=True)
         self.private_ppa.newSubscription(
             self.person, registrant=self.team.teamowner)
+        self.distroseries = self.factory.makeDistroSeries(
+            distribution=self.private_ppa.distribution)
 
     def createPackage(self):
         with celebrity_logged_in('admin'):
             pkg = self.factory.makeBinaryPackagePublishingHistory(
+                distroarchseries=self.factory.makeDistroArchSeries(
+                    distroseries=self.distroseries),
                 status=PackagePublishingStatus.PUBLISHED,
                 archive=self.private_ppa)
         return pkg

=== modified file 'lib/lp/soyuz/browser/tests/test_publishing_webservice.py'
--- lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2015-09-29 01:38:34 +0000
+++ lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2015-11-26 13:41:51 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test webservice methods related to the publisher."""
@@ -60,7 +60,7 @@
             with person_logged_in(person):
                 self.factory.makeBinaryPackageFile(
                     binarypackagerelease=bpph.binarypackagerelease)
-        self.assertEqual(query_counts[0] - 1, query_counts[-1])
+        self.assertEqual(query_counts[0], query_counts[-1])
 
         self.assertEqual(200, response.status)
         urls = response.jsonBody()

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2015-09-08 09:09:28 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2015-11-26 13:41:51 +0000
@@ -52,6 +52,7 @@
     api_url,
     launchpadlib_for,
     person_logged_in,
+    record_two_runs,
     StormStatementRecorder,
     TestCaseWithFactory,
     )
@@ -1361,13 +1362,10 @@
 
     def test_getPackageUploads_query_count(self):
         person = self.makeQueueAdmin([self.universe])
-        uploads = []
-        for i in range(5):
-            upload, _ = self.makeBinaryPackageUpload(
-                person, component=self.universe)
-            uploads.append(upload)
         ws_distroseries = self.load(self.distroseries, person)
-        IStore(uploads[0].__class__).invalidate()
-        with StormStatementRecorder() as recorder:
-            ws_distroseries.getPackageUploads()
-        self.assertThat(recorder, HasQueryCount(Equals(29)))
+        recorder1, recorder2 = record_two_runs(
+            ws_distroseries.getPackageUploads,
+            lambda: self.makeBinaryPackageUpload(
+                person, component=self.universe),
+            5)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))