Merge lp:~wgrant/loggerhead/bug-929275 into lp:loggerhead

[William Grant]

Fix more non-existent fileid or revid crashes.

[lp:~wgrant/loggerhead/bug-929275]

469. By William Grant on 2012-02-09

blah

[Steve Kowalik (stevenk)]

This looks like excellent work. And thanks for fixing the import I mentioned on IRC.

review: Approve (code)

=== modified file 'NEWS'
--- NEWS	2012-02-03 01:29:46 +0000
+++ NEWS	2012-02-09 03:39:18 +0000
@@ -55,8 +55,9 @@
     - Make tz calculations consistent and use UTC in the UI everywhere we show
       a precise timestamp. (Robert Collins, #594591)
 
-    - Avoid crashing when viewing or annotating a non-existent file.
-      (William Grant, #728209)
+    - Avoid crashing when viewing, annotating or downloading a
+      non-existent file or revision.
+      (William Grant, #728209, #929275)
 
 
 1.18.1 [24Mar2011]

=== modified file 'loggerhead/controllers/download_ui.py'
--- loggerhead/controllers/download_ui.py	2011-11-25 01:41:07 +0000
+++ loggerhead/controllers/download_ui.py	2012-02-09 03:39:18 +0000
@@ -19,9 +19,12 @@
 
 import logging
 import mimetypes
-import os
 import urllib
 
+from bzrlib.errors import (
+    NoSuchId,
+    NoSuchRevision,
+    )
 from paste import httpexceptions
 from paste.request import path_info_pop
 
@@ -55,7 +58,10 @@
                 self._branch.absolute_url('/changes'))
         revid = h.fix_revid(args[0])
         file_id = args[1]
-        path, filename, content = h.get_file(file_id, revid)
+        try:
+            path, filename, content = h.get_file(file_id, revid)
+        except (NoSuchId, NoSuchRevision):
+            raise httpexceptions.HTTPNotFound()
         mime_type, encoding = mimetypes.guess_type(filename)
         if mime_type is None:
             mime_type = 'application/octet-stream'

=== modified file 'loggerhead/controllers/view_ui.py'
--- loggerhead/controllers/view_ui.py	2012-02-03 01:25:58 +0000
+++ loggerhead/controllers/view_ui.py	2012-02-09 03:39:18 +0000
@@ -18,9 +18,12 @@
 #
 
 import os
-import time
 
-import bzrlib.errors
+from bzrlib.errors import (
+    BinaryFile,
+    NoSuchId,
+    NoSuchRevision,
+    )
 import bzrlib.textfile
 import bzrlib.osutils
 
@@ -76,7 +79,7 @@
     def file_contents(self, file_id, revid):
         try:
             file_lines = self.text_lines(file_id, revid)
-        except bzrlib.errors.BinaryFile:
+        except BinaryFile:
             # bail out; this isn't displayable text
             return ['(This is a binary file.)']
 
@@ -92,17 +95,17 @@
             raise HTTPBadRequest('No file_id or filename '
                                  'provided to view')
 
-        if file_id is None:
-            file_id = history.get_file_id(revid, path)
-
-        # no navbar for revisions
-        navigation = util.Container()
-
-        if path is None:
-            path = history.get_path(revid, file_id)
+        try:
+            if file_id is None:
+                file_id = history.get_file_id(revid, path)
+            if path is None:
+                path = history.get_path(revid, file_id)
+        except (NoSuchId, NoSuchRevision):
+            raise HTTPNotFound()
+
         filename = os.path.basename(path)
 
-        change = history.get_changes([ revid ])[0]
+        change = history.get_changes([revid])[0]
         # If we're looking at the tip, use head: in the URL instead
         if revid == branch.last_revision():
             revno_url = 'head:'
@@ -126,12 +129,15 @@
 
         try:
             file = inv[file_id]
-        except bzrlib.errors.NoSuchId:
+        except NoSuchId:
             raise HTTPNotFound()
 
         if file.kind == "directory":
             raise HTTPMovedPermanently(self._branch.context_url(['/files', revno_url, path]))
 
+        # no navbar for revisions
+        navigation = util.Container()
+
         return {
             # In AnnotateUI, "annotated" is a dictionary mapping lines to changes.
             # We exploit the fact that bool({}) is False when checking whether

=== modified file 'loggerhead/tests/test_controllers.py'
--- loggerhead/tests/test_controllers.py	2012-02-03 01:25:58 +0000
+++ loggerhead/tests/test_controllers.py	2012-02-09 03:39:18 +0000
@@ -14,27 +14,19 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
-from cStringIO import StringIO
-import logging
 import tarfile
 import tempfile
 
 from paste.fixture import (
     AppError,
     )
-from paste.httpexceptions import (
-    HTTPNotFound,
-    HTTPServerError,
-    )
+from paste.httpexceptions import HTTPNotFound
 
 from testtools.matchers import (
     Matcher,
     Mismatch,
     )
 
-from bzrlib import errors
-import simplejson
-
 from loggerhead.apps.branch import BranchWSGIApp
 from loggerhead.controllers.annotate_ui import AnnotateUI
 from loggerhead.controllers.inventory_ui import InventoryUI
@@ -159,7 +151,6 @@
         self.assertOkJsonResponse(revision_ui, env)
 
 
-
 class TestAnnotateUI(BasicTests):
 
     def make_annotate_ui_for_file_history(self, file_id, rev_ids_texts):
@@ -192,12 +183,13 @@
         history = [('rev1', 'old\nold\n', '.'), ('rev2', 'new\nold\n', '')]
         ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
         ann_ui.args = ['rev2']
-        annotate_info = ann_ui.get_values('filename',
-            kwargs={'file_id': 'file_id'}, headers={})
+        ann_ui.get_values(
+            'filename', kwargs={'file_id': 'file_id'}, headers={})
 
     def test_annotate_file_zero_sized(self):
-        # Test against a zero-sized file without breaking. No annotation must be present.
-        history = [('rev1', '' , '.')]
+        # Test against a zero-sized file without breaking. No annotation
+        # must be present.
+        history = [('rev1', '', '.')]
         ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
         ann_ui.args = ['rev1']
         annotate_info = ann_ui.get_values('filename',
@@ -206,12 +198,19 @@
         self.assertEqual(0, len(annotated))
 
     def test_annotate_nonexistent_file(self):
-        history = [('rev1', '' , '.')]
+        history = [('rev1', '', '.')]
         ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
         ann_ui.args = ['rev1']
         self.assertRaises(
             HTTPNotFound, ann_ui.get_values, 'not-filename', {}, {})
 
+    def test_annotate_nonexistent_rev(self):
+        history = [('rev1', '', '.')]
+        ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
+        ann_ui.args = ['norev']
+        self.assertRaises(
+            HTTPNotFound, ann_ui.get_values, 'not-filename', {}, {})
+
 
 class TestFileDiffUI(BasicTests):
 
@@ -308,6 +307,55 @@
         self.assertEquals("I am hooked", app.lookup_app(env))
 
 
+class MatchesDownloadHeaders(Matcher):
+
+    def __init__(self, expect_filename, expect_mimetype):
+        self.expect_filename = expect_filename
+        self.expect_mimetype = expect_mimetype
+
+    def match(self, response):
+        # Maybe the c-t should be more specific, but this is probably good for
+        # making sure it gets saved without the client trying to decompress it
+        # or anything.
+        if (response.header('Content-Type') == self.expect_mimetype
+            and response.header('Content-Disposition') ==
+            "attachment; filename*=utf-8''" + self.expect_filename):
+            pass
+        else:
+            return Mismatch("wrong response headers: %r"
+                % response.headers)
+
+    def __str__(self):
+        return 'MatchesDownloadHeaders(%r, %r)' % (
+            self.expect_filename, self.expect_mimetype)
+
+
+class TestDownloadUI(TestWithSimpleTree):
+
+    def test_download(self):
+        app = self.setUpLoggerhead()
+        response = app.get('/download/1/myfilename-id/myfilename')
+        self.assertEqual(
+            'some\nmultiline\ndata\nwith<htmlspecialchars\n', response.body)
+        self.assertThat(
+            response,
+            MatchesDownloadHeaders('myfilename', 'application/octet-stream'))
+
+    def test_download_bad_revision(self):
+        app = self.setUpLoggerhead()
+        e = self.assertRaises(
+            AppError,
+            app.get, '/download/norev/myfilename-id/myfilename')
+        self.assertContainsRe(str(e), '404 Not Found')
+
+    def test_download_bad_fileid(self):
+        app = self.setUpLoggerhead()
+        e = self.assertRaises(
+            AppError,
+            app.get, '/download/1/myfilename-notid/myfilename')
+        self.assertContainsRe(str(e), '404 Not Found')
+
+
 class IsTarfile(Matcher):
 
     def __init__(self, compression):
@@ -323,29 +371,8 @@
             f.close()
 
 
-class MatchesTarballHeaders(Matcher):
-
-    def __init__(self, expect_filename):
-        self.expect_filename = expect_filename
-
-    def match(self, response):
-        # Maybe the c-t should be more specific, but this is probably good for
-        # making sure it gets saved without the client trying to decompress it
-        # or anything.
-        if (response.header('Content-Type') == 'application/octet-stream'
-            and response.header('Content-Disposition') ==
-            "attachment; filename*=utf-8''" + self.expect_filename):
-            pass
-        else:
-            return Mismatch("wrong response headers: %r"
-                % response.headers)
-
-
 class TestDownloadTarballUI(TestWithSimpleTree):
 
-    def setUp(self):
-        super(TestDownloadTarballUI, self).setUp()
-
     def test_download_tarball(self):
         # Tarball downloads are enabled by default.
         app = self.setUpLoggerhead()
@@ -355,7 +382,7 @@
             IsTarfile('gz'))
         self.assertThat(
             response,
-            MatchesTarballHeaders('branch.tgz'))
+            MatchesDownloadHeaders('branch.tgz', 'application/octet-stream'))
 
     def test_download_tarball_of_version(self):
         app = self.setUpLoggerhead()
@@ -365,12 +392,13 @@
             IsTarfile('gz'))
         self.assertThat(
             response,
-            MatchesTarballHeaders('branch-r1.tgz'))
+            MatchesDownloadHeaders(
+                'branch-r1.tgz', 'application/octet-stream'))
 
     def test_download_tarball_forbidden(self):
         app = self.setUpLoggerhead(export_tarballs=False)
         e = self.assertRaises(
-            AppError, 
+            AppError,
             app.get,
             '/tarball')
         self.assertContainsRe(