Launchpad itself

Merge lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199 into lp:launchpad

  1. introduce-IParticipationWithAnnotations-bug-623199
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Rejected
Rejected by: Michael Hudson-Doyle
Proposed branch: lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199
Merge into: lp:launchpad
Diff against target: 226 lines (+55/-17)
6 files modified
lib/canonical/launchpad/doc/launchbag.txt (+0/-3)
lib/canonical/launchpad/doc/webapp-authorization.txt (+2/-2)
lib/canonical/launchpad/webapp/interaction.py (+10/-7)
lib/canonical/launchpad/webapp/interfaces.py (+25/-0)
lib/canonical/launchpad/webapp/servers.py (+11/-5)
lib/canonical/launchpad/webapp/tests/test_servers.py (+7/-0)
To merge this branch: bzr merge lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199
Reviewer Review Type Date Requested Status
Ian Booth (community) code Approve
Review via email: mp+77611@code.launchpad.net

Description of the change

Hi there,

This is the very start of fixing 623199: ensuring that all (or at least, the overwhelming majority) of the IParticipation objects we use have an annotations dictionary so that we can start to replace thread local variables with values stuffed into this dictionary.

Grovelling around at this level is always bit risky I guess but this change doesn't actually really do anything yet, and I've run the branch through EC2 and everything passed. I could understand not wanting to land this until something uses it -- I was thinking of trying to move feature flags over to annotations as a test of the idea.

Cheers,
mwh

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

I think this is the start of an excellent improvement. ParticipationWithAnnotations is trivial and there are no tests for it which is understandable. I think though we should write a test which checks that BasicLaunchpadRequest implements the new interface.

I agree that this branch could be chosen to be not landed until a concrete change is made which requires it. It may be that there turns out to be code needing to be added here that only becomes apparent when we go to use this new work.

Minor typo:

replace out with our
126 + # because we want all out participations to have annotations, but not to

review: Approve (code)
lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199 updated
14065. By Michael Hudson-Doyle

fix comment

14066. By Michael Hudson-Doyle

test that LaunchpadBrowserRequest and LaunchpadTestRequest implement
IParticipationWithAnnotations

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I tried a different approach in https://code.launchpad.net/~mwhudson/launchpad/permit_timeout_from_features-on-participation-bug-861510/+merge/78355 which seems to have more legs (although that isn't ready for merging yet either ...)

Unmerged revisions

14066. By Michael Hudson-Doyle

test that LaunchpadBrowserRequest and LaunchpadTestRequest implement
IParticipationWithAnnotations

14065. By Michael Hudson-Doyle

fix comment

14064. By Michael Hudson-Doyle

* create an IParticipationWithAnnotations class
* have LaunchpadTestRequest and BasicLaunchpadRequest (and so all request
  classes used in real servers) declare that they implement this interface,
  even though they already implement it via superclasses
* modify default Participation object to be called ParticipationWithAnnotations
  and implement IParticipationWithAnnotations.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/launchbag.txt'
2--- lib/canonical/launchpad/doc/launchbag.txt 2010-10-09 16:36:22 +0000
3+++ lib/canonical/launchpad/doc/launchbag.txt 2011-10-02 23:05:23 +0000
4@@ -15,9 +15,6 @@
5 ... id = 23
6
7 >>> principal = Principal()
8->>> class Participation(object):
9-... principal = principal
10-... interaction = None
11
12 >>> class Response(object):
13 ... def getCookie(self, name):
14
15=== modified file 'lib/canonical/launchpad/doc/webapp-authorization.txt'
16--- lib/canonical/launchpad/doc/webapp-authorization.txt 2011-06-28 15:04:29 +0000
17+++ lib/canonical/launchpad/doc/webapp-authorization.txt 2011-10-02 23:05:23 +0000
18@@ -42,7 +42,7 @@
19 access level will be able to read, but not change, anything.
20
21 >>> from canonical.launchpad.webapp.interaction import (
22- ... Participation, setupInteraction)
23+ ... ParticipationWithAnnotations, setupInteraction)
24 >>> from canonical.launchpad.webapp.interfaces import (
25 ... AccessLevel, IPlacelessAuthUtility)
26 >>> login('test@canonical.com')
27@@ -91,7 +91,7 @@
28 Users logged in through the web application have full access, which
29 means they can read/change any object they have access to.
30
31- >>> mock_participation = Participation()
32+ >>> mock_participation = ParticipationWithAnnotations()
33 >>> login('test@canonical.com', mock_participation)
34 >>> mock_participation.principal.access_level
35 <DBItem AccessLevel.WRITE_PRIVATE...
36
37=== modified file 'lib/canonical/launchpad/webapp/interaction.py'
38--- lib/canonical/launchpad/webapp/interaction.py 2010-08-20 20:31:18 +0000
39+++ lib/canonical/launchpad/webapp/interaction.py 2011-10-02 23:05:23 +0000
40@@ -38,7 +38,6 @@
41 from zope.component import getUtility
42 from zope.interface import implements
43 from zope.publisher.interfaces import IPublicationRequest
44-from zope.security.interfaces import IParticipation
45 from zope.security.management import (
46 endInteraction,
47 newInteraction,
48@@ -47,6 +46,7 @@
49
50 from canonical.launchpad.webapp.interfaces import (
51 IOpenLaunchBag,
52+ IParticipationWithAnnotations,
53 IPlacelessAuthUtility,
54 )
55
56@@ -57,7 +57,7 @@
57 'setupInteraction',
58 'setupInteractionByEmail',
59 'setupInteractionForPerson',
60- 'Participation',
61+ 'ParticipationWithAnnotations',
62 ]
63
64
65@@ -90,7 +90,7 @@
66 The login gets added to the launch bag.
67
68 You can optionally pass in a participation to be used. If no
69- participation is given, a Participation is used.
70+ participation is given, a ParticipationWithAnnotations is used.
71 """
72 # If principal is None, this method acts just like endInteraction.
73 if principal is None:
74@@ -102,7 +102,7 @@
75 principal = authutil.unauthenticatedPrincipal()
76
77 if participation is None:
78- participation = Participation()
79+ participation = ParticipationWithAnnotations()
80
81 # First end any running interaction, and start a new one.
82 endInteraction()
83@@ -149,7 +149,7 @@
84 principal = authutil.unauthenticatedPrincipal()
85
86 if participation is None:
87- participation = Participation()
88+ participation = ParticipationWithAnnotations()
89
90 setupInteraction(principal, login=email, participation=participation)
91
92@@ -165,9 +165,12 @@
93 return setupInteractionByEmail(naked_email.email, participation)
94
95
96-class Participation:
97+class ParticipationWithAnnotations:
98 """A very simple participation."""
99- implements(IParticipation)
100+ implements(IParticipationWithAnnotations)
101+
102+ def __init__(self):
103+ self.annotations = {}
104
105 interaction = None
106 principal = None
107
108=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
109--- lib/canonical/launchpad/webapp/interfaces.py 2011-08-11 19:42:23 +0000
110+++ lib/canonical/launchpad/webapp/interfaces.py 2011-10-02 23:05:23 +0000
111@@ -34,6 +34,7 @@
112 Text,
113 TextLine,
114 )
115+from zope.security.interfaces import IParticipation
116 from zope.traversing.interfaces import IContainmentRoot
117
118 from canonical.launchpad import _
119@@ -310,10 +311,34 @@
120 '''
121
122
123+class IParticipationWithAnnotations(IParticipation):
124+
125+ # These is copied from zope.publisher.interfaces.IApplicationRequest,
126+ # because we want all our participations to have annotations, but not to
127+ # implement all of the other baggage that comes along with
128+ # IApplicationRequest.
129+
130+ annotations = Attribute(
131+ """Stores arbitrary application data under package-unique keys.
132+
133+ By "package-unique keys", we mean keys that are are unique by
134+ virtue of including the dotted name of a package as a prefex. A
135+ package name is used to limit the authority for picking names for
136+ a package to the people using that package.
137+
138+ For example, when implementing annotations for hypothetical
139+ request-persistent adapters in a hypothetical zope.persistentadapter
140+ package, the key would be (or at least begin with) the following::
141+
142+ "zope.persistentadapter"
143+ """)
144+
145+
146 #
147 # Request
148 #
149
150+
151 class IBasicLaunchpadRequest(Interface):
152 stepstogo = Attribute(
153 'The StepsToGo object for this request, allowing you to inspect and'
154
155=== modified file 'lib/canonical/launchpad/webapp/servers.py'
156--- lib/canonical/launchpad/webapp/servers.py 2011-08-31 20:49:46 +0000
157+++ lib/canonical/launchpad/webapp/servers.py 2011-10-02 23:05:23 +0000
158@@ -49,7 +49,6 @@
159 XMLRPCResponse,
160 )
161 from zope.security.interfaces import (
162- IParticipation,
163 Unauthorized,
164 )
165 from zope.security.proxy import (
166@@ -88,6 +87,7 @@
167 ILaunchpadProtocolError,
168 INotificationRequest,
169 INotificationResponse,
170+ IParticipationWithAnnotations,
171 IPlacelessAuthUtility,
172 IPlacelessLoginSource,
173 OAuthPermission,
174@@ -556,7 +556,7 @@
175 class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
176 """Mixin request class to provide stepstogo."""
177
178- implements(IBasicLaunchpadRequest)
179+ implements(IBasicLaunchpadRequest, IParticipationWithAnnotations)
180
181 def __init__(self, body_instream, environ, response=None):
182 self.traversed_objects = []
183@@ -864,9 +864,15 @@
184 False
185
186 """
187- implements(INotificationRequest, IBasicLaunchpadRequest, IParticipation,
188- canonical.launchpad.layers.LaunchpadLayer)
189- # These two attributes satisfy IParticipation.
190+ implements(
191+ INotificationRequest, IBasicLaunchpadRequest,
192+ IParticipationWithAnnotations,
193+ canonical.launchpad.layers.LaunchpadLayer)
194+
195+ # These two attributes satisfy IParticipation, which
196+ # IParticipationWithAnnotations inherits from. The annotations attribute
197+ # is provided by TestRequest (via BrowerRequest -> HTTPRequest ->
198+ # BaseRequest).
199 principal = None
200 interaction = None
201
202
203=== modified file 'lib/canonical/launchpad/webapp/tests/test_servers.py'
204--- lib/canonical/launchpad/webapp/tests/test_servers.py 2011-08-16 19:10:40 +0000
205+++ lib/canonical/launchpad/webapp/tests/test_servers.py 2011-10-02 23:05:23 +0000
206@@ -32,6 +32,9 @@
207 Interface,
208 )
209
210+from canonical.launchpad.webapp.interfaces import (
211+ IParticipationWithAnnotations,
212+ )
213 from canonical.launchpad.webapp.servers import (
214 ApplicationServerSettingRequestFactory,
215 LaunchpadBrowserRequest,
216@@ -427,6 +430,10 @@
217 "/sabbra/cadabra?tuesday=gone",
218 request.getURL(include_query=True, path_only=True))
219
220+ def test_provides_IParticipationWithAnnotations(self):
221+ request = self.request_factory(StringIO.StringIO(''), {})
222+ self.assertProvides(request, IParticipationWithAnnotations)
223+
224
225 class TestLaunchpadBrowserRequestMixinWithLaunchpadBrowserRequest(
226 TestLaunchpadBrowserRequestMixin, TestCase):