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

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

+# 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.
+
+import logging
+import tarfile
+from StringIO import StringIO
+
+from file_system import FileSystem, FileNotFoundError, StatInfo, ToUnicode
+from future import Future
+
+# Use a special value other than None to represent a deleted file in the patch.
+_FILE_NOT_FOUND_VALUE = (None,)
+
+class _AsyncFetchFuture(object):
+  def __init__(self,
+               base_path,
+               paths,
+               cached_files,
+               missing_paths,
+               binary,
+               patch_data,
+               fetcher,
+               object_store):
+    self._base_path = base_path
+    self._paths = paths
+    self._cached_value = cached_files
+    self._missing_paths = missing_paths
+    self._binary = binary
+    self._patch_data = patch_data
+    self._object_store = object_store
+    if missing_paths is not None:
+      self._tarball = fetcher.FetchAsync('tarball/%s/%s' %
+          (patch_data.issue, patch_data.patchset))
+
+  def _GetMissingPaths(self):
+    tarball_result = self._tarball.Get()
+    if tarball_result.status_code != 200:
+      raise FileNotFoundError('Failed to download tarball.')
+
+    try:
+      tar = tarfile.open(fileobj=StringIO(tarball_result.content))
+    except tarfile.TarError as e:
+      raise FileNotFoundError('Invalid tarball.')
+
+    self._uncached_value = {}
+    for path in self._patch_data.files:
+      if self._cached_value.get(path) is not None:
+        continue
+
+      if self._base_path:
+        tar_path = 'b/%s/%s' % (self._base_path, path)
+      else:
+        tar_path = 'b/%s' % path
+      try:
+        patched_file = tar.extractfile(tar_path)
+        data = patched_file.read()
+        patched_file.close()

[not at google - send to devlin, 2013/04/29 16:56:21]

you may be able to do

with tar.extractfile(tar_path) as patched_file:
  data = patched_file.read()

(no close())

[方觉(Fang Jue), 2013/04/30 14:25:08]

It ended up with AttributeError: __exit__.

+      except tarfile.TarError as e:
+        self._uncached_value[path] = _FILE_NOT_FOUND_VALUE
+        continue
+
+      # Deleted files still exist in the tarball, but they are empty.
+      if len(data) == 0:
+        self._uncached_value[path] = _FILE_NOT_FOUND_VALUE
+      elif self._binary:
+        self._uncached_value[path] = data
+      else:
+        self._uncached_value[path] = ToUnicode(data)
+
+    self._object_store.SetMulti(self._uncached_value)
+
+    for path in self._missing_paths:
+      if self._uncached_value.get(path) is None:
+        raise FileNotFoundError('File %s was not found in the patch.' % path)
+      self._cached_value[path] = self._uncached_value[path]
+
+  def Get(self):
+    if self._missing_paths is not None:
+      self._GetMissingPaths()
+
+    # Make sure all paths exist before returning.
+    for path in self._paths:
+      if self._cached_value[path] == _FILE_NOT_FOUND_VALUE:
+        raise FileNotFoundError('File %s was deleted in the patch.' % path)
+    return self._cached_value
+
+class RietveldFileSystem(FileSystem):
+  ''' Class to fetch resources from a patchset in Rietveld.
+  '''
+  def __init__(self,
+               base_path,
+               patch_data,
+               fetcher,
+               object_store_creator_factory):
+    self._base_path = base_path
+    self._patch_data = patch_data
+    self._fetcher = fetcher
+    self._object_store = object_store_creator_factory.Create(
+        RietveldFileSystem).Create()
+
+  def Read(self, paths, binary=False):
+    cached_files = self._object_store.GetMulti(paths).Get()
+    missing_paths = list(set(paths) - set(cached_files.keys()))
+    if len(missing_paths) == 0:
+      missing_paths = None
+    return Future(delegate=_AsyncFetchFuture(
+        self._base_path,
+        paths,
+        cached_files,
+        missing_paths,
+        binary,
+        self._patch_data,
+        self._fetcher,
+        self._object_store))

[not at google - send to devlin, 2013/04/29 16:38:49]

I'm confused what the role of PatchedFileSystem vs RietveldFileSystem is. By its
name PatchedFileSystem seems like a generic class which applies some patch to
the output of an existing file system then serves it. However, it has Rietveld
specific knowledge. This class obviously also has Rietveld knowledge and it
seems the division is semi arbitrary.

Here is an architecture that I'd prefer: and as a preface, I think that we need
to do as we discussed earlier and not re-create these for each patchset. It
feels a bit off the way it's happening at the moment.

PatchedFileSystem - like I mention above, take the Rietveld knowledge out of it.
It takes a file system and some way of patching files, then serves those patched
files. The "way of patching files" is just a simple interface which takes a path
and file contents, then returns new file contents.

class Patcher(object):
  class Patch(object):
    def Apply(self, file_contents):
      '''Returns |file_contents| with its contents patched
      against |self|.
      '''
      raise NotImplemented()
    def GetVersion(self):
      raise NotImplemented()

  def GetPatches(self, paths):
    '''Returns a Future Patch for each paths.
    Implemented this way so that we can do parallel fetches.
    '''
    raise NotImplemented()
  def GetPatch(self, path):
    # implemented against GetPatches

class PatchedFileSystem(FileSystem):
  def __init__(self, file_system, patcher):
    self._file_system = file_system
    self._patcher = patcher

  def Read(self, paths):
    # equivalent to:
    return self._patcher.GetPatches(paths).Apply(
        self.file_system.Read(paths))

  def Stat(self, path):
    # equivalent to:
    return self._file_system.Stat(path) +
           self._patcher.GetPatch(path).GetVersion())
    # though + won't quite work obviously.

then the main implementation of FilePatcher becomes RietveldPatcher (with all of
the logic you've already written) and we can write a TestPatcher for testing.

Sorry to keep on turning you around like this. Once I submit that servlet patch
I think it'll become clearer what the architecture should be.

[方觉(Fang Jue), 2013/04/30 01:07:11]

Perhaps just combine RietveldFileSystem and PatchedFileSystem into one
PatchedFileSystem?