Merge lp:~rharding/charmworld/apply-cowboys into lp:charmworld

[Richard Harding]

Update scoring to be limited on ES side.

- Drive-by to make sure we log the exception from ES for debugging purposes.

Update scoring to be limited on ES side.

- Drive-by to make sure we log the exception from ES for debugging purposes.

[Benji York (benji)]

There is a nice alternative to log.error(str(exc)) in the logging
package: log.exception()

If the logging is important, we should have tests to ensure it keeps
working.

If the default value for min_score were set to MIN_ES_SCORE then we
wouldn't need the inline-if expression to set if no value was provided.

review: Approve (code)

[Richard Harding (rharding)]

> There is a nice alternative to log.error(str(exc)) in the logging
> package: log.exception()

Thanks, will update.

> If the default value for min_score were set to MIN_ES_SCORE then we
> wouldn't need the inline-if expression to set if no value was provided.

I originally did this, but the caller would end up passing in a None value which would reset the default. So I did this instead to make sure you can adjust the min score, but you can't leave it None if you're not fetching all explicitly.

[lp:~rharding/charmworld/apply-cowboys]

490. By Richard Harding on 2014-03-18

Update per review

[Richard Harding (rharding)]

review: Approve

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

review: Approve (continuous-integration)

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2014-03-12 19:22:56 +0000
+++ charmworld/search.py	2014-03-18 15:21:03 +0000
@@ -20,6 +20,8 @@
     Timer,
 )
 
+MIN_ES_SCORE = 0.5
+
 # Where wildcards are used, the final var name must be used, to avoid
 # selecting any fields that may be integers, such as config.options.*.default,
 # provides.*.interface.limit, or changes.revno.  Selecting an integer field
@@ -553,7 +555,7 @@
     def api_search(self, text='', filters=None, type_=None,
                    limit=None, valid_only=True, sort=None,
                    autocomplete=False, doctype=CHARM,
-                   min_score=1.0):
+                   min_score=None):
         """Search charms and bundles (as used by the API).
 
         :param text: A string to search for -- will be analyzed (i.e. split
@@ -606,7 +608,12 @@
         elif sort is not None:
             raise ValueError('Invalid sort: %s.' % sort)
 
+        # Make sure we always get scores back from ES.
+        dsl['track_scores'] = True
+
         if not all_requested:
+            min_score = min_score if min_score is not None else MIN_ES_SCORE
+            dsl['min_score'] = min_score
             if limit is None:
                 limit = self.default_search_limit
             else:
@@ -617,20 +624,10 @@
         except ElasticHttpError:
             hits = []
 
-        # Sorted queries do not return scores, so don't filter if we have a
-        # sort.
-        if not all_requested and sort is None:
-            hits = self._filter_by_score(hits, min_score)
-
         return [
             dict(doctype=hit['_type'], data=hit['_source']['data'])
             for hit in hits]
 
-    @staticmethod
-    def _filter_by_score(results, min_score):
-        """Filter the results by a minimum score."""
-        return [res for res in results if res['_score'] >= min_score]
-
     def get_status(self):
         with translate_error():
             return self._client.status(index=self.index_name)['indices']
 
=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2014-03-12 19:22:56 +0000
+++ charmworld/tests/test_search.py	2014-03-18 15:21:03 +0000
@@ -5,7 +5,6 @@
 
 
 from datetime import datetime
-import random
 from textwrap import dedent
 from time import sleep
 
@@ -684,14 +683,6 @@
         result = self.index_client.api_search('charms', {}, None)
         self.assertEqual(22, len(result))
 
-    def test_filter_by_score(self):
-        hits = [{'_score': i} for i in xrange(10)]
-        random.shuffle(hits)
-        cutoff = 4
-        filtered = self.index_client._filter_by_score(hits, cutoff)
-        for item in filtered:
-            self.assertTrue(item['_score'] >= cutoff)
-
     def test_api_search_with_store_error(self):
         """api_search() does not return charms withs errors by default."""
         self.makeCharm(name='evil-charm', charm_error=True)
 
=== modified file 'charmworld/views/api/__init__.py'
--- charmworld/views/api/__init__.py	2014-03-12 19:22:56 +0000
+++ charmworld/views/api/__init__.py	2014-03-18 15:21:03 +0000
@@ -720,9 +720,7 @@
                       if key in ('series', 'owner', 'name', 'categories'))
         if text is None:
             text = ['']
-        if min_score is None:
-            min_score = 1
-        else:
+        if min_score is not None:
             min_score = float(min_score[0])
         params['i_provides'] = provides
         params['i_requires'] = requires
 
=== modified file 'charmworld/views/search.py'
--- charmworld/views/search.py	2013-11-15 20:02:03 +0000
+++ charmworld/views/search.py	2014-03-18 15:21:03 +0000
@@ -31,10 +31,11 @@
                                       request=request)
         response.status_code = 503
         return response
-    except ElasticHttpError:
+    except ElasticHttpError as exc:
         text = request.params["search_text"]
         log.warning(
             "User search error with search text='{}'".format(text))
+        log.exception(exc)
         response = render_to_response(
             '../templates/search-terms-bad.pt',
             {'search_text': text},