add GCS support to docs server

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

Index: chrome/common/extensions/docs/server2/gcs_file_system.py
diff --git a/chrome/common/extensions/docs/server2/gcs_file_system.py b/chrome/common/extensions/docs/server2/gcs_file_system.py
new file mode 100644
index 0000000000000000000000000000000000000000..b8079cf6b4d897d49cb1bbf7a6d04356a8a78e37
--- /dev/null
+++ b/chrome/common/extensions/docs/server2/gcs_file_system.py
@@ -0,0 +1,111 @@
+# Copyright (c) 2014 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.
+

[Jeffrey Yasskin, 2014/01/29 01:08:08]

Please comment that this is mostly documented in gcs_file_system_provider.py.

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+from third_party.cloudstorage import cloudstorage_api
+from third_party.cloudstorage import common
+from third_party.cloudstorage import errors
+
+from docs_server_utils import StringIdentity
+from file_system import FileSystem, FileNotFoundError, StatInfo
+from future import Gettable, Future
+
+import logging
+
+def _ReadFile(filename):
+  try:
+    with cloudstorage_api.open(filename, 'r') as f:
+      return f.read()
+  except errors.Error as e:
+    raise FileNotFoundError('Read failed for %s: %s' % (filename, e))

[Jeffrey Yasskin, 2014/01/29 01:08:08]

Usually traceback.format_exc() is a better context string than str(e).

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

[Jeffrey Yasskin, 2014/02/03 22:12:56]

Please do this everywhere you're currently using %s on an exception object.

+
+def _ListDir(dir_name):
+  all_files = []
+  try:
+    files = cloudstorage_api.listbucket(dir_name)
+  except errors.Error as e:
+    raise FileNotFoundError('cloudstorage.listbucket failed for %s: %s' %
+                            (dir_name, e))
+  for os_path in files:

[Jeffrey Yasskin, 2014/01/29 01:08:08]

Maybe just "return [os_path.filename for os_path in files]"?

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+    all_files.append(os_path.filename)
+  return all_files
+
+def _CreateStatInfo(bucket, path):
+  bucket = "/%s" % bucket
+  full_path = '/'.join( (bucket, path.lstrip('/')) )
+  try:
+    if full_path.endswith('/'):
+      full_path = full_path.rstrip('/');

[Jeffrey Yasskin, 2014/01/29 01:08:08]

"In this mode, the "path_prefix" argument should end in the delimiter specified"
implies you shouldn't strip the trailing '/'.

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+      child_versions = dict()
+      version = 0
+      for _file in cloudstorage_api.listbucket(full_path, delimiter="/"):

[Jeffrey Yasskin, 2014/01/29 01:08:08]

Be consistent about ' vs ". In this file, it looks like ' is more common.

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+        if not _file.is_dir:
+          # GCS doesn't have metadata for dirs
+          child_stat = cloudstorage_api.stat('%s' % _file.filename).st_ctime
+          filename = _file.filename[len(bucket)+1:]
+          child_versions[filename] = child_stat
+          version = max(version, child_stat)

[Jeffrey Yasskin, 2014/01/29 01:08:08]

Please comment whether you want the maximum st_ctime for any file directly in
the |path| or the max st_ctime for any file recursively under it. If you want
the recursive one, you need to do use listbucket without |delimiter="/"|.

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+    else:
+      child_versions = None
+      version = cloudstorage_api.stat(full_path).st_ctime
+    return StatInfo(version, child_versions)
+  except (TypeError, errors.Error) as e:
+    raise FileNotFoundError('cloudstorage.stat failed for %s: %s' % (path, e))
+
+
+class CloudStorageFileSystem(FileSystem):
+  '''FileSystem implementation which fetches resources from Google Cloud Storage
+  '''
+  def __init__(self, bucket, debug_access_token=None, debug_bucket_prefix=None):
+    self._bucket = bucket
+    if debug_access_token:
+      logging.debug('gcs: using debug access token: %s' % debug_access_token)
+      common.set_access_token(debug_access_token)
+    if debug_bucket_prefix:
+      logging.debug('gcs: prefixing all bucket names with %s' %
+                    debug_bucket_prefix)
+      self._bucket = debug_bucket_prefix + self._bucket
+
+  def Read(self, paths):
+    def resolve():
+      try:
+        result = {}
+        for path in paths:
+          full_path = '/%s/%s' % (self._bucket, path.lstrip('/'))
+          logging.debug('gcs: requested path %s, reading %s' % (path, full_path))

[Jeffrey Yasskin, 2014/01/29 01:08:08]

We have an 80-column limit for Chrome Python, right?

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

+          if path == '' or path.endswith('/'):
+            result[path] = _ListDir(full_path)
+          else:
+            result[path] = _ReadFile(full_path)
+        return result
+      except errors.AuthorizationError as authError:
+        self._warnAboutAuthError()
+        raise authError
+
+    return Future(delegate=Gettable(resolve))

[Jeffrey Yasskin, 2014/01/29 01:08:08]

There's no benefit to returning a delayed Future if you don't start some
processing outside of the delegate. In the case of _ReadFile, it looks like
open() does start some work, so if you call that outside resolve() and read()
inside, you'll avoid blocking in some cases. listbucket() also seems to start
some work, which is waited for when __iter__ is called, so you can profitably
split those too.

[Renato Mangini (chromium), 2014/01/31 02:29:04]

But by splitting open and read I will loose the benefit of the constrained
"with" context with auto-closing and would need to keep a reference to the open
connection for an unforeseeable time, until the Future delegate is executed. Is
it worth?

BTW: I used the same approach of local_file_system, since I wasn't familiar with
this implementation of Futures.

[Jeffrey Yasskin, 2014/02/03 22:12:56]

I think so, given all the fixes Ben had to make to get the docserver to respond
quickly. However, there's also a risk that the Future will never have get()
called, in which case the connection might never get closed (unless it's
automatically closed by GC, which is possible). It's up to you which risk you
want to take.
Yeah, they're a little weird. It'd be good eventually to replace them with the
appengine ndb futures so we can use 'yield' syntax to use them, and so they'll
have more than 2 phases of execution.

+
+  def Refresh(self):
+    return Future(value=())
+
+  def Stat(self, path):
+    try:
+      return _CreateStatInfo(self._bucket, path)
+    except errors.AuthorizationError as authError:
+      self._warnAboutAuthError()
+      raise authError

[Jeffrey Yasskin, 2014/01/29 01:08:08]

You should just use "raise" here in order to keep the original stack trace.
http://docs.python.org/2/reference/simple_stmts.html#the-raise-statement

[Renato Mangini (chromium), 2014/01/31 02:29:04]

Done.

[Jeffrey Yasskin, 2014/02/03 22:12:56]

Not done.

+
+  def GetIdentity(self):
+    return '@'.join((self.__class__.__name__, StringIdentity(self._bucket)))
+
+  def __repr__(self):
+    return 'LocalFileSystem(%s)' % self._bucket
+
+  def _warnAboutAuthError(self):
+    logging.warn(("Authentication error on Cloud Storage. Check if your"
+                  " appengine project has permissions to Read the GCS"
+                  " buckets. If you are running a local appengine server,"
+                  " you need to set an access_token in"
+                  " local_debug/gcs_debug.conf."
+                  " Remember that this token expires in less than 10"
+                  " minutes, so keep it updated. See"
+                  " gcs_file_system_provider.py for instructions."));