Merge lp:~salgado/launchpad/bug-535071 into lp:launchpad

[Guilherme Salgado]

StreamOrRedirectLibraryFileAliasView no longer OOPSes when trying to
stream a restricted file while the librarian is down.

Just catches the exception and return a text/plain error message. The
choice for text/plain as the content-type is because I don't think it'd
make sense to render a regular LP page with just an error message in
this case.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/librarian/client.py
  lib/canonical/launchpad/browser/librarian.py
  lib/canonical/launchpad/browser/tests/test_librarian.py

== Pylint notices ==

lib/canonical/librarian/client.py
    27: [F0401] Unable to import 'zope.interface'

lib/canonical/launchpad/browser/librarian.py
    21: [F0401] Unable to import 'zope.interface'
    22: [F0401] Unable to import 'zope.publisher.interfaces'
    23: [F0401] Unable to import 'zope.publisher.interfaces.browser'
    24: [F0401] Unable to import 'zope.security.interfaces'
    38: [F0401] Unable to import 'lazr.delegates'

[Gary Poster (gary)]

Thank you

Gary

review: Approve

[Björn Tillenius (bjornt)]

Am I reading correctly, in that the status code is code 200? The test doesn't show, but I don't see the status code being set anywhere. What's the status code if the Librarian is down? Why?

review: Abstain

[Guilherme Salgado (salgado)]

Good catch; the status code in the response is still 200 (the default). I guess 503 would be the correct one in this case?

[Gary Poster (gary)]

Yes, thank you, Björn.

I agree, Salgado, 503 makes sense.

=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py	2009-11-24 15:36:44 +0000
+++ lib/canonical/launchpad/browser/librarian.py	2010-03-26 14:07:32 +0000
@@ -32,6 +32,7 @@
     IWebBrowserOriginatingRequest)
 from canonical.launchpad.webapp.url import urlappend
 from canonical.lazr.utils import get_current_browser_request
+from canonical.librarian.interfaces import LibrarianServerError
 from canonical.librarian.utils import filechunks, guess_librarian_encoding
 
 from lazr.delegates import delegates
@@ -82,18 +83,7 @@
 
     __used_for__ = ILibraryFileAlias
 
-    def __call__(self):
-        """Streams the contents of the context `ILibraryFileAlias`.
-
-        The file content is downloaded in chunks directly to a
-        `tempfile.TemporaryFile` avoiding using large amount of memory.
-
-        The temporary file is returned to the zope publishing machinery as
-        documented in lib/zope/publisher/httpresults.txt, after adjusting
-        the response 'Content-Type' appropriately.
-
-        This method explicit ignores the local 'http_proxy' settings.
-        """
+    def getFileContents(self):
         # Reset system proxy setting if it exists. The urllib2 default
         # opener is cached that's why it has to be re-installed after
         # the shell environment changes. Download the library file
@@ -113,6 +103,26 @@
             if original_proxy is not None:
                 os.environ['http_proxy'] = original_proxy
                 urllib2.install_opener(urllib2.build_opener())
+        return tmp_file
+
+    def __call__(self):
+        """Streams the contents of the context `ILibraryFileAlias`.
+
+        The file content is downloaded in chunks directly to a
+        `tempfile.TemporaryFile` avoiding using large amount of memory.
+
+        The temporary file is returned to the zope publishing machinery as
+        documented in lib/zope/publisher/httpresults.txt, after adjusting
+        the response 'Content-Type' appropriately.
+
+        This method explicit ignores the local 'http_proxy' settings.
+        """
+        try:
+            tmp_file = self.getFileContents()
+        except LibrarianServerError:
+            self.request.response.setHeader('Content-Type', 'text/plain')
+            return (u'There was a problem fetching the contents of this '
+                     'file. Please try again in a few minutes.')
 
         # XXX: Brad Crittenden 2007-12-05 bug=174204: When encodings are
         # stored as part of a file's metadata this logic will be replaced.
 
=== added file 'lib/canonical/launchpad/browser/tests/test_librarian.py'
--- lib/canonical/launchpad/browser/tests/test_librarian.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/browser/tests/test_librarian.py	2010-03-26 14:07:32 +0000
@@ -0,0 +1,30 @@
+# Copyright 2010 Canonical Ltd.  All rights reserved.
+
+__metaclass__ = type
+
+__all__ = []
+
+from canonical.launchpad.browser.librarian import (
+    StreamOrRedirectLibraryFileAliasView)
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.librarian.interfaces import LibrarianServerError
+
+from lp.testing import TestCase
+
+
+class FakeRestrictedLibraryFileAlias:
+    deleted = False
+    restricted = True
+
+    def open(self):
+        raise LibrarianServerError('Librarian is down')
+
+
+class TestStreamOrRedirectLibraryFileAliasView(TestCase):
+
+    def test_restricted_file_when_librarian_is_down(self):
+        view = StreamOrRedirectLibraryFileAliasView(
+            FakeRestrictedLibraryFileAlias(), LaunchpadTestRequest())
+        html = view()
+        self.assertIn(
+            'There was a problem fetching the contents of this file', html)
 
=== modified file 'lib/canonical/librarian/client.py'
--- lib/canonical/librarian/client.py	2010-02-09 00:17:40 +0000
+++ lib/canonical/librarian/client.py	2010-03-26 14:07:32 +0000
@@ -159,7 +159,7 @@
                 id=contentID, filesize=size,
                 sha1=shaDigester.hexdigest(),
                 md5=md5Digester.hexdigest())
-            alias = LibraryFileAlias(
+            LibraryFileAlias(
                 id=aliasID, content=content, filename=name.decode('UTF-8'),
                 mimetype=contentType, expires=expires,
                 restricted=self.restricted)
@@ -356,6 +356,11 @@
                     if  time.time() <= try_until:
                         time.sleep(1)
                     else:
+                        # There's a test (in
+                        # lib/c/l/browser/tests/test_librarian.py) which 
+                        # simulates a librarian server error by raising this
+                        # exception, so if you change the exception raised
+                        # here, make sure you update the test.
                         raise LibrarianServerError(str(error))
                 else:
                     raise