Merge lp:~adeuring/charmworld/charm-revision-files into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

new method CharmFileSet.save_revision_files()

his branch adds a method save_revision_files() to class CharmFileSet.

Ir is similar to the existing method save_files(), but it stores only
a dictionary file_path -> GridFS_id as charm['files'].

The method uses the function slurp_files(), which was moved from
jobs.ingest to models.

slurp_files() stored all files from a branch in GridFS, while we want
to store only a subset of these files for charms. On the other hand,
slurp_files() ignored symlinks. I fixed this by adding an optional
parameter entry_source(), which should return a sequence of
(path_name, bzr-file-id) items. A new method CharmFileSet.find_files()
generates such a sequence for save_revision_files(); it is also uyed
by the already existing method save_files().

The charm sample data has a symlink hooks/linkstart, pointing to
hooks/start, which means that slurp_files() tries to save two files
with identical content, when called by save_revision_files().
GridFS.put() raises an exception when it is called with an already
existing _id.

[Curtis Hovey (sinzui)]

Hi Abel.

This branch looks good. I think It fixes bug 1189992. I have some questions.

I would like a doc comment added to find_files(). Maybe?
   """Yield file_path, file_id of valid charm files.

    Unrecognised files and directories are ignored. Symlink outside of the charm
    are ignored.
    """

I know we have tests for the conditions described by my proposed docstring, but the are probably still names ...save_files....

save_revision_files docstring needs fixing:
    """Store relevant files from a charm revision."""

slurp_files() needs a docstring now that there is a non-obvious contract proved by entry_source. Maybe
    """Return a dict of file paths and file content hashes.

    :param fs: A file system, such as gridFS.
    :param tree: A bazaar tree.
    :param entry_source: A generator of (file_path, file_id) tuples to be slurped.
        The default entry_source only finds files.
    """

I think test_save_revision_files_from_old_revision() and test_save_revision_files_from_new_revision() are functionally identical. I think the two extra lines in the "new" test will pass when added to the "old" test. Is the setup wrong or is the revision under test wrong?

I expected to see a test where the content of a file is changed in a new revision. A comparison of charm_data['files'] for the old and new revisions will confirm the changed file has a new hash.

I think test_slurp_files_saves_files_with_identical_content() needs a different name. My reading of the code is that the method does not save duplicate files. It guarantees that it returns the hash/id of the file content, and may save the file content if it does not already exist.

review: Needs Information (code)

[lp:~adeuring/charmworld/charm-revision-files]

327. By Abel Deuring on 2013-07-29

several doc strings added; minor test refactoring; test that file changes are properly tracked by slurp_files()

[Abel Deuring (adeuring)]

On 26.07.2013 18:30, Curtis Hovey wrote:
> Review: Needs Information code
>
> Hi Abel.
>
> This branch looks good. I think It fixes bug 1189992. I have some questions.

well, not immediately -- we'll need to use the new data and the new
method that creates it.

>
> I would like a doc comment added to find_files(). Maybe?
> """Yield file_path, file_id of valid charm files.
>
> Unrecognised files and directories are ignored. Symlink outside of the charm
> are ignored.
> """

Added (with minor changes).

>
> I know we have tests for the conditions described by my proposed docstring, but the are probably still names ...save_files....
>
> save_revision_files docstring needs fixing:
> """Store relevant files from a charm revision."""
>
> slurp_files() needs a docstring now that there is a non-obvious contract proved by entry_source. Maybe
> """Return a dict of file paths and file content hashes.
>
> :param fs: A file system, such as gridFS.
> :param tree: A bazaar tree.
> :param entry_source: A generator of (file_path, file_id) tuples to be slurped.
> The default entry_source only finds files.
> """
>
> I think test_save_revision_files_from_old_revision() and test_save_revision_files_from_new_revision() are functionally identical. I think the two extra lines in the "new" test will pass when added to the "old" test. Is the setup wrong or is the revision under test wrong?

They are of course similar, but not fully equivalent. The idea of the
two tests is to show that it is possible to successfully select a
revision of a branch and that all files from the specified revision are
stored, but not files that were added later.

The two lines

        expected = self.expected_files()
        expected['hooks/new_hook'] = 'A new hook'

include the file added in the second commit to the expected data. The
method check_revision_files() ensures that the set of expected and
actually added files are identical:

        expected_names = set(expected_files)
        found_names = set(charm_revision['files'])
        self.assertEqual(expected_names, found_names)

So, adding hooks/now_hook to expected_files in
test_save_revision_files_from_old_revision() would cause a test failure.

(There is a subtle difference in the two helper functions
add_revision_two() defined in
test_save_revision_files_from_old_revision() and
test_save_revision_files_from_new_revision(): The one in
...old_revision() returns None, while the other returns the new revision
ID. This has influence one the value returned by save_revision_files().
This somewhat convoluted behaviour is the result of a review discussion
of older work related to saving files for s specific revision... I've
refactored the two helper functions which makes the difference hopefully
a bit more obvious.)

>
> I expected to see a test where the content of a file is changed in a new revision. A comparison of charm_data['files'] for the old and new revisions will confirm the changed file has a new hash.

The changes between the two version of the test branch now include a
file change.

>
> I think test_slurp_files_saves_files_with_identical_content() needs a different name...

[Curtis Hovey (sinzui)]

Thank you.

I missed the subtle meaning of `return tree.commit('revision 2')` and `return`. Your helper method make it clear how the test is setup.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-07-25 19:48:14 +0000
+++ charmworld/jobs/ingest.py	2013-07-29 11:31:27 +0000
@@ -8,7 +8,6 @@
     datetime,
     timedelta,
 )
-import hashlib
 import logging
 import os
 import shutil
@@ -21,7 +20,6 @@
 from bzrlib.branch import Branch
 from bzrlib.revision import NULL_REVISION
 from bzrlib.transport import get_transport
-import magic
 import requests
 import yaml
 
@@ -41,6 +39,7 @@
     getdb,
     getfs,
     options_to_storage,
+    slurp_files,
     store_bundles,
 )
 from charmworld.search import (
@@ -303,19 +302,6 @@
         store_bundles(collection, deployer_config, basket_data['_id'])
 
 
-def slurp_files(fs, tree):
-    hashes = {}
-    for path, entry in tree.iter_entries_by_dir():
-        if entry.kind != 'file':
-            continue
-        bytes = tree.get_file_text(entry.file_id)
-        content_type = magic.from_buffer(bytes, mime=True)
-        _id = hashlib.sha256(bytes).hexdigest()
-        fs.put(bytes, contentType=content_type, _id=_id)
-        hashes[path] = _id
-    return hashes
-
-
 def _rev_info(r, branch):
     d = {
         'authors': r.get_apparent_authors(),

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-07-22 20:49:00 +0000
+++ charmworld/models.py	2013-07-29 11:31:27 +0000
@@ -5,6 +5,7 @@
 from bzrlib.branch import Branch
 from bzrlib.errors import NoSuchRevision
 from calendar import timegm
+import hashlib
 import logging
 from mimetypes import guess_type
 import os
@@ -732,7 +733,7 @@
         )
 
     @classmethod
-    def save_file(cls, fs, logger, charm_data, file_info, tree):
+    def save_file(cls, fs, logger, charm_data, file_path, file_id, tree):
         """Do the actual storage of the file contents.
 
         :param fs: The mongo fs connection to use.
@@ -741,29 +742,6 @@
         :param file_info: The file's data as returned by tree.list_files().
         :tree: The working tree of the branch.
         """
-        file_path, ignore, file_type, file_id = file_info[:4]
-        if file_type == 'symlink':
-            target = tree.get_symlink_target(file_id)
-            if target.startswith('/'):
-                logger.warn(
-                    "Invalid file path: symlink %s points to the absolute "
-                    "path %s." % (
-                        file_path, target))
-                return
-            directory = dirname(file_path)
-            resolved = normpath(join(directory, target))
-            if resolved.startswith('../'):
-                logger.warn(
-                    "Invalid file path: Path %s points to %s, which is not "
-                    "inside the charm's root directory" % (
-                        file_path, resolved))
-                return
-            file_id = tree.path2id(resolved)
-            if file_id is None:
-                logger.warn(
-                    "File does not exist: %s (resolving to %s)." % (
-                        file_path, resolved))
-                return
         plain_file_name = split(file_path)[1]
         charm_file = cls.make_charm_file(
             fs, charm_data, file_path, plain_file_name)
@@ -812,43 +790,95 @@
         return cf
 
     @classmethod
-    def save_files(cls, fs, charm_data, charm_bzr_path, logger):
-        """Given a bzr branch, load the files we care about."""
+    def get_tree(cls, charm_data, charm_bzr_path, logger):
         bzr_revision = charm_data['store_data']['digest']
         branch = Branch.open(charm_bzr_path)
         try:
-            tree = branch.repository.revision_tree(bzr_revision)
+            return branch.repository.revision_tree(bzr_revision)
         except NoSuchRevision:
             logger.warn('Revision %s not found' % bzr_revision)
+            return None
+
+    @classmethod
+    def find_files(cls, tree, logger):
+        """Yield file_path, file_id of relevant charm files.
+
+        Irrelevant files and directories are ignored. Symlinks outside
+        of the charm are ignored.
+        """
+        for file_path, entry in tree.iter_entries_by_dir():
+            if entry.kind == 'directory':
+                continue
+            found = False
+            for pattern in cls.store_files:
+                if re.match(pattern, file_path, flags=re.I):
+                    found = True
+                    break
+            if not found:
+                for directory in cls.store_directories:
+                    if file_path.startswith(directory):
+                        found = True
+                        break
+            if not found:
+                continue
+
+            if entry.kind == 'symlink':
+                target = tree.get_symlink_target(entry.file_id)
+                if target.startswith('/'):
+                    logger.warn(
+                        "Invalid file path: symlink %s points to the absolute "
+                        "path %s." % (
+                            file_path, target))
+                    continue
+                directory = dirname(file_path)
+                resolved = normpath(join(directory, target))
+                if resolved.startswith('../'):
+                    logger.warn(
+                        "Invalid file path: Path %s points to %s, which is "
+                        "not inside the charm's root directory" % (
+                            file_path, resolved))
+                    continue
+                file_id = tree.path2id(resolved)
+                if file_id is None:
+                    logger.warn(
+                        "File does not exist: %s (resolving to %s)." % (
+                            file_path, resolved))
+                    continue
+            else:
+                file_id = entry.file_id
+            yield file_path, file_id
+
+    @classmethod
+    def save_files(cls, fs, charm_data, charm_bzr_path, logger):
+        """Given a bzr branch, load the files we care about."""
+        tree = cls.get_tree(charm_data, charm_bzr_path, logger)
+        if tree is None:
             return []
-        with read_locked(branch):
+        with read_locked(tree):
             stored = []
-            for file_info in tree.list_files():
-                file_path = file_info[0]
-                file_type = file_info[2]
-                if file_type == 'directory':
-                    continue
-                found = False
-                for pattern in cls.store_files:
-                    if re.match(pattern, file_path, flags=re.I):
-                        found = True
-                        break
-                if not found:
-                    for directory in cls.store_directories:
-                        if file_path.startswith(directory):
-                            found = True
-                            break
-                if found:
-                    charm_file = cls.save_file(
-                        fs,
-                        logger,
-                        charm_data,
-                        file_info,
-                        tree)
-                    if charm_file is not None:
-                        stored.append(charm_file)
+            for file_path, file_id in cls.find_files(tree, logger):
+                charm_file = cls.save_file(
+                    fs,
+                    logger,
+                    charm_data,
+                    file_path,
+                    file_id,
+                    tree)
+                if charm_file is not None:
+                    stored.append(charm_file)
         return stored
 
+    @classmethod
+    def save_revision_files(cls, fs, charm_data, charm_bzr_path, logger):
+        """Store relevant files from a charm revision"""
+        tree = cls.get_tree(charm_data, charm_bzr_path, logger)
+        if tree is None:
+            charm_data['files'] = {}
+            return
+        with read_locked(tree):
+            charm_data['files'] = slurp_files(
+                fs, tree, cls.find_files(tree, logger))
+
 
 class CharmFile(object):
     """Representation of the GridFS stored document.
@@ -1190,3 +1220,30 @@
             'data': get_flattened_deployment(deployer_data, key),
         }
         collection.save(bundle_doc)
+
+
+def slurp_files(fs, tree, entry_source=None):
+    """Return a dict of file paths and file content hashes.
+
+    :param fs: A file system, such as gridFS.
+    :param tree: A bazaar tree.
+    :param entry_source: A generator of (file_path, file_id) tuples to
+        be slurped. The default entry_source only finds files.
+    """
+    if entry_source is None:
+        def entry_source():
+            for path, entry in tree.iter_entries_by_dir():
+                if entry.kind != 'file':
+                    continue
+                yield path, entry.file_id
+        entry_source = entry_source()
+
+    hashes = {}
+    for path, file_id in entry_source:
+        bytes = tree.get_file_text(file_id)
+        content_type = magic.from_buffer(bytes, mime=True)
+        _id = hashlib.sha256(bytes).hexdigest()
+        if not fs.exists(_id):
+            fs.put(bytes, contentType=content_type, _id=_id)
+        hashes[path] = _id
+    return hashes

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-07-22 17:57:14 +0000
+++ charmworld/tests/test_models.py	2013-07-29 11:31:27 +0000
@@ -3,16 +3,19 @@
 # the file LICENSE).
 
 """Unit tests for our application models."""
+from bzrlib.branch import Branch
 from datetime import (
     date,
     datetime,
 )
+import hashlib
 import logging
 import md5
 import os
 from os.path import dirname
 from os.path import join
 from os.path import split
+import stat
 from textwrap import dedent
 
 from mock import patch
@@ -32,6 +35,7 @@
     find_charms,
     options_to_storage,
     QAData,
+    slurp_files,
     storage_to_options,
     store_bundles,
     sync_index,
@@ -48,6 +52,7 @@
 from charmworld.testing.factory import charm_branch
 from charmworld.utils import (
     quote_key,
+    read_locked,
 )
 
 
@@ -740,6 +745,16 @@
                 branch_dir,
                 self.logger)
 
+    def save_revision_files(self, charm_path=None, extra_revisions=[]):
+        with charm_branch(charm_path, extra_revisions) as (
+                branch_dir, revision):
+            charm_data = self.charm_data.copy()
+            charm_data['branch_dir'] = branch_dir
+            charm_data['store_data'] = {'digest': revision}
+            CharmFileSet.save_revision_files(
+                self.fs, charm_data, branch_dir, self.logger)
+            return charm_data
+
     def test_fileid_generation(self):
         """A charm must generate an unique fileid."""
         self.assertEqual(
@@ -966,6 +981,60 @@
         saved = [charm_file._filename for charm_file in saved]
         self.assertEqual(['a_hook'], saved)
 
+    def check_revision_files(self, expected_files, charm_revision):
+        expected_names = set(expected_files)
+        found_names = set(charm_revision['files'])
+        self.assertEqual(expected_names, found_names)
+        for name in expected_names:
+            expected_fs_id = hashlib.sha256(expected_files[name]).hexdigest()
+            self.assertEqual(expected_fs_id, charm_revision['files'][name])
+
+    def test_save_revision_files(self):
+        charm_revision = self.save_revision_files()
+        self.check_revision_files(self.expected_files(), charm_revision)
+
+    def add_revision(self, cwd, tree, return_new_revision):
+        path = join(cwd, 'branch/trunk/hooks/new_hook')
+        with open(path, 'w') as f:
+            f.write('A new hook')
+            tree.add(['hooks/new_hook'])
+        path = join(cwd, 'branch/trunk/README.md')
+        with open(path, 'a') as f:
+            f.write('appended text')
+        if return_new_revision:
+            return tree.commit('revision 2')
+        else:
+            return None
+
+    def test_save_revision_files_from_old_revision(self):
+        def add_revision_two(cwd, tree):
+            return self.add_revision(cwd, tree, False)
+        revision = self.save_revision_files(extra_revisions=[add_revision_two])
+        self.check_revision_files(self.expected_files(), revision)
+
+    def test_save_revision_files_from_new_revision(self):
+        def add_revision_two(cwd, tree):
+            return self.add_revision(cwd, tree, True)
+        revision = self.save_revision_files(extra_revisions=[add_revision_two])
+        expected = self.expected_files()
+        expected['hooks/new_hook'] = 'A new hook'
+        expected['README.md'] = expected['README.md'] + 'appended text'
+        self.check_revision_files(expected, revision)
+
+    def test_save_revision_files_with_non_existent_bzr_revision(self):
+        with charm_branch() as (branch_dir, revision):
+            charm_data = self.charm_data.copy()
+            charm_data['branch_dir'] = branch_dir
+            charm_data['store_data'] = {'digest': 'bad-revision-id'}
+            CharmFileSet.save_revision_files(
+                self.fs,
+                charm_data,
+                branch_dir,
+                self.logger)
+        self.assertEqual({}, charm_data['files'])
+        messages = [rec.msg for rec in self.log_handler.buffer]
+        self.assertEqual(['Revision bad-revision-id not found'], messages)
+
 
 class TestCharmFile(MongoTestBase):
 
@@ -1327,3 +1396,55 @@
              })
         self.assertEqual(
             "jc:~bac/apache-6/tiny", bundle.permanent_url)
+
+
+class TestSlurpFiles(MongoTestBase):
+
+    def test_slurp_files_saves_all_files_by_default(self):
+        with charm_branch() as (branch_dir, revision):
+            branch = Branch.open(branch_dir)
+            tree = branch.repository.revision_tree(revision)
+            with read_locked(tree):
+                result = slurp_files(self.fs, tree)
+        test_data_dir = join(
+            dirname(dirname(__file__)), 'testing/data/sample_charm')
+        expected_file_paths = set()
+        for dirpath, dirnames, filenames in os.walk(test_data_dir):
+            for filename in filenames:
+                filepath = os.path.join(dirpath, filename)
+                if stat.S_ISLNK(os.lstat(filepath).st_mode):
+                    continue
+                relpath = filepath[len(test_data_dir) + 1:]
+                expected_file_paths.add(relpath)
+                content = open(filepath).read()
+                expected_hash = hashlib.sha256(content).hexdigest()
+                self.assertEqual(expected_hash, result[relpath])
+        self.assertEqual(expected_file_paths, set(result))
+
+    def test_slurp_files_with_identical_content_saved_only_once(self):
+
+        def add_revision_two(cwd, tree):
+            src_path = join(cwd, 'branch/trunk/common')
+            dst_path = join(cwd, 'branch/trunk/also-common')
+            with open(dst_path, 'w') as f:
+                f.write(open(src_path).read())
+            tree.add(['also-common'])
+            return tree.commit('revision 2')
+
+        with charm_branch(extra=[add_revision_two]) as (branch_dir, revision):
+            branch = Branch.open(branch_dir)
+            tree = branch.repository.revision_tree(revision)
+            with read_locked(tree):
+                result = slurp_files(self.fs, tree)
+        test_data_dir = join(
+            dirname(dirname(__file__)), 'testing/data/sample_charm')
+        expected_file_paths = set()
+        for dirpath, dirnames, filenames in os.walk(test_data_dir):
+            for filename in filenames:
+                filepath = os.path.join(dirpath, filename)
+                if stat.S_ISLNK(os.lstat(filepath).st_mode):
+                    continue
+                relpath = filepath[len(test_data_dir) + 1:]
+                expected_file_paths.add(relpath)
+        expected_file_paths.add('also-common')
+        self.assertEqual(result['common'], result['also-common'])