Merge lp:~bjornt/launchpad/bug-376990 into lp:launchpad/db-devel

[Björn Tillenius]

[Björn Tillenius (bjornt)]

This branch makes canonical_url() produce browser URLs, even though the
current request is a web service request. We have some problems that API
URLs are used instead of browser URLs, for example in XHTML.
presentations and when e-mail notifications are generated inside API.
methods.

IMHO, canonical_url() should be split into two functions, but it's too.
late to do that now. I'd like one function that always returns a browser
URL, and one that returns either a browser or web service URL, depending
on which request is currently used. That's basically how I made.
canonical_url() work with this change. If no request is explicitly.
passed to it, a browser URL is always returned. In the case where you.
may want an API URL, you have to pass in the current request explicitly.
That's why I had to pass in the request to canonical_url() in the.
/people/foo rediretor, since it's used in both the app and web service
servers. Basically all code that redirects objects to canonical_url().
will need to pass in the request explicitly. At the moment all tests.
pass, but we might find a few bugs later. I don't expect this to be a.
big deal, though.

In this branch I also made canonical_url.txt run in FunctionalLayer, so
that it runs a bit faster.

BTW, I'm not going to land this branch before someone with more web.
service foo has commented on it, but I have discussed this approach with.
them already.

[Leonard Richardson (leonardr)]

Approved.*

review: Approve

[Leonard Richardson (leonardr)]

> Approved.*

I'm not totally happy with this solution, but there are so many call sites that changing the default behavior is the only real way to fix this.

[Brad Crittenden (bac)]

* line 11: s/want/wants
* line 13: typo: therefore
* The last sentence reads better as:
Therefore, if no request is explicitly given canonical_url() returns the browser
URL, even if the current request is a web service request.

Thanks for making the change to FunctionalLayer. Did you measure the improvement?

review: Approve (mentor)

[Björn Tillenius (bjornt)]

On Tue, Jul 28, 2009 at 01:13:29PM -0000, Brad Crittenden wrote:
> Review: Approve mentor
> * line 11: s/want/wants
> * line 13: typo: therefore

Why do I always make that typo? :)

> * The last sentence reads better as:
> Therefore, if no request is explicitly given canonical_url() returns the browser
> URL, even if the current request is a web service request.

Thanks, I'll fix it.

>
> Thanks for making the change to FunctionalLayer. Did you measure the
> improvement?

Yes. It's still quite slow, but took it down from 40 seconds to 20
seconds.

--
Björn Tillenius | https://launchpad.net/~bjornt

=== modified file 'lib/canonical/launchpad/doc/canonical_url.txt'
--- lib/canonical/launchpad/doc/canonical_url.txt	2009-05-13 16:51:38 +0000
+++ lib/canonical/launchpad/doc/canonical_url.txt	2009-07-27 13:45:52 +0000
@@ -492,6 +492,37 @@
     NoCanonicalUrl: No url for <...ObjectThatHasUrl...> because <...object...> broke the chain.
 
 
+== canonical_url in the web service ==
+
+canonical_url() is sometimes used in code that doesn't have direct
+access to the current request, and always want a URL that can be used in
+a browser (for example e-mail notifications or XHTML representations of
+objects). Therefor, if no request is explicitly given, and the current
+request is a web service request, canonical_url() returns the browser
+URL.
+
+    >>> from zope.app.security.principalregistry import (
+    ...     UnauthenticatedPrincipal)
+    >>> from canonical.launchpad.webapp.servers import WebServiceTestRequest
+    >>> from canonical.launchpad.webapp.interaction import setupInteraction
+    >>> from lazr.restful.utils import get_current_browser_request
+    >>> anonymous = UnauthenticatedPrincipal(None, None, None)
+    >>> api_request = WebServiceTestRequest()
+    >>> setupInteraction(anonymous, participation=api_request)
+    >>> get_current_browser_request() is api_request
+    True
+
+    >>> canonical_url(countryset_instance)
+    u'http://launchpad.dev/countries'
+
+
+If an URL that can be used in the web service is required, a web service
+request has to be passed in explicitly.
+
+    >>> canonical_url(countryset_instance, request=api_request)
+    u'http://api.launchpad.dev/countries'
+
+
 == The end ==
 
 We've finished with our interfaces and utility component, so remove them from

=== modified file 'lib/canonical/launchpad/ftests/test_system_documentation.py'
--- lib/canonical/launchpad/ftests/test_system_documentation.py	2009-07-17 00:26:05 +0000
+++ lib/canonical/launchpad/ftests/test_system_documentation.py	2009-07-24 08:59:59 +0000
@@ -273,6 +273,12 @@
             tearDown=updateRemoteProductTeardown,
             layer=LaunchpadZopelessLayer
             ),
+    'canonical_url.txt': LayeredDocFileSuite(
+            '../doc/canonical_url.txt',
+            setUp=setUp,
+            tearDown=tearDown,
+            layer=FunctionalLayer,
+            ),
     }
 
 

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2009-07-24 12:52:33 +0000
@@ -38,7 +38,7 @@
 from zope.traversing.browser.interfaces import IAbsoluteURL
 
 from canonical.cachedproperty import cachedproperty
-from canonical.launchpad.layers import setFirstLayer
+from canonical.launchpad.layers import setFirstLayer, WebServiceLayer
 from canonical.launchpad.webapp.vhosts import allvhosts
 from canonical.launchpad.webapp.interfaces import (
     ICanonicalUrlData, ILaunchBag, ILaunchpadApplication, ILaunchpadContainer,
@@ -463,6 +463,21 @@
         # Look for a request from the interaction.
         current_request = get_current_browser_request()
         if current_request is not None:
+            if WebServiceLayer.providedBy(current_request):
+                from canonical.launchpad.webapp.publication import (
+                    LaunchpadBrowserPublication)
+                from canonical.launchpad.webapp.servers import (
+                    LaunchpadBrowserRequest)
+                current_request = LaunchpadBrowserRequest(
+                    current_request.bodyStream.getCacheStream(),
+                    dict(current_request.environment))
+                current_request.setPublication(
+                    LaunchpadBrowserPublication(None))
+                current_request.setVirtualHostRoot(names=[])
+                main_root_url = current_request.getRootURL(
+                    'mainsite')
+                current_request._app_server = main_root_url.rstrip('/')
+
             request = current_request
 
     if view_name is not None:

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2009-07-20 15:27:26 +0000
+++ lib/lp/registry/browser/person.py	2009-07-27 13:37:23 +0000
@@ -616,14 +616,16 @@
         if person is None:
             raise NotFoundError(name)
         # Redirect to /~name
-        return self.redirectSubTree(canonical_url(person))
+        return self.redirectSubTree(
+            canonical_url(person, request=self.request))
 
     @stepto('+me')
     def me(self):
         me = getUtility(ILaunchBag).user
         if me is None:
             raise Unauthorized("You need to be logged in to view this URL.")
-        return self.redirectSubTree(canonical_url(me), status=303)
+        return self.redirectSubTree(
+            canonical_url(me, request=self.request), status=303)
 
 
 class PersonSetContextMenu(ContextMenu):