Code review comment for lp:~bac/charmworld/bug-1379397

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:45:53 +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)

Index: charmworld/views/api/__init__.py
=== 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 14:45:53 +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):