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
Information | |
---|---|
justy777 | |
Review Board | |
master | |
6399 | |
0b75a40... | |
Reviewers | |
reviewboard, students | |
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). … |
|
|
I don't see how this would trigger your bug. _find_user is only called when setting the target_people field. |
|
|
Did you test this at all? "raise" doesn't work by itself unless it's in an except: block. |
|
|
Rather than changing the return types and plumbing a new error state through, I think you could just add 'value' … |
|
|
This will probably mess up the UI if someone puts in an invalid name. I'd just leave it to use … |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+23 -4) |

-
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) 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. -
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 2) I don't see how this would trigger your bug.
_find_user
is only called when setting thetarget_people
field.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 3 (+25 -1) |

-
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) Did you test this at all? "raise" doesn't work by itself unless it's in an except: block.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 4 (+34 -3) |

-
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) 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: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 5 (+34 -2) |

-
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) 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+28 -2) |

-
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: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Description: |
|
---|