Summary

Sandbox reviewboard.attachments.mimetypes.MimetypeHandler

Review Request #6532 — Created Oct. 29, 2014 and submitted Nov. 12, 2014, 3:26 p.m.

Information

Owner

justy777

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

aac3e3d...

Reviewers

Groups

reviewboard, students

People

Description

Extensions that create a MimetypeHandler subclass can throw exceptions inside Review Board. The sections of Review Board that call those methods have been sandboxed.

Now when a MimetypeHandler subclass from an extension throws an exception, and Review Board logs the errors. The error logs give enough information to find which method in the MimetypeHandler subclass threw the exception.

Testing Done

Unit tests have been written to make sure that functions from an MimetypeHandler 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
"... for MimetypeHandler %r: ...' Same below.	chipx86	Oct. 30, 2014, 1:04 a.m.
This isn't testing extension sandboxing. Missing a trailing period.	chipx86	Oct. 30, 2014, 1:16 a.m.
On this test and the others, we should also ensure the function we're expecting to call is in fact called …	chipx86	Oct. 30, 2014, 1:11 a.m.
No need for the _mimetypehandler on these.	chipx86	Oct. 30, 2014, 1:06 a.m.
We shouldn't be accessing private functions. Instead, access the property.	chipx86	Oct. 30, 2014, 1:23 a.m.
As above, we should be using the property, not the private function.	chipx86	Oct. 30, 2014, 1:23 a.m.
These are only testing our code and not anything with a custom MimetypeHandler. Not necessary to do that. Also, there's …	chipx86	Oct. 30, 2014, 1:12 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 30, 2014, 1:15 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Oct. 30, 2014, 1:15 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 30, 2014, 1:15 a.m.
'register_mimetype_handler' imported but unused	reviewbot	Oct. 30, 2014, 1:45 a.m.
'unregister_mimetype_handler' imported but unused	reviewbot	Oct. 30, 2014, 1:45 a.m.
'FileAttachmentThumbnailHook' imported but unused	reviewbot	Oct. 30, 2014, 1:45 a.m.
undefined name 'MimetypeHandler'	reviewbot	Oct. 30, 2014, 2:10 a.m.
'SpyAgency' imported but unused	reviewbot	Oct. 30, 2014, 2:13 a.m.
I believe you'll want to tear down both parent classes explicitly: SpyAgency.tearDown(self) BaseFileAttachmentTestCase.tearDown(self)	david	Oct. 31, 2014, 3:09 p.m.
Instead of creating a filediff and using create_from_filediff, you can just call self.create_file_attachment(). That way you can probably also avoid …	david	Nov. 8, 2014, 8:04 p.m.
This still needs to return something (None).	chipx86	Nov. 10, 2014, 12:04 a.m.
This still needs to return something (None).	chipx86	Nov. 10, 2014, 12:04 a.m.
I prefer these to be in the form of: """Tested FileAttachment sandboxes MimetypeHandler.get_thumbnail""" And similar. The reason being that these …	chipx86	Nov. 10, 2014, 12:06 a.m.
This should be "Testing" (not "Tested")	david	Nov. 12, 2014, 1:40 a.m.
And here.	david	Nov. 12, 2014, 1:40 a.m.
And here.	david	Nov. 12, 2014, 1:40 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

Summary:

-	[WIP] reviewboard.attachments.mimetypes.MimetypeHandler sandboxing
+	reviewboard.attachments.mimetypes.MimetypeHandler sandboxing

Description:

		Unit tests for reviewboard.attachments.mimetypes.MimetypeHandler, and then sandboxing call to get_thumbnail, set_thumbnail, and get_icon_url on the reviewboard.attachments.mimetypes.MimetypeHandler subclass

~		Hook to be sandboxed: FileAttachmentThumbnailHook
	~	Hook sandboxed: FileAttachmentThumbnailHook

		Methods sandboxed:
		get_thumbnail()
		set_thumbnail()
		get_icon_url()

Testing Done:

~		Ran unit tests.
	~	Ran unit tests; tests initially fail and succeed when sandboxing is added.

Commit:

-	6c182c19d044e203ae2d7f1e17d0dde6099bc9e9
+	7a38681eca956dce3af056ea6bc6d2df94b54419

Diff:

Revision 2 (+110 -4)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py

reviewboard/extensions/tests.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/extensions/tests.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/extensions/tests.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/attachments/models.py (Diff revision 2)
Show all issues
```
"... for MimetypeHandler %r: ...'
Same below.
```

reviewboard/attachments/tests.py (Diff revision 2)

Show all issues

This isn't testing extension sandboxing.

Missing a trailing period.

reviewboard/attachments/tests.py (Diff revision 2)

Show all issues

On this test and the others, we should also ensure the function we're expecting to call is in fact called by using a spy (and making sure to set it to call the original function).

reviewboard/attachments/tests.py (Diff revision 2)
Show all issues
```
No need for the _mimetypehandler on these.
```

reviewboard/attachments/tests.py (Diff revision 2)

Show all issues

We shouldn't be accessing private functions. Instead, access the property.

reviewboard/attachments/tests.py (Diff revision 2)

Show all issues

As above, we should be using the property, not the private function.

reviewboard/extensions/tests.py (Diff revision 2)

Show all issues

These are only testing our code and not anything with a custom MimetypeHandler. Not necessary to do that. Also, there's no sandboxing there anyway.

Commit:

-	7a38681eca956dce3af056ea6bc6d2df94b54419
+	5c6334fbd291d197299dbc770777491f0e1009c4

Diff:

Revision 3 (+86 -4)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py

reviewboard/extensions/tests.py (Diff revision 3)
Show all issues
```
 'register_mimetype_handler' imported but unused
```
reviewboard/extensions/tests.py (Diff revision 3)
Show all issues
```
 'unregister_mimetype_handler' imported but unused
```
reviewboard/extensions/tests.py (Diff revision 3)
Show all issues
```
 'FileAttachmentThumbnailHook' imported but unused
```

Commit:

-	5c6334fbd291d197299dbc770777491f0e1009c4
+	06ade1a7cb0afcd5f9fa2c429e847565256afbe8

Diff:

Revision 4 (+82 -4)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py

reviewboard/extensions/tests.py (Diff revision 4)
Show all issues
```
 undefined name 'MimetypeHandler'
```

Commit:

-	06ade1a7cb0afcd5f9fa2c429e847565256afbe8
+	18f09ab07bee750a8c0c43fdbb6a15e04cbba18b

Diff:

Revision 5 (+77 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/extensions/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/extensions/tests.py
    reviewboard/attachments/models.py

reviewboard/extensions/tests.py (Diff revision 5)
Show all issues
```
 'SpyAgency' imported but unused
```

Commit:

-	18f09ab07bee750a8c0c43fdbb6a15e04cbba18b
+	3bf22dca73b37324189b807e5128ed7f56ce23c5

Diff:

Revision 6 (+76 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

reviewboard/attachments/tests.py (Diff revision 6)

Show all issues

I believe you'll want to tear down both parent classes explicitly:

SpyAgency.tearDown(self)
BaseFileAttachmentTestCase.tearDown(self)

chipx86 Nov. 1, 2014, 4:20 p.m.

It's valid to use super() here. We do it elsewhere, and I just tested that all tearDown methods are properly called with super().
I had him switch to using super() in a previous change. Poor guy is going back and forth :)

Commit:

-	3bf22dca73b37324189b807e5128ed7f56ce23c5
+	21010008031c06151c846a8e03bcb7d3b381997d

Diff:

Revision 7 (+77 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

Description:

~		Unit tests for reviewboard.attachments.mimetypes.MimetypeHandler, and then sandboxing call to get_thumbnail, set_thumbnail, and get_icon_url on the reviewboard.attachments.mimetypes.MimetypeHandler subclass
	~	Extensions that create a MimetypeHandler subclass can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

~		Hook sandboxed: FileAttachmentThumbnailHook
	~	Now when a MimetypeHandler subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the MimetypeHandler subclass threw the exception.
-
-		Methods sandboxed:
-		get_thumbnail()
-		set_thumbnail()
-		get_icon_url()

Testing Done:

~		Ran unit tests; tests initially fail and succeed when sandboxing is added.
	~	Unit tests have been written to make sure that functions from an MimetypeHandler subclass have been called, and when an exception is thrown it gets logged.
	+
	+	The tests fail without the sanboxing, and succeed with it.

Commit:

-	21010008031c06151c846a8e03bcb7d3b381997d
+	deee5bf58e2c1f9d9824bc9fc4983dd73c4e29ee

Diff:

Revision 8 (+76 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

reviewboard/attachments/tests.py (Diff revision 8)

Show all issues

Instead of creating a filediff and using create_from_filediff, you can just call self.create_file_attachment(). That way you can probably also avoid using the test_scmtools fixtures, which will speed up execution of these tests a bit.

Commit:

-	deee5bf58e2c1f9d9824bc9fc4983dd73c4e29ee
+	d3467b75d0d75cdc9e85efb3234c1f42bac2f122

Diff:

Revision 9 (+74 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

reviewboard/attachments/models.py (Diff revision 9)
Show all issues
```
This still needs to return something (None).
```
reviewboard/attachments/models.py (Diff revision 9)
Show all issues
```
This still needs to return something (None).
```

reviewboard/attachments/tests.py (Diff revision 9)

Show all issues

I prefer these to be in the form of:

"""Tested FileAttachment sandboxes MimetypeHandler.get_thumbnail"""

And similar.

The reason being that these functions (get_thumbnail, etc.) are not themselves sandboxing anything. FileAttachment is the thing doing the sandboxing.

Commit:

-	d3467b75d0d75cdc9e85efb3234c1f42bac2f122
+	0a349c04a3043e1607fd821e427761359f098ec8

Diff:

Revision 10 (+76 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

Commit:

-	0a349c04a3043e1607fd821e427761359f098ec8
+	e1b721b30fa83e8ff417ba3d2a2d29d68a6c87cf

Diff:

Revision 11 (+77 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/models.py

Summary:

-	reviewboard.attachments.mimetypes.MimetypeHandler sandboxing
+	Sandbox reviewboard.attachments.mimetypes.MimetypeHandler

In your description, there are a couple changes I want to make. First, it should always be "Review Board" (not "Reviewboard"). Second, your second paragraph in the description is full of fragments and an incorrect use of a semicolon. A basic fix for this would be to change the semicolon to a comma and combine it into one sentence. Nicer might be to use shorter sentences but be a bit more specific about what happens.

reviewboard/attachments/tests.py (Diff revision 11)
Show all issues
```
This should be "Testing" (not "Tested")
```
reviewboard/attachments/tests.py (Diff revision 11)
Show all issues
```
And here.
```
reviewboard/attachments/tests.py (Diff revision 11)
Show all issues
```
And here.
```

Description:

~		Extensions that create a MimetypeHandler subclass can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.
	~	Extensions that create a MimetypeHandler subclass can throw exceptions inside Review Board. The sections of Review Board that call those methods have been sandboxed.

~		Now when a MimetypeHandler subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the MimetypeHandler subclass threw the exception.
	~	Now when a MimetypeHandler subclass from an extension throws an exception, and Review Board logs the errors. The error logs give enough information to find which method in the MimetypeHandler subclass threw the exception.

Commit:

-	e1b721b30fa83e8ff417ba3d2a2d29d68a6c87cf
+	aac3e3dbb9e81613b255dc539ae43e1e2c41693a

Diff:

Revision 12 (+77 -3)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (8635ce4)