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 @@
... '[email protected]')
>>> registry_experts = getUtility(IPersonSet).getByName('registry')
>>> ignored = registry_experts.addMember(commercial_member, reviewer=mark)
+ >>> transaction.commit()
>>> login('[email protected]')
>>> 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('[email protected]')
>>> ignored = a_team.addMember(cprov, mark)
+ >>> transaction.commit()
>>> login('[email protected]')
>>> 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))
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
