Docserver: Add support for viewing docs with a codereview patch applied

chrome/common/extensions/docs/server2/patched_file_system.py

Index: chrome/common/extensions/docs/server2/patched_file_system.py
===================================================================
--- chrome/common/extensions/docs/server2/patched_file_system.py	(revision 0)
+++ chrome/common/extensions/docs/server2/patched_file_system.py	(revision 0)
@@ -0,0 +1,122 @@
+# Copyright 2013 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from file_system import FileSystem, StatInfo, FileNotFoundError
+from future import Future
+import logging
+
+class _AsyncFetchFuture(object):
+  def __init__(self,
+               svn_files_future,
+               patched_files_future,
+               svn_dirs_future,
+               dir_paths,
+               patcher):
+    self._svn_files_future = svn_files_future
+    self._patched_files_future = patched_files_future
+    self._svn_dirs_future = svn_dirs_future
+    self._dir_paths = dir_paths
+    self._patcher = patcher
+
+  def Get(self):
+    value = self._svn_files_future.Get()
+    value.update(self._patched_files_future.Get())
+    dirs_value = self._svn_dirs_future.Get()

[not at google - send to devlin, 2013/04/30 15:37:42]

maybe call these files/dirs rather than generic value/dirs_value.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+    value.update({path: self._patcher.PatchDirectoryListing(path,
+                                                            dirs_value[path])
+                  for path in self._dir_paths})
+    return value
+
+class Patcher(object):

[not at google - send to devlin, 2013/04/30 15:37:42]

I think it would be nicer to have this in a separate file, it's pretty meaty -
and the idea was to have Patcher be as dumb as possible so that unit testing
PatchedFileSystemis as meaningful as possible. There's quite a lot of logic in
here.

I mean - I guess it's ok, you'd just need to separately unit-test Patcher.

+  # Returns (added_files, deleted_files, modified_files).
+  def GetPatchedFiles(self):

[not at google - send to devlin, 2013/04/30 15:37:42]

comments should be like:

def GetPatchedFiles(self):
  '''Returns (added_files, deleted_files, modified_files).
  '''

same for others.

And please document all of the other public methods here.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+    raise NotImplementedError()
+
+  # Returns patch version.
+  def GetVersion(self):
+    raise NotImplementedError()
+
+  # Apply the patch to files. Returns Future with patched data.
+  def Apply(self, paths, file_system, binary):
+    raise NotImplementedError()
+
+  def GetPatchedChildrenInPath(self, path):
+    assert path.endswith('/')
+    def _FindChildrenInPath(files, path):
+      result = []
+      for f in files:
+        if f.startswith(path):

[not at google - send to devlin, 2013/04/30 15:37:42]

something like:

result = [f[len(path):] for f in files if f.startswith(path)]

except I don't understand the "if '/' in f: f = f.split('/, 1)[0] + '/'" line.

I presume this whole method is suppose to list the patched files underneath
|path|? Perhaps GetPatchedFilesInDir would be better, call |path| |dir| instead.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Suppose a patch contains:
c/c/e/docs/templates/public/apps/manifest/name.html
c/c/e/docs/templates/public/apps/runtime.html
...
When calling GetPatchedFilesInDir('c/c/e/docs/templates/public/apps/'),
it should return ['manifest/', 'runtime.html', ...] instead of
'manifest/name.html'.

It turned out that dir is a built-in function.

+          f = f.split(path, 1)[1]
+          if '/' in f:
+            f = f.split('/', 1)[0] + '/'
+          result.append(f)
+      return result
+
+    added_files, deleted_files, modified_files = (
+        tuple(_FindChildrenInPath(files) for files in self.GetPatchedFiles()))

[not at google - send to devlin, 2013/04/30 15:37:42]

this method doesn't appear to return anything..?

[方觉(Fang Jue), 2013/05/01 15:27:25]

oops.

+
+  def PatchDirectoryListing(self, path, value):

[not at google - send to devlin, 2013/04/30 15:37:42]

s/value/original_dir_listing/ or similar.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+    added, deleted, modified = self.GetPatchedChildrenInPath(path)
+    return list(set(value) + set(added) - set(deleted))
+
+  def PatchStat(self, path, stat_info):
+    version = self.GetVersion()

[not at google - send to devlin, 2013/04/30 15:37:42]

when would GetVersion return None? can we just guarantee that it's never None?

[方觉(Fang Jue), 2013/05/01 15:27:25]

When it fails to fetch issue data from Rietveld or when the patch doesn't
contain changes to extension docs.

+    if version is None:
+      return stat_info
+    version = 'patched_' + version

[not at google - send to devlin, 2013/04/30 15:37:42]

This kind of logic is FileSystem specific, doesn't belong in Patcher. I looks
like the PatchDirectoryListing/PatchStat methods belong in PatchedFileSystem.

Incidentally, for more complex string building (e.g really anything beyond
appending a '/') it's nice to use 'patched_%s' % version.

[方觉(Fang Jue), 2013/05/01 15:27:25]

It seems that only the three NotImplemented methods should belong to Patcher.
The other three are specifically used by FileSystem to patch directory listing
and stat, which are FileSystem's implementation details.
Sorry about that.

+    directory = path.rsplit('/', 1)[0]
+    added, deleted, modified = GetPatchedChildrenInPath(directory + '/')
+    existing_children = added + modified
+

[not at google - send to devlin, 2013/04/30 15:37:42]

It's a bit unclear to me how the logic below relates to patching a stat. It's a
bit indirect with conditions that aren't clear - e.g. like I mentioned, it seems
like stat_info can be None right at the bottom if this existing_children thing
is empty.

I would imagine it might look something more like:

if path in deleted:
  raise FileNotFoundException(...)

if path in added:
  assert stat_info is None
  if path.endswith('/'):
    child_versions = ...
    return StatInfo(version, child_versions=child_versions)
  return StatInfo(version)

assert path in modified
... etc

+    # path is added in the patch
+    if stat_info is None and len(existing_children) != 0:

[not at google - send to devlin, 2013/04/30 15:37:42]

how can existing_children be non-empty if the path is added in the patch? 

Actually, perhaps this is my misconception of what "existing_children" means,
since it's assigned to (added + modified) - it seems like it's actually the
children *after* the patch has been applied, children_after_patch or something?

+      if path.endswith('/'):
+        stat_info = StatInfo(version,
+                             {child: version for child in existing_children})

[not at google - send to devlin, 2013/04/30 15:37:42]

see comment at the end of this method. might as well just return the StatInfo
directly from here.

+      else:
+        filename = path.rsplit('/', 1)[-1]

[not at google - send to devlin, 2013/04/30 15:37:42]

perhaps assign this at the same time you assign directory i.e.

filename, directory = path.rsplit('/', 1)

+        if filename in existing_children:
+          stat_info = StatInfo(version,
+                               {child: version for child in existing_children})
+    else:
+      if len(existing_children) > 0:

[not at google - send to devlin, 2013/04/30 15:37:42]

you're using > 0 here and != 0 above - nice to be consistent. > 0 is probably
more idiomatic.

+        stat_info.version = version
+        stat_info.child_versions.update(
+            {child: version for child in existing_children})
+      elif len(deleted) > 0:
+        stat_info.version = version

[not at google - send to devlin, 2013/04/30 15:37:42]

It looks like stat_info can be None here, if existing_children is empty?

+
+      return stat_info

[not at google - send to devlin, 2013/04/30 15:37:42]

indentation looks incorrect here, the "if stat_info is None and
len(existing_children != 0" never returns a value.

+
+class PatchedFileSystem(FileSystem):
+  ''' Class to fetch resources with a patch applied.
+  '''
+  def __init__(self, svn_file_system, patcher):
+    self._svn_file_system = svn_file_system
+    self._patcher = patcher
+
+  def Read(self, paths, binary=False):
+    patched_files = []
+    for files in self._patcher.GetPatchedFiles():
+      patched_files += files

[not at google - send to devlin, 2013/04/30 15:37:42]

patched_files = set(self._patcher.GetPatchedFiles())?

[方觉(Fang Jue), 2013/05/01 15:27:25]

It doesn't work.

+    dir_paths = [path for path in paths if path.endswith('/')]

[not at google - send to devlin, 2013/04/30 15:37:42]

assign directly into a set here? I suspect that file_system.Read works with a
set as well as a list, and so does the _AsyncFetchFuture stuff.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+    file_paths = list(set(paths) - set(dir_paths))
+    patched_paths = list(set(file_paths) & set(patched_files))

[not at google - send to devlin, 2013/04/30 15:37:42]

looks like you're converting from a set to a list here, then in both times it's
used back into a set.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+    unpatched_paths = list(set(file_paths) - set(patched_files))
+    return Future(delegate=_AsyncFetchFuture(
+        self._svn_file_system.Read(unpatched_paths, binary),
+        self._patcher.Apply(patched_paths, self._svn_file_system, binary),
+        self._svn_file_system.Read(dir_paths, binary),
+        dir_paths,
+        self._patcher))
+
+  def Stat(self, path):
+    try:
+      stat_info = self._svn_file_system.Stat(path)
+    except FileNotFoundError as e:

[not at google - send to devlin, 2013/04/30 15:37:42]

The unfortunate part of writing it like this is that Python is silly and doesn't
preserve stack traces when you re-throw errors - and stack traces *really* help
when trying to debug failures.

So, try to write this in such a way that this call ends up falling through to a
FileNotFoundError - e.g. first check whether this file is being added, and if
it's not then grab the stat (let FileNotFoundError fall through).

It would actually make sense to me to move the logic in PatchStat into here, as
I mentioned.

[方觉(Fang Jue), 2013/05/01 15:27:25]

Done.

+      stat_info = None
+    stat_info = self._patcher.PatchStat(path, stat_info)
+    if stat_info is None:
+      raise FileNotFoundError('Path %s was not found' % path)
+    return stat_info
Property changes on: chrome/common/extensions/docs/server2/patched_file_system.py
___________________________________________________________________
Added: svn:eol-style
   + LF