Merge lp:~adeuring/charmworld/1206659-simpler-es-mapping into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

Define a (mostly) static mapping for charms.

This branch is a first step to fix bug 1206659: Charmworld indexes far too
many fields due to Elasticsearch dynamic mapping.

Limitations:

  - It defines a static mapping only for charms, not for bundles
  - The attributes "requires" and "provides" of a charm still have
    a dynamic mapping.

Details:

The method ElasticSearchClient.put_mapping() now defines a static mapping
for charms. All fields needed for text searching, for filtering and ordering
are indexed, but no other fields.

charm['requires'] and charm['provides'] are dictionaries with user-defined
keys, hence it is impossible to define a static mapping. Fixing bug
1194907 will allow to define a static mapping for these fields too.

test_mapping_changes_during_indexing() shows that indexing a charm
changes only the mapping for requires/provides.

TestCharmSource.test_save_deletes_on_error() needed a change: Attempts
to index two charm with different types of a property named "b" now just
work because charm['b'] is now ignored by ES.

Similary, TestCharmSource.test_sync_index_script() needed a change
because the field "maintainer" is no longer indexed. (When I tried to
fix this test, I noticed bug 1214328.)

Testing the new migration showed that ES considers the new static mapping
to be compatible with the old dynamic mapping: If
ElasticSearchClient.put_mapping() is called when the index already contains
data with the previous dynamic mapping, the new index specifications are
simply accepted but the mapping stays dynamic. Hence I added the parameter
force_reindex to charmworld.search.update().

[Curtis Hovey (sinzui)]

Hi Able.

I think our intent is to treat charm_text_index, charm_integer_index as constants. Can you capitalise these identifiers and change them from lists to tuples (to ensure nothing can mutate them)?

Please place an XXX in front of the comment on line 230 because we intend to fix this issue soon.

I think this is good to land.

review: Approve (code)

[lp:~adeuring/charmworld/1206659-simpler-es-mapping]

353. By Abel Deuring on 2013-08-20

implemented reviewer's comments.

[Abel Deuring (adeuring)]

review: Approve

=== added file 'charmworld/migrations/versions/016_fixed_ES_mapping_for_charms.py'
--- charmworld/migrations/versions/016_fixed_ES_mapping_for_charms.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/016_fixed_ES_mapping_for_charms.py	2013-08-20 17:21:26 +0000
@@ -0,0 +1,8 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# fixed ElasticSearch mapping for charms.
+from charmworld.search import update as es_update
+
+def upgrade(db, index_client):
+    es_update(index_client)

=== modified file 'charmworld/migrations/versions/tests/test_migrations.py'
--- charmworld/migrations/versions/tests/test_migrations.py	2013-08-14 15:28:02 +0000
+++ charmworld/migrations/versions/tests/test_migrations.py	2013-08-20 17:21:26 +0000
@@ -18,6 +18,7 @@
 )
 from charmworld.migrations import migrate
 from charmworld.migrations.migrate import Versions
+from charmworld.tests.test_search import put_mapping
 
 import mock
 
@@ -208,3 +209,22 @@
         # No exception is raised.
         self.versions.run_migration(self.db, self.index_client,
                                     '015_drop_bundle_doctype.py')
+
+
+class TestMigration016(MigrationTestBase):
+
+    def test_es_mapping_is_static(self):
+        self.use_index_client(put_mapping=False)
+        put_mapping(
+            self.index_client,
+            {'box': {'type': 'string', 'index': 'not_analyzed'}},
+            dynamic=True)
+        self.index_client.index_charm(factory.get_charm_json())
+        old_mapping = self.index_client.get_mapping()
+        self.assertIn(
+            'files', old_mapping['charm']['properties']['data']['properties'])
+        self.versions.run_migration(self.db, self.index_client,
+                                    '016_fixed_ES_mapping_for_charms.py')
+        new_mapping = self.index_client.get_mapping()
+        self.assertNotIn(
+            'files', new_mapping['charm']['properties']['data']['properties'])

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-08-14 15:12:04 +0000
+++ charmworld/search.py	2013-08-20 17:21:26 +0000
@@ -190,6 +190,47 @@
         charm_properties = dict(
             (name, {'type': 'string', 'index': 'not_analyzed'})
             for name in charm_exact_index)
+
+        CHARM_TEXT_INDEX = (
+            'summary',
+            'description',
+        )
+        charm_properties.update(dict(
+            (name, {'type': 'string'}) for name in CHARM_TEXT_INDEX))
+
+        CHARM_INTEGER_INDEX = (
+            'downloads',
+            'downloads_in_past_30_days',
+            'downloads_in_past_7_days',
+            'downloads_in_past_half_year',
+        )
+        charm_properties.update(dict(
+            (name, {'type': 'long'}) for name in CHARM_INTEGER_INDEX))
+
+        charm_properties['date_created'] = {
+            'type': 'date',
+        }
+        charm_properties['is_featured'] = {
+            'type': 'boolean',
+        }
+        charm_properties['store_data'] = {
+            'properties': {
+                '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.
+        charm_properties['requires'] = {
+            'type': 'object',
+            'dynamic': True,
+        }
+        charm_properties['provides'] = {
+            'type': 'object',
+            'dynamic': True,
+        }
+
         charm_properties['config'] = {
             'properties': {
                 'options': {
@@ -218,10 +259,13 @@
             for name in bundle_exact_index)
 
         with translate_error():
-            for (name, properties) in ((CHARM, charm_properties),
-                                       (BUNDLE, bundle_properties)):
+            for (name, properties, dynamic) in (
+                    (CHARM, charm_properties, False),
+                    (BUNDLE, bundle_properties, True)):
                 self._client.put_mapping(self.index_name, name, {
                     name: {
+                        'type': 'object',
+                        'dynamic': dynamic,
                         'properties': properties,
                     }
                 })
@@ -645,7 +689,7 @@
     return new_index
 
 
-def update(index_client):
+def update(index_client, force_reindex=False):
     """Update the index to use the current mapping.
 
     If the index isn't present, a new index is created and aliased to it.
@@ -654,7 +698,10 @@
     """
     actual_client = index_client
     try:
-        index_client.put_mapping()
+        if force_reindex:
+            actual_client = reindex(index_client)
+        else:
+            index_client.put_mapping()
     except IncompatibleMapping:
         actual_client = reindex(index_client)
     except IndexMissing:

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-08-19 14:45:29 +0000
+++ charmworld/tests/test_models.py	2013-08-20 17:21:26 +0000
@@ -1420,10 +1420,10 @@
             self.assertEqual(foo, bar)
 
     def test_sync_index_script(self):
-        factory.makeCharm(self.db)[0]
+        factory.makeCharm(self.db, owner='b')[0]
         # Create a charm which is index-incompatible with the previous one.
-        factory.makeCharm(self.db,
-                          maintainer={'first': 'J', 'last': 'Random'})[0]
+        factory.makeCharm(self.db, owner='a',
+                          description={'text': 'whatever'})[0]
         handler = self.get_handler('sync-index')
         with patch('charmworld.models.configure_logging'):
             with patch('charmworld.models.get_ini', lambda: INI):
@@ -1438,18 +1438,19 @@
         source = CharmSource.from_request(self)
         source.save({
             '_id': 'a',
-            'b': {},
             'owner': 'foo',
             'series': 'bar',
             'name': 'baz',
+            'description': 'whatever',
         })
         with self.assertRaises(ElasticHttpError):
             source.save({
                 '_id': 'a',
-                'b': 'c',
                 'owner': 'foo',
                 'series': 'bar',
                 'name': 'baz',
+                # A dictionary as the value conflicts with the mapping.
+                'description': {'text': 'whatever'},
             })
         self.assertIs(None, self.index_client.get('a'))
 

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-08-14 15:12:04 +0000
+++ charmworld/tests/test_search.py	2013-08-20 17:21:26 +0000
@@ -848,6 +848,23 @@
                     "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']
+        charm = factory.get_charm_json()
+        self.index_client.index_charm(charm)
+        after = self.index_client.get_mapping()['charm']['properties']
+        after = after['data']['properties']
+        before_provides = before.pop('provides')
+        before_requires = before.pop('requires')
+        after_provides = after.pop('provides')
+        after_requires = after.pop('requires')
+        self.assertEqual(before, after)
+        self.assertNotEqual(before_provides, after_provides)
+        self.assertNotEqual(before_requires, after_requires)
+
 
 class TestIndexingBundles(TestCase):
 
@@ -877,10 +894,11 @@
                 self.index_client.index_bundles([])
 
 
-def put_mapping(client, properties):
+def put_mapping(client, properties, dynamic=True):
     client._client.put_mapping(
         client.index_name, CHARM, {
             CHARM: {
+                'dynamic': dynamic,
                 'properties': {'data': {'properties': properties}}
             }
         }
@@ -930,6 +948,46 @@
         self.assertEqual('not_analyzed',
                          mapping['properties']['name']['index'])
 
+    def update_to_static_mapping(self, force_reindex):
+        index_client = self.use_index_client(put_mapping=False)
+        put_mapping(
+            index_client,
+            {'box': {'type': 'string', 'index': 'not_analyzed'}},
+            dynamic=True)
+        update(index_client, force_reindex)
+        updated_mapping = index_client.get_mapping()
+        # A property 'files' is not defined in the current mapping.
+        self.assertNotIn(
+            'files',
+            updated_mapping['charm']['properties']['data']['properties'])
+        index_client.index_charm(factory.get_charm_json())
+        return index_client
+
+    def test_simple_change_dynamic_to_static_mapping(self):
+        # If an existing mapping is dynamic (the default for ElasticSearch)
+        # and if a new mapping is specified as static, the two mappings
+        # are considered compatible, but the resulting mapping is
+        # still dynamic.
+        index_client = self.update_to_static_mapping(force_reindex=False)
+        updated_mapping = index_client.get_mapping()
+        # charm['files'] is not supposed to be included in the new
+        # mapping, but it still exists if force_reindex is not used.
+        self.assertIn(
+            'files',
+            updated_mapping['charm']['properties']['data']['properties'])
+
+    def test_dynamic_to_static_mapping_forced_reindex(self):
+        # If an existing mapping is dynamic (the defult) and if a
+        # new mapping is specified as static, the two mappings
+        # are considered compatible, but the resulting mapping is
+        # still dynamic.
+        index_client = self.update_to_static_mapping(force_reindex=True)
+        updated_mapping = index_client.get_mapping()
+        # The mapping is indeed static; charm['files'] is not indexed.
+        self.assertNotIn(
+            'files',
+            updated_mapping['charm']['properties']['data']['properties'])
+
 
 class TestReindex(TestCase):