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
1=== modified file 'charmworld/search.py'
2--- charmworld/search.py 2014-07-15 19:09:59 +0000
3+++ charmworld/search.py 2014-10-15 18:06:00 +0000
4@@ -63,6 +63,10 @@
5 """Raised when a negative limit is specified."""
6
7
8+class InvalidStart(Exception):
9+ """Raised when an invalid 'start' is specified."""
10+
11+
12 class IncompatibleMapping(Exception):
13 """Raised when desired mapping is not compatible with active mapping."""
14
15@@ -615,7 +619,7 @@
16 def api_search(self, text='', filters=None, type_=None,
17 limit=None, valid_only=True, sort=None,
18 autocomplete=False, doctype=CHARM,
19- min_score=None):
20+ min_score=None, start=None):
21 """Search charms and bundles (as used by the API).
22
23 :param text: A string to search for -- will be analyzed (i.e. split
24@@ -641,12 +645,18 @@
25 overridden by use of 'charm:' or 'bundle:' query string prefix.
26 :param min_score: Only return results with a score of at least
27 min_score.
28+ :param start: Return the 'limit' number of items from the start
29+ index, e.g. items[start:start+limit]. Start is zero-based.
30 return: A list of {doctype=<charm|bundle>, data=entity}.
31 """
32 if limit is not None:
33 if limit < 0:
34 raise NegativeLimit()
35
36+ if start is not None:
37+ if start < 0:
38+ raise InvalidStart()
39+
40 text, doctype, all_requested = extract_doctype_from_search_terms(
41 text, doctype)
42
43@@ -679,8 +689,13 @@
44 else:
45 limit = min(limit, self.default_search_limit)
46
47+ if start is not None and limit is not None:
48+ limit = limit + start
49+
50 try:
51 hits = self._unlimited_search(dsl, limit)
52+ if start is not None:
53+ hits = hits[start:]
54 except ElasticHttpError:
55 hits = []
56
57
58=== modified file 'charmworld/tests/test_search.py'
59--- charmworld/tests/test_search.py 2014-07-15 19:09:59 +0000
60+++ charmworld/tests/test_search.py 2014-10-15 18:06:00 +0000
61@@ -35,6 +35,7 @@
62 IndexMissing,
63 IndexNotReady,
64 InvalidCharmType,
65+ InvalidStart,
66 update,
67 NegativeLimit,
68 reindex,
69@@ -801,6 +802,47 @@
70 result = self.index_client.api_search('charms', {}, None)
71 self.assertEqual(22, len(result))
72
73+ def test_charms_start(self):
74+ """Start provides the starting offset for results."""
75+ result = self.index_client.api_search(
76+ '', {}, None, limit=6, min_score=0, start=0)
77+ self.assertEqual(0, len(result))
78+ # Make ten charms
79+ for i in range(10):
80+ self.makeCharm()
81+ # Starting at 0, all charms are returned.
82+ result = self.index_client.api_search(
83+ '', {}, None, min_score=0, start=0)
84+ self.assertEqual(10, len(result))
85+ # Starting at 1, nine are returned.
86+ result = self.index_client.api_search(
87+ '', {}, None, min_score=0, start=1)
88+ self.assertEqual(9, len(result))
89+ # Starting at 9, one is returned.
90+ result = self.index_client.api_search(
91+ '', {}, None, min_score=0, start=9)
92+ self.assertEqual(1, len(result))
93+ # Test in combination with limit.
94+ # Starting at 0, the limit number are returned.
95+ result = self.index_client.api_search(
96+ '', {}, None, limit=5, min_score=0, start=0)
97+ self.assertEqual(5, len(result))
98+ # Starting at 3, the limit number are still returned.
99+ result = self.index_client.api_search(
100+ '', {}, None, limit=5, min_score=0, start=3)
101+ self.assertEqual(5, len(result))
102+ # If we start beyond 'limit' point, not all are returned.
103+ result = self.index_client.api_search(
104+ '', {}, None, limit=5, min_score=0, start=6)
105+ self.assertEqual(4, len(result))
106+ # Start is zero-based, so asking for start=10 returns no results.
107+ result = self.index_client.api_search(
108+ '', {}, None, limit=1, min_score=0, start=10)
109+ self.assertEqual(0, len(result))
110+ with self.assertRaises(InvalidStart):
111+ result = self.index_client.api_search(
112+ '', {}, None, start=-1, min_score=0)
113+
114 def test_api_search_with_store_error(self):
115 """api_search() does not return charms withs errors by default."""
116 self.makeCharm(name='evil-charm', charm_error=True)
117
118=== modified file 'charmworld/views/api/__init__.py'
119--- charmworld/views/api/__init__.py 2014-05-06 20:49:19 +0000
120+++ charmworld/views/api/__init__.py 2014-10-15 18:06:00 +0000
121@@ -37,6 +37,7 @@
122 BUNDLE,
123 CHARM,
124 InvalidCharmType,
125+ InvalidStart,
126 NegativeLimit,
127 )
128 from charmworld.utils import (
129@@ -716,7 +717,7 @@
130 def _search(self, limit=None, name=None, series=None, owner=None,
131 provides=None, requires=None, type_=None, provider=None,
132 scope=None, categories=None, text=None, autocomplete=False,
133- doctype=None, min_score=None):
134+ doctype=None, min_score=None, start=None):
135 """Search for charms and bundles matching parameters.
136
137 :limit: A query limit. (The max number of results.) Dispatched as a
138@@ -724,6 +725,11 @@
139 :min_score: The minimum score for filtering. Dispatched as a
140 list of numeric characters representing a float, e.g. ['1.1'].
141 """
142+
143+ def _check_scalar(value):
144+ if value is not None and len(value) > 1:
145+ raise ValueError()
146+
147 autocomplete = autocomplete == ['true']
148 params = dict((key, value) for key, value in locals().items()
149 if key in ('series', 'owner', 'name', 'categories'))
150@@ -734,16 +740,24 @@
151 params['i_provides'] = provides
152 params['i_requires'] = requires
153 filters = dict(item for item in params.items() if item[1] is not None)
154+ for name in ('limit', 'start'):
155+ try:
156+ _check_scalar(locals()[name])
157+ except ValueError:
158+ return json_response(
159+ 406,
160+ {
161+ 'type': 'multiple_values',
162+ 'parameter': name,
163+ })
164 if limit is not None:
165- if len(limit) > 1:
166- return json_response(406, {
167- 'type': 'multiple_values',
168- 'parameter': 'limit'})
169 limit = int(limit[0])
170+ if start is not None:
171+ start = int(start[0])
172 try:
173 results = self.request.index_client.api_search(
174 text[0], filters, type_, limit, autocomplete=autocomplete,
175- doctype=doctype, min_score=min_score)
176+ doctype=doctype, min_score=min_score, start=start)
177 except InvalidCharmType as e:
178 return json_response(406, {
179 'type': 'unsupported_value',
180@@ -753,6 +767,10 @@
181 return json_response(406, {
182 'type': 'negative_value',
183 'parameter': 'limit'})
184+ except InvalidStart:
185+ return json_response(406, {
186+ 'type': 'invalid_value',
187+ 'parameter': 'start'})
188 return {'result': self._item_results(results)}
189
190 def _item_results(self, items):

Subscribers

People subscribed via source and target branches

to all changes: