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
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.
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 | 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
|
|
|
I don't see how this would trigger your bug. _find_user is only called when setting the target_people field. |
david
|
|
|
Did you test this at all? "raise" doesn't work by itself unless it's in an except: block. |
david
|
|
|
Rather than changing the return types and plumbing a new error state through, I think you could just add 'value' … |
david
|
|
|
This will probably mess up the UI if someone puts in an invalid name. I'd just leave it to use … |
david
|
-
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
-
-
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. -
I don't see how this would trigger your bug.
_find_useris only called when setting thetarget_peoplefield.
- 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:
-
b971302ffb0a7dbbf8310c787ee7643fdb68a910f1f74f9cd44c4d7d9d3de8c5cd0f8a32bbdc9f75
-
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 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:
-
f1f74f9cd44c4d7d9d3de8c5cd0f8a32bbdc9f75c90534751920895efc37fe55b5671d1ce86a75fd
-
Tool: Pyflakes 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 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:
-
c90534751920895efc37fe55b5671d1ce86a75fdebae21288129dc9a78e00ea5a6b8251b89b10b4c
-
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
-
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.
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_draft.py