Docserver: Pull knowledge of host file systems into a single

Docserver: Pull knowledge of host file systems into a single
HostFileSystemProvider class, basically a rename of HostFileSystemCreator with
a stricter notion of trunk vs branches.

In some ways this is a follow-up to r218556, except solving the problem in a
type-safe (rather than special-case) way, as requested in that review. I also
need it to be able to support "host" file systems as a content provider, see
bug 293678.

This patch also enforces that the "branch" of a channel is a string, and fixes a
bunch of callers, because a bug there was exposed as part of this work.

BUG=273105, 293678
R=jyasskin@chromium.org

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

[not at google - send to devlin, 2013-10-08 15:48:39]

I can have a look at submitting the branch-to-string fix separately if you want.
It was needed to make me trust this patch but it can probably be done as a
followup.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:10: 
no amount of --similarity=? will convince git cl that this is a move.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider_test.py
(right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider_test.py:5: 
ditto

[Jeffrey Yasskin, 2013-10-08 17:46:05]

https://codereview.chromium.org/26418002/diff/3001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider_test.py
(right):

https://codereview.chromium.org/26418002/diff/3001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider_test.py:27:
ObjectStoreCreator.ForTest(),
If you want git to notice moves (which is helpful for keeping the long-term
blame history, in addition to making reviews easier), try to avoid extraneous
changes in the same CL that moves things.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:23: assert
isinstance(channel, basestring), channel
Do you want basestring or str here? How are you handling unicode?

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:184: None,
Wow, this is terrible style. sorted() wants keyword arguments.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_creator.py (left):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_creator.py:1: # Copyright
2013 The Chromium Authors. All rights reserved.
Note that this file shows up as an 'M' in rietveld, rather than a D. Make sure
you `git rm`ed it?

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_iterator_test.py
(right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_iterator_test.py:1:
#!/usr/bin/env python
This change is great because you changed the setup of the test and a bunch of
details in the production code, but the actual assertions didn't change at all,
so I know you didn't change any behavior.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:10: 
On 2013/10/08 15:48:39, kalman wrote:
> no amount of --similarity=? will convince git cl that this is a move.

Half the time I try to increase the --similarity argument, but it's actually
lowering it that tries to find more moves. I've used --similarity=10 before, and
you might have to go lower if you changed a bunch of lines in addition to moving
it.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:12: class
HostFileSystemProvider(object):
What's the difference between a "Creator" and a "Provider"? The memoization?

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:39:
self._constructor_for_test = constructor_for_test
Don't do it in this change (to keep the diff small and help git notice the
move), but if you use:

  self._constructor = SubversionFileSystem.Create
  if constructor_for_test is not None:
    self._constructor = constructor_for_test

you can avoid the 'if' in _Create and make the default clearer to readers. You
can even put the default in the argument, although then you'd want to change the
name to just 'constructor' or 'filesystem_factory'.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:41: @memoize
Memoization is dangerous in long-lived servers when the data might change. The
effect here looks like it'll be to pin a single trunk HEAD revision for the life
of the server process. Is that what you want? If so, please comment it. If I've
mis-read the actual behavior, please comment that.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:42: def
GetTrunk(self, revision=None):
Please describe the meaning of 'revision' on trunk.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:51: @memoize
Branches also change over time, but maybe we don't care about having up-to-date
data about features merged into branches?

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:53:
'''Creates either SVN file systems or specialized file systems from the
This seems like it still belongs on _Create. You should document GetBranch at a
higher level.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:72:
@staticmethod
If these had been @classmethod, you wouldn't have had to change them at all when
you renamed the class:

@classmethod
def ForLocal(cls, object_store_creator):
  return cls(object_store_creator, ...)

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:78:
constructor_for_test=lambda **_: LocalFileSystem.Create())
'constructor_for_test' seems like the wrong name if you're using it outside of
tests.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/patch_servlet.py:59: # The trunk file
system for this creator should be the patched one.
"the patched one" doesn't match "unpatched_trunk_host_file_system". Which is it?

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/patched_file_system.py:42:
self._file_system = file_system
I think I'd rather "base_file_system", since it's not the file system being
exposed by this class. I agree that "host" doesn't mean anything to me.

[not at google - send to devlin, 2013-10-08 18:27:37]

I used a smaller similarity and it worked!

Now to try to figure out why it's still not D'ing those other files.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:23: assert
isinstance(channel, basestring), channel
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Do you want basestring or str here? How are you handling unicode?

Does unicode need to be handled any differently? I'm not sure whether this
stuff, when grabbed from the json, will end up unicode or not. Hopefully unicode
since JSON might have unicode but I'm not sure.

But it's moot if it doesn't matter.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:184: None,
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Wow, this is terrible style. sorted() wants keyword arguments.

Heh.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_creator.py (left):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_creator.py:1: # Copyright
2013 The Chromium Authors. All rights reserved.
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Note that this file shows up as an 'M' in rietveld, rather than a D. Make sure
> you `git rm`ed it?

It might be my trying to fiddle with similarity in git cl upload, because:

M       chrome/common/extensions/docs/server2/api_data_source_test.py
M       chrome/common/extensions/docs/server2/app.yaml
M       chrome/common/extensions/docs/server2/app_yaml_helper.py
M       chrome/common/extensions/docs/server2/app_yaml_helper_test.py
M       chrome/common/extensions/docs/server2/availability_finder_test.py
M       chrome/common/extensions/docs/server2/branch_utility.py
M       chrome/common/extensions/docs/server2/branch_utility_test.py
M       chrome/common/extensions/docs/server2/cron.yaml
M       chrome/common/extensions/docs/server2/cron_servlet.py
M       chrome/common/extensions/docs/server2/cron_servlet_test.py
D       chrome/common/extensions/docs/server2/host_file_system_creator.py
D       chrome/common/extensions/docs/server2/host_file_system_creator_test.py
M       chrome/common/extensions/docs/server2/host_file_system_iterator.py
M       chrome/common/extensions/docs/server2/host_file_system_iterator_test.py
A       chrome/common/extensions/docs/server2/host_file_system_provider.py
A       chrome/common/extensions/docs/server2/host_file_system_provider_test.py
M       chrome/common/extensions/docs/server2/instance_servlet.py
M       chrome/common/extensions/docs/server2/patch_servlet.py
M       chrome/common/extensions/docs/server2/patch_servlet_test.py
M       chrome/common/extensions/docs/server2/patched_file_system.py
M       chrome/common/extensions/docs/server2/render_servlet.py
M       chrome/common/extensions/docs/server2/server_instance.py
M       chrome/common/extensions/docs/server2/test_data/canned_data.py
M       chrome/common/extensions/docs/server2/test_servlet.py
M       chrome/common/extensions/docs/server2/test_servlet_test.py
M       tools/json_schema_compiler/memoize.py


git says it's a D.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:10: 
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> On 2013/10/08 15:48:39, kalman wrote:
> > no amount of --similarity=? will convince git cl that this is a move.
> 
> Half the time I try to increase the --similarity argument, but it's actually
> lowering it that tries to find more moves. I've used --similarity=10 before,
and
> you might have to go lower if you changed a bunch of lines in addition to
moving
> it.

hm ok I'll give it a go.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:12: class
HostFileSystemProvider(object):
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> What's the difference between a "Creator" and a "Provider"? The memoization?

the memoization is an implementation detail, though it does mean that "Creator"
is obviously wrong.

It's the job of this class to provide "host" file systems to callers in some
configuration. It makes sense for those to be memoized so that the in-memory
caches don't get reset each time. Anyway, provide, hence "provider". I think
it's a very common OO term.

I updated the comment here.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:39:
self._constructor_for_test = constructor_for_test
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Don't do it in this change (to keep the diff small and help git notice the
> move), but if you use:
> 
>   self._constructor = SubversionFileSystem.Create
>   if constructor_for_test is not None:
>     self._constructor = constructor_for_test
> 
> you can avoid the 'if' in _Create and make the default clearer to readers. You
> can even put the default in the argument, although then you'd want to change
the
> name to just 'constructor' or 'filesystem_factory'.

SGTM.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:41: @memoize
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Memoization is dangerous in long-lived servers when the data might change. The
> effect here looks like it'll be to pin a single trunk HEAD revision for the
life
> of the server process. Is that what you want? If so, please comment it. If
I've
> mis-read the actual behavior, please comment that.

that's not the level that the memoization happens at. in the head case this
means it'll always provide the same file system that tracks head, but that
doesn't mean that file system doesn't change as head changes.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:42: def
GetTrunk(self, revision=None):
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Please describe the meaning of 'revision' on trunk.

Done.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:51: @memoize
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> Branches also change over time, but maybe we don't care about having
up-to-date
> data about features merged into branches?

correct, branches change, and it would be seriously confusing if that happened
mid-cron run.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:53:
'''Creates either SVN file systems or specialized file systems from the
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> This seems like it still belongs on _Create. You should document GetBranch at
a
> higher level.

Done.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:72:
@staticmethod
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> If these had been @classmethod, you wouldn't have had to change them at all
when
> you renamed the class:
> 
> @classmethod
> def ForLocal(cls, object_store_creator):
>   return cls(object_store_creator, ...)

that sounds like a nice pattern in general, thanks.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:78:
constructor_for_test=lambda **_: LocalFileSystem.Create())
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> 'constructor_for_test' seems like the wrong name if you're using it outside of
> tests.

Local only makes sense when testing (or when running preview.py), though in this
case it's an implementation detail. Perhaps it makes sense to combine "default
trunk instance" and "constructor for test" into a more general abstraction but
it's approaching abstraction overload.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/patch_servlet.py:59: # The trunk file
system for this creator should be the patched one.
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> "the patched one" doesn't match "unpatched_trunk_host_file_system". Which is
it?

gah. thank you. it's... very odd that this didn't cause any tests to fail.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/patched_file_system.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/patched_file_system.py:42:
self._file_system = file_system
On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> I think I'd rather "base_file_system", since it's not the file system being
> exposed by this class. I agree that "host" doesn't mean anything to me.

Good point. "Base" is a useful prefix. "Host" means nothing in this context.

[Jeffrey Yasskin, 2013-10-08 20:49:38]

lgtm, but note the sorted() argument.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:23: assert
isinstance(channel, basestring), channel
On 2013/10/08 18:27:37, kalman wrote:
> On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > Do you want basestring or str here? How are you handling unicode?
> 
> Does unicode need to be handled any differently? I'm not sure whether this
> stuff, when grabbed from the json, will end up unicode or not. Hopefully
unicode
> since JSON might have unicode but I'm not sure.
> 
> But it's moot if it doesn't matter.

Unicode probably needs to be encoded before you make an http request using it,
but maybe urlfetch will handle it automatically. It's not a huge deal, but since
all of the arguments I know of are ascii, maybe it makes sense to explicitly
constrain the inputs that way. On the other hand, your profile indicates we're
actually handling a lot of unicode, so maybe tightening the assert will break a
bunch. I'm fine with either ignoring this or doing it in a separate CL.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:41: @memoize
On 2013/10/08 18:27:37, kalman wrote:
> On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > Memoization is dangerous in long-lived servers when the data might change.
The
> > effect here looks like it'll be to pin a single trunk HEAD revision for the
> life
> > of the server process. Is that what you want? If so, please comment it. If
> I've
> > mis-read the actual behavior, please comment that.
> 
> that's not the level that the memoization happens at. in the head case this
> means it'll always provide the same file system that tracks head, but that
> doesn't mean that file system doesn't change as head changes.

'k, sounds good.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:51: @memoize
On 2013/10/08 18:27:37, kalman wrote:
> On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > Branches also change over time, but maybe we don't care about having
> up-to-date
> > data about features merged into branches?
> 
> correct, branches change, and it would be seriously confusing if that happened
> mid-cron run.

Given my misunderstanding about this memoization that you corrected above, do we
need this function to take an optional |revision| also? (Not in this change, of
course.)

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/patch_servlet.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/patch_servlet.py:59: # The trunk file
system for this creator should be the patched one.
On 2013/10/08 18:27:37, kalman wrote:
> On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > "the patched one" doesn't match "unpatched_trunk_host_file_system". Which is
> it?
> 
> gah. thank you. it's... very odd that this didn't cause any tests to fail.

Write more tests! ;-)

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

https://codereview.chromium.org/26418002/diff/20001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/branch_utility.py:183: sorted_numbers =
sorted(numbers.iteritems(), key=operator.itemgetter(1))
The original call also had reversed=True.

[not at google - send to devlin, 2013-10-08 21:29:36]

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/branch_utility.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/branch_utility.py:23: assert
isinstance(channel, basestring), channel
On 2013/10/08 20:49:39, Jeffrey Yasskin wrote:
> On 2013/10/08 18:27:37, kalman wrote:
> > On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > > Do you want basestring or str here? How are you handling unicode?
> > 
> > Does unicode need to be handled any differently? I'm not sure whether this
> > stuff, when grabbed from the json, will end up unicode or not. Hopefully
> unicode
> > since JSON might have unicode but I'm not sure.
> > 
> > But it's moot if it doesn't matter.
> 
> Unicode probably needs to be encoded before you make an http request using it,
> but maybe urlfetch will handle it automatically. It's not a huge deal, but
since
> all of the arguments I know of are ascii, maybe it makes sense to explicitly
> constrain the inputs that way. On the other hand, your profile indicates we're
> actually handling a lot of unicode, so maybe tightening the assert will break
a
> bunch. I'm fine with either ignoring this or doing it in a separate CL.

I'm not sure what encoding we specify on fetches. Off the top of my head I'd say
we don't. We do parse text responses as unicode though.

As part of the ContentProvider stuff I will need to tighten up our unicode
handling, but, that will come later.

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/host_file_system_provider.py (right):

https://codereview.chromium.org/26418002/diff/8001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/host_file_system_provider.py:51: @memoize
On 2013/10/08 20:49:39, Jeffrey Yasskin wrote:
> On 2013/10/08 18:27:37, kalman wrote:
> > On 2013/10/08 17:46:05, Jeffrey Yasskin wrote:
> > > Branches also change over time, but maybe we don't care about having
> > up-to-date
> > > data about features merged into branches?
> > 
> > correct, branches change, and it would be seriously confusing if that
happened
> > mid-cron run.
> 
> Given my misunderstanding about this memoization that you corrected above, do
we
> need this function to take an optional |revision| also? (Not in this change,
of
> course.)

Strictly speaking yes. We should be making sure that no branches change
underneath us while we're reading.

Realistically, only trunk ever changes.

I will add comment.

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

https://codereview.chromium.org/26418002/diff/20001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/branch_utility.py:183: sorted_numbers =
sorted(numbers.iteritems(), key=operator.itemgetter(1))
On 2013/10/08 20:49:39, Jeffrey Yasskin wrote:
> The original call also had reversed=True.

mind blown, again, why no tests failed.

[not at google - send to devlin, 2013-10-08 21:36:39]

Committed patchset #7 manually as r227602 (presubmit successful).