Merge lp:~adeuring/charmworld/fixed-interfaces-mapping into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

use a static mapping for the charm index.

his branch removes the last remainig dynamic part of the mapping used
by the ElasticSearch index for charms.

The properties "requires" and "provides" of a charm are dictionaries
of the form

    {'relation_name': {'interface': 'http', ...}, ...}

which means that each relation name must appear as a property in the mapping.

The ES server gets charm data in the form

    {'_id': ...,
     'data': real_charm_data
     }

ElasticSearchClient._wrap_charm() now adds two entries "requires" and
"provides" that are lists of the relation data from charm["provides"]
and charm["requires"], respectively. The relation name is added to
the list entries.

This allows to define a static mapping for the relation data.

test_mapping_changes_during_indexing() requires an obvious change.
The test itself is now a bit flaky -- it might succeed accidentally:
As the comment for the old version notes, the result of the call of
get_mapping() does not necessarily incorporate possible mapping
changes after data has been added to the index. Hence it might make
sense to add a sleep(0.5) call to the test.

The change of the mapping and the related change of
ElasticSearchClient._get_text_query() require to reindex all charms.
This is done in an exodus.

[Benji York (benji)]

This branch looks good to me.

review: Approve

[Charmworld Lander (charmworld-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/charmworld-merger-trunk/453/console reported an error when processing this lp:~adeuring/charmworld/fixed-interfaces-mapping branch.
Not merging it.

[lp:~adeuring/charmworld/fixed-interfaces-mapping]

383. By Abel Deuring on 2013-09-11

test failure fixed.

[Abel Deuring (adeuring)]

review: Approve

=== added file 'charmworld/migrations/versions/019_fixed_mapping_for_interfaces.py'
--- charmworld/migrations/versions/019_fixed_mapping_for_interfaces.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/019_fixed_mapping_for_interfaces.py	2013-09-11 09:52:08 +0000
@@ -0,0 +1,10 @@
+
+# fixed mapping for interfaces.
+
+def exodus_update(source, target, current_version):
+    """Just copy the data from source to target.
+
+    The goal of this exodus is to use a new index with a static mapping.
+    """
+    for charm in source.collection.find():
+        target.save(charm)
 
=== modified file 'charmworld/migrations/versions/tests/test_migrations.py'
--- charmworld/migrations/versions/tests/test_migrations.py	2013-09-05 21:36:14 +0000
+++ charmworld/migrations/versions/tests/test_migrations.py	2013-09-11 09:52:08 +0000
@@ -6,6 +6,7 @@
 
 from charmworld.models import (
     Bundle,
+    CharmSource,
     store_bundles,
 )
 from charmworld.search import (
@@ -13,6 +14,7 @@
     IndexMissing,
 )
 from charmworld.testing import (
+    factory,
     MongoTestBase,
 )
 from charmworld.migrations import migrate
@@ -84,3 +86,45 @@
             self.db, self.index_client, '018_delete_all_bundles.py')
         # Put the index back for the test case to handle.
         self.index_client.create_index()
+
+
+class TestMigration019(MigrationTestBase):
+
+    def setUp(self):
+        super(TestMigration019, self).setUp()
+        self.use_index_client()
+
+    def test_new_db_and_index_have_all_old_data(self):
+        source = CharmSource.from_request(self)
+        charm_1 = factory.get_charm_json(
+            name='one',
+            provides={
+                'website': {
+                    'interface': 'http',
+                },
+            },
+            requires={
+                'database': {
+                    'interface': 'mysql',
+                }
+            })
+        source.save(charm_1)
+        charm_2 = factory.get_charm_json(
+            name='two',
+            provides={
+                'database': {
+                    'interface': 'mysql',
+                }
+            })
+        source.save(charm_2)
+
+        exodus_update = self.versions.get_exodus(
+            '019_fixed_mapping_for_interfaces.py')
+        target = self.versions.get_pending_source(source, 19)
+        target.index_client.create_index()
+        target.index_client.put_mapping()
+        self.addCleanup(target.index_client.delete_index)
+        exodus_update(source, target, 19)
+        self.assertEqual(2, target.collection.find().count())
+        self.assertEqual(2, target.index_client.search('mysql')['matches'])
+        self.assertEqual(1, target.index_client.search('http')['matches'])
 
=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-09-04 14:48:30 +0000
+++ charmworld/search.py	2013-09-11 09:52:08 +0000
@@ -27,8 +27,6 @@
 charm_exact_fields = [
     'owner',
     'series',
-    'provides.*.interface',
-    'requires.*.interface',
 ]
 charm_free_text_fields = {
     'name': 10,
@@ -238,19 +236,6 @@
                 'errors': {'type': 'string'},
             }
         }
-        # XXX Abel Deuring 2013-08-19 bug=1194907: The fields requires
-        # and provides are user defined dictionaries. We can't specify
-        # all possible keys. This partly reintroduces a large mapping;
-        # the proper way to fix this is described in bug 1194907.
-        inner_charm_properties['requires'] = {
-            'type': 'object',
-            'dynamic': True,
-        }
-        inner_charm_properties['provides'] = {
-            'type': 'object',
-            'dynamic': True,
-        }
-
         inner_charm_properties['config'] = {
             'properties': {
                 'options': {
@@ -261,10 +246,23 @@
                 }
             }
         }
+
+        searchable_interfaces = {
+            'type': 'object',
+            'dynamic': False,
+            'properties': {
+                'relation_name': {'type': 'string', 'index': 'not_analyzed'},
+                'interface': {'type': 'string', 'index': 'not_analyzed'},
+                'scope': {'type': 'string', 'index': 'not_analyzed'},
+            },
+        }
+
         charm_properties = {
             'data': {
                 'properties': inner_charm_properties,
-            }
+            },
+            'requires': searchable_interfaces,
+            'provides': searchable_interfaces,
         }
 
         bundle_exact_index = [
@@ -315,10 +313,19 @@
     def _wrap_charm(charm_data):
         # Avoid circular import.
         from charmworld.models import construct_charm_id
-        return {
+        result = {
             '_id': construct_charm_id(charm_data, use_revision=False),
             'data': charm_data,
         }
+        for relation_type in ('requires', 'provides'):
+            if relation_type in charm_data:
+                searchable_infos = []
+                for name, info in charm_data[relation_type].items():
+                    info = info.copy()
+                    info['relation_name'] = name
+                    searchable_infos.append(info)
+                result[relation_type] = searchable_infos
+        return result
 
     def index_charm(self, charm):
         self._index_item(self._wrap_charm(charm), CHARM)
@@ -383,15 +390,18 @@
         bundle_fields.extend(bundle_exact_fields)
 
         if text != '':
-            def make_query(fields):
+            def make_query(data_fields, other_fields=[]):
+                fields = ['data.' + field for field in data_fields]
+                fields.extend(other_fields)
                 return {
                     'query_string': {
                     'query': text,
-                    'fields': ['data.' + field for field in fields],
+                    'fields': fields,
                     }}
 
             charm_dsl = {'filtered': {
-                'query': make_query(charm_fields),
+                'query': make_query(
+                    charm_fields, ['requires.*', 'provides.*']),
                 'filter': {'type': {'value': CHARM}}
             }}
             bundle_dsl = {'filtered': {
 
=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-09-05 20:17:16 +0000
+++ charmworld/tests/test_search.py	2013-09-11 09:52:08 +0000
@@ -261,6 +261,7 @@
     def test_search_matches_on_search_terms(self):
         fields = charm_free_text_fields.keys() + charm_exact_fields
         fields = [field.split('.')[0] for field in fields]
+        fields.extend(['requires', 'provides'])
         charm = factory.get_charm_json(categories=['unknown'])
         # Ensure all fields have unique values.
         charm['address'] = 'cs:bogus-address'
@@ -894,31 +895,13 @@
                     "Character '%s' at the end of a search term causes "
                     "ElasticHttpError" % char)
 
-    def test_mapping_changes_during_indexing(self):
-        # When a charm is indexed, only the mappings for the properties
-        # "requires" and "provides" change.
-        before = self.index_client.get_mapping()['charm']['properties']
-        before = before['data']['properties']
-        before_provides = before.pop('provides')
-        before_requires = before.pop('requires')
+    def test_no_mapping_changes_during_indexing(self):
+        # The mapping does not change when charm is indexed.
+        before = self.index_client.get_mapping()
         charm = factory.get_charm_json()
         self.index_client.index_charm(charm)
-        # Avoid spurious failures. The new mapping is not immediately
-        # available after the index_charm() call. Calling
-        # index_client.wait_for_green_status() does not help here.
-        retry = 0
-        while retry < 10:
-            after = self.index_client.get_mapping()['charm']['properties']
-            after = after['data']['properties']
-            after_provides = after.pop('provides')
-            after_requires = after.pop('requires')
-            if after_provides != before_provides:
-                break
-            sleep(0.1)
-            retry += 1
+        after = self.index_client.get_mapping()
         self.assertEqual(before, after)
-        self.assertNotEqual(before_provides, after_provides)
-        self.assertNotEqual(before_requires, after_requires)
 
 
 class TestIndexingBundles(TestCase):