Docserver: refactor Servlet, ObjectStore, and ServerInstance architecture to

Docserver: refactor Servlet, ObjectStore, and ServerInstance architecture to
simplify it and make more components of the server testable.


BUG=236600
R=cduvall@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198952

[not at google - send to devlin, 2013-05-07 18:26:18]

Hey Clark,

This is basically a replacement for https://codereview.chromium.org/14856006/
which had problems. Your note about preview.py not working properly really
helped untangle some problems. It *still* doesn't quite work but that's a
separate bug which when https://codereview.chromium.org/12521030/ is fixed will
go away.

Unfortunately it's quite invasive. I'll be emailing the people working on docs
server patches when this (or something like it) goes in, as an FYI and how to
convert patches to be compatible.

In fact I actually need this for the Subversion revision pinning (see bug linked
to in this patch; and https://codereview.chromium.org/14247024/ which I haven't
finished yet). This CL is based off that one, with the pinning stuff pulled out.
The thing is, the subversion pinning is actually blocking those
manifest/permission/intro etc patches anyway, so, I'm working hard to finish it.

Some cooler things to note here:

- the whole channel-is-toplevel-separator thing is pretty much gone. It's now
just an implementation detail of SubversionFileSystem (and a couple of other
places e.g. the branch picker). Channel (branch) separation now is achieved by
FileSystem.GetIdentity - SubversionFileSystem's implementation includes the
path, so the caches will be separated (this was very important for the revision
pinning stuff - probably the most important really).
- no more ObjectStoreCreator.Factory! Just ObjectStoreCreator. That always
bothered me.
- no more ServerInstance.Factory either. It's now just a bundle of state, and
it's up to Servlets (or tests) to populate it as necessary.

Some less cool things to note:
- all the Delegates in the Servlet classes. I've been vainly trying to achieve
everything with constructor dependency injection but it's just not working
cleanly for the servlets. They're quite hard to test. LMK if you see some
overdesign there, there probably is.
- the number of TODOs I added in this patch.
- sheesh, crons are slow. I realised a design flaw (your original cron
implementation was in many ways better) in that *rendering* these is very slow.
The new handlebar.py will make this much better, but better still would be for
data sources to declare what they need to be cron'd. I have a plan for this -
involving a slight rework of how DataSources work (including making it an
interface) - but that will come later.

Sorry again. This will be my last sweeping change I think.

[cduvall, 2013-05-08 03:09:14]

I do think the servlet + delegate architecture might be a little overkill. I'm
still thinking about what would be better besides just injecting the
dependencies the delegates are responsible for in the servlet constructors.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/cron_servlet.py:125: # Render all of the
publically accessible files.
publically -> publicly

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/file_system.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/file_system.py:71: namespace their caches.
The will usually depend on the configuration of that
The -> This

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/handler.py:31: path = path.split('/')
this is a little confusing reassigning the list of path parts to the path var

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/mock_file_system_test.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/mock_file_system_test.py:32: class
TestFileSystemTest(unittest.TestCase):
MockFileSystemTest?

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/object_store_creator.py:68: if component
is not None and (component.find('&') != -1 or
use:
'&' in component or '=' in component

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_branch_utility.py (left):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:1: #!/bin/bash
good job git

[not at google - send to devlin, 2013-05-08 14:53:47]

Thanks Clark.

> I do think the servlet + delegate architecture might be a little overkill. I'm
> still thinking about what would be better besides just injecting the
> dependencies the delegates are responsible for in the servlet constructors.

Quick note here before I go through this review: the Delegate stuff sort of is
like injecting dependencies in the constructor, Delegates are just a more
specific-to-the-class sort of thing, which I don't really like because it
doesn't have a clearly defined identity. If that makes sense. Not sure it does.

Anyway, I tried quite hard to design it like an independent entity (like a
Factory) - in fact, I originally had it so that all of the Servlets just  took
ServerInstances or Factories of them in their constructor (in the case of
CronServlet) - but then the tests weren't particularly meaningful (particularly
for InstanceServlet, where all of its divergent logic from RenderServlet is in
how it constructs a ServerInstance).

Delegate is actually a Chromium concept, I don't really like the name, maybe I
should call it Dependencies or RuntimeDependencies at least.

The other option would be to use ConfigureFakeFetchers, but I'd like to try to
confine that to the integration tests for obvious reasons.

There may be a better abstraction than what I'm doing with Servlets, kind of
what I was hoping for, but still got nothing.

[not at google - send to devlin, 2013-05-08 18:26:19]

Ok, fixed the redirect problem I mentioned in IRC. I needed to add a host field
to Request.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/cron_servlet.py:125: # Render all of the
publically accessible files.
On 2013/05/08 03:09:14, cduvall wrote:
> publically -> publicly

Done.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/file_system.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/file_system.py:71: namespace their caches.
The will usually depend on the configuration of that
On 2013/05/08 03:09:14, cduvall wrote:
> The -> This

Done.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/handler.py:31: path = path.split('/')
On 2013/05/08 03:09:14, cduvall wrote:
> this is a little confusing reassigning the list of path parts to the path var

Oops. Done. Moving these back into instance methods.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/mock_file_system_test.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/mock_file_system_test.py:32: class
TestFileSystemTest(unittest.TestCase):
On 2013/05/08 03:09:14, cduvall wrote:
> MockFileSystemTest?

Done.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/mock_file_system_test.py:32: class
TestFileSystemTest(unittest.TestCase):
On 2013/05/08 03:09:14, cduvall wrote:
> MockFileSystemTest?

Done.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/object_store_creator.py (right):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/object_store_creator.py:68: if component
is not None and (component.find('&') != -1 or
On 2013/05/08 03:09:14, cduvall wrote:
> use:
> '&' in component or '=' in component

Done.

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/test_branch_utility.py (left):

https://codereview.chromium.org/15009006/diff/6001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_branch_utility.py:1: #!/bin/bash
On 2013/05/08 03:09:14, cduvall wrote:
> good job git

Reckon. I'll add a class comment to try to trick it.

[cduvall, 2013-05-08 18:32:57]

lgtm

https://codereview.chromium.org/15009006/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://codereview.chromium.org/15009006/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/render_servlet.py:57: return
Response.Redirect('/' + (
Use '/%s' % instead?

[not at google - send to devlin, 2013-05-08 18:44:49]

Committed patchset #4 manually as r198952 (presubmit successful).

[not at google - send to devlin, 2013-05-08 18:54:16]

https://codereview.chromium.org/15009006/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/render_servlet.py (right):

https://codereview.chromium.org/15009006/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/render_servlet.py:57: return
Response.Redirect('/' + (
On 2013/05/08 18:32:57, cduvall wrote:
> Use '/%s' % instead?

Done.

[not at google - send to devlin, 2013-05-08 19:14:45]

Damn forgot to rename Delegate. Oh well.