Extensions Docs Server: Samples zip files

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

Index: chrome/common/extensions/docs/server2/example_zipper.py
diff --git a/chrome/common/extensions/docs/server2/example_zipper.py b/chrome/common/extensions/docs/server2/example_zipper.py
new file mode 100644
index 0000000000000000000000000000000000000000..502282b94fd79c705075f6682b8696abfd979594
--- /dev/null
+++ b/chrome/common/extensions/docs/server2/example_zipper.py
@@ -0,0 +1,47 @@
+# Copyright (c) 2012 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 os
+from io import BytesIO
+import re
+from zipfile import ZipFile
+
+class ExampleZipper(object):
+  """This class creates a zip file given a samples directory.
+  """
+  def __init__(self, cache_builder, base_path, match_path):
+    self._base_path = base_path
+    self._zip_cache = cache_builder.build(self._MakeZipFile)
+    self._svn_cache = cache_builder.build(lambda x: x)

[not at google - send to devlin, 2012/07/12 00:22:48]

Is it actually worth having a cache for the svn stuff here? It seems like all we
really need is the zip cache + urlfetch (with its automatic memcaching) since
we're unlikely to need to read any of those files except while zipping them...
especially from within this class.

[cduvall, 2012/07/12 01:51:23]

Done.

+    self._match_path = match_path
+
+  def _MakeZipFile(self, files):
+    zip_bytes = BytesIO()
+    zip_file = ZipFile(zip_bytes, mode='w')
+    zip_path = os.path.commonprefix(files).rsplit('/', 1)[-2]
+    prefix = zip_path.rsplit('/', 1)[-2]
+    if zip_path + '/manifest.json' not in files:
+      return None

[not at google - send to devlin, 2012/07/12 00:22:48]

This leaks zip_file, but see comment below.

[cduvall, 2012/07/12 01:51:23]

Done.

+    for filename in files:
+      try:
+        zip_file.writestr(
+            filename.replace(prefix, ''),

[not at google - send to devlin, 2012/07/12 00:22:48]

filename[len(prefix):] ?

This actually leaves a leading / when these paths are absolute (which seems to
be happening), but not when relative, so you'll also need to strip the leading
'/' before putting them in the zip file, or you'll get messages like

warning:  stripped absolute path spec from /make_page_red/background.js

[cduvall, 2012/07/12 01:51:23]

Done.

+            self._svn_cache.get(filename))
+      except Exception as e:
+        logging.info(e)

[not at google - send to devlin, 2012/07/12 00:22:48]

Should we be serving partial zip files on exceptions?

Perhaps let this exception fall through to the handlers and put the
zip_file.close() in a finally block.

zip_bytes = ...
zip_path = ...
prefix = ...
if zip_path ...
  return None

zip_file = ZipFile(...)
try:
  for filename in files:
    ...
finally:
  zip_file.close()
return zip_bytes.getvalue()

[cduvall, 2012/07/12 01:51:23]

Done.

+    zip_file.close()
+    return zip_bytes.getvalue()
+
+  def __getitem__(self, key):

[not at google - send to devlin, 2012/07/12 00:22:48]

Remove this method?

[cduvall, 2012/07/12 01:51:23]

Done.

+    return self.get(key)
+
+  def get(self, key):

[not at google - send to devlin, 2012/07/12 00:22:48]

A more sensible interface for this class, I think, would be like

def create(self, path):
  """ Creates a new zip file from the recursive contents of |path|
  as returned by |_zip_cache|.
  Paths within the zip file are given relative to and including |path|.
  """

[cduvall, 2012/07/12 01:51:23]

Done.

+    if not re.match(self._match_path + '/.*\.zip$', key):

[not at google - send to devlin, 2012/07/12 00:22:48]

See comment just above; none of this checking should be necessary, nor should
this class need to strip out ".zip".

[cduvall, 2012/07/12 01:51:23]

Done.

+      return None
+    base, ext = os.path.splitext(key)
+    try:
+      return self._zip_cache.get(self._base_path + '/' + base, True)

[not at google - send to devlin, 2012/07/12 00:22:48]

This really threw me. Can we put two methods on the Fetchers, "getFromFile" and
"getFromListingFiles" or something? getFromFile would throw an error if it's
given a directory, getFromListingFiles would throw an error if it's not given a
directory.

Would make this easier to read, less magical, probably less error prone.

[cduvall, 2012/07/12 01:51:23]

Done.

+    except:
+      return None

[not at google - send to devlin, 2012/07/12 00:22:48]

We should let the top-level handler deal with error exceptions like these (we'd
want to serve a 500 not a 404, and log it properly).

[cduvall, 2012/07/12 01:51:23]

Done.