From 9d00e50e9de437e2b696c40d4590de7c33c064c4 Mon Sep 17 00:00:00 2001 From: Federico Ceratto Date: Tue, 22 Sep 2015 11:56:45 +0100 Subject: [PATCH] Fix #1494799 handle limit=max on v2 and Admin APIs Handle mixed case "max", raise ValueError on unexpected strings on v2 and Admin APIs. Add unit tests. Change-Id: I2cf8d11bd2f4bc4bdff9d67812f6eb7437eb59ae Closes-bug: 1494799 --- designate/api/admin/__init__.py | 5 ++ designate/api/admin/views/base.py | 25 +++++-- designate/objects/adapters/api_v2/base.py | 25 ++++--- .../tests/unit/test_api/test_admin_api.py | 67 +++++++++++++++++++ designate/tests/unit/test_api/test_api_v2.py | 63 +++++++++++++++++ etc/designate/designate.conf.sample | 7 ++ 6 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 designate/tests/unit/test_api/test_admin_api.py create mode 100644 designate/tests/unit/test_api/test_api_v2.py diff --git a/designate/api/admin/__init__.py b/designate/api/admin/__init__.py index b3f8a5cc8..4c5a01fc1 100644 --- a/designate/api/admin/__init__.py +++ b/designate/api/admin/__init__.py @@ -21,6 +21,11 @@ LOG = logging.getLogger(__name__) OPTS = [ cfg.ListOpt('enabled-extensions-admin', default=[], help='Enabled Admin API Extensions'), + cfg.IntOpt('default-limit-admin', default=20, + help='Default per-page limit for the Admin API, a value of None' + ' means show all results by default'), + cfg.IntOpt('max-limit-admin', default=1000, + help='Max per-page limit for the Admin API'), ] cfg.CONF.register_opts(OPTS, group='service:api') diff --git a/designate/api/admin/views/base.py b/designate/api/admin/views/base.py index 246a65389..e405811c2 100644 --- a/designate/api/admin/views/base.py +++ b/designate/api/admin/views/base.py @@ -128,21 +128,36 @@ class BaseView(object): # when there are more/previous items.. This is what nova # does.. But I think we can do better. - params = request.GET + # Duplicated code, see bug 1498432 - result = { + links = { "self": self._get_collection_href(request, parents), } + params = request.GET # See above # if 'marker' in params: # result['previous'] = self._get_previous_href(request, items, # parents) - if 'limit' in params and int(params['limit']) == len(items): - result['next'] = self._get_next_href(request, items, parents) + # defined in etc/designate/designate.conf.sample + limit = cfg.CONF['service:api'].default_limit_admin - return result + if 'limit' in params: + limit = params['limit'] + if limit.lower() == 'max': + limit = cfg.CONF['service:api'].max_limit_admin + else: + try: + limit = int(limit) + except ValueError: + raise exceptions.ValueError( + "'limit' should be an integer or 'max'") + + if limit is not None and limit == len(items): + links['next'] = self._get_next_href(request, items) + + return links def _get_base_href(self, parents=None): href = "%s/v2/%s" % (self.base_uri, self._collection_name) diff --git a/designate/objects/adapters/api_v2/base.py b/designate/objects/adapters/api_v2/base.py index 72ad87ddc..a7a4d6949 100644 --- a/designate/objects/adapters/api_v2/base.py +++ b/designate/objects/adapters/api_v2/base.py @@ -18,6 +18,7 @@ from oslo.config import cfg from designate.objects.adapters import base from designate.objects import base as obj_base +from designate import exceptions LOG = logging.getLogger(__name__) @@ -92,23 +93,31 @@ class APIv2Adapter(base.DesignateAdapter): item_path += '/' + part @classmethod - def _get_collection_links(cls, list, request): + def _get_collection_links(cls, item_list, request): links = { 'self': cls._get_collection_href(request) } params = request.GET + # defined in etc/designate/designate.conf.sample limit = cfg.CONF['service:api'].default_limit_v2 - if 'limit' in params and params['limit'] == 'max': - limit = cfg.CONF['service:api'].max_limit_v2 + if 'limit' in params: + limit = params['limit'] + if limit.lower() == 'max': + limit = cfg.CONF['service:api'].max_limit_v2 + else: + try: + limit = int(limit) + except ValueError: + raise exceptions.ValueError( + "'limit' should be an integer or 'max'") - elif 'limit' in params: - limit = int(params['limit']) - - if limit is not None and limit == len(list): - links['next'] = cls._get_next_href(request, list) + # Bug: this creates a link to "next" even on the last page if + # len(item_list) happens to be == limit + if limit is not None and limit == len(item_list): + links['next'] = cls._get_next_href(request, item_list) return links diff --git a/designate/tests/unit/test_api/test_admin_api.py b/designate/tests/unit/test_api/test_admin_api.py new file mode 100644 index 000000000..430c0906a --- /dev/null +++ b/designate/tests/unit/test_api/test_admin_api.py @@ -0,0 +1,67 @@ +# Copyright 2015 Hewlett-Packard Development Company, L.P. +# +# Author: Federico Ceratto +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from designate.tests import TestCase +from designate import exceptions + +from oslo_config import cfg +from oslo_config import fixture as cfg_fixture + +from designate.api.admin.views import base + + +class MockRequest(object): + + def __init__(self, GET=None): + self.GET = GET + + +class TestAdminAPI(TestCase): + + def setUp(self): + super(TestCase, self).setUp() + self.CONF = self.useFixture(cfg_fixture.Config(cfg.CONF)).conf + + @mock.patch.object(base.BaseView, '_get_collection_href') + @mock.patch.object(base.BaseView, '_get_next_href') + def test_limit_max(self, mock_coll_href, mock_next_href): + # Bug 1494799 + # The code being tested should be deduplicated, see bug 1498432 + mock_coll_href.return_value = None + mock_next_href.return_value = None + item_list = range(200) + + bv = base.BaseView() + + request = MockRequest(GET=dict(limit="max")) + links = bv._get_collection_links(request, item_list) + self.assertDictEqual(links, dict(self=None)) + + request = MockRequest(GET=dict(limit="MAX")) + links = bv._get_collection_links(request, item_list) + self.assertDictEqual(links, dict(self=None)) + + request = MockRequest(GET=dict(limit="200")) + links = bv._get_collection_links(request, item_list) + self.assertDictEqual(links, dict(self=None, next=None)) + + request = MockRequest(GET=dict(limit="BOGUS_STRING")) + self.assertRaises( + exceptions.ValueError, + bv._get_collection_links, request, item_list + ) diff --git a/designate/tests/unit/test_api/test_api_v2.py b/designate/tests/unit/test_api/test_api_v2.py new file mode 100644 index 000000000..622cd7bfe --- /dev/null +++ b/designate/tests/unit/test_api/test_api_v2.py @@ -0,0 +1,63 @@ +# Copyright 2015 Hewlett-Packard Development Company, L.P. +# +# Author: Federico Ceratto +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from designate.tests import TestCase +from designate import exceptions + +from oslo_config import cfg +from oslo_config import fixture as cfg_fixture + +from designate.objects.adapters.api_v2 import base + + +class MockRequest(object): + + def __init__(self, GET=None): + self.GET = GET + + +class TestAPIv2(TestCase): + + def setUp(self): + super(TestCase, self).setUp() + self.CONF = self.useFixture(cfg_fixture.Config(cfg.CONF)).conf + + @mock.patch.object(base.APIv2Adapter, '_get_collection_href') + @mock.patch.object(base.APIv2Adapter, '_get_next_href') + def test_limit_max(self, mock_coll_href, mock_next_href): + # Bug 1494799 bug:1494799 + mock_coll_href.return_value = None + mock_next_href.return_value = None + item_list = range(200) + request = MockRequest(GET=dict(limit="max")) + links = base.APIv2Adapter._get_collection_links(item_list, request) + self.assertDictEqual(links, dict(self=None)) + + request = MockRequest(GET=dict(limit="MAX")) + links = base.APIv2Adapter._get_collection_links(item_list, request) + self.assertDictEqual(links, dict(self=None)) + + request = MockRequest(GET=dict(limit="200")) + links = base.APIv2Adapter._get_collection_links(item_list, request) + self.assertDictEqual(links, dict(self=None, next=None)) + + request = MockRequest(GET=dict(limit="BOGUS_STRING")) + self.assertRaises( + exceptions.ValueError, + base.APIv2Adapter._get_collection_links, item_list, request + ) diff --git a/etc/designate/designate.conf.sample b/etc/designate/designate.conf.sample index 7ee1aca1b..273f4714e 100644 --- a/etc/designate/designate.conf.sample +++ b/etc/designate/designate.conf.sample @@ -105,6 +105,13 @@ debug = False # zone import / export is in zones extension #enabled_extensions_admin = +# Default per-page limit for the Admin API, a value of None means show all results +# by default +#default_limit_admin = 20 + +# Max page size in the Admin API +#max_limit_admin = 1000 + # Show the pecan HTML based debug interface (v2 only) # This is only useful for development, and WILL break python-designateclient # if an error occurs