From 8866961fe236ecdd8b2ed6eaf7510b6bd6f3daf3 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Fri, 17 Jun 2016 12:21:51 +0100 Subject: [PATCH 1/4] [#3120] Move _get_page_number to helpers --- ckan/controllers/feed.py | 4 ++-- ckan/controllers/group.py | 4 ++-- ckan/controllers/package.py | 2 +- ckan/controllers/revision.py | 2 +- ckan/controllers/tag.py | 2 +- ckan/controllers/user.py | 2 +- ckan/lib/base.py | 18 ------------------ ckan/lib/helpers.py | 21 +++++++++++++++++++++ 8 files changed, 29 insertions(+), 26 deletions(-) diff --git a/ckan/controllers/feed.py b/ckan/controllers/feed.py index 3b41af9d80d..9da30215a4b 100644 --- a/ckan/controllers/feed.py +++ b/ckan/controllers/feed.py @@ -310,7 +310,7 @@ def custom(self): search_params[param] = value fq += ' %s:"%s"' % (param, value) - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) limit = ITEMS_LIMIT data_dict = { @@ -455,7 +455,7 @@ def _parse_url_params(self): Returns the constructed search-query dict, and the valid URL query parameters. """ - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) limit = ITEMS_LIMIT data_dict = { diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index bd353537aaa..fb05af7a05f 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -150,7 +150,7 @@ def add_group_type(cls, group_type): def index(self): group_type = self._guess_group_type() - page = self._get_page_number(request.params) or 1 + page = h.get_page_number(request.params) or 1 items_per_page = 21 context = {'model': model, 'session': model.Session, @@ -247,7 +247,7 @@ def _read(self, id, limit, group_type): context['return_query'] = True - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) # most search operations should reset the page counter: params_nopage = [(k, v) for k, v in request.params.items() diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 5b41aa95e81..3aed9543437 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -146,7 +146,7 @@ def search(self): # unicode format (decoded from utf8) q = c.q = request.params.get('q', u'') c.query_error = False - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) limit = g.datasets_per_page diff --git a/ckan/controllers/revision.py b/ckan/controllers/revision.py index 2c5962f943b..dd3527cc4ea 100644 --- a/ckan/controllers/revision.py +++ b/ckan/controllers/revision.py @@ -124,7 +124,7 @@ def list(self): query = model.Session.query(model.Revision) c.page = h.Page( collection=query, - page=self._get_page_number(request.params), + page=h.get_page_number(request.params), url=h.pager_url, items_per_page=20 ) diff --git a/ckan/controllers/tag.py b/ckan/controllers/tag.py index 67b761af444..dc857d3b7d6 100644 --- a/ckan/controllers/tag.py +++ b/ckan/controllers/tag.py @@ -36,7 +36,7 @@ def index(self): data_dict = {'all_fields': True} if c.q: - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) data_dict['q'] = c.q data_dict['limit'] = LIMIT data_dict['offset'] = (page - 1) * LIMIT diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index c7637fa122d..3f758f7bf67 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -97,7 +97,7 @@ def _get_repoze_handler(self, handler_name): handler_name) def index(self): - page = self._get_page_number(request.params) + page = h.get_page_number(request.params) c.q = request.params.get('q', '') c.order_by = request.params.get('order_by', 'name') diff --git a/ckan/lib/base.py b/ckan/lib/base.py index 86b10c1399c..1dfe6476ab1 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -377,24 +377,6 @@ def _get_user_for_apikey(self): user = query.filter_by(apikey=apikey).first() return user - def _get_page_number(self, params, key='page', default=1): - """ - Returns the page number from the provided params after - verifies that it is an integer. - - If it fails it will abort the request with a 400 error - """ - p = params.get(key, default) - - try: - p = int(p) - if p < 1: - raise ValueError("Negative number not allowed") - except ValueError, e: - abort(400, ('"page" parameter must be a positive integer')) - - return p - # Include the '_' function in the public names __all__ = [__name for __name in locals().keys() if not __name.startswith('_') diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 5ae9929416a..b1c061317b5 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -1081,6 +1081,27 @@ def _range(self, regexp_match): return re.sub(current_page_span, current_page_link, html) +@core_helper +def get_page_number(params, key='page', default=1): + ''' + Return the page number from the provided params after verifying that it is + an positive integer. + + If it fails it will abort the request with a 400 error. + ''' + p = params.get(key, default) + + try: + p = int(p) + if p < 1: + raise ValueError("Negative number not allowed") + except ValueError: + import ckan.lib.base as base + base.abort(400, ('"page" parameter must be a positive integer')) + + return p + + @core_helper def get_display_timezone(): ''' Returns a pytz timezone for the display_timezone setting in the From 0d9f680fbdaa2abfc758b974d4c30ea67299ed55 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Fri, 17 Jun 2016 12:58:56 +0100 Subject: [PATCH 2/4] [#3120] Remove module var PAGINATE_ITEMS_PER_PAGE The PAGINATE_ITEMS_PER_PAGE variable was used in previous versions of the BaseController, but the related code has since been removed. --- ckan/lib/base.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ckan/lib/base.py b/ckan/lib/base.py index 1dfe6476ab1..959053f0e8d 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -34,8 +34,6 @@ log = logging.getLogger(__name__) -PAGINATE_ITEMS_PER_PAGE = 50 - APIKEY_HEADER_NAME_KEY = 'apikey_header_name' APIKEY_HEADER_NAME_DEFAULT = 'X-CKAN-API-Key' From 811d8889bf6a49d5eb2f1a833fa26a0d45eec4f2 Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Fri, 17 Jun 2016 13:04:30 +0100 Subject: [PATCH 3/4] [#3120] Remove module var ALLOWED_FIELDSET_PARAMS. ALLOWED_FIELDSET_PARAMS was used in the BaseController, but the method that used it has since been removed. --- ckan/lib/base.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ckan/lib/base.py b/ckan/lib/base.py index 959053f0e8d..5b0241a4cef 100644 --- a/ckan/lib/base.py +++ b/ckan/lib/base.py @@ -37,8 +37,6 @@ APIKEY_HEADER_NAME_KEY = 'apikey_header_name' APIKEY_HEADER_NAME_DEFAULT = 'X-CKAN-API-Key' -ALLOWED_FIELDSET_PARAMS = ['package_form', 'restrict'] - def abort(status_code=None, detail='', headers=None, comment=None): '''Abort the current request immediately by returning an HTTP exception. From 5ce8284c1f127f1f8b4279bbc96a2422b13bef6a Mon Sep 17 00:00:00 2001 From: Brook Elgie Date: Fri, 17 Jun 2016 14:40:32 +0100 Subject: [PATCH 4/4] [#3120] Change working of abort message. 'key' can be defined in the call, so may not always be 'page'. --- ckan/lib/helpers.py | 2 +- ckan/tests/controllers/test_feed.py | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index b1c061317b5..a19ade3132c 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -1097,7 +1097,7 @@ def get_page_number(params, key='page', default=1): raise ValueError("Negative number not allowed") except ValueError: import ckan.lib.base as base - base.abort(400, ('"page" parameter must be a positive integer')) + base.abort(400, ('"{key}" parameter must be a positive integer')) return p diff --git a/ckan/tests/controllers/test_feed.py b/ckan/tests/controllers/test_feed.py index 0fc448544eb..57adcade8ea 100644 --- a/ckan/tests/controllers/test_feed.py +++ b/ckan/tests/controllers/test_feed.py @@ -2,7 +2,6 @@ from routes import url_for -from ckan import model import ckan.tests.helpers as helpers import ckan.tests.factories as factories @@ -15,7 +14,7 @@ def test_atom_feed_page_zero_gives_error(self): id=group['name']) + '?page=0' app = self._get_test_app() res = app.get(offset, status=400) - assert '"page" parameter must be a positive integer' in res, res + assert '"{key}" parameter must be a positive integer' in res, res def test_atom_feed_page_negative_gives_error(self): group = factories.Group() @@ -23,7 +22,7 @@ def test_atom_feed_page_negative_gives_error(self): id=group['name']) + '?page=-2' app = self._get_test_app() res = app.get(offset, status=400) - assert '"page" parameter must be a positive integer' in res, res + assert '"{key}" parameter must be a positive integer' in res, res def test_atom_feed_page_not_int_gives_error(self): group = factories.Group() @@ -31,4 +30,4 @@ def test_atom_feed_page_not_int_gives_error(self): id=group['name']) + '?page=abc' app = self._get_test_app() res = app.get(offset, status=400) - assert '"page" parameter must be a positive integer' in res, res + assert '"{key}" parameter must be a positive integer' in res, res