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

Review Board
master
0b75a40...

Reviewers

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). …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
    
    
  2. 
      
justy777
reviewbot
  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
    
    
  2. 
      
david
  1. 
      
  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.

    1. 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.

  3. Show all issues

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

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

  4. 
      
justy777
reviewbot
  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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

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

    1. 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.

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
  2. 
      
david
  1. 
      
  2. 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.

  3. 
      
justy777
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. 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.

  3. 
      
justy777
reviewbot
  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
    
    
  2. 
      
justy777
justy777
david
  1. Ship It!

  2. 
      
justy777
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (ac2f20d)