Docserver: Add commit history and _reset_commit servlet

Docserver: Add commit history and _reset_commit servlet

This adds commit history to named commits in the CommitTracker. The history can be viewed by requesting /_query_commit/<commit-name>.

This also adds a _reset_commit servlet which will reset the current commit ID of a named commit. This servlet will only obey the request if the given commit ID is present in the named commit's history.

BUG=None
R=kalman@chromium.org
NOTRY=true

Committed: https://crrev.com/34477f1239c60bf8a7e3243ad3aa6ae893072cba
Cr-Commit-Position: refs/heads/master@{#314395}

[Ken Rockot(use gerrit already), 2014-11-18 22:54:55]

hey im code

[not at google - send to devlin, 2014-11-18 23:05:48]

https://codereview.chromium.org/736773002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/736773002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/handler.py:76: commit_name, commit_id =
self._request.path.split('/')
Is there some kind of validation that you can do on the commit ID? It's pretty
scary just allowing the commit ID to be set arbitrarily.

[Ken Rockot(use gerrit already), 2014-11-18 23:09:11]

Yeah, actually I thought about this too. My next patch will solve this, so
I guess I'll wait for this one (we'll have a list of the last 50 cached
commit IDs and this can validate against that list).

On Tue, Nov 18, 2014 at 3:05 PM, <kalman@chromium.org> wrote:

>
> https://codereview.chromium.org/736773002/diff/20001/
> chrome/common/extensions/docs/server2/handler.py
> File chrome/common/extensions/docs/server2/handler.py (right):
>
> https://codereview.chromium.org/736773002/diff/20001/
> chrome/common/extensions/docs/server2/handler.py#newcode76
> chrome/common/extensions/docs/server2/handler.py:76: commit_name,
> commit_id = self._request.path.split('/')
> Is there some kind of validation that you can do on the commit ID? It's
> pretty scary just allowing the commit ID to be set arbitrarily.
>
> https://codereview.chromium.org/736773002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2014-11-18 23:10:15]

On 2014/11/18 23:09:11, Ken Rockot wrote:
> Yeah, actually I thought about this too. My next patch will solve this, so
> I guess I'll wait for this one (we'll have a list of the last 50 cached
> commit IDs and this can validate against that list).

Sounds good, thanks.

[Ken Rockot(use gerrit already), 2014-11-19 00:24:37]

Here we go. I just added the history tracking bits to this CL. Also moved the
administrative servlets into their own source module for some minor cleanup.

[not at google - send to devlin, 2014-11-19 19:32:42]

Would be nice to see tests for some of these things, like, a least a test that
the server refuses to modify the commit ID if it hasn't been seen in the last 50
commits.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/admin_servlets.py (right):

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/admin_servlets.py:81: commit_name,
commit_id = self._request.path.split('/')
You might want to use .split('/', 1) so that this doesn't crash if there are
more than 1 '/' tokens.

You might also consider using one of the utilities in path.py. I think that has
something which does that, and either way, they make nice assertions about valid
paths.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/admin_servlets.py:84: return
Response.NotFound('Commit %s not found.' % commit_id)
Nit, but I don't think 404 is actually the right error code here. Maybe 400 (bad
request)? Or something else you find in
http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_Error which
sounds right.

Of course, you should add a BadRequest (or whatever) utility to Response if you
go with that.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:20: self.datetime =
datetime
Maybe mention what datetime is used for? Because I doubt it's reliably correct.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:50: history =
collections.deque([], maxlen=50)
You're sure that .deque() pickles and unpickles properly? Seems safer to use a
vanilla list just in case.

[Ken Rockot(use gerrit already), 2015-02-02 23:57:45]

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/admin_servlets.py (right):

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/admin_servlets.py:81: commit_name,
commit_id = self._request.path.split('/')
On 2014/11/19 19:32:41, kalman wrote:
> You might want to use .split('/', 1) so that this doesn't crash if there are
> more than 1 '/' tokens.
> 
> You might also consider using one of the utilities in path.py. I think that
has
> something which does that, and either way, they make nice assertions about
valid
> paths.

Done.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/admin_servlets.py:84: return
Response.NotFound('Commit %s not found.' % commit_id)
On 2014/11/19 19:32:41, kalman wrote:
> Nit, but I don't think 404 is actually the right error code here. Maybe 400
(bad
> request)? Or something else you find in
> http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_Error which
> sounds right.
> 
> Of course, you should add a BadRequest (or whatever) utility to Response if
you
> go with that.

Done. 400 seems like the only reasonable option.

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:20: self.datetime =
datetime
On 2014/11/19 19:32:41, kalman wrote:
> Maybe mention what datetime is used for? Because I doubt it's reliably
correct.

Done. FOR DISPLAY PURPOSES ONLY

https://codereview.chromium.org/736773002/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:50: history =
collections.deque([], maxlen=50)
On 2014/11/19 19:32:41, kalman wrote:
> You're sure that .deque() pickles and unpickles properly? Seems safer to use a
> vanilla list just in case.

Yar, deques support pickling

[not at google - send to devlin, 2015-02-03 18:17:20]

lgtm

https://codereview.chromium.org/736773002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/736773002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:18: |datetime| is used
as a timestamp for when the commit cache was completed,
TBH I'd just construct this here rather than passing it in, since it's only for
debugging. Up to you.

[Ken Rockot(use gerrit already), 2015-02-03 20:12:10]

https://codereview.chromium.org/736773002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/commit_tracker.py (right):

https://codereview.chromium.org/736773002/diff/60001/chrome/common/extensions...
chrome/common/extensions/docs/server2/commit_tracker.py:18: |datetime| is used
as a timestamp for when the commit cache was completed,
On 2015/02/03 18:17:20, kalman wrote:
> TBH I'd just construct this here rather than passing it in, since it's only
for
> debugging. Up to you.

We want it to be this way so we can store the datetime at which a given commit
was made live; that chronological ordering makes the debugging tool more useful.

[Ken Rockot(use gerrit already), 2015-02-03 20:12:34]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:13:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/736773002/60001

[commit-bot: I haz the power, 2015-02-03 20:14:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-03 20:16:56]

Patchset 4 (id:??) landed as
https://crrev.com/34477f1239c60bf8a7e3243ad3aa6ae893072cba
Cr-Commit-Position: refs/heads/master@{#314395}