Add a commit_id field to ReviewRequest

Review Request #4237 — Created June 12, 2013 and submitted

Information

Review Board
master

Reviewers

Add a commit_id field to ReviewRequest

This change adds a new field called 'commit_id' to the ReviewRequest model. This
field obsoletes the 'changenum' field. There's also a property called "commit"
which will aggregate together the old "changenum" field and the new "commit_id"
field.

This new field is a 64-character string instead of an int, in order to handle
SCMs that don't use numeric revisions (like git).

Testing done:
- Tested the migration with a database that had review requests with both null
  and non-null changenums
- Ran unit tests
- Did some smoke testing of various pieces of UI that deal with changenums

Reviewed at http://reviews.reviewboard.org/r/4237/
- Tested the migration with a database that had review requests with both null
  and numeric changenum fields.
- Ran unit tests.
- Did some smoke testing of various pieces of UI that deal with changenums.
Description From Last Updated

This will blow up for non-integers.

chipx86chipx86

This will blow up for non-integers.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (112 > 79 characters)

reviewbotreviewbot

Should do Q(changenum=changenum) | Q(commit_id=changenum) to reduce queries.

chipx86chipx86

The database field should still be commit_id. You can specify this with db_column='commit_id'. That'll help make the queries more clear.

chipx86chipx86

Can we make a setter for this, so that updating commit_id sets the right thing?

chipx86chipx86

Do we want to clear out changenum, or keep it? I wasn't sure if we wanted to preserve it for …

chipx86chipx86

test_commit_id. Also, if this is the only test here that needs the above fixtures, you can use @add_fixtures. That'll help …

chipx86chipx86

Should have a try/except for the int.

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (112 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (112 > 79 characters)

reviewbotreviewbot

Given the new 'commit' property, do we want to be referencing commit_id, or commit?

chipx86chipx86

This class isn't even used anymore. You can nuke this. In a separate commit, we should just remove this file …

chipx86chipx86

Col: 80 E501 line too long (112 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (112 > 79 characters)

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
        reviewboard/reviews/management/commands/index.py
      Ignored Files:
        reviewboard/reviews/fixtures/test_reviewrequests.json
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. Need to make sure this works in sqlite, MySQL, and PostgreSQL. I *think* they will, but I'd hate to release and hit some major problem.
    
    I'm also afraid this is going to take a very long time on large databases. Wish we had some numbers on it. Wanna generate a database in MySQL of 400,000 review requests with changenums and see how long it takes? :)
    
    Depending on that, another option is to keep the old field but stop populating it, and instead use the new one from here on out. The actual field could be prefixed with a _ in the model, and a @property could be used to get from `_commit_id or str(self.changenum)`, and set to _commit_id.
  3. reviewboard/webapi/encoder.py (Diff revision 1)
     
     
    Show all issues
    This will blow up for non-integers.
  4. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    This will blow up for non-integers.
  5. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/forms.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/forms.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  6. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. reviewboard/webapi/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (112 > 79 characters)
    
  8. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/webapi/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (112 > 79 characters)
    
  4. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues
    Should do Q(changenum=changenum) | Q(commit_id=changenum) to reduce queries.
  3. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Show all issues
    The database field should still be commit_id. You can specify this with db_column='commit_id'. That'll help make the queries more clear.
  4. reviewboard/reviews/models.py (Diff revision 3)
     
     
     
    Show all issues
    Can we make a setter for this, so that updating commit_id sets the right thing?
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
     
     
    Show all issues
    Do we want to clear out changenum, or keep it? I wasn't sure if we wanted to preserve it for the API or not (which would make sense).
  6. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    test_commit_id.
    
    Also, if this is the only test here that needs the above fixtures, you can use @add_fixtures. That'll help reduce extra load times for the other tests in this class.
  7. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Should have a try/except for the int.
  8. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (112 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/forms.py (Diff revision 4)
     
     
    So I was thinking that these would let us query by what was in db_column, but I was wrong. I'm really not comfortable with having to deal with "_commit_id." I'm not sure what the best option is.
    
    Maybe we just don't be fancy, and instead keep this "commit_id." There's only a couple places where we really access this. Just thinking out loud here, but maybe we change our property to just be get_commit_id, or get_commit_id_or_changenum?
    
    I think, since we're keeping the old changenum column around and not moving data from it (just copying), that maybe we don't actually want to pretend commit_id is both, implicitly, and make that determining explicit.
    
    I'm not completely sure, honestly. I just don't want the queries to seem so weird, or to be named so similarly to the accessor while producing different things.
    
    Thoughts?
    1. That's quite a stream of consciousness.
      
      I think I'm going to rename the field to be "commit_id" and have the accessor property be "commit"
  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (112 > 79 characters)
    
  3. 
      
chipx86
  1. This looks good. I just have two small comments from the latest change.
  2. Show all issues
    Given the new 'commit' property, do we want to be referencing commit_id, or commit?
  3. reviewboard/webapi/encoder.py (Diff revision 5)
     
     
    Show all issues
    This class isn't even used anymore. You can nuke this. In a separate commit, we should just remove this file and the accompanying decorator.
    1. I'd rather keep it for now just to be internally consistent and you can nuke the whole thing whenever you want.
  4. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (42399c7).
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/encoder.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/evolutions/commit_id.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/managers.py
      Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/webapi/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (112 > 79 characters)
    
  3.