Summary:

BUG FIX: prevents the database for accepting furthur database queries when _find_user() fails in ReviewRequestDraftResource

Review Request #6559 — Created Nov. 5, 2014 and submitted Nov. 10, 2014, 12:28 a.m.

Information
Owner:	justy777
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Blocks:	6399
Commit:	0b75a40...
Reviewers
Groups:	reviewboard, students
People:

Description

The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.
Now, after find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() it checks for invalid_fields, which will contain target_people if no user was found and returns a webAPI error of INVALID_FORM_DATA. So the HTTP response gives an informative error, and None never gets added to the database.

Testing Done

The Unit test asserts that find_user() returns null, then checks that find_user() is called, checks the response code for 105 (INVALID_FORM_DATA), and that there is no TransactionManagmentError thrown by the database.

Issues

Description	From	Last Updated
I don't think this is correct (it will make it so it will never try anything but the first backend). …	david	Nov. 6, 2014, 3:54 p.m.
I don't see how this would trigger your bug. _find_user is only called when setting the target_people field.	david	Nov. 6, 2014, 3:36 p.m.
Did you test this at all? "raise" doesn't work by itself unless it's in an except: block.	david	Nov. 6, 2014, 6:58 p.m.
Rather than changing the return types and plumbing a new error state through, I think you could just add 'value' …	david	Nov. 7, 2014, 4:32 p.m.
This will probably mess up the UI if someone puts in an invalid name. I'd just leave it to use …	david	Nov. 8, 2014, 8:19 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py

Commit:

-	fbe3698be5721ff130ff4db4ecfc41a847a64a11
+	b971302ffb0a7dbbf8310c787ee7643fdb68a910

Diff:

Revision 2 (+23 -4)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)

Show all issues

I don't think this is correct (it will make it so it will never try anything but the first backend). The correct fix should be to check the results in the place that you call _find_user, and raise an exception there.

chipx86 Nov. 9, 2014, 10:41 p.m.

I actually wanted to have the raise in _find_user in order to be consistent with the other query-ish calls, and because find_user is only used in this one place. Not a big deal, but that was based on my initial conversation with Justin.

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2)

Show all issues

I don't see how this would trigger your bug. _find_user is only called when setting the target_people field.

justy777 Nov. 6, 2014, 3:01 p.m.

my bad, not sure why I changed depends_on from target_people.

Description:

		The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.

~		Now instead of returning None, find_user() re-raises the exception when it fails to find a user, so we never try to add None to the database.
	~	Now, after find user find_user() is called the return value is tested. If the return value is None, it re-raises the exception, so we never try to add None to the database.

Testing Done:

~		Unit test that checks for the exception coming back from find_user() and that the database does not throw a TransactionManagmentError.
	~	The Unit test asserts that find_user() returns null, then checks that find_user() is called, and that there is no TransactionManagmentError thrown by the database.

Commit:

-	b971302ffb0a7dbbf8310c787ee7643fdb68a910
+	f1f74f9cd44c4d7d9d3de8c5cd0f8a32bbdc9f75

Diff:

Revision 3 (+25 -1)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

reviewboard/webapi/resources/review_request_draft.py (Diff revision 3)

Show all issues

Did you test this at all? "raise" doesn't work by itself unless it's in an except: block.

justy777 Nov. 6, 2014, 6:24 p.m.

Yes, I did. It works to fix the bug. Upon furthur inspection, raise does raise an exception 'TypeError. To fix this, I'll make it behave like the ReviewRequest resource and return INVALID_USER.

Description:

		The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.

~		Now, after find user find_user() is called the return value is tested. If the return value is None, it re-raises the exception, so we never try to add None to the database.
	~	Now, after find user find_user() is called the return value is tested and skips getting added to the database. If the return value is None, it returns a value of True for invalid_user, and then returns a webAPI error of INVALID_USER. So the HTTP response gives a more informative error, and None never gets added to the database.

Commit:

-	f1f74f9cd44c4d7d9d3de8c5cd0f8a32bbdc9f75
+	c90534751920895efc37fe55b5671d1ce86a75fd

Diff:

Revision 4 (+34 -3)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

reviewboard/webapi/resources/review_request_draft.py (Diff revision 4)

Show all issues

Rather than changing the return types and plumbing a new error state through, I think you could just add 'value' to invalid_entries here.

Description:

		The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.

~		Now, after find user find_user() is called the return value is tested and skips getting added to the database. If the return value is None, it returns a value of True for invalid_user, and then returns a webAPI error of INVALID_USER. So the HTTP response gives a more informative error, and None never gets added to the database.
	~	Now, after find user find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() invalid_entries is checked for target_people (the only field that can cause _find_user() to be run), and if true it returns a webAPI error of INVALID_USER. So the HTTP response gives a more informative error, and None never gets added to the database.

Testing Done:

~		The Unit test asserts that find_user() returns null, then checks that find_user() is called, and that there is no TransactionManagmentError thrown by the database.
	~	The Unit test asserts that find_user() returns null, then checks that find_user() is called, checks the response code for 208 (INVALID_USER), and that there is no TransactionManagmentError thrown by the database.

Commit:

-	c90534751920895efc37fe55b5671d1ce86a75fd
+	ebae21288129dc9a78e00ea5a6b8251b89b10b4c

Diff:

Revision 5 (+34 -2)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

reviewboard/webapi/resources/review_request_draft.py (Diff revision 5)

Show all issues

This will probably mess up the UI if someone puts in an invalid name. I'd just leave it to use the existing INVALID_FORM_DATA error.

Commit:

-	ebae21288129dc9a78e00ea5a6b8251b89b10b4c
+	0b75a40d7e8ef7cca7c8d8a2b001f662ac418970

Diff:

Revision 6 (+28 -2)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Description:

		The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.

~		Now, after find user find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() invalid_entries is checked for target_people (the only field that can cause _find_user() to be run), and if true it returns a webAPI error of INVALID_USER. So the HTTP response gives a more informative error, and None never gets added to the database.
	~	Now, after find user find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() it checks for invalid_fields, which will contain target_people if no user was found and returns a webAPI error of INVALID_FORM_DATA. So the HTTP response gives an informative error, and None never gets added to the database.

Testing Done:

~		The Unit test asserts that find_user() returns null, then checks that find_user() is called, checks the response code for 208 (INVALID_USER), and that there is no TransactionManagmentError thrown by the database.
	~	The Unit test asserts that find_user() returns null, then checks that find_user() is called, checks the response code for 105 (INVALID_FORM_DATA), and that there is no TransactionManagmentError thrown by the database.

Description:

		The problem occurs when a PUT webAPI call goes to ReviewRequestDraftResource and find_user() returns None. We then try to add None to the database, which generates an error, aborting the transaction, and prevents any further database queries.

~		Now, after find user find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() it checks for invalid_fields, which will contain target_people if no user was found and returns a webAPI error of INVALID_FORM_DATA. So the HTTP response gives an informative error, and None never gets added to the database.
	~	Now, after find_user() is called the return value is tested; if None then it throws an ObjectDoesNotExist exception and skips getting added to the database. In update() it checks for invalid_fields, which will contain target_people if no user was found and returns a webAPI error of INVALID_FORM_DATA. So the HTTP response gives an informative error, and None never gets added to the database.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (ac2f20d)