Merge lp:~reedobrien/charmworld/es-migration into lp:charmworld

[Reed O'Brien]

This makes the ngram work migrate cleanly.

Hopefully this makes the ngram work migrate cleanly.

QA:

Go back to r508

$ bzr revert -r 508

Drop the version and delte the index:

$ mongo juju --eval "db.migration_version.drop()"
$ curl -XDELETE http://localhost:9200/charms?pretty=true

Create the r508 index:

$ bin/es-update

Setup the migration system and update to r508 version (026)

$ bin/migrations current
$ bin/migrations prepare-upgrade -i
$ bin/migrations upgrade
$ bin/migrations latest

Reindex the stuff...without ngrams

$ bin/sync-index

Go back to tip (r515)

$ bzr revert

Do the migration...

$ bin/migrations latest
$ bin/migrations current
$ bin/es-update
$ bin/migrations upgrade

Verify it worked

$ curl localhost:9200/charms/_count?pretty=true
$ curl localhost:9200/charms/charm/_count?q=ngrams:crm\&pretty=true&& echo
$ curl localhost:9200/charms/charm/_count?q=ngrams:couch\&pretty=true&& echo

Make sure the tests all pass...

$ make test

[lp:~reedobrien/charmworld/es-migration]

516. By Reed O'Brien on 2014-05-20

Remove unused bits of the exists in index helper method.

[Brad Crittenden (bac)]

Reed this branch looks great, modulo some stylistic issues.

The QA steps you provide are fantastic. The last two steps do make assumptions about what charms have been ingested locally.

Note this review is based on the version of your code as of 8:00am, not the branch you subsequently pushed.

I've marked the branch as approved, which means you are free to land after making (or discussing) the suggested changes. Had I wanted to see the changes made before you land I would've used 'needs fixing' but I don't, so I didn't.

Thanks for persevering and getting this in so we can do the deploy.

review: Approve (bac)

[Brad Crittenden (bac)]

Oh, and do see the in-line comments!

[Brad Crittenden (bac)]

Actually you should pass the kind in the exists method:

    def exists_in_index(self, id_, kind=CHARM):
        client = self.index_client
        return client.get(id_, kind) is not None

[Reed O'Brien (reedobrien)]

> Reed this branch looks great, modulo some stylistic issues.
>
> The QA steps you provide are fantastic. The last two steps do make
> assumptions about what charms have been ingested locally.

Thanks. I thought I had put some ingest bits at the top. I missed that I guess -- nearly fantastic...

> I've marked the branch as approved, which means you are free to land after
> making (or discussing) the suggested changes. Had I wanted to see the changes
> made before you land I would've used 'needs fixing' but I don't, so I didn't.

I'll resubmit for sanity check.

[Reed O'Brien (reedobrien)]

> Oh, and do see the in-line comments!

Waiting for them to show here. But will work from email until they do.

[Reed O'Brien (reedobrien)]

> Actually you should pass the kind in the exists method:
>
> def exists_in_index(self, id_, kind=CHARM):
> client = self.index_client
> return client.get(id_, kind) is not None

This shrank to:

def exists_in_index(self, id_):
    return self.index_client.get(id_) is not None

in this test I am only inserting a charm, so adding a specifier seems like noise. If adding a charm let's you find something else, there is a problem and that should be tested elsewhere, IMO. Unless we are aiming to test that bundles upgrade as well, which AFAIU we are not.

[lp:~reedobrien/charmworld/es-migration]

517. By Reed O'Brien on 2014-05-20

Assuages stylistic deviations.

[Reed O'Brien (reedobrien)]

On Tue, May 20, 2014 at 9:04 AM, Brad Crittenden <email address hidden> wrote:

> Review: Approve bac
>
> Reed this branch looks great, modulo some stylistic issues.
>
> The QA steps you provide are fantastic. The last two steps do make
> assumptions about what charms have been ingested locally.
>
> Note this review is based on the version of your code as of 8:00am, not
> the branch you subsequently pushed.
>
> I've marked the branch as approved, which means you are free to land after
> making (or discussing) the suggested changes. Had I wanted to see the
> changes made before you land I would've used 'needs fixing' but I don't, so
> I didn't.
>
> Thanks for persevering and getting this in so we can do the deploy.
>
> Inline comments:
>
>
> > + except IndexMissing:
> > + # for whatever reason it is updated already
> > + pass
>
> Remember to Capitalize and punctuate comments.
>

Done.

> > --- charmworld/migrations/versions/tests/__init__.py 1970-01-01
> 00:00:00 +0000
> > +++ charmworld/migrations/versions/tests/__init__.py 2014-05-20
> 01:08:32 +0000
> > @@ -0,0 +1,1 @@
> > +# a package
> >
>
> I'd actually prefer to see a totally empty file than just this comment.
> See
>

Emptied. This is a vestigial habit from some old VCS that wouldn't commit
empty files and directories.
I can't even remember what that was... Removed.

> > +old_charm_mapping = {"charm": {
> > + "type": "object",
>
> Thanks for figuring out the old mapping structure to provide the test.
> Makes for much more thorough testing.
>

Getting the mapping was easy. Formatting it and converting to python were
less so.

> > === modified file
> 'charmworld/migrations/versions/tests/test_migrations.py'
> > --- charmworld/migrations/versions/tests/test_migrations.py 2014-04-22
> 20:36:12 +0000
> > +
> > +from charmworld.search import CHARM
> > +
>
> If you import BUNDLE too you can use them instead of strings, like at line
> 304.
>

I am actually not importing CHARM at this point either. See other comment
here.

>
> > +from pyelasticsearch.exceptions import (
> > + ElasticHttpError,
> > + ElasticHttpNotFoundError,
> > +)
> >
> > import logging
> > import mock
> > @@ -126,3 +138,79 @@
> > def test_exodus_restrictions(self):
> > """Ensure that deploy-from-scratch does not violate exodus
> rules."""
> > self.versions._check_exodus(self.versions.list_pending(0))
> > +
> > +
> > +class TestUseNgrams027(MigrationTestBase):
> > +
> > + version = 27
> > + module_name = '027_use_ngrams.py'
> > +
> > + def setUp(self):
> > + super(TestUseNgrams027, self).setUp()
> > + self.use_index_client()
> > + self.ds = DataStore(self.db)
> > + self.cs = CharmSource(self.db, self.index_client)
>
> If this were long-lived code I'd suggest more explicit names. We tend to
> the follow the Python guide that code is to be read many more times than
> written so more readable names are better at the sake of brevity.
>

Embiggened.

> > +
> > + def exists_in_index(self, id_, kind=CHARM):
> > + client = self.index_client
> > + if kind == CHARM:
> > + return client.get(id_) is not None
> > +...

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== added file 'charmworld/migrations/versions/027_use_ngrams.py'
--- charmworld/migrations/versions/027_use_ngrams.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/027_use_ngrams.py	2014-05-20 14:13:54 +0000
@@ -0,0 +1,35 @@
+# use ngrams
+import logging
+
+from charmworld.models import CharmSource
+
+from charmworld.search import (
+    IndexMissing,
+    update,
+)
+
+
+def upgrade(db, index_client):
+    """db is the pymongo db instance for our datastore. Charms are in db.charms
+    for instance.
+
+    index_client is the ElasticSearch client.
+    """
+    log = logging.getLogger('migration_027')
+    log.warning('Starting')
+    charm_source = CharmSource(db, index_client)
+    try:
+        index_client.delete_index()
+        log.warning("deleted index")
+    except IndexMissing:
+        # It is already updated.
+        pass
+        log.warning("Index already deleted")
+    update(index_client)
+    cids = charm_source.sync_index()
+    while True:
+        try:
+            next(cids)
+        except StopIteration:
+            break
+    log.warning("Index updated")

=== added file 'charmworld/migrations/versions/tests/__init__.py'
--- charmworld/migrations/versions/tests/__init__.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/tests/__init__.py	2014-05-20 14:13:54 +0000
@@ -0,0 +1,1 @@
+# a package

=== added directory 'charmworld/migrations/versions/tests/data'
=== added file 'charmworld/migrations/versions/tests/data/__init__.py'
=== added file 'charmworld/migrations/versions/tests/data/migration_027.py'
--- charmworld/migrations/versions/tests/data/migration_027.py	1970-01-01 00:00:00 +0000
+++ charmworld/migrations/versions/tests/data/migration_027.py	2014-05-20 14:13:54 +0000
@@ -0,0 +1,166 @@
+old_charm_mapping = {"charm": {
+    "type": "object",
+    "dynamic": "false",
+    "properties": {
+        "data": {
+            "properties": {
+                "_id": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "categories": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "config": {
+                    "properties": {
+                        "options": {
+                            "properties": {
+                                "default": {
+                                    "type": "string"},
+                                "description": {
+                                    "type": "string"}
+                            }
+                        }
+                    }
+                },
+                "date_created": {
+                    "type": "date",
+                    "format": "dateOptionalTime"
+                },
+                "description": {"type": "string"},
+                "downloads": {"type": "long"},
+                "downloads_in_past_30_days": {"type": "long"},
+                "downloads_in_past_7_days": {"type": "long"},
+                "downloads_in_past_half_year": {"type": "long"},
+                "i_provides": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "i_requires": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "is_featured": {"type": "boolean"},
+                "name": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "owner": {
+                    "type": "string",
+                    "index": "not_analyzed", "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "series": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "store_data": {
+                    "properties": {
+                        "errors": {"type": "string"}
+                    }
+                },
+                "store_url": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "summary": {"type": "string"}
+            }
+        },
+        "provides": {
+            "dynamic": "false",
+            "properties": {
+                "interface": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "relation_name": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "scope": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                }
+            }
+        },
+        "requires": {
+            "dynamic": "false",
+            "properties": {
+                "interface": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "relation_name": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "scope": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                }
+            }
+        }
+    }
+}}
+
+old_bundle_mapping = {"bundle": {
+    "type": "object",
+    "dynamic": "false",
+    "properties": {
+        "data": {
+            "properties": {
+                "name": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "owner": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "series": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                },
+                "title": {
+                    "type": "string",
+                    "index": "not_analyzed",
+                    "omit_norms": True,
+                    "index_options": "docs"
+                }
+            }
+        }
+    }
+}}

=== modified file 'charmworld/migrations/versions/tests/test_migrations.py'
--- charmworld/migrations/versions/tests/test_migrations.py	2014-04-22 20:36:12 +0000
+++ charmworld/migrations/versions/tests/test_migrations.py	2014-05-20 14:13:54 +0000
@@ -1,16 +1,22 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from charmworld.models import (
-    CharmSource,
-)
 from charmworld.testing import (
     factory,
     MongoTestBase,
 )
 
 from charmworld.migrations import migrate
-from charmworld.migrations.migrate import Versions
+from charmworld.migrations.migrate import (
+    DataStore,
+    Versions
+)
+
+from charmworld.migrations.versions.tests.data import migration_027 as mdata
+
+from charmworld.models import CharmSource
+
+from pyelasticsearch.exceptions import ElasticHttpError
 
 import logging
 import mock
@@ -126,3 +132,72 @@
     def test_exodus_restrictions(self):
         """Ensure that deploy-from-scratch does not violate exodus rules."""
         self.versions._check_exodus(self.versions.list_pending(0))
+
+
+class TestUseNgrams027(MigrationTestBase):
+
+    version = 27
+    module_name = '027_use_ngrams.py'
+
+    def setUp(self):
+        super(TestUseNgrams027, self).setUp()
+        self.use_index_client()
+        self.data_store = DataStore(self.db)
+        self.charm_source = CharmSource(self.db, self.index_client)
+
+    def exists_in_index(self, id_):
+        return self.index_client.get(id_) is not None
+
+    def sync_index(self):
+        charm_ids = self.charm_source.sync_index()
+        while True:
+            try:
+                next(charm_ids)
+            except StopIteration:
+                break
+
+    def put_old_mapping(self):
+        # Delete the temp_index.
+        self.index_client.delete_index()
+        # Create an empty index.
+        self.index_client._client.create_index(self.index_client.index_name)
+        # Put up the old mappings.
+        for type_ in ('charm', 'bundle'):
+            self.index_client._client.put_mapping(
+                self.index_client.index_name,
+                type_,
+                getattr(mdata, 'old_{}_mapping'.format(type_)))
+
+    def test_index_updated(self):
+        """Ensure that running upgrade reindexes charms."""
+        self.put_old_mapping()
+        self.versions.ensure_initialized(self.data_store, True)
+        ## Pretend upgrades through 26 are complete.
+        self.data_store.update_version(26)
+        charm = factory.get_charm_json()
+        self.db.charms.insert(charm)
+        self.assertFalse(self.exists_in_index(charm['_id']))
+        self.sync_index()
+        self.assertTrue(self.exists_in_index(charm['_id']))
+        self.versions.upgrade(self.data_store, self.index_client, True)
+        self.assertEquals(self.data_store.current_version, self.version)
+        self.assertTrue(self.exists_in_index(charm['_id']))
+
+    def test_exception_raised(self):
+        """Putting new mapping without creating the filter and analyzer
+        raises the expected exception.
+        """
+        self.versions.ensure_initialized(self.data_store, True)
+        ## Pretend upgrades through 26 are complete.
+        self.data_store.update_version(26)
+        self.put_old_mapping()
+        charm = factory.get_charm_json()
+        self.assertFalse(self.exists_in_index(charm['_id']))
+        self.index_client.index_charm(charm)
+        self.assertTrue(self.exists_in_index(charm['_id']))
+        with self.assertRaises(ElasticHttpError) as e:
+            self.index_client.put_mapping()
+            self.assertEquals(e.status_code, 400)
+            self.assertEquals(e.error,
+                              u'MapperParsingException[Analyzer [n3_20grams] '
+                              u'not found for field [ngrams]]')

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2014-05-15 20:36:22 +0000
+++ charmworld/search.py	2014-05-20 14:13:54 +0000
@@ -180,43 +180,6 @@
                                 ]
                             }
                         }
-                    },
-                    "mappings": {
-                        "charm": {
-                            "properties": {
-                                "name": {
-                                    "type": "multi_field",
-                                    "fields": {
-                                        "ngrams": {
-                                            "type": "string",
-                                            "analyzer": "n3_20grams"
-                                        },
-                                        "name": {
-                                            "type": "string",
-                                            "index": "not_analyzed"
-                                        }
-                                    }
-                                }
-                            }
-
-                        },
-                        "bundle": {
-                            "properties": {
-                                "name": {
-                                    "type": "multi_field",
-                                    "fields": {
-                                        "ngrams": {
-                                            "type": "string",
-                                            "analyzer": "n3_20grams"
-                                        },
-                                        "name": {
-                                            "type": "string",
-                                            "index": "not_analyzed"
-                                        }
-                                    }
-                                }
-                            }
-                        }
                     }
                 }
             })