charmworld

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

  1. bug-1379397
  2. Merge into trunk
Proposed by Brad Crittenden
Status: Merged
Approved by: Brad Crittenden
Approved revision: 520
Merged at revision: 518
Proposed branch: lp:~bac/charmworld/bug-1379397
Merge into: lp:charmworld
Diff against target: 190 lines (+82/-7)
3 files modified
charmworld/search.py (+16/-1)
charmworld/tests/test_search.py (+42/-0)
charmworld/views/api/__init__.py (+24/-6)
To merge this branch: bzr merge lp:~bac/charmworld/bug-1379397
Reviewer Review Type Date Requested Status
Juju Gui Bot continuous-integration Approve
Charmworld Developers Pending
Review via email: mp+238457@code.launchpad.net

Commit message

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

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (8.6 KiB)

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...

Read more...

Revision history for this message
j.c.sackett (jcsackett) wrote :

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/

Revision history for this message
Brad Crittenden (bac) wrote :

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/

Revision history for this message
Brad Crittenden (bac) wrote :

.

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 updated
520. By Brad Crittenden

Rearrange code to be more clear.

Revision history for this message
Brad Crittenden (bac) wrote :

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/

Revision history for this message
Juju Gui Bot (juju-gui-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
63 """Raised when a negative limit is specified."""63 """Raised when a negative limit is specified."""
6464
6565
66class InvalidStart(Exception):
67 """Raised when an invalid 'start' is specified."""
68
69
66class IncompatibleMapping(Exception):70class IncompatibleMapping(Exception):
67 """Raised when desired mapping is not compatible with active mapping."""71 """Raised when desired mapping is not compatible with active mapping."""
6872
@@ -615,7 +619,7 @@
615 def api_search(self, text='', filters=None, type_=None,619 def api_search(self, text='', filters=None, type_=None,
616 limit=None, valid_only=True, sort=None,620 limit=None, valid_only=True, sort=None,
617 autocomplete=False, doctype=CHARM,621 autocomplete=False, doctype=CHARM,
618 min_score=None):622 min_score=None, start=None):
619 """Search charms and bundles (as used by the API).623 """Search charms and bundles (as used by the API).
620624
621 :param text: A string to search for -- will be analyzed (i.e. split625 :param text: A string to search for -- will be analyzed (i.e. split
@@ -641,12 +645,18 @@
641 overridden by use of 'charm:' or 'bundle:' query string prefix.645 overridden by use of 'charm:' or 'bundle:' query string prefix.
642 :param min_score: Only return results with a score of at least646 :param min_score: Only return results with a score of at least
643 min_score.647 min_score.
648 :param start: Return the 'limit' number of items from the start
649 index, e.g. items[start:start+limit]. Start is zero-based.
644 return: A list of {doctype=<charm|bundle>, data=entity}.650 return: A list of {doctype=<charm|bundle>, data=entity}.
645 """651 """
646 if limit is not None:652 if limit is not None:
647 if limit < 0:653 if limit < 0:
648 raise NegativeLimit()654 raise NegativeLimit()
649655
656 if start is not None:
657 if start < 0:
658 raise InvalidStart()
659
650 text, doctype, all_requested = extract_doctype_from_search_terms(660 text, doctype, all_requested = extract_doctype_from_search_terms(
651 text, doctype)661 text, doctype)
652662
@@ -679,8 +689,13 @@
679 else:689 else:
680 limit = min(limit, self.default_search_limit)690 limit = min(limit, self.default_search_limit)
681691
692 if start is not None and limit is not None:
693 limit = limit + start
694
682 try:695 try:
683 hits = self._unlimited_search(dsl, limit)696 hits = self._unlimited_search(dsl, limit)
697 if start is not None:
698 hits = hits[start:]
684 except ElasticHttpError:699 except ElasticHttpError:
685 hits = []700 hits = []
686701
687702
=== 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 @@
35 IndexMissing,35 IndexMissing,
36 IndexNotReady,36 IndexNotReady,
37 InvalidCharmType,37 InvalidCharmType,
38 InvalidStart,
38 update,39 update,
39 NegativeLimit,40 NegativeLimit,
40 reindex,41 reindex,
@@ -801,6 +802,47 @@
801 result = self.index_client.api_search('charms', {}, None)802 result = self.index_client.api_search('charms', {}, None)
802 self.assertEqual(22, len(result))803 self.assertEqual(22, len(result))
803804
805 def test_charms_start(self):
806 """Start provides the starting offset for results."""
807 result = self.index_client.api_search(
808 '', {}, None, limit=6, min_score=0, start=0)
809 self.assertEqual(0, len(result))
810 # Make ten charms
811 for i in range(10):
812 self.makeCharm()
813 # Starting at 0, all charms are returned.
814 result = self.index_client.api_search(
815 '', {}, None, min_score=0, start=0)
816 self.assertEqual(10, len(result))
817 # Starting at 1, nine are returned.
818 result = self.index_client.api_search(
819 '', {}, None, min_score=0, start=1)
820 self.assertEqual(9, len(result))
821 # Starting at 9, one is returned.
822 result = self.index_client.api_search(
823 '', {}, None, min_score=0, start=9)
824 self.assertEqual(1, len(result))
825 # Test in combination with limit.
826 # Starting at 0, the limit number are returned.
827 result = self.index_client.api_search(
828 '', {}, None, limit=5, min_score=0, start=0)
829 self.assertEqual(5, len(result))
830 # Starting at 3, the limit number are still returned.
831 result = self.index_client.api_search(
832 '', {}, None, limit=5, min_score=0, start=3)
833 self.assertEqual(5, len(result))
834 # If we start beyond 'limit' point, not all are returned.
835 result = self.index_client.api_search(
836 '', {}, None, limit=5, min_score=0, start=6)
837 self.assertEqual(4, len(result))
838 # Start is zero-based, so asking for start=10 returns no results.
839 result = self.index_client.api_search(
840 '', {}, None, limit=1, min_score=0, start=10)
841 self.assertEqual(0, len(result))
842 with self.assertRaises(InvalidStart):
843 result = self.index_client.api_search(
844 '', {}, None, start=-1, min_score=0)
845
804 def test_api_search_with_store_error(self):846 def test_api_search_with_store_error(self):
805 """api_search() does not return charms withs errors by default."""847 """api_search() does not return charms withs errors by default."""
806 self.makeCharm(name='evil-charm', charm_error=True)848 self.makeCharm(name='evil-charm', charm_error=True)
807849
=== 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 @@
37 BUNDLE,37 BUNDLE,
38 CHARM,38 CHARM,
39 InvalidCharmType,39 InvalidCharmType,
40 InvalidStart,
40 NegativeLimit,41 NegativeLimit,
41)42)
42from charmworld.utils import (43from charmworld.utils import (
@@ -716,7 +717,7 @@
716 def _search(self, limit=None, name=None, series=None, owner=None,717 def _search(self, limit=None, name=None, series=None, owner=None,
717 provides=None, requires=None, type_=None, provider=None,718 provides=None, requires=None, type_=None, provider=None,
718 scope=None, categories=None, text=None, autocomplete=False,719 scope=None, categories=None, text=None, autocomplete=False,
719 doctype=None, min_score=None):720 doctype=None, min_score=None, start=None):
720 """Search for charms and bundles matching parameters.721 """Search for charms and bundles matching parameters.
721722
722 :limit: A query limit. (The max number of results.) Dispatched as a723 :limit: A query limit. (The max number of results.) Dispatched as a
@@ -724,6 +725,11 @@
724 :min_score: The minimum score for filtering. Dispatched as a725 :min_score: The minimum score for filtering. Dispatched as a
725 list of numeric characters representing a float, e.g. ['1.1'].726 list of numeric characters representing a float, e.g. ['1.1'].
726 """727 """
728
729 def _check_scalar(value):
730 if value is not None and len(value) > 1:
731 raise ValueError()
732
727 autocomplete = autocomplete == ['true']733 autocomplete = autocomplete == ['true']
728 params = dict((key, value) for key, value in locals().items()734 params = dict((key, value) for key, value in locals().items()
729 if key in ('series', 'owner', 'name', 'categories'))735 if key in ('series', 'owner', 'name', 'categories'))
@@ -734,16 +740,24 @@
734 params['i_provides'] = provides740 params['i_provides'] = provides
735 params['i_requires'] = requires741 params['i_requires'] = requires
736 filters = dict(item for item in params.items() if item[1] is not None)742 filters = dict(item for item in params.items() if item[1] is not None)
743 for name in ('limit', 'start'):
744 try:
745 _check_scalar(locals()[name])
746 except ValueError:
747 return json_response(
748 406,
749 {
750 'type': 'multiple_values',
751 'parameter': name,
752 })
737 if limit is not None:753 if limit is not None:
738 if len(limit) > 1:
739 return json_response(406, {
740 'type': 'multiple_values',
741 'parameter': 'limit'})
742 limit = int(limit[0])754 limit = int(limit[0])
755 if start is not None:
756 start = int(start[0])
743 try:757 try:
744 results = self.request.index_client.api_search(758 results = self.request.index_client.api_search(
745 text[0], filters, type_, limit, autocomplete=autocomplete,759 text[0], filters, type_, limit, autocomplete=autocomplete,
746 doctype=doctype, min_score=min_score)760 doctype=doctype, min_score=min_score, start=start)
747 except InvalidCharmType as e:761 except InvalidCharmType as e:
748 return json_response(406, {762 return json_response(406, {
749 'type': 'unsupported_value',763 'type': 'unsupported_value',
@@ -753,6 +767,10 @@
753 return json_response(406, {767 return json_response(406, {
754 'type': 'negative_value',768 'type': 'negative_value',
755 'parameter': 'limit'})769 'parameter': 'limit'})
770 except InvalidStart:
771 return json_response(406, {
772 'type': 'invalid_value',
773 'parameter': 'start'})
756 return {'result': self._item_results(results)}774 return {'result': self._item_results(results)}
757775
758 def _item_results(self, items):776 def _item_results(self, items):

Subscribers

People subscribed via source and target branches

to all changes: