loggerhead

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

  1. bug-929275
  2. Merge into trunk-rich
Proposed by William Grant
Status: Merged
Merged at revision: 468
Proposed branch: lp:~wgrant/loggerhead/bug-929275
Merge into: lp:loggerhead
Diff against target: 308 lines (+97/-56)
4 files modified
NEWS (+3/-2)
loggerhead/controllers/download_ui.py (+8/-2)
loggerhead/controllers/view_ui.py (+19/-13)
loggerhead/tests/test_controllers.py (+67/-39)
To merge this branch: bzr merge lp:~wgrant/loggerhead/bug-929275
Reviewer Review Type Date Requested Status
Steve Kowalik (community) code Approve
Review via email: mp+92194@code.launchpad.net

Description of the change

Fix more non-existent fileid or revid crashes.

To post a comment you must log in.
lp:~wgrant/loggerhead/bug-929275 updated
469. By William Grant

blah

Revision history for this message
Steve Kowalik (stevenk) wrote :

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

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2012-02-03 01:29:46 +0000
+++ NEWS 2012-02-09 03:39:18 +0000
@@ -55,8 +55,9 @@
55 - Make tz calculations consistent and use UTC in the UI everywhere we show55 - Make tz calculations consistent and use UTC in the UI everywhere we show
56 a precise timestamp. (Robert Collins, #594591)56 a precise timestamp. (Robert Collins, #594591)
5757
58 - Avoid crashing when viewing or annotating a non-existent file.58 - Avoid crashing when viewing, annotating or downloading a
59 (William Grant, #728209)59 non-existent file or revision.
60 (William Grant, #728209, #929275)
6061
6162
621.18.1 [24Mar2011]631.18.1 [24Mar2011]
6364
=== 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 @@
1919
20import logging20import logging
21import mimetypes21import mimetypes
22import os
23import urllib22import urllib
2423
24from bzrlib.errors import (
25 NoSuchId,
26 NoSuchRevision,
27 )
25from paste import httpexceptions28from paste import httpexceptions
26from paste.request import path_info_pop29from paste.request import path_info_pop
2730
@@ -55,7 +58,10 @@
55 self._branch.absolute_url('/changes'))58 self._branch.absolute_url('/changes'))
56 revid = h.fix_revid(args[0])59 revid = h.fix_revid(args[0])
57 file_id = args[1]60 file_id = args[1]
58 path, filename, content = h.get_file(file_id, revid)61 try:
62 path, filename, content = h.get_file(file_id, revid)
63 except (NoSuchId, NoSuchRevision):
64 raise httpexceptions.HTTPNotFound()
59 mime_type, encoding = mimetypes.guess_type(filename)65 mime_type, encoding = mimetypes.guess_type(filename)
60 if mime_type is None:66 if mime_type is None:
61 mime_type = 'application/octet-stream'67 mime_type = 'application/octet-stream'
6268
=== 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 @@
18#18#
1919
20import os20import os
21import time
2221
23import bzrlib.errors22from bzrlib.errors import (
23 BinaryFile,
24 NoSuchId,
25 NoSuchRevision,
26 )
24import bzrlib.textfile27import bzrlib.textfile
25import bzrlib.osutils28import bzrlib.osutils
2629
@@ -76,7 +79,7 @@
76 def file_contents(self, file_id, revid):79 def file_contents(self, file_id, revid):
77 try:80 try:
78 file_lines = self.text_lines(file_id, revid)81 file_lines = self.text_lines(file_id, revid)
79 except bzrlib.errors.BinaryFile:82 except BinaryFile:
80 # bail out; this isn't displayable text83 # bail out; this isn't displayable text
81 return ['(This is a binary file.)']84 return ['(This is a binary file.)']
8285
@@ -92,17 +95,17 @@
92 raise HTTPBadRequest('No file_id or filename '95 raise HTTPBadRequest('No file_id or filename '
93 'provided to view')96 'provided to view')
9497
95 if file_id is None:98 try:
96 file_id = history.get_file_id(revid, path)99 if file_id is None:
97100 file_id = history.get_file_id(revid, path)
98 # no navbar for revisions101 if path is None:
99 navigation = util.Container()102 path = history.get_path(revid, file_id)
100103 except (NoSuchId, NoSuchRevision):
101 if path is None:104 raise HTTPNotFound()
102 path = history.get_path(revid, file_id)105
103 filename = os.path.basename(path)106 filename = os.path.basename(path)
104107
105 change = history.get_changes([ revid ])[0]108 change = history.get_changes([revid])[0]
106 # If we're looking at the tip, use head: in the URL instead109 # If we're looking at the tip, use head: in the URL instead
107 if revid == branch.last_revision():110 if revid == branch.last_revision():
108 revno_url = 'head:'111 revno_url = 'head:'
@@ -126,12 +129,15 @@
126129
127 try:130 try:
128 file = inv[file_id]131 file = inv[file_id]
129 except bzrlib.errors.NoSuchId:132 except NoSuchId:
130 raise HTTPNotFound()133 raise HTTPNotFound()
131134
132 if file.kind == "directory":135 if file.kind == "directory":
133 raise HTTPMovedPermanently(self._branch.context_url(['/files', revno_url, path]))136 raise HTTPMovedPermanently(self._branch.context_url(['/files', revno_url, path]))
134137
138 # no navbar for revisions
139 navigation = util.Container()
140
135 return {141 return {
136 # In AnnotateUI, "annotated" is a dictionary mapping lines to changes.142 # In AnnotateUI, "annotated" is a dictionary mapping lines to changes.
137 # We exploit the fact that bool({}) is False when checking whether143 # We exploit the fact that bool({}) is False when checking whether
138144
=== 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 @@
14# along with this program; if not, write to the Free Software14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA15# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
1616
17from cStringIO import StringIO
18import logging
19import tarfile17import tarfile
20import tempfile18import tempfile
2119
22from paste.fixture import (20from paste.fixture import (
23 AppError,21 AppError,
24 )22 )
25from paste.httpexceptions import (23from paste.httpexceptions import HTTPNotFound
26 HTTPNotFound,
27 HTTPServerError,
28 )
2924
30from testtools.matchers import (25from testtools.matchers import (
31 Matcher,26 Matcher,
32 Mismatch,27 Mismatch,
33 )28 )
3429
35from bzrlib import errors
36import simplejson
37
38from loggerhead.apps.branch import BranchWSGIApp30from loggerhead.apps.branch import BranchWSGIApp
39from loggerhead.controllers.annotate_ui import AnnotateUI31from loggerhead.controllers.annotate_ui import AnnotateUI
40from loggerhead.controllers.inventory_ui import InventoryUI32from loggerhead.controllers.inventory_ui import InventoryUI
@@ -159,7 +151,6 @@
159 self.assertOkJsonResponse(revision_ui, env)151 self.assertOkJsonResponse(revision_ui, env)
160152
161153
162
163class TestAnnotateUI(BasicTests):154class TestAnnotateUI(BasicTests):
164155
165 def make_annotate_ui_for_file_history(self, file_id, rev_ids_texts):156 def make_annotate_ui_for_file_history(self, file_id, rev_ids_texts):
@@ -192,12 +183,13 @@
192 history = [('rev1', 'old\nold\n', '.'), ('rev2', 'new\nold\n', '')]183 history = [('rev1', 'old\nold\n', '.'), ('rev2', 'new\nold\n', '')]
193 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)184 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
194 ann_ui.args = ['rev2']185 ann_ui.args = ['rev2']
195 annotate_info = ann_ui.get_values('filename',186 ann_ui.get_values(
196 kwargs={'file_id': 'file_id'}, headers={})187 'filename', kwargs={'file_id': 'file_id'}, headers={})
197188
198 def test_annotate_file_zero_sized(self):189 def test_annotate_file_zero_sized(self):
199 # Test against a zero-sized file without breaking. No annotation must be present.190 # Test against a zero-sized file without breaking. No annotation
200 history = [('rev1', '' , '.')]191 # must be present.
192 history = [('rev1', '', '.')]
201 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)193 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
202 ann_ui.args = ['rev1']194 ann_ui.args = ['rev1']
203 annotate_info = ann_ui.get_values('filename',195 annotate_info = ann_ui.get_values('filename',
@@ -206,12 +198,19 @@
206 self.assertEqual(0, len(annotated))198 self.assertEqual(0, len(annotated))
207199
208 def test_annotate_nonexistent_file(self):200 def test_annotate_nonexistent_file(self):
209 history = [('rev1', '' , '.')]201 history = [('rev1', '', '.')]
210 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)202 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
211 ann_ui.args = ['rev1']203 ann_ui.args = ['rev1']
212 self.assertRaises(204 self.assertRaises(
213 HTTPNotFound, ann_ui.get_values, 'not-filename', {}, {})205 HTTPNotFound, ann_ui.get_values, 'not-filename', {}, {})
214206
207 def test_annotate_nonexistent_rev(self):
208 history = [('rev1', '', '.')]
209 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
210 ann_ui.args = ['norev']
211 self.assertRaises(
212 HTTPNotFound, ann_ui.get_values, 'not-filename', {}, {})
213
215214
216class TestFileDiffUI(BasicTests):215class TestFileDiffUI(BasicTests):
217216
@@ -308,6 +307,55 @@
308 self.assertEquals("I am hooked", app.lookup_app(env))307 self.assertEquals("I am hooked", app.lookup_app(env))
309308
310309
310class MatchesDownloadHeaders(Matcher):
311
312 def __init__(self, expect_filename, expect_mimetype):
313 self.expect_filename = expect_filename
314 self.expect_mimetype = expect_mimetype
315
316 def match(self, response):
317 # Maybe the c-t should be more specific, but this is probably good for
318 # making sure it gets saved without the client trying to decompress it
319 # or anything.
320 if (response.header('Content-Type') == self.expect_mimetype
321 and response.header('Content-Disposition') ==
322 "attachment; filename*=utf-8''" + self.expect_filename):
323 pass
324 else:
325 return Mismatch("wrong response headers: %r"
326 % response.headers)
327
328 def __str__(self):
329 return 'MatchesDownloadHeaders(%r, %r)' % (
330 self.expect_filename, self.expect_mimetype)
331
332
333class TestDownloadUI(TestWithSimpleTree):
334
335 def test_download(self):
336 app = self.setUpLoggerhead()
337 response = app.get('/download/1/myfilename-id/myfilename')
338 self.assertEqual(
339 'some\nmultiline\ndata\nwith<htmlspecialchars\n', response.body)
340 self.assertThat(
341 response,
342 MatchesDownloadHeaders('myfilename', 'application/octet-stream'))
343
344 def test_download_bad_revision(self):
345 app = self.setUpLoggerhead()
346 e = self.assertRaises(
347 AppError,
348 app.get, '/download/norev/myfilename-id/myfilename')
349 self.assertContainsRe(str(e), '404 Not Found')
350
351 def test_download_bad_fileid(self):
352 app = self.setUpLoggerhead()
353 e = self.assertRaises(
354 AppError,
355 app.get, '/download/1/myfilename-notid/myfilename')
356 self.assertContainsRe(str(e), '404 Not Found')
357
358
311class IsTarfile(Matcher):359class IsTarfile(Matcher):
312360
313 def __init__(self, compression):361 def __init__(self, compression):
@@ -323,29 +371,8 @@
323 f.close()371 f.close()
324372
325373
326class MatchesTarballHeaders(Matcher):
327
328 def __init__(self, expect_filename):
329 self.expect_filename = expect_filename
330
331 def match(self, response):
332 # Maybe the c-t should be more specific, but this is probably good for
333 # making sure it gets saved without the client trying to decompress it
334 # or anything.
335 if (response.header('Content-Type') == 'application/octet-stream'
336 and response.header('Content-Disposition') ==
337 "attachment; filename*=utf-8''" + self.expect_filename):
338 pass
339 else:
340 return Mismatch("wrong response headers: %r"
341 % response.headers)
342
343
344class TestDownloadTarballUI(TestWithSimpleTree):374class TestDownloadTarballUI(TestWithSimpleTree):
345375
346 def setUp(self):
347 super(TestDownloadTarballUI, self).setUp()
348
349 def test_download_tarball(self):376 def test_download_tarball(self):
350 # Tarball downloads are enabled by default.377 # Tarball downloads are enabled by default.
351 app = self.setUpLoggerhead()378 app = self.setUpLoggerhead()
@@ -355,7 +382,7 @@
355 IsTarfile('gz'))382 IsTarfile('gz'))
356 self.assertThat(383 self.assertThat(
357 response,384 response,
358 MatchesTarballHeaders('branch.tgz'))385 MatchesDownloadHeaders('branch.tgz', 'application/octet-stream'))
359386
360 def test_download_tarball_of_version(self):387 def test_download_tarball_of_version(self):
361 app = self.setUpLoggerhead()388 app = self.setUpLoggerhead()
@@ -365,12 +392,13 @@
365 IsTarfile('gz'))392 IsTarfile('gz'))
366 self.assertThat(393 self.assertThat(
367 response,394 response,
368 MatchesTarballHeaders('branch-r1.tgz'))395 MatchesDownloadHeaders(
396 'branch-r1.tgz', 'application/octet-stream'))
369397
370 def test_download_tarball_forbidden(self):398 def test_download_tarball_forbidden(self):
371 app = self.setUpLoggerhead(export_tarballs=False)399 app = self.setUpLoggerhead(export_tarballs=False)
372 e = self.assertRaises(400 e = self.assertRaises(
373 AppError, 401 AppError,
374 app.get,402 app.get,
375 '/tarball')403 '/tarball')
376 self.assertContainsRe(404 self.assertContainsRe(

Subscribers

People subscribed via source and target branches