launchpad-reviewers team mailing list archive

[Michael Hudson-Doyle <michael.hudson+lp@xxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #623199 in Launchpad itself: "scripts do not establish valid zope participations"
  https://bugs.launchpad.net/launchpad/+bug/623199

For more details, see:
https://code.launchpad.net/~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199/+merge/77611

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
-- 
https://code.launchpad.net/~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199/+merge/77611
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/introduce-IParticipationWithAnnotations-bug-623199 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/doc/launchbag.txt'
--- lib/canonical/launchpad/doc/launchbag.txt	2010-10-09 16:36:22 +0000
+++ lib/canonical/launchpad/doc/launchbag.txt	2011-09-29 21:06:28 +0000
@@ -15,9 +15,6 @@
 ...     id = 23
 
 >>> principal = Principal()
->>> class Participation(object):
-...     principal = principal
-...     interaction = None
 
 >>> class Response(object):
 ...     def getCookie(self, name):

=== modified file 'lib/canonical/launchpad/doc/webapp-authorization.txt'
--- lib/canonical/launchpad/doc/webapp-authorization.txt	2011-06-28 15:04:29 +0000
+++ lib/canonical/launchpad/doc/webapp-authorization.txt	2011-09-29 21:06:28 +0000
@@ -42,7 +42,7 @@
 access level will be able to read, but not change, anything.
 
     >>> from canonical.launchpad.webapp.interaction import (
-    ...     Participation, setupInteraction)
+    ...     ParticipationWithAnnotations, setupInteraction)
     >>> from canonical.launchpad.webapp.interfaces import (
     ...     AccessLevel, IPlacelessAuthUtility)
     >>> login('test@xxxxxxxxxxxxx')
@@ -91,7 +91,7 @@
 Users logged in through the web application have full access, which
 means they can read/change any object they have access to.
 
-    >>> mock_participation = Participation()
+    >>> mock_participation = ParticipationWithAnnotations()
     >>> login('test@xxxxxxxxxxxxx', mock_participation)
     >>> mock_participation.principal.access_level
     <DBItem AccessLevel.WRITE_PRIVATE...

=== modified file 'lib/canonical/launchpad/webapp/interaction.py'
--- lib/canonical/launchpad/webapp/interaction.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/interaction.py	2011-09-29 21:06:28 +0000
@@ -38,7 +38,6 @@
 from zope.component import getUtility
 from zope.interface import implements
 from zope.publisher.interfaces import IPublicationRequest
-from zope.security.interfaces import IParticipation
 from zope.security.management import (
     endInteraction,
     newInteraction,
@@ -47,6 +46,7 @@
 
 from canonical.launchpad.webapp.interfaces import (
     IOpenLaunchBag,
+    IParticipationWithAnnotations,
     IPlacelessAuthUtility,
     )
 
@@ -57,7 +57,7 @@
     'setupInteraction',
     'setupInteractionByEmail',
     'setupInteractionForPerson',
-    'Participation',
+    'ParticipationWithAnnotations',
     ]
 
 
@@ -90,7 +90,7 @@
     The login gets added to the launch bag.
 
     You can optionally pass in a participation to be used.  If no
-    participation is given, a Participation is used.
+    participation is given, a ParticipationWithAnnotations is used.
     """
     # If principal is None, this method acts just like endInteraction.
     if principal is None:
@@ -102,7 +102,7 @@
         principal = authutil.unauthenticatedPrincipal()
 
     if participation is None:
-        participation = Participation()
+        participation = ParticipationWithAnnotations()
 
     # First end any running interaction, and start a new one.
     endInteraction()
@@ -149,7 +149,7 @@
         principal = authutil.unauthenticatedPrincipal()
 
     if participation is None:
-        participation = Participation()
+        participation = ParticipationWithAnnotations()
 
     setupInteraction(principal, login=email, participation=participation)
 
@@ -165,9 +165,12 @@
         return setupInteractionByEmail(naked_email.email, participation)
 
 
-class Participation:
+class ParticipationWithAnnotations:
     """A very simple participation."""
-    implements(IParticipation)
+    implements(IParticipationWithAnnotations)
+
+    def __init__(self):
+        self.annotations = {}
 
     interaction = None
     principal = None

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2011-08-11 19:42:23 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2011-09-29 21:06:28 +0000
@@ -34,6 +34,7 @@
     Text,
     TextLine,
     )
+from zope.security.interfaces import IParticipation
 from zope.traversing.interfaces import IContainmentRoot
 
 from canonical.launchpad import _
@@ -310,10 +311,34 @@
         '''
 
 
+class IParticipationWithAnnotations(IParticipation):
+
+    # These is copied from zope.publisher.interfaces.IApplicationRequest,
+    # because we want all out participations to have annotations, but not to
+    # implement all of the other baggage that comes along with
+    # IApplicationRequest.
+
+    annotations = Attribute(
+        """Stores arbitrary application data under package-unique keys.
+
+        By "package-unique keys", we mean keys that are are unique by
+        virtue of including the dotted name of a package as a prefex.  A
+        package name is used to limit the authority for picking names for
+        a package to the people using that package.
+
+        For example, when implementing annotations for hypothetical
+        request-persistent adapters in a hypothetical zope.persistentadapter
+        package, the key would be (or at least begin with) the following::
+
+          "zope.persistentadapter"
+        """)
+
+
 #
 # Request
 #
 
+
 class IBasicLaunchpadRequest(Interface):
     stepstogo = Attribute(
         'The StepsToGo object for this request, allowing you to inspect and'

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2011-08-31 20:49:46 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2011-09-29 21:06:28 +0000
@@ -49,7 +49,6 @@
     XMLRPCResponse,
     )
 from zope.security.interfaces import (
-    IParticipation,
     Unauthorized,
     )
 from zope.security.proxy import (
@@ -88,6 +87,7 @@
     ILaunchpadProtocolError,
     INotificationRequest,
     INotificationResponse,
+    IParticipationWithAnnotations,
     IPlacelessAuthUtility,
     IPlacelessLoginSource,
     OAuthPermission,
@@ -556,7 +556,7 @@
 class BasicLaunchpadRequest(LaunchpadBrowserRequestMixin):
     """Mixin request class to provide stepstogo."""
 
-    implements(IBasicLaunchpadRequest)
+    implements(IBasicLaunchpadRequest, IParticipationWithAnnotations)
 
     def __init__(self, body_instream, environ, response=None):
         self.traversed_objects = []
@@ -864,9 +864,15 @@
     False
 
     """
-    implements(INotificationRequest, IBasicLaunchpadRequest, IParticipation,
-               canonical.launchpad.layers.LaunchpadLayer)
-    # These two attributes satisfy IParticipation.
+    implements(
+        INotificationRequest, IBasicLaunchpadRequest,
+        IParticipationWithAnnotations,
+        canonical.launchpad.layers.LaunchpadLayer)
+
+    # These two attributes satisfy IParticipation, which
+    # IParticipationWithAnnotations inherits from.  The annotations attribute
+    # is provided by TestRequest (via BrowerRequest -> HTTPRequest ->
+    # BaseRequest).
     principal = None
     interaction = None