Add a commit_id field to ReviewRequest
Review Request #4237 — Created June 12, 2013 and submitted
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. |
 chipx86
|
|
|
This will blow up for non-integers. |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (112 > 79 characters) |
reviewbot
|
|
|
Should do Q(changenum=changenum) | Q(commit_id=changenum) to reduce queries. |
chipx86
|
|
|
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. |
chipx86
|
|
|
Can we make a setter for this, so that updating commit_id sets the right thing? |
chipx86
|
|
|
Do we want to clear out changenum, or keep it? I wasn't sure if we wanted to preserve it for … |
chipx86
|
|
|
test_commit_id. Also, if this is the only test here that needs the above fixtures, you can use @add_fixtures. That'll help … |
chipx86
|
|
|
Should have a try/except for the int. |
chipx86
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (112 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (112 > 79 characters) |
reviewbot
|
|
|
Given the new 'commit' property, do we want to be referencing commit_id, or commit? |
chipx86
|
|
|
This class isn't even used anymore. You can nuke this. In a separate commit, we should just remove this file … |
chipx86
|
|
|
Col: 80 E501 line too long (112 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (112 > 79 characters) |
reviewbot
|
-
-
reviewboard/reviews/evolutions/commit_id.py (Diff revision 1) 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.
-
-
Change Summary:
So, the migration itself was quite fast (which I expected, given that it can operate a row at a time, with no joins). That said, getting the syntax to work across all our supported DBs is hard, and the testing is annoying. This update changes it to just add a new field and migrate data as it's used.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+175 -46) |
-
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 -
-
-
-
-
-
-
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 -
-
-
-
reviewboard/reviews/forms.py (Diff revision 3) Should do Q(changenum=changenum) | Q(commit_id=changenum) to reduce queries.
-
reviewboard/reviews/models.py (Diff revision 3) 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.
-
reviewboard/reviews/models.py (Diff revision 3) Can we make a setter for this, so that updating commit_id sets the right thing?
-
reviewboard/reviews/models.py (Diff revision 3) 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).
-
reviewboard/reviews/tests.py (Diff revision 3) 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.
-
-
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 -
-
-
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?
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+188 -52) |
-
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 -
-
This looks good. I just have two small comments from the latest change.
-
reviewboard/templates/reviews/review_request_box.html (Diff revision 5) Given the new 'commit' property, do we want to be referencing commit_id, or commit?
-
reviewboard/webapi/encoder.py (Diff revision 5) 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.
-
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 -
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