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 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/04/30 15:37:42]

i usually use _FILE_NOT_FOUND_VALUE = object(), and then to check whether
something is that value use "is" not ==.

That said, why do you need this rather than throwing FileNotFoundErrors at the
time it's assigned? I don't see much difference.

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

Well, object() is not object() and object() != object(). (None,) == (None,) but
(None,) is not (None,).

See the comment in the latest patchset for the reason.

+
+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.INTRO_PATH,
+  svn_constants.ARTICLE_PATH,
+  svn_constants.PUBLIC_TEMPLATE_PATH,
+  svn_constants.PRIVATE_TEMPLATE_PATH,
+  svn_constants.JSON_PATH,
+  svn_constants.STATIC_PATH
+]
+RIETVELD_ISSUE_JSON = 'api/%s'
+RIETVELD_PATCHSET_JSON = 'api/%s/%s'
+
+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._files = []
+    for files in patched_files:
+      self._files += files
+    self._object_store = object_store
+    if missing_paths is not None:
+      print 'Fetching tarball/%s/%s' % (issue, patchset)

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

use logging.info. I think it's a useful log to have, at least for now.

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

Done.

+      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.')

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

include status code?

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

Done.

+
+    try:
+      tar = tarfile.open(fileobj=StringIO(tarball_result.content))
+    except tarfile.TarError as e:
+      raise FileNotFoundError('Invalid tarball.')

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

include some data about the exception?

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

Done.

+
+    self._uncached_value = {}
+    for path in self._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/30 15:37:42]

in lieu of the "with..." thing, patched_file.close() should go in a finally:
block after the exception.

also - what exactly can fail here? the extractfile or the read(), or both? how
can they fail? should one of the failures be converted into a FileNotFoundError?
write a brief comment for people like me who shouldn't need to dig up the python
library reference.

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

Done.

+      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 RietveldPatcher(Patcher):

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

We need a RietveldPatcherTest, which should use the fake data you've checked in.

(and as I mentioned, PatchedFileSystemTest should just set up fake stuff, not
rely on that rietveld data since it's more abstract than that)

+  ''' 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()
+    self._object_store_creator_factory = object_store_creator_factory
+
+  def GetVersion(self):
+    return self._GetPatchset()

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

right - I perhaps make this

return self._GetPatchset() or "0"

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

No. PatchedFileSystem interprets GetVersion() is None as nothing to patch. I
added a comment there.

+
+  def _GetPatchset(self):
+    key = '@%s' % self._issue
+    patchset = self._object_store.Get(key).Get()

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

this is what category= is for in the Create() method of
object_store_creator_factory.

+    if patchset is not None:
+      return patchset
+
+    try:
+      issue_json = json.loads(self._fetcher.Fetch(
+          RIETVELD_ISSUE_JSON % 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(key, patchset)
+    return patchset
+
+  def GetPatchedFiles(self):
+    patchset = self._GetPatchset()
+    key = '@%s.%s' % (self._issue, patchset)

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

We need a consistent scheme for this - it looks like you're using just the issue
above, but the issue + patchset here. Maybe they're two separate object stores,
see comment above.

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

In the latest patchset, GetPatchset uses an object store created in a way
similar to BranchUtility.
GetPatchedFiles's cache is shared with files' data.

+    empty = ([], [], [])
+    patched_files = self._object_store.Get(key).Get()
+    if patched_files is not None:
+      return patched_files
+
+    try:
+      patchset_json = json.loads(self._fetcher.Fetch(
+          RIETVELD_PATCHSET_JSON % (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 (f.startswith(svn_constants.DOCS_PATH) or
+          f.startswith(svn_constants.API_PATH)):
+        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(key, patched_files)
+    return patched_files
+
+  def Apply(self, paths, file_system, 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
+    patchset = self._GetPatchset()
+    category = ''
+    for c in patchset:
+      if c.isdigit():
+        category += 'abcdefghij'[int(c)]
+      else:
+        category += c
+    return Future(delegate=_AsyncFetchFuture(
+        self._base_path,
+        paths,
+        cached_files,
+        missing_paths,
+        binary,
+        self._issue,
+        patchset,
+        self.GetPatchedFiles(),
+        self._fetcher,
+        self._object_store_creator_factory.Create(RietveldPatcher).

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

What is this for? I don't quite understand, but it looks like a misuse of
category. If files change then delete or update them in object store, though
you'd need to keep a list around or something.

Anyway, as a general rule, only Create these in the constructor.

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

Yes. It's too weird. I decide not to create separate object store for different
patchset. Instead, include patchset in keys for patched files' data so that data
from different patchsets won't conflict with each other.

+            Create(category)))