Summary

reviewboard.reviews.ui.ReviewUI sandboxing

Review Request #6551 — Created Nov. 2, 2014 and submitted Nov. 7, 2014, 5:26 p.m.

Information

Owner

justy777

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

f65c7f4...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

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	Nov. 2, 2014, 12:40 p.m.
====================================================================== 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	Nov. 2, 2014, 12:40 p.m.
====================================================================== 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	Nov. 2, 2014, 12:40 p.m.
local variable 'comment_text_1' is assigned to but never used	reviewbot	Nov. 3, 2014, 2:59 p.m.
undefined name 'comment'	reviewbot	Nov. 3, 2014, 2:59 p.m.
Blank line between statement and block. Same below.	brennie	Nov. 4, 2014, 11:49 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 5, 2014, 5:21 p.m.
You can pass in your local review_ui variable here.	david	Nov. 6, 2014, 3:09 p.m.
Same here.	david	Nov. 6, 2014, 3:09 p.m.
And here.	david	Nov. 6, 2014, 3:09 p.m.
Instead of "calling __init__ for", can you say "instantiating"?	david	Nov. 6, 2014, 3:10 p.m.
This can use the context manager form, which makes it so you don't have to explicitly close, and makes it …	david	Nov. 6, 2014, 4:15 p.m.

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

reviewboard/reviews/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

======================================================================

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)

The issue has been resolved. Show all issues

======================================================================

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)

The issue has been resolved. Show all issues

======================================================================

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:

		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.
	+
	+	TODO:
	+	stop init from raising an exception in every test but its own
	+
	+	write test for is_enabled_for(), get_extra_context(), get_js_model_data(), get_js_view_data(), serialize_comments() and serialize_comment()

Commit:

3a9030e679fd236b93ebc6c35bb5a8864bdd0035

04159eef24b83da9f83782e32d5a7de476b6df16

Diff:

Revision 2 (+137 -7)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/ui/base.py

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)
The issue has been resolved. Show all issues
```
 local variable 'comment_text_1' is assigned to but never used
```
reviewboard/reviews/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 undefined name 'comment'
```

Description:

		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.

		TODO:
~		stop init from raising an exception in every test but its own
	~	write test for is_enabled_for(), get_js_model_data(), and get_js_view_data()
-
-		write test for is_enabled_for(), get_extra_context(), get_js_model_data(), get_js_view_data(), serialize_comments() and serialize_comment()

Commit:

04159eef24b83da9f83782e32d5a7de476b6df16

c7b600362ee1442e41c28c6277c02ac2ee44a7a6

Diff:

Revision 3 (+237 -13)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/ui/base.py

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)
The issue has been resolved. Show all issues
```
Blank line between statement and block. Same below.
```

Commit:

c7b600362ee1442e41c28c6277c02ac2ee44a7a6

8ca8c437b10ccb3b1377ee5c889590c6e1066f85

Diff:

Revision 4 (+238 -13)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/ui/base.py

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:

[WIP] reviewboard.reviews.ui.ReviewUI sandboxing

reviewboard.reviews.ui.ReviewUI sandboxing

Description:

		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.
-
-		TODO:
-		write test for is_enabled_for(), get_js_model_data(), and get_js_view_data()

Commit:

8ca8c437b10ccb3b1377ee5c889590c6e1066f85

917fca36905c46d7278292c72861e2a620ba6d3b

Diff:

Revision 5 (+336 -27)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/ui/base.py

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/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

Commit:

917fca36905c46d7278292c72861e2a620ba6d3b

d0d74252d94cbe2f700c07776c88f2ad78a793c2

Diff:

Revision 6 (+337 -27)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/ui/base.py

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)
The issue has been resolved. Show all issues
```
You can pass in your local review_ui variable here.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 6)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/reviews/ui/base.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Instead of "calling __init__ for", can you say "instantiating"?
```

Commit:

d0d74252d94cbe2f700c07776c88f2ad78a793c2

8d3f1a869ec436c17e315a49206ecaf220d64a85

Diff:

Revision 7 (+337 -27)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/ui/base.py

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)

The issue has been resolved. Show all issues

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:

8d3f1a869ec436c17e315a49206ecaf220d64a85

f65c7f4a1e4f54d3b725954786e3e28e66bf6135

Diff:

Revision 8 (+337 -27)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/ui/base.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (7ac7115)