launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-607154 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #607154 page performance report should break out API methods
  https://bugs.launchpad.net/bugs/607154


Calls to the web service generate page IDs that aren't as helpful as
they could be because the named operation is not included and is often a
critical differentiator (see bug 607154).

In this branch the named operation (if any) is appended to the page ID
(after a colon).

This was done by refactoring the page ID generation code out of
lib/canonical/launchpad/webapp/publication.py:LaunchpadBrowserPublication.callObject
and into its own method (LaunchpadBrowserPublication.constructPageID)
and then overriding constructPageID in WebServicePublication to add the
new behavior.

The behavior and approach were discussed with Gary and Leonard.

To run the tests for the new behavior run

    bin/test -t test_pageid

I also cleaned up quite a bit of lint.  Most of it was whitespace or
silencing false positives.  I could not force this lint to be silent
though (suggestions welcome):

./lib/canonical/launchpad/webapp/publication.py
     193: E231 missing whitespace after ','

-- 
https://code.launchpad.net/~benji/launchpad/bug-607154/+merge/30578
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-607154 into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2010-07-14 15:33:36 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2010-07-21 20:49:47 +0000
@@ -133,6 +133,7 @@
     # If this becomes untrue at some point, the code will need to be
     # revisited.
 
+    # pylint: disable-msg=E0301 what you talkin' about pylint?
     def beforeTraversal(self, request):
         notify(StartRequestEvent(request))
         request._traversalticks_start = tickcount.tickcount()
@@ -370,6 +371,28 @@
         if hostname not in allvhosts.hostnames:
             raise OffsiteFormPostError(referrer)
 
+    def constructPageID(self, view, context):
+        """Given a view, figure out what its page ID should be.
+
+        This provides a hook point for subclasses to override.
+        """
+        if context is None:
+            pageid = ''
+        else:
+            # ZCML registration will set the name under which the view
+            # is accessible in the instance __name__ attribute. We use
+            # that if it's available, otherwise fall back to the class
+            # name.
+            if getattr(view, '__name__', None) is not None:
+                view_name = view.__name__
+            else:
+                view_name = view.__class__.__name__
+            pageid = '%s:%s' % (context.__class__.__name__, view_name)
+        # The view name used in the pageid usually comes from ZCML and so
+        # it will be a unicode string although it shouldn't.  To avoid
+        # problems we encode it into ASCII.
+        return pageid.encode('US-ASCII')
+
     def callObject(self, request, ob):
         """See `zope.publisher.interfaces.IPublication`.
 
@@ -386,31 +409,14 @@
         request.setInWSGIEnvironment(
             'launchpad.userid', request.principal.id)
 
-        # launchpad.pageid contains an identifier of the form
-        # ContextName:ViewName. It will end up in the page log.
+        # The view may be security proxied
         view = removeSecurityProxy(ob)
         # It's possible that the view is a bounded method.
         view = getattr(view, 'im_self', view)
+        # ??? can view.context actually be security proxied?
         context = removeSecurityProxy(getattr(view, 'context', None))
-        if context is None:
-            pageid = ''
-        else:
-            # ZCML registration will set the name under which the view
-            # is accessible in the instance __name__ attribute. We use
-            # that if it's available, otherwise fall back to the class
-            # name.
-            if getattr(view, '__name__', None) is not None:
-                view_name = view.__name__
-            else:
-                view_name = view.__class__.__name__
-            pageid = '%s:%s' % (context.__class__.__name__, view_name)
-        # The view name used in the pageid usually comes from ZCML and so
-        # it will be a unicode string although it shouldn't.  To avoid
-        # problems we encode it into ASCII.
-        pageid = pageid.encode('US-ASCII')
-
+        pageid = self.constructPageID(view, context)
         request.setInWSGIEnvironment('launchpad.pageid', pageid)
-
         # And spit the pageid out to our tracelog.
         tracelog(request, 'p', pageid)
 
@@ -720,6 +726,7 @@
     w3m
     )""")
 
+
 def is_browser(request):
     """Return True if we believe the request was from a browser.
 

=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py	2010-07-19 14:45:32 +0000
+++ lib/canonical/launchpad/webapp/servers.py	2010-07-21 20:49:47 +0000
@@ -352,7 +352,7 @@
         else:
             request_factory = ProtocolErrorRequest
             publication_factory = ProtocolErrorPublicationFactory(
-                405, headers={'Allow':" ".join(self.methods)})
+                405, headers={'Allow': " ".join(self.methods)})
             factories = (request_factory, publication_factory)
 
         return factories
@@ -614,6 +614,7 @@
         """See `ISynchronizer`."""
         pass
 
+
 class BrowserFormNG:
     """Wrapper that provides IBrowserFormNG around a regular form dict."""
 
@@ -680,6 +681,7 @@
     def install(cls):
         """Install the monkey patch."""
         assert not cls.installed, "Monkey patch is already installed."
+
         def _getFormInput_single(self):
             """Return the submitted form value.
 
@@ -975,7 +977,7 @@
                 userid,
                 pageid,
                 referer,
-                user_agent
+                user_agent,
                 )
            )
 
@@ -1020,6 +1022,7 @@
 class BlueprintBrowserRequest(LaunchpadBrowserRequest):
     implements(canonical.launchpad.layers.BlueprintLayer)
 
+
 class BlueprintPublication(LaunchpadBrowserPublication):
     """The publication used for the Blueprint site."""
 
@@ -1028,6 +1031,7 @@
 class CodePublication(LaunchpadBrowserPublication):
     """The publication used for the Code site."""
 
+
 class CodeBrowserRequest(LaunchpadBrowserRequest):
     implements(canonical.launchpad.layers.CodeLayer)
 
@@ -1036,6 +1040,7 @@
 class TranslationsPublication(LaunchpadBrowserPublication):
     """The publication used for the Translations site."""
 
+
 class TranslationsBrowserRequest(LaunchpadBrowserRequest):
     implements(canonical.launchpad.layers.TranslationsLayer)
 
@@ -1051,6 +1056,7 @@
 class BugsPublication(LaunchpadBrowserPublication):
     """The publication used for the Bugs site."""
 
+
 class BugsBrowserRequest(LaunchpadBrowserRequest):
     implements(canonical.launchpad.layers.BugsLayer)
 
@@ -1059,6 +1065,7 @@
 class AnswersPublication(LaunchpadBrowserPublication):
     """The publication used for the Answers site."""
 
+
 class AnswersBrowserRequest(LaunchpadBrowserRequest):
     implements(canonical.launchpad.layers.AnswersLayer)
 
@@ -1147,6 +1154,15 @@
 
     root_object_interface = IWebServiceApplication
 
+    def constructPageID(self, view, context):
+        op = (view.request.get('ws.op')
+            or view.request.query_string_params.get('ws.op'))
+        if op:
+            return '%s:%s' % (context.__class__.__name__, op)
+        else:
+            return super(WebServicePublication, self).constructPageID(
+                view, context)
+
     def getApplication(self, request):
         """See `zope.publisher.interfaces.IPublication`.
 
@@ -1334,10 +1350,10 @@
 
 class PublicXMLRPCPublication(LaunchpadBrowserPublication):
     """The publication used for public XML-RPC requests."""
+
     def handleException(self, object, request, exc_info, retry_allowed=True):
         LaunchpadBrowserPublication.handleException(
-                self, object, request, exc_info, retry_allowed
-                )
+                self, object, request, exc_info, retry_allowed)
         OpStats.stats['xml-rpc faults'] += 1
 
     def endRequest(self, request, object):
@@ -1493,7 +1509,7 @@
         WebServiceRequestPublicationFactory(
             'api', WebServiceClientRequest, WebServicePublication),
         XMLRPCRequestPublicationFactory(
-            'xmlrpc', PublicXMLRPCRequest, PublicXMLRPCPublication)
+            'xmlrpc', PublicXMLRPCRequest, PublicXMLRPCPublication),
         ]
 
     if config.launchpad.enable_test_openid_provider:

=== added file 'lib/canonical/launchpad/webapp/tests/test_pageid.py'
--- lib/canonical/launchpad/webapp/tests/test_pageid.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_pageid.py	2010-07-21 20:49:47 +0000
@@ -0,0 +1,70 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test page ID generation."""
+
+__metaclass__ = type
+
+
+import unittest
+
+from lp.testing import TestCase
+from canonical.launchpad.webapp.servers import WebServicePublication
+
+
+class FakeContext:
+    """A context object that doesn't do anything."""
+
+
+class FakeRequest:
+    """A request that has just enough request-ness for page ID generation."""
+
+    query_string_params = {}
+    form_values = {}
+
+    def get(self, key):
+        return self.form_values.get(key)
+
+
+class FakeView:
+    """A view object that just has a fake context and request."""
+    context = FakeContext()
+    request = FakeRequest()
+
+
+class TestAuthenticationOfPersonlessAccounts(TestCase):
+
+    def test_pageid_without_op(self):
+        # When the HTTP request does not have a named operation (ws.op) field
+        # (either in the body or query string), the operation is included in
+        # the page ID.
+        publication = WebServicePublication(db=None)
+        view = FakeView()
+        context = FakeContext()
+        pageid = publication.constructPageID(view, context)
+        self.assertEqual(pageid, 'FakeContext:FakeView')
+
+    def test_pageid_without_op_in_form(self):
+        # When the HTTP request does not have a named operation (ws.op) field
+        # (either in the body or query string), the operation is included in
+        # the page ID.
+        publication = WebServicePublication(db=None)
+        view = FakeView()
+        view.request.form_values['ws.op'] = 'operation-name'
+        context = FakeContext()
+        pageid = publication.constructPageID(view, context)
+        self.assertEqual(pageid, 'FakeContext:operation-name')
+
+    def test_pageid_without_op_in_query_string(self):
+        # When the HTTP request does not have a named operation (ws.op) field
+        # (either in the body or query string), the operation is included in
+        # the page ID.
+        publication = WebServicePublication(db=None)
+        view = FakeView()
+        view.request.query_string_params['ws.op'] = 'operation-name'
+        context = FakeContext()
+        pageid = publication.constructPageID(view, context)
+        self.assertEqual(pageid, 'FakeContext:operation-name')
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)