Merge "Fix #1494799 handle limit=max on v2 and Admin APIs" into stable/kilo

6 files changed, 180 insertions, 14 deletions

diff --git a/designate/api/admin/__init__.py b/designate/api/admin/__init__.py
index b3f8a5cc..4c5a01fc 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 246a6538..e405811c 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 72ad87dd..a7a4d694 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
-
-        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)
+        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'")
+
+        # 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 00000000..430c0906
--- /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 <federico.ceratto@hpe.com>
+#
+# 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 00000000..622cd7bf
--- /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 <federico.ceratto@hpe.com>
+#
+# 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 7ee1aca1..273f4714 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