Merge lp:~benji/charmworld/bundle-indexing into lp:~juju-jitsu/charmworld/trunk

[Benji York]

Index bundles as part of ingest.

[Aaron Bentley (abentley)]

Could you explain why quote_yaml was introduced in slurp_files? The keys in the input are supposed to be hex digits, so I don't see how they could contain '.' or '$'. I think it's preferable to raise a ValueError if inappropriate keys are supplied.

quote_yaml lets us work around the fact that some of our keys are user-generated, but we want to change our internal representation so that no keys are user-generated. So ideally, we will not add any new quote_yaml call sites.

Because the index is also used as a data store, it needs to be as tightly synchronized with the database as possible. So I'd like to see indexing and storing in the database presented as a single operation. In fact, this is one of the reasons CharmSource exists-- to present a unified CharmSource.save method. It's important that the same representation is stored in the index and the db, because we want to be able to use them interchangeably. Right now, models.store_bundles is storing a different representation from UpdateBundle.index_bundles. Both of these concerns could be addressed by having models.store_bundles accept an index client and update the index as well.

At 143, consider making the side-effect a lambda. I think that might be more readable.

Somehow, we've misspelled 'promulgated' as 'promultated'. Could you fix that (since you're renaming those tests anyway)?

review: Needs Information

[Benji York (benji)]

> Could you explain why quote_yaml was introduced in slurp_files? The keys in
> the input are supposed to be hex digits, so I don't see how they could contain
> '.' or '$'. I think it's preferable to raise a ValueError if inappropriate
> keys are supplied.

Good point. I guess that was a think-o on my part. I've removed the
quoting.

> quote_yaml lets us work around the fact that some of our keys are user-
> generated, but we want to change our internal representation so that no keys
> are user-generated. So ideally, we will not add any new quote_yaml call
> sites.

Since we store paths in hashed-files, we will still need to quote keys
(since the paths are keys and they can contain dots). I considered
writing a MongoDB-safe mapping that (un)quotes its keys as necessary,
but it felt a bit over-engineered for what we needed at this point in
time.

> Because the index is also used as a data store, it needs to be as tightly
> synchronized with the database as possible. So I'd like to see indexing and
> storing in the database presented as a single operation. In fact, this is one
> of the reasons CharmSource exists-- to present a unified CharmSource.save
> method.

I have addressed this by adding the indexing to the store_bundles function.

> At 143, consider making the side-effect a lambda. I think that might be more
> readable.

Done.

> Somehow, we've misspelled 'promulgated' as 'promultated'.

"Promultated" is the act of distributing potatoes.

Fixed.

[Aaron Bentley (abentley)]

I was wrong about quote_yaml. The keys in dict from slurp_map are paths, not "hex digits", as I said. So they do need to be quoted, after all Please restore quote_yaml to decorate_basket and land. Sorry.

review: Approve

[Charmworld Lander (charmworld-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Benji York (benji)]

Dear Charmworld Lander, I, being of sound mind and body, do hereby declare my latest revisions reviewed and approved and therefore designate them landable, by you, forthwith.

review: Approve (code)

[Charmworld Lander (charmworld-lander)]

Attempt to merge into lp:charmworld failed due to conflicts:

text conflict in charmworld/jobs/ingest.py

[Benji York (benji)]

Conflicts fixed.

review: Approve (code)

[Charmworld Lander (charmworld-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Benji York (benji)]

Conflicts fixed.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-07-29 14:33:27 +0000
+++ charmworld/jobs/ingest.py	2013-07-29 15:07:28 +0000
@@ -49,9 +49,11 @@
 from charmworld.utils import (
     quote_key,
     quote_yaml,
+    unquote_yaml,
     timestamp,
 )
 
+# XXX Why not "from charmworld.jobs.config import..."? (Benji)
 from config import CHARM_DIR
 from config import CHARM_PROOF_PATH
 from config import settings
@@ -274,32 +276,42 @@
         self.working_dir = working_dir
         super(UpdateBundleJob, self).__init__()
 
-    def run(self, payload):
-        self.log.info('Saving %s' % payload['branch_spec'])
-        fs = getfs(self.db, collection='hashed-files')
-        self.decorate_basket(payload, fs)
-        self.db.baskets.save(payload)
-        self.store_bundles(fs, payload, self.db.bundles)
+    def store_bundles(self, deployer_config, basket_id):
+        store_bundles(self.db.bundles, deployer_config, basket_id)
 
     @staticmethod
-    def construct_id(basket_data, revno):
+    def construct_basket_id(basket_data, revno):
         owner_segment = '~%s/' % basket_data['owner']
         return '%s%s-%d' % (owner_segment, basket_data['name'], revno)
 
+    @staticmethod
+    def decorate_bundles(basket_data):
+        # Give each bundle a unique ID.
+        for bundle_name, bundle in basket_data.items():
+            bundle['_id'] = '%s/%s' % (basket_data['_id'], bundle_name)
+
     def decorate_basket(self, basket_data, fs):
         branch_dir = fetch_branch(self.working_dir, basket_data, self.log)
         branch = Branch.open(branch_dir)
         revno = branch.revision_id_to_revno(basket_data['commit'])
         tree = branch.repository.revision_tree(basket_data['commit'])
-        basket_data['_id'] = self.construct_id(basket_data, revno)
-        basket_data['file_hashes'] = slurp_files(fs, tree)
+        basket_data['_id'] = self.construct_basket_id(basket_data, revno)
+        basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree))
 
     @staticmethod
-    def store_bundles(fs, basket_data, collection):
-        hashes = basket_data['file_hashes']
+    def get_deployer_config(fs, basket_data):
+        hashes = unquote_yaml(basket_data['file_hashes'])
         deployer_config_bytes = fs.get(hashes['bundles.yaml'])
-        deployer_config = yaml.safe_load(deployer_config_bytes)
-        store_bundles(collection, deployer_config, basket_data['_id'])
+        return yaml.safe_load(deployer_config_bytes)
+
+    def run(self, basket_data):
+        self.log.info('Saving %s' % basket_data['branch_spec'])
+        fs = getfs(self.db, collection='hashed-files')
+        self.decorate_basket(basket_data, fs)
+        self.db.baskets.save(basket_data)
+        deployer_config = self.get_deployer_config(fs, basket_data)
+        self.decorate_bundles(deployer_config)
+        self.store_bundles(deployer_config, basket_data['_id'])
 
 
 def _rev_info(r, branch):
@@ -690,28 +702,3 @@
     charm['address'] = address
     charm['store_data'] = data
     charm['store_url'] = make_store_url(data['revision'], address)
-
-
-# XXX j.c.sackett Jan 31 2013 Bug:1111708 scan_repo is swapped for
-# scan_charm to reindex what's on disk; we should probably just
-# have a different job for this that's not part of ingest.
-#def scan_repo(db, root_dir):
-    #log = logging.getLogger("charm.scan")
-    #charms = os.listdir(root_dir)
-    #for c in charms:
-        #charm_dir = os.path.join(root_dir, c)
-        #if not os.path.isdir(charm_dir):
-            #continue
-        ##log.info("Processing %s", c)
-        #try:
-            #scan_charm(
-                #db, c, charm_dir, repo="~charmers/charm/oneiric/%s" % c)
-        #except:
-            #log.exception("Unknown scan error")
-            #raise
-            #import pdb
-            #import sys
-            #import traceback
-            #traceback.print_exc()
-            #pdb.post_mortem(sys.exc_info()[-1])
-            #raise

=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-07-25 19:48:14 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-07-29 15:07:28 +0000
@@ -21,6 +21,7 @@
 )
 from mock import patch
 from pyelasticsearch.exceptions import ElasticHttpNotFoundError
+import yaml
 
 from charmworld.charmstore import (
     CharmStore,
@@ -50,6 +51,9 @@
     response404,
     TestCase,
 )
+from charmworld.utils import (
+    quote_key,
+    )
 
 
 class TestUpdateDateCreated(TestCase):
@@ -288,6 +292,36 @@
             fs = getfs(self.db, 'hashed-files')
             self.assertEqual(fs.get(expected_hash).read(), bytes)
 
+    def test_dots_in_file_paths_do_not_break_things(self):
+        # Since mongodb doesn't like keys with dots in them and we want to
+        # store a mapping from file name (potentially with dots) to file hash,
+        # we need to quote the file patth.
+        job = UpdateBundleJob()
+        job.setup(self.db)
+        with patch.object(job, 'get_deployer_config'):
+            with bundle_update_environment():
+                with patch(
+                        'charmworld.jobs.ingest.slurp_files',
+                        side_effect=lambda *_: {'a.b': 'X'}) as slurp_files:
+                    bundle_data = factory.get_payload_json()
+                    job.run(bundle_data)
+                    self.assertTrue(slurp_files.called)
+
+    def test_file_paths_with_dots_can_be_retrieved(self):
+        # Since we have to escape file paths for mongodb, it is important that
+        # we check that file paths containing dots can be retrieved.
+        job = UpdateBundleJob()
+        basket_data = {'file_hashes': {quote_key('bundles.yaml'): 'HASH'}}
+        class FauxFS:
+            @staticmethod
+            def get(hash):
+                self.assertEqual(hash, 'HASH')
+                self.get_called = True
+                return ''
+        job.get_deployer_config(FauxFS(), basket_data)
+        self.assertTrue(self.get_called)
+
+
     def test_run_pulls_in_all_branch_files(self):
         # The ingest job puts all files in the branch into the database.
         NAME = 'our-source-package'
@@ -303,32 +337,32 @@
         # database.
         job = UpdateBundleJob()
         job.setup(self.db)
-        with bundle_update_environment():
-            def slurp_files_side_effect(*args):
-                return {}
-            with patch(
-                'charmworld.jobs.ingest.slurp_files',
-                side_effect=slurp_files_side_effect) as slurp_files:
-                bundle_data = factory.get_payload_json()
-                job.run(bundle_data)
-                self.assertTrue(slurp_files.called)
+        with patch.object(job, 'get_deployer_config'):
+            with bundle_update_environment():
+                with patch(
+                        'charmworld.jobs.ingest.slurp_files',
+                        side_effect=lambda *_: {}) as slurp_files:
+                    bundle_data = factory.get_payload_json()
+                    job.run(bundle_data)
+                    self.assertTrue(slurp_files.called)
 
     def test_bundle_knows_where_its_files_are(self):
         # The mapping of bundle file paths to hashes (keys in gridfs) is
         # stored in the bundle document.
         job = UpdateBundleJob()
         job.setup(self.db)
-        with bundle_update_environment():
-            def slurp_files_side_effect(*args):
-                return {'path1': 'hash1', 'path2': 'hash2'}
-            with patch(
-                    'charmworld.jobs.ingest.slurp_files',
-                    side_effect=slurp_files_side_effect):
-                bundle_data = factory.get_payload_json()
-                job.run(bundle_data)
-                self.assertEqual(
-                    bundle_data['file_hashes'],
-                    {'path1': 'hash1', 'path2': 'hash2'})
+        with patch.object(job, 'get_deployer_config'):
+            with bundle_update_environment():
+                def slurp_files_side_effect(*args):
+                    return {'path1': 'hash1', 'path2': 'hash2'}
+                with patch(
+                        'charmworld.jobs.ingest.slurp_files',
+                        side_effect=slurp_files_side_effect):
+                    bundle_data = factory.get_payload_json()
+                    job.run(bundle_data)
+                    self.assertEqual(
+                        bundle_data['file_hashes'],
+                        {'path1': 'hash1', 'path2': 'hash2'})
 
     def test_bundle_ids_include_their_revno(self):
         job = UpdateBundleJob()
@@ -363,19 +397,19 @@
                 with self.assertRaises(bzrlib.errors.NoSuchRevision):
                     job.decorate_basket(bundle_data, None)
 
-    def test_construct_id_includes_owner_if_promultated(self):
+    def test_construct_basket_id_includes_owner_if_promulgated(self):
         payload = factory.get_payload_json(name='mysql', promulgated=True)
-        basket_id = UpdateBundleJob.construct_id(payload, 5)
+        basket_id = UpdateBundleJob.construct_basket_id(payload, 5)
         self.assertEqual('~charmers/mysql-5', basket_id)
 
-    def test_construct_id_includes_owner_if_not_promultated(self):
+    def test_construct_basket_id_includes_owner_if_not_promulgated(self):
         payload = factory.get_payload_json(name='mysql', promulgated=False)
-        basket_id = UpdateBundleJob.construct_id(payload, 5)
+        basket_id = UpdateBundleJob.construct_basket_id(payload, 5)
         self.assertEqual('~charmers/mysql-5', basket_id)
 
     def test_job_can_store_bundles_in_the_db(self):
         basket_data = factory.get_payload_json(name='wordpress')
-        basket_id = UpdateBundleJob.construct_id(basket_data, 5)
+        basket_id = UpdateBundleJob.construct_basket_id(basket_data, 5)
         bundle_id = '%s/%s' % (basket_id, 'stage')
         basket_data['_id'] = basket_id
         job = UpdateBundleJob()
@@ -397,20 +431,18 @@
                             engine: apache
         """)
         fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH)
-        job.store_bundles(fs, basket_data, self.db.bundles)
+        job.store_bundles(yaml.safe_load(deployer_config), basket_id)
         self.assertIsNotNone(self.db.bundles.find_one(bundle_id))
 
     def test_job_run_stores_bundles_in_the_db(self):
         basket_data = factory.get_payload_json(name='wordpress')
         job = UpdateBundleJob()
         job.setup(self.db)
-        with patch.object(job, 'decorate_basket'):
-            with patch.object(job, 'store_bundles') as store_bundles:
-                job.run(basket_data)
-                self.assertTrue(store_bundles.called)
-
-
-    # TODO add bundles to index
+        with patch.object(job, 'get_deployer_config'):
+            with patch.object(job, 'decorate_basket'):
+                with patch.object(job, 'store_bundles') as store_bundles:
+                    job.run(basket_data)
+                    self.assertTrue(store_bundles.called)
 
 
 class TestUpdateCharm(MongoTestBase):

=== modified file 'charmworld/jobs/tests/test_worker.py'
--- charmworld/jobs/tests/test_worker.py	2013-07-25 19:48:14 +0000
+++ charmworld/jobs/tests/test_worker.py	2013-07-29 15:07:28 +0000
@@ -67,6 +67,21 @@
             yield
 
 
+def FauxWorkerFactory(test_case):
+    class FauxWorker:
+        run_called = False
+
+        def __init__(self, *args):
+            pass
+
+        @classmethod
+        def run(cls, run_forever):
+            test_case.assertFalse(run_forever)
+            cls.run_called = True
+
+    return FauxWorker
+
+
 class WorkerTest(MongoTestBase):
 
     def setUp(self):
@@ -185,31 +200,13 @@
             self.assertIn(queue_entry, queue_entries_run)
 
     def test_worker_does_not_run_forever_by_default(self):
-        run_called = []
-        class FauxWorker:
-            def __init__(self, *args):
-                pass
-
-            @staticmethod
-            def run(run_forever):
-                self.assertFalse(run_forever)
-                run_called.append(True)
-
+        FauxWorker = FauxWorkerFactory(self)
         with patch('charmworld.jobs.worker.QueueWorker', new=FauxWorker):
             self.run_main_with_exit()
-        self.assertTrue(run_called)
+        self.assertTrue(FauxWorker.run_called)
 
     def test_worker_runs_forever_if_command_line_says_to(self):
-        run_called = []
-        class FauxWorker:
-            def __init__(self, *args):
-                pass
-
-            @staticmethod
-            def run(run_forever):
-                self.assertFalse(run_forever)
-                run_called.append(True)
-
+        FauxWorker = FauxWorkerFactory(self)
         with patch('charmworld.jobs.worker.QueueWorker', new=FauxWorker):
             self.run_main_with_exit()
-        self.assertTrue(run_called)
+        self.assertTrue(FauxWorker.run_called)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-07-29 11:29:28 +0000
+++ charmworld/models.py	2013-07-29 15:07:28 +0000
@@ -28,6 +28,7 @@
     quote_key,
 )
 from charmworld.search import ElasticSearchClient
+from charmworld.jobs.config import settings
 
 OBSOLETE_SERIES = set(['oneiric'])
 
@@ -1213,11 +1214,14 @@
     return options
 
 
-def store_bundles(collection, deployer_data, basket_id):
-    for key in deployer_data:
+def store_bundles(collection, deployer_config, basket_id):
+    """Store a basket of bundles into both MongoDB and ElasticSearch."""
+    index_client = ElasticSearchClient.from_settings(settings)
+    index_client.index_bundles(deployer_config.values())
+    for key in deployer_config:
         bundle_doc = {
             '_id': '%s/%s' % (basket_id, key),
-            'data': get_flattened_deployment(deployer_data, key),
+            'data': get_flattened_deployment(deployer_config, key),
         }
         collection.save(bundle_doc)
 

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-07-29 11:29:28 +0000
+++ charmworld/tests/test_models.py	2013-07-29 15:07:28 +0000
@@ -1316,6 +1316,25 @@
             store_bundles(self.db.bundles, parsed, 'wordpress-5')
         self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys)
 
+    def test_storing_a_bundle_includes_indexing_it(self):
+        class FauxElasticSearchClient:
+            index_bundles_called = False
+
+            @classmethod
+            def from_settings(cls, *args):
+                return cls
+
+            @classmethod
+            def index_bundles(cls, *args):
+                cls.index_bundles_called = True
+
+        with patch(
+                'charmworld.models.ElasticSearchClient',
+                FauxElasticSearchClient):
+            store_bundles(self.db.bundles, {}, 'wordpress-5')
+
+        self.assertTrue(FauxElasticSearchClient.index_bundles_called)
+
 
 class BundleTestCase(TestCase):
     """Unit tests for the Bundle model that wraps bundle representations."""