Merge ~cjwatson/launchpad:remove-storm-workarounds into launchpad:master

Proposed by Colin Watson on 2019-10-04
Status: Merged
Approved by: Colin Watson on 2019-10-11
Approved revision: 4042fcdc849e26cd1048a3c00188a5004886fe0d
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:remove-storm-workarounds
Merge into: launchpad:master
Diff against target: 92 lines (+13/-17)
2 files modified
lib/lp/registry/model/product.py (+9/-8)
lib/lp/registry/tests/test_product.py (+4/-9)
Reviewer Review Type Date Requested Status
William Grant code 2019-10-04 Approve on 2019-10-11
Review via email: mp+373695@code.launchpad.net

Commit message

Remove workarounds for Storm reference cycles

https://code.launchpad.net/~cjwatson/storm/more-weakrefs/+merge/365639
fixed a long-standing Storm bug that sometimes resulted in GC cycles.
We've had this fix deployed in Launchpad for a while with no problems,
so it now makes sense to remove workarounds for it in favour of more
natural code.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/registry/model/product.py b/lib/lp/registry/model/product.py
2index 97c6138..8b47075 100644
3--- a/lib/lp/registry/model/product.py
4+++ b/lib/lp/registry/model/product.py
5@@ -42,6 +42,8 @@ from storm.expr import (
6 SQL,
7 )
8 from storm.locals import (
9+ Int,
10+ List,
11 Store,
12 Unicode,
13 )
14@@ -363,6 +365,11 @@ class Product(SQLBase, BugTargetBase, MakesAnnouncements,
15 name='remote_product', allow_none=True, default=None)
16 vcs = EnumCol(enum=VCSType, notNull=False)
17
18+ # Cache of AccessPolicy.ids that convey launchpad.LimitedView.
19+ # Unlike artifacts' cached access_policies, an AccessArtifactGrant
20+ # to an artifact in the policy is sufficient for access.
21+ access_policies = List(type=Int())
22+
23 @property
24 def displayname(self):
25 return self.display_name
26@@ -759,18 +766,12 @@ class Product(SQLBase, BugTargetBase, MakesAnnouncements,
27 # Security was somehow left around when a project was
28 # transitioned to Proprietary.
29 if self.information_type in PROPRIETARY_INFORMATION_TYPES:
30- policy_ids = [
31+ self.access_policies = [
32 policy.id for policy in
33 getUtility(IAccessPolicySource).find(
34 [(self, type) for type in PROPRIETARY_INFORMATION_TYPES])]
35 else:
36- policy_ids = None
37- # XXX wgrant 2015-06-30: We must set this manually, as Storm's
38- # MutableValueVariable causes a PostgresConnection to be
39- # uncollectable in some circumstances involving a Store.reset.
40- Store.of(self).execute(
41- "UPDATE Product SET access_policies = ? WHERE id = ?",
42- (policy_ids, self.id))
43+ self.access_policies = None
44
45 def _pruneUnusedPolicies(self):
46 allowed_bug_types = set(
47diff --git a/lib/lp/registry/tests/test_product.py b/lib/lp/registry/tests/test_product.py
48index 7387ed2..6dc0e96 100644
49--- a/lib/lp/registry/tests/test_product.py
50+++ b/lib/lp/registry/tests/test_product.py
51@@ -495,29 +495,24 @@ class TestProduct(TestCaseWithFactory):
52 # principal LimitedView on the Product.
53 aps = getUtility(IAccessPolicySource)
54
55- def get_aps(product):
56- return Store.of(product).execute(
57- "SELECT access_policies FROM product WHERE id = ?",
58- (product.id,)).get_one()[0]
59-
60 # Public projects don't need a cache.
61 product = self.factory.makeProduct()
62 naked_product = removeSecurityProxy(product)
63 self.assertContentEqual(
64 [InformationType.USERDATA, InformationType.PRIVATESECURITY],
65 [p.type for p in aps.findByPillar([product])])
66- self.assertIs(None, get_aps(product))
67+ self.assertIs(None, naked_product.access_policies)
68
69 # A private project normally just allows the Proprietary policy,
70 # even if there is still another policy like Private Security.
71 naked_product.information_type = InformationType.PROPRIETARY
72 [prop_policy] = aps.find([(product, InformationType.PROPRIETARY)])
73- self.assertEqual([prop_policy.id], get_aps(naked_product))
74+ self.assertEqual([prop_policy.id], naked_product.access_policies)
75
76 # If we switch it back to public, the cache is no longer
77 # required.
78 naked_product.information_type = InformationType.PUBLIC
79- self.assertIs(None, get_aps(naked_product))
80+ self.assertIs(None, naked_product.access_policies)
81
82 # Proprietary projects can have both Proprietary and Embargoed
83 # artifacts, and someone who can see either needs LimitedView on
84@@ -528,7 +523,7 @@ class TestProduct(TestCaseWithFactory):
85 BugSharingPolicy.EMBARGOED_OR_PROPRIETARY)
86 [emb_policy] = aps.find([(product, InformationType.EMBARGOED)])
87 self.assertContentEqual(
88- [prop_policy.id, emb_policy.id], get_aps(naked_product))
89+ [prop_policy.id, emb_policy.id], naked_product.access_policies)
90
91 def test_checkInformationType_bug_supervisor(self):
92 # Bug supervisors of proprietary products must not have inclusive

Subscribers

People subscribed via source and target branches