reviewboard.reviews.ui.ReviewUI sandboxing
Review Request #6551 — Created Nov. 2, 2014 and submitted
Extensions that create a FileAttachmentReviewUI subclass (using the ReviewUIHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.
Now when a FileAttachmentReviewUI subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the FileAttachmentReviewUI subclass threw the exception.
Unit tests have been written to make sure that functions from an FileAttachmentReviewUI subclass have been called, and when an exception is thrown it gets logged.
The tests fail without the sanboxing, and succeed with it.
| Description | From | Last Updated |
|---|---|---|
|
====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_thumbnail Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2888, in test_get_comment_thumbnail file_attachment_comments.thumbnail File … |
 justy777
|
|
|
====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_url Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2919, in test_get_comment_link_url file_attachment_comments.get_absolute_url() File … |
justy777
|
|
|
====================================================================== ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_text Traceback (most recent call last): File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2950, in test_get_comment_link_text file_attachment_comments.get_link_text() File … |
justy777
|
|
|
local variable 'comment_text_1' is assigned to but never used |
 reviewbot
|
|
|
undefined name 'comment' |
reviewbot
|
|
|
Blank line between statement and block. Same below. |
 brennie
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
You can pass in your local review_ui variable here. |
 david
|
|
|
Same here. |
david
|
|
|
And here. |
david
|
|
|
Instead of "calling __init__ for", can you say "instantiating"? |
david
|
|
|
This can use the context manager form, which makes it so you don't have to explicitly close, and makes it … |
david
|
-
-
reviewboard/reviews/tests.py (Diff revision 1) ======================================================================
ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_thumbnail
Traceback (most recent call last):
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2888, in test_get_comment_thumbnail
file_attachment_comments.thumbnail
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 34, in thumbnail
return self.file_attachment.review_ui.get_comment_thumbnail(self)
File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
return self.func(args, *kwargs)
TypeError: unbound method get_comment_thumbnail() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead) -
reviewboard/reviews/tests.py (Diff revision 1) ======================================================================
ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_url
Traceback (most recent call last):
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2919, in test_get_comment_link_url
file_attachment_comments.get_absolute_url()
File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/Django-1.6.8-py2.7.egg/django/utils/functional.py", line 15, in _curried
return _curried_func((args + moreargs), dict(kwargs, morekwargs))
File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/Django-1.6.8-py2.7.egg/django/db/models/base.py", line 1009, in get_absolute_url
return settings.ABSOLUTE_URL_OVERRIDES.get('%s.%s' % (opts.app_label, opts.model_name), func)(self, args, kwargs)
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 41, in get_absolute_url
return self.file_attachment.review_ui.get_comment_link_url(self)
File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
return self.func(*args, kwargs)
TypeError: unbound method get_comment_link_url() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead) -
reviewboard/reviews/tests.py (Diff revision 1) ======================================================================
ERROR: Testing FileAttachmentReviewUI sandboxes for get_comment_link_text
Traceback (most recent call last):
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/tests.py", line 2950, in test_get_comment_link_text
file_attachment_comments.get_link_text()
File "/home/justin/developmentSource/reviewboard/reviewboard/reviews/models/file_attachment_comment.py", line 48, in get_link_text
return self.file_attachment.review_ui.get_comment_link_text(self)
File "/home/justin/developmentEnvironments/reviewboard/lib/python2.7/site-packages/kgb-0.5.1-py2.7.egg/kgb/spies.py", line 199, in call
return self.func(args, *kwargs)
TypeError: unbound method get_comment_link_text() must be called with SandboxReviewUI instance as first argument (got FileAttachmentComment instance instead)
Description: |
|
||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 2 (+137 -7) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py -
reviewboard/reviews/tests.py (Diff revision 2) local variable 'comment_text_1' is assigned to but never used
-
Description: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 3 (+237 -13) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py
-
-
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 3) Blank line between statement and block. Same below.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+238 -13) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py
Summary: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 5 (+336 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py -
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+337 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py
-
-
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 6) You can pass in your local
review_uivariable here. -
-
-
reviewboard/reviews/ui/base.py (Diff revision 6) Instead of "
calling __init__ for", can you say "instantiating"?
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+337 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py
-
One last thing I noticed:
-
reviewboard/reviews/tests.py (Diff revision 7) This can use the context manager form, which makes it so you don't have to explicitly close, and makes it exception safe.
with open(filename, 'r') as f: self.file = SimpleUploadedFile(f.name, f.read(), content_type='image/png')
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+337 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/templatetags/reviewtags.py
Tool: Pyflakes Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/ui/base.py reviewboard/reviews/tests.py