Merge lp:~adeuring/charmworld/check-charm-exists-in-store-1160527 into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

return only charms without errors in search() and api_search() by default.

Return only valid charms in elasticsearch results.

For each (possible) charm it knows about, charmworld records its status
in the main charm store (store.juju.ubuntu.com), but we did not
use this status information.

One important detail we ignored is if the charm store says that a given
charm is valid, or if it exists at all. We store this kind of status
information as a dictionary charm['store_data']; if the store "sees" an
error, this dictionary has the key 'errors'.

This branch adds an additional elasticsearch filter to the "search
expression" used by ElasticSearchClient.api_search() and
ElasticSearchClient.search() that ensures that only valid charms
are returned in search results. This behaviour can be changed by
setting the new parameter valid_charm_only to False.

As a drive-by fix, I changed the the conversion from a datetime instance
to "seconds since the Unix epoch" so that the local timezone is not
used. This fixes a failure of test_api.TestAPI0Charms.test_charms()
that is not related to my changes.

https://codereview.appspot.com/8641044/

[Aaron Bentley (abentley)]

On 2013/04/15 15:02:29, abel.deuring wrote:
> Please take a look.

lgtm

https://codereview.appspot.com/8641044/

[Abel Deuring (adeuring)]

review: Approve

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-04-11 16:40:32 +0000
+++ charmworld/search.py	2013-04-15 15:08:44 +0000
@@ -123,12 +123,16 @@
         return dsl
 
     @staticmethod
-    def _get_filtered(dsl, filters, type_):
+    def _get_filtered(dsl, filters, type_, valid_charms_only):
         and_clause = {
             'and': [
                 {'terms': {key: value}} for key, value in filters.items()
             ]
         }
+        if valid_charms_only:
+            and_clause['and'].append(
+                {'missing': {'field': 'store_data.errors'}}
+            )
         if type_ is not None:
             typeclauses = []
             for value in type_:
@@ -164,10 +168,11 @@
         result = self._client.get(self.index_name, 'charm', charm_id)
         return result['_source']
 
-    def api_search(self, text, filters, type_, limit=None):
+    def api_search(self, text, filters, type_, limit=None,
+                   valid_charms_only=True):
         dsl = self._get_text_query(text)
         dsl = self._get_official_boost(dsl)
-        dsl = self._get_filtered(dsl, filters, type_)
+        dsl = self._get_filtered(dsl, filters, type_, valid_charms_only)
         if limit is None:
             status = self._client.status(index=self.index_name)['indices']
             limit = status[self.index_name]['docs']['num_docs']
@@ -176,13 +181,14 @@
         hits = self._client.search(dsl, index=self.index_name, size=limit)
         return [hit['_source'] for hit in hits['hits']['hits']]
 
-    def search(self, terms):
+    def search(self, terms, valid_charms_only=True):
         size = None
         if terms == '':
             size = 0
 
         dsl = self._get_text_query(terms)
-        dsl = {'query': self._get_official_boost(dsl)}
+        dsl = self._get_official_boost(dsl)
+        dsl = self._get_filtered(dsl, {}, None, valid_charms_only)
 
         with Timer() as timer:
             status = self._client.status(index=self.index_name)['indices']

=== modified file 'charmworld/test_search.py'
--- charmworld/test_search.py	2013-04-11 18:08:49 +0000
+++ charmworld/test_search.py	2013-04-15 15:08:44 +0000
@@ -296,3 +296,32 @@
         self.assertEqual(0, len(result))
         with self.assertRaises(NegativeLimit):
             result = self.index_client.api_search('', {}, None, -1)
+
+    def test_api_search_charm_with_error(self):
+        """api_search() does not return charms withs errors by default."""
+        self.makeCharm(name='evil-charm', charm_error=True)
+        self.makeCharm(name='good-charm')
+        result = self.index_client.api_search('', {}, None)
+        self.assertEqual(1, len(result))
+        self.assertEqual('good-charm', result[0]['name'])
+        # Charms with error are included in the search result
+        # if the parameter valid_charms_only is set to False.
+        result = self.index_client.api_search(
+            '', {}, None, valid_charms_only=False)
+        self.assertEqual(2, len(result))
+        names = set(charm['name'] for charm in result)
+        self.assertEqual(set(('good-charm', 'evil-charm')), names)
+
+    def test_search_charm_with_error(self):
+        """search() does not return charms withs errors by default."""
+        self.makeCharm(name='evil-charm', owner='foo', charm_error=True)
+        self.makeCharm(name='good-charm', owner='foo')
+        result = self.index_client.search('foo')
+        self.assertEqual(1, result['matches'])
+        self.assertEqual('good-charm', result['results'][0]['data']['name'])
+        # Charms with error are included in the search result
+        # if the parameter valid_charms_only is set to False.
+        result = self.index_client.search('foo', valid_charms_only=False)
+        self.assertEqual(2, result['matches'])
+        names = set(entry['data']['name'] for entry in result['results'])
+        self.assertEqual(set(('good-charm', 'evil-charm')), names)

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-04-11 18:08:49 +0000
+++ charmworld/testing/factory.py	2013-04-15 15:08:44 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helpers to generate test data for use."""
+from calendar import timegm
 from datetime import date
 import hashlib
 import os
@@ -9,7 +10,7 @@
 from random import randint
 import random
 import string
-from time import mktime, time
+from time import time
 
 from charmworld.jobs.ingest import (
     JENKINS_PROVIDERS,
@@ -93,7 +94,7 @@
 
     if isinstance(created, date):
         # Change `created` dates should be timestamps.
-        created = mktime(created.timetuple())
+        created = timegm(created.timetuple())
     return {
         'authors': authors,
         'committer': committer,
@@ -107,7 +108,7 @@
                    series=None, owner=None, description=None, revision=1,
                    summary=None, revno=12, maintainer=None,
                    commit_message='maintainer', provides=None, requires=None,
-                   options=None, files=None):
+                   options=None, files=None, charm_error=False):
     """Return the json of a charm."""
     if not description:
         description = """byobu-class provides remote terminal access through
@@ -165,6 +166,18 @@
                 'subdir': 'hooks',
             },
         }
+    store_data = {
+            "address": "cs:precise/byobu-classroom",
+            "store_checked": "Thu Dec  6 17:42:05 2012"
+        }
+    if charm_error:
+        store_data["errors"] = ['entry not found']
+    else:
+        store_data.update({
+            "digest": "[e-mail]-20120723223205-n5zke9r9ahb40su",
+            "revision": 1,
+            "sha256": "107c230dc3481f8b8de9decfff6439a23caae2ea670bddb413ddf1",
+        })
     charm = {
         "bname": bname,
         "branch_dir": os.path.join(branch_root, name),
@@ -210,13 +223,7 @@
         "revision": revision,
         "series": series,
         "short_url": "/charms/precise/byobu-classroom",
-        "store_data": {
-            "address": "cs:precise/byobu-classroom",
-            "digest": "[e-mail]-20120723223205-n5zke9r9ahb40su",
-            "revision": 1,
-            "sha256": "107c230dc3481f8b8de9decfff6439a23caae2ea670bddb413ddf1",
-            "store_checked": "Thu Dec  6 17:42:05 2012"
-        },
+        "store_data": store_data,
         "summary": summary,
         'config': {
             'options': options,