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

@@ -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')

designate/api/admin/views/base.py

@@ -127,21 +127,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)

designate/objects/adapters/api_v2/base.py

@@ -17,6 +17,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__)
 
@@ -101,23 +102,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':
+        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
 

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
+        )

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
+        )

etc/designate/designate.conf.sample

@@ -132,6 +132,13 @@ debug = False
 # zone 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