Summary

Add a commit_id field to ReviewRequest

Review Request #4237 — Created June 12, 2013 and submitted June 15, 2013, 1:16 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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/

Testing Done

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

Issues

Description	From	Last Updated
This will blow up for non-integers.	chipx86	June 13, 2013, 1:29 a.m.
This will blow up for non-integers.	chipx86	June 13, 2013, 1:31 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	June 12, 2013, 11:06 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	June 13, 2013, 9:36 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	June 13, 2013, 9:37 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	June 13, 2013, 9:37 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	June 13, 2013, 9:38 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	June 13, 2013, 9:38 p.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	June 13, 2013, 9:38 p.m.
Should do Q(changenum=changenum) \| Q(commit_id=changenum) to reduce queries.	chipx86	June 14, 2013, 12:38 a.m.
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	June 14, 2013, 12:38 a.m.
Can we make a setter for this, so that updating commit_id sets the right thing?	chipx86	June 14, 2013, 12:40 a.m.
Do we want to clear out changenum, or keep it? I wasn't sure if we wanted to preserve it for …	chipx86	June 14, 2013, 12:39 a.m.
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	June 14, 2013, 12:42 a.m.
Should have a try/except for the int.	chipx86	June 14, 2013, 12:41 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	June 13, 2013, 9:49 p.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	June 13, 2013, 9:48 p.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	June 14, 2013, 1:06 a.m.
Given the new 'commit' property, do we want to be referencing commit_id, or commit?	chipx86	June 15, 2013, 1:07 a.m.
This class isn't even used anymore. You can nuke this. In a separate commit, we should just remove this file …	chipx86	June 15, 2013, 1:05 a.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	June 14, 2013, 8:46 p.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	June 15, 2013, 8:31 p.m.

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

reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

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.

reviewboard/webapi/encoder.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This will blow up for non-integers.
```
reviewboard/webapi/resources.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This will blow up for non-integers.
```

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:

Change ReviewRequest.changenum to ReviewRequest.commit_id

Add a commit_id field to ReviewRequest

Description:

~		Change ReviewRequest.changenum to ReviewRequest.commit_id
	~	Add a commit_id field to ReviewRequest

~		The 'changenum' field on a ReviewRequest has so far been used for SCMs that keep
~		track of pending changes on the server side, like Perforce and Plastic. Now that
~		I'm working on post-commit reviews, I'd like to use it as well for other SCMs
~		like SVN and Git. Unfortunately, not all SCMs store their commits with numeric
	~	This change adds a new field called 'commit_id' to the ReviewRequest model. This
	~	field obsoletes the 'changenum' field. The actual database entry is called
	~	_commit_id, and there's a property which will migrate old data from changenum to
	~	commit_id.
-		revisions (lookin' at you, git).

~		This change adds a new field called commit_id which is a 64-character string.
~		There's a migration to convert the existing changenum entries, and adapters for
~		the webapi to maintain compatibility.
	~	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/

Diff:

Revision 2 (+175 -46)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/commit_id.py
	reviewboard/templates/reviews/review_request_box.html
	3 more

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 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (112 > 79 characters)
```

Diff:

Revision 3 (+177 -47)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/commit_id.py
	reviewboard/templates/reviews/review_request_box.html
	3 more

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/webapi/resources.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (112 > 79 characters)
```

reviewboard/reviews/forms.py (Diff revision 3)

The issue has been resolved. Show all issues

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

reviewboard/reviews/models.py (Diff revision 3)

The issue has been resolved. 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.

reviewboard/reviews/models.py (Diff revision 3)

The issue has been resolved. Show all issues

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

reviewboard/reviews/models.py (Diff revision 3)

The issue has been dropped. 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).

david June 14, 2013, 12:54 a.m.
```
I was thinking keep.
```
chipx86 June 14, 2013, 12:55 a.m.
```
Works for me.
```

reviewboard/reviews/tests.py (Diff revision 3)

The issue has been resolved. 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.

reviewboard/webapi/resources.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should have a try/except for the int.
```

Diff:

Revision 4 (+187 -52)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/commit_id.py
	reviewboard/templates/reviews/review_request_box.html
	3 more

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/webapi/tests.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (112 > 79 characters)
```

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?

david June 14, 2013, 1:41 p.m.

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"

Description:

		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. The actual database entry is called
~		_commit_id, and there's a property which will migrate old data from changenum to
~		commit_id.
	~	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/

Diff:

Revision 5 (+188 -52)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/commit_id.py
	reviewboard/templates/reviews/review_request_box.html
	3 more

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/webapi/tests.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (112 > 79 characters)
```

This looks good. I just have two small comments from the latest change.

reviewboard/templates/reviews/review_request_box.html (Diff revision 5)
The issue has been resolved. Show all issues
```
Given the new 'commit' property, do we want to be referencing commit_id, or commit?
```

reviewboard/webapi/encoder.py (Diff revision 5)

The issue has been dropped. 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.

david June 15, 2013, 1:07 a.m.

I'd rather keep it for now just to be internally consistent and you can nuke the whole thing whenever you want.

Diff:

Revision 6 (+188 -52)

Show changes

	reviewboard/reviews/admin.py
	reviewboard/reviews/forms.py
	reviewboard/reviews/managers.py
	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/evolutions/__init__.py
	reviewboard/reviews/evolutions/commit_id.py
	reviewboard/templates/reviews/review_request_box.html
	3 more

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (42399c7).

Fix it!

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/webapi/tests.py (Diff revision 6)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (112 > 79 characters)
```