Invalid values for offset and limit params in http requests now return a 400

response with a useful message in the body. Also added and updated tests.

3 files changed, 39 insertions, 20 deletions

diff --git a/nova/api/openstack/common.py b/nova/api/openstack/common.py
index 9f85c5c8a..f7a9cc3f0 100644
--- a/nova/api/openstack/common.py
+++ b/nova/api/openstack/common.py
@@ -36,15 +36,18 @@ def limited(items, request, max_limit=1000):
     try:
         offset = int(request.GET.get('offset', 0))
     except ValueError:
-        offset = 0
+        raise webob.exc.HTTPBadRequest(_('offset param must be an integer'))
 
     try:
         limit = int(request.GET.get('limit', max_limit))
     except ValueError:
-        limit = max_limit
+        raise webob.exc.HTTPBadRequest(_('limit param must be an integer'))
 
-    if offset < 0 or limit < 0:
-        raise webob.exc.HTTPBadRequest()
+    if limit < 0:
+        raise webob.exc.HTTPBadRequest(_('limit param must be positive'))
+
+    if offset < 0:
+        raise webob.exc.HTTPBadRequest(_('offset param must be positive'))
 
     limit = min(max_limit, limit or max_limit)
     range_end = offset + limit
diff --git a/nova/tests/api/openstack/test_common.py b/nova/tests/api/openstack/test_common.py
index 92023362c..8f57c5b67 100644
--- a/nova/tests/api/openstack/test_common.py
+++ b/nova/tests/api/openstack/test_common.py
@@ -79,20 +79,14 @@ class LimiterTest(test.TestCase):
         Test offset key works with a blank offset.
         """
         req = Request.blank('/?offset=')
-        self.assertEqual(limited(self.tiny, req), self.tiny)
-        self.assertEqual(limited(self.small, req), self.small)
-        self.assertEqual(limited(self.medium, req), self.medium)
-        self.assertEqual(limited(self.large, req), self.large[:1000])
+        self.assertRaises(webob.exc.HTTPBadRequest, limited, self.tiny, req)
 
     def test_limiter_offset_bad(self):
         """
         Test offset key works with a BAD offset.
         """
         req = Request.blank(u'/?offset=\u0020aa')
-        self.assertEqual(limited(self.tiny, req), self.tiny)
-        self.assertEqual(limited(self.small, req), self.small)
-        self.assertEqual(limited(self.medium, req), self.medium)
-        self.assertEqual(limited(self.large, req), self.large[:1000])
+        self.assertRaises(webob.exc.HTTPBadRequest, limited, self.tiny, req)
 
     def test_limiter_nothing(self):
         """
@@ -166,18 +160,12 @@ class LimiterTest(test.TestCase):
         """
         Test a negative limit.
         """
-        def _limit_large():
-            limited(self.large, req, max_limit=2000)
-
         req = Request.blank('/?limit=-3000')
-        self.assertRaises(webob.exc.HTTPBadRequest, _limit_large)
+        self.assertRaises(webob.exc.HTTPBadRequest, limited, self.tiny, req)
 
     def test_limiter_negative_offset(self):
         """
         Test a negative offset.
         """
-        def _limit_large():
-            limited(self.large, req, max_limit=2000)
-
         req = Request.blank('/?offset=-30')
-        self.assertRaises(webob.exc.HTTPBadRequest, _limit_large)
+        self.assertRaises(webob.exc.HTTPBadRequest, limited, self.tiny, req)
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index 78beb7df9..10fb2bafb 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -184,6 +184,34 @@ class ServersTest(test.TestCase):
             self.assertEqual(s.get('imageId', None), None)
             i += 1
 
+    def test_get_servers_with_limit(self):
+        req = webob.Request.blank('/v1.0/servers?limit=3')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        self.assertEqual([s['id'] for s in servers], [0, 1, 2])
+
+        req = webob.Request.blank('/v1.0/servers?limit=aaa')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+        self.assertTrue('limit' in res.body)
+
+    def test_get_servers_with_offset(self):
+        req = webob.Request.blank('/v1.0/servers?offset=2')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        self.assertEqual([s['id'] for s in servers], [2, 3, 4])
+
+        req = webob.Request.blank('/v1.0/servers?offset=aaa')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+        self.assertTrue('offset' in res.body)
+
+    def test_get_servers_with_limit_and_offset(self):
+        req = webob.Request.blank('/v1.0/servers?limit=2&offset=1')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        self.assertEqual([s['id'] for s in servers], [1, 2])
+
     def test_create_instance(self):
         def instance_create(context, inst):
             return {'id': '1', 'display_name': ''}