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

chrome/common/extensions/docs/server2/rietveld_patcher.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 json
+import logging
+import tarfile
+from datetime import datetime, timedelta
+from StringIO import StringIO
+
+from file_system import FileNotFoundError, ToUnicode
+from future import Future
+from patched_file_system import Patcher
+import svn_constants
+
+# Use a special value other than None to represent a deleted file in the patch.
+_FILE_NOT_FOUND_VALUE = (None,)

[not at google - send to devlin, 2013/05/03 19:12:22]

This is a bit messy, can you store the deleted files in a separate set?

+
+_ISSUE_CACHE_MAXAGE = timedelta(minutes=5)
+
+_CHROMIUM_REPO_BASEURLS = [
+  'https://src.chromium.org/svn/trunk/src/',
+  'http://src.chromium.org/svn/trunk/src/',
+  'svn://svn.chromium.org/chrome/trunk/src',
+  'https://chromium.googlesource.com/chromium/src.git@master',
+  'http://git.chromium.org/chromium/src.git@master',
+]
+
+_DOCS_PATHS = [
+  svn_constants.API_PATH,
+  svn_constants.TEMPLATE_PATH,
+  svn_constants.STATIC_PATH
+]
+
+''' Append @patchset for keys to distinguish between different patchsets of an
+issue.
+'''
+def _MakeKey(path_or_paths, patchset):
+  if isinstance(path_or_paths, list):
+    result = []
+    for p in path_or_paths:
+      result.append('%s@%s' % (p, patchset))
+    return result

[not at google - send to devlin, 2013/05/03 19:12:22]

use a list comprehension

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+  else:

[not at google - send to devlin, 2013/05/03 19:12:22]

previous statement is a return, don't need an else:

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+    return _MakeKey([path_or_paths], patchset)[0]
+
+def _ToObjectStoreValue(value, patchset):
+  result = {}
+  for key in value:
+    result[_MakeKey(key, patchset)] = value[key]
+  return result

[not at google - send to devlin, 2013/05/03 19:12:22]

comprehension here too

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+
+def _FromObjectStoreValue(value):
+  result = {}

[not at google - send to devlin, 2013/05/03 19:12:22]

and here

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+  for key in value:
+    result[key[0:key.find('@')]] = value[key]
+  return result
+
+class _AsyncFetchFuture(object):
+  def __init__(self,
+               base_path,
+               paths,
+               cached_files,
+               missing_paths,
+               binary,
+               issue,
+               patchset,
+               patched_files,
+               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._issue = issue
+    self._patchset = patchset
+    self._files = []
+    for files in patched_files:
+      self._files += files
+    self._object_store = object_store
+    if missing_paths is not None:
+      logging.info('Fetching tarball/%s/%s' % (issue, patchset))
+      self._tarball = fetcher.FetchAsync('tarball/%s/%s' % (issue, patchset))
+
+  def _GetMissingPaths(self):
+    tarball_result = self._tarball.Get()
+    if tarball_result.status_code != 200:
+      raise FileNotFoundError(
+          'Failed to download tarball for issue %s patchset %s. Status: %s' %
+          (self._issue, self._patchset, tarball_result.status_code))
+

[not at google - send to devlin, 2013/05/03 19:12:22]

It would actually be nice to show a more informative page than a 404, such as
"patchset not found", maybe raise a different exception to catch in
PatchServlet. Can do that in a follow-up though.

+    try:
+      tar = tarfile.open(fileobj=StringIO(tarball_result.content))
+    except tarfile.TarError as e:
+      raise FileNotFoundError('Invalid tarball for issue %s patchset %s.' %
+                              (self._issue, self._patchset))
+
+    self._uncached_value = {}
+    for path in self._files:
+      if self._cached_value.get(path) is not None:

[not at google - send to devlin, 2013/05/03 19:12:22]

"path in self._cached_value"?

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+        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()
+      # In the unlikely case that the tarball is corrupted, throw
+      # FileNotFoundError instead of 500 Internal Server Error.
+      except tarfile.TarError as e:
+        raise FileNotFoundError(
+            'Error extracting tarball for issue %s patchset %s.' %
+            (self._issue, self._patchset))
+      finally:
+        patched_file.close()
+
+      # Deleted files still exist in the tarball, but they are empty.
+      if len(data) == 0:
+        # Mark it empty instead of throwing FileNotFoundError here to make sure
+        # self._object_store.SetMulti below is called and all files read are
+        # cached.
+        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(_ToObjectStoreValue(self._uncached_value,
+                                                    self._patchset))
+
+    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 RietveldPatcher(Patcher):
+  ''' Class to fetch resources from a patchset in Rietveld.
+  '''
+  def __init__(self,
+               base_path,
+               issue,
+               fetcher,
+               object_store_creator_factory):
+    self._base_path = base_path
+    self._issue = issue
+    self._fetcher = fetcher
+    self._object_store = object_store_creator_factory.Create(
+        RietveldPatcher).Create()

[not at google - send to devlin, 2013/05/03 19:12:22]

For the _FILE_NOT_FOUND_VALUE stuff,

self._modified_object_store = oscf.Create(RP).Create(category='modified')
self._removed_object_store = ...('removed')
self._added_object_store = ...('added')

it seems like trying to put them in the same place is causing a bit of
unnecessary complication, particularly with the removed (not sure about added).
it would be easier to reason about if they were separated.

+
+  def GetVersion(self):

[not at google - send to devlin, 2013/05/03 19:12:22]

No longer used, can delete this.

+    return self._GetPatchset()
+
+  def _GetPatchset(self):
+    value = self._object_store.Get(self._issue).Get()
+    if value is not None:
+      patchset, time = value
+      if datetime.now() - time < _ISSUE_CACHE_MAXAGE:

[not at google - send to devlin, 2013/05/03 19:12:22]

We should implement the caching with a CachingPatcher that delegates to another
one. That will make this file targeted to just fetching to rietveld, and
implementation details like caching elsewhere.

Another thing that would let you do - clear that cache on every request that
goes into PatchServlet. I don't think that would greatly hurt performance, and
it would be more useful of a feature if the patch updated immediately.

[方觉(Fang Jue), 2013/05/04 02:05:41]

Maybe GetPatchset without cache when constructing RietveldPatcher? List of files
in a patchset and their content cannot change after a patchset is uploaded.

[not at google - send to devlin, 2013/05/04 06:47:43]

but new patches can be uploaded. That part needs to be as quick as possible.
(not sure I understand what you're saying).

+        return patchset
+
+    try:
+      issue_json = json.loads(self._fetcher.Fetch(
+          'api/%s' % self._issue).content)
+    except Exception as e:
+      return None
+
+    if issue_json.get('closed'):
+      return None
+
+    patchsets = issue_json.get('patchsets')
+    if not isinstance(patchsets, list) or len(patchsets) == 0:
+      return None
+
+    if not issue_json.get('base_url') in _CHROMIUM_REPO_BASEURLS:
+      return None
+
+    patchset = str(patchsets[-1])
+    self._object_store.Set(self._issue, (patchset, datetime.now()))
+    return patchset
+
+  def GetPatchedFiles(self):
+    patchset = self._GetPatchset()
+    object_store_key = '@%s' % patchset
+    empty = ([], [], [])

[not at google - send to devlin, 2013/05/03 19:12:22]

having this as a variable is a bit less readable than inlining IMO

[方觉(Fang Jue), 2013/05/04 02:05:41]

Done.

+    patched_files = self._object_store.Get(object_store_key).Get()

[not at google - send to devlin, 2013/05/03 19:12:22]

you could also store the list of files in a separate object store. Implementing
these categories via key conventions can get out of control and hard to follow.

In fact I'm finding it hard to follow already, have like a
patched_file_object_store or somethign.

+    if patched_files is not None:
+      return patched_files
+
+    try:
+      patchset_json = json.loads(self._fetcher.Fetch(
+          'api/%s/%s' % (self._issue, patchset)).content)
+    except Exception as e:
+      return empty
+
+    files = patchset_json.get('files')
+    if files is None or not isinstance(files, dict):
+      return empty
+
+    added = []
+    deleted = []
+    modified = []
+    for key in files:
+      f = key.split(self._base_path + '/', 1)[1]
+      if any(f.startswith(path) for path in _DOCS_PATHS):
+        status = (files[key].get('status') or 'M').strip()
+        if status == 'A':
+          added.append(f)
+        elif status == 'D':
+          deleted.append(f)
+        else:
+          modified.append(f)
+
+    patched_files = (added, deleted, modified)
+    self._object_store.Set(object_store_key, patched_files)
+    return patched_files
+
+  def Apply(self, paths, file_system, binary=False):
+    patchset = self._GetPatchset()
+    cached_files = _FromObjectStoreValue(
+        self._object_store.GetMulti(_MakeKey(paths, patchset)).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._issue,
+        patchset,
+        self.GetPatchedFiles(),
+        self._fetcher,
+        self._object_store))