Merge lp:~bac/charmworld/bug-1379397 into lp:charmworld

[Brad Crittenden]

Add 'start' parameter to API search.

The start is the zero-based index into the search results. It can be used
in conjunction with limit.

https://codereview.appspot.com/158010043/

R=jcsackett

Add 'start' parameter to API search.

The start is the zero-based index into the search results. It can be used
in conjunction with limit.

https://codereview.appspot.com/158010043/

[Brad Crittenden (bac)]

Reviewers: mp+238457_code.launchpad.net,

Message:
Please take a look.

Description:
Add 'start' parameter to API search.

The start is the zero-based index into the search results. It can be
used
in conjunction with limit.

https://code.launchpad.net/~bac/charmworld/bug-1379397/+merge/238457

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/158010043/

Affected files (+85, -7 lines):
   A [revision details]
   M charmworld/search.py
   M charmworld/tests/test_search.py
   M charmworld/views/api/__init__.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140905151929-pbmcu2i5i3s9vgkx
+New revision: <email address hidden>

Index: charmworld/search.py
=== modified file 'charmworld/search.py'
--- charmworld/search.py 2014-07-15 19:09:59 +0000
+++ charmworld/search.py 2014-10-15 14:45:53 +0000
@@ -63,6 +63,10 @@
      """Raised when a negative limit is specified."""

+class InvalidStart(Exception):
+ """Raised when an invalid 'start' is specified."""
+
+
  class IncompatibleMapping(Exception):
      """Raised when desired mapping is not compatible with active
mapping."""

@@ -615,7 +619,7 @@
      def api_search(self, text='', filters=None, type_=None,
                     limit=None, valid_only=True, sort=None,
                     autocomplete=False, doctype=CHARM,
- min_score=None):
+ min_score=None, start=None):
          """Search charms and bundles (as used by the API).

          :param text: A string to search for -- will be analyzed (i.e. split
@@ -641,12 +645,18 @@
              overridden by use of 'charm:' or 'bundle:' query string prefix.
          :param min_score: Only return results with a score of at least
              min_score.
+ :param start: Return the 'limit' number of items from the start
+ index, e.g. items[start:start+limit]. Start is zero-based.
          return: A list of {doctype=<charm|bundle>, data=entity}.
          """
          if limit is not None:
              if limit < 0:
                  raise NegativeLimit()

+ if start is not None:
+ if start < 0:
+ raise InvalidStart()
+
          text, doctype, all_requested = extract_doctype_from_search_terms(
              text, doctype)

@@ -679,11 +689,17 @@
              else:
                  limit = min(limit, self.default_search_limit)

+ if start is not None and limit is not None:
+ limit = limit + start
+
          try:
              hits = self._unlimited_search(dsl, limit)
          except ElasticHttpError:
              hits = []

+ if start is not None:
+ hits = hits[start:]
+
          return [
              dict(doctype=hit['_type'], data=hit['_source']['data'])
              for hit in hits]

Index: charmworld/tests/test_search.py
=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py 2014-07-15 19:09:59 +0000
+++ charmworld/tests/test_search.py 2014-10-15 14...

[j.c.sackett (jcsackett)]

Brad--

Thanks, just one minor note.

https://codereview.appspot.com/158010043/diff/1/charmworld/search.py
File charmworld/search.py (right):

https://codereview.appspot.com/158010043/diff/1/charmworld/search.py#newcode701
charmworld/search.py:701: hits = hits[start:]
If there's an ElasticHttpError hits will be []; couldn't this then throw
an IndexError?

Perhaps this should be in the try: block above.

https://codereview.appspot.com/158010043/

[Brad Crittenden (bac)]

Thanks for the review JC.

The code works as is but I'll move it up to avoid any confusion.

https://codereview.appspot.com/158010043/

[Brad Crittenden (bac)]

.

https://codereview.appspot.com/158010043/diff/1/charmworld/search.py
File charmworld/search.py (right):

https://codereview.appspot.com/158010043/diff/1/charmworld/search.py#newcode701
charmworld/search.py:701: hits = hits[start:]
The way it is is harmless:

>>> h = []
>>> h[5:]
[]

https://codereview.appspot.com/158010043/

[lp:~bac/charmworld/bug-1379397]

520. By Brad Crittenden on 2014-10-15

Rearrange code to be more clear.

[Brad Crittenden (bac)]

Jon I assume you intended to mark the review as LGTM but forgot.

So I'm doing it myself so I can land. Apologies if that wasn't your
intent.

--bac

https://codereview.appspot.com/158010043/

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

review: Approve (continuous-integration)

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2014-07-15 19:09:59 +0000
+++ charmworld/search.py	2014-10-15 18:06:00 +0000
@@ -63,6 +63,10 @@
     """Raised when a negative limit is specified."""
 
 
+class InvalidStart(Exception):
+    """Raised when an invalid 'start' is specified."""
+
+
 class IncompatibleMapping(Exception):
     """Raised when desired mapping is not compatible with active mapping."""
 
@@ -615,7 +619,7 @@
     def api_search(self, text='', filters=None, type_=None,
                    limit=None, valid_only=True, sort=None,
                    autocomplete=False, doctype=CHARM,
-                   min_score=None):
+                   min_score=None, start=None):
         """Search charms and bundles (as used by the API).
 
         :param text: A string to search for -- will be analyzed (i.e. split
@@ -641,12 +645,18 @@
             overridden by use of 'charm:' or 'bundle:' query string prefix.
         :param min_score: Only return results with a score of at least
             min_score.
+        :param start: Return the 'limit' number of items from the start
+            index, e.g. items[start:start+limit].  Start is zero-based.
         return: A list of {doctype=<charm|bundle>, data=entity}.
         """
         if limit is not None:
             if limit < 0:
                 raise NegativeLimit()
 
+        if start is not None:
+            if start < 0:
+                raise InvalidStart()
+
         text, doctype, all_requested = extract_doctype_from_search_terms(
             text, doctype)
 
@@ -679,8 +689,13 @@
             else:
                 limit = min(limit, self.default_search_limit)
 
+        if start is not None and limit is not None:
+            limit = limit + start
+
         try:
             hits = self._unlimited_search(dsl, limit)
+            if start is not None:
+                hits = hits[start:]
         except ElasticHttpError:
             hits = []
 

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2014-07-15 19:09:59 +0000
+++ charmworld/tests/test_search.py	2014-10-15 18:06:00 +0000
@@ -35,6 +35,7 @@
     IndexMissing,
     IndexNotReady,
     InvalidCharmType,
+    InvalidStart,
     update,
     NegativeLimit,
     reindex,
@@ -801,6 +802,47 @@
         result = self.index_client.api_search('charms', {}, None)
         self.assertEqual(22, len(result))
 
+    def test_charms_start(self):
+        """Start provides the starting offset for results."""
+        result = self.index_client.api_search(
+            '', {}, None, limit=6, min_score=0, start=0)
+        self.assertEqual(0, len(result))
+        # Make ten charms
+        for i in range(10):
+            self.makeCharm()
+        # Starting at 0, all charms are returned.
+        result = self.index_client.api_search(
+            '', {}, None, min_score=0, start=0)
+        self.assertEqual(10, len(result))
+        # Starting at 1, nine are returned.
+        result = self.index_client.api_search(
+            '', {}, None, min_score=0, start=1)
+        self.assertEqual(9, len(result))
+        # Starting at 9, one is returned.
+        result = self.index_client.api_search(
+            '', {}, None, min_score=0, start=9)
+        self.assertEqual(1, len(result))
+        # Test in combination with limit.
+        # Starting at 0, the limit number are returned.
+        result = self.index_client.api_search(
+            '', {}, None, limit=5, min_score=0, start=0)
+        self.assertEqual(5, len(result))
+        # Starting at 3, the limit number are still returned.
+        result = self.index_client.api_search(
+            '', {}, None, limit=5, min_score=0, start=3)
+        self.assertEqual(5, len(result))
+        # If we start beyond 'limit' point, not all are returned.
+        result = self.index_client.api_search(
+            '', {}, None, limit=5, min_score=0, start=6)
+        self.assertEqual(4, len(result))
+        # Start is zero-based, so asking for start=10 returns no results.
+        result = self.index_client.api_search(
+            '', {}, None, limit=1, min_score=0, start=10)
+        self.assertEqual(0, len(result))
+        with self.assertRaises(InvalidStart):
+            result = self.index_client.api_search(
+                '', {}, None, start=-1, min_score=0)
+
     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-05-06 20:49:19 +0000
+++ charmworld/views/api/__init__.py	2014-10-15 18:06:00 +0000
@@ -37,6 +37,7 @@
     BUNDLE,
     CHARM,
     InvalidCharmType,
+    InvalidStart,
     NegativeLimit,
 )
 from charmworld.utils import (
@@ -716,7 +717,7 @@
     def _search(self, limit=None, name=None, series=None, owner=None,
                 provides=None, requires=None, type_=None, provider=None,
                 scope=None, categories=None, text=None, autocomplete=False,
-                doctype=None, min_score=None):
+                doctype=None, min_score=None, start=None):
         """Search for charms and bundles matching parameters.
 
         :limit: A query limit.  (The max number of results.)  Dispatched as a
@@ -724,6 +725,11 @@
         :min_score: The minimum score for filtering.  Dispatched as a
            list of numeric characters representing a float, e.g. ['1.1'].
         """
+
+        def _check_scalar(value):
+            if value is not None and len(value) > 1:
+                raise ValueError()
+
         autocomplete = autocomplete == ['true']
         params = dict((key, value) for key, value in locals().items()
                       if key in ('series', 'owner', 'name', 'categories'))
@@ -734,16 +740,24 @@
         params['i_provides'] = provides
         params['i_requires'] = requires
         filters = dict(item for item in params.items() if item[1] is not None)
+        for name in ('limit', 'start'):
+            try:
+                _check_scalar(locals()[name])
+            except ValueError:
+                return json_response(
+                    406,
+                    {
+                        'type': 'multiple_values',
+                        'parameter': name,
+                    })
         if limit is not None:
-            if len(limit) > 1:
-                return json_response(406, {
-                                     'type': 'multiple_values',
-                                     'parameter': 'limit'})
             limit = int(limit[0])
+        if start is not None:
+            start = int(start[0])
         try:
             results = self.request.index_client.api_search(
                 text[0], filters, type_, limit, autocomplete=autocomplete,
-                doctype=doctype, min_score=min_score)
+                doctype=doctype, min_score=min_score, start=start)
         except InvalidCharmType as e:
             return json_response(406, {
                                  'type': 'unsupported_value',
@@ -753,6 +767,10 @@
             return json_response(406, {
                                  'type': 'negative_value',
                                  'parameter': 'limit'})
+        except InvalidStart:
+            return json_response(406, {
+                                 'type': 'invalid_value',
+                                 'parameter': 'start'})
         return {'result': self._item_results(results)}
 
     def _item_results(self, items):