Merge lp:~ironcamel/nova/http-error-codes into lp:~hudson-openstack/nova/trunk

[Naveed Massjouni]

Fixes bug #729400. 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.

[justinsb (justin-fathomdb)]

Can this logic be packaged into a reusable function? DRY :-)

[Gabe Westmaas (westmaas)]

lgtm.

I 100% agree we shouldn't repeat ourselves, but I don't think we need to worry about it until someone does something that would repeat this. YAGNI :)

If this is already being done elsewhere though, we should combine it, absolutely.

[Todd Willey (xtoddx)]

looks straightforward enough

review: Approve

[Rick Harris (rconradharris)]

lgtm

review: Approve

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

=== modified file 'nova/api/openstack/common.py'
--- nova/api/openstack/common.py	2011-02-28 19:49:03 +0000
+++ nova/api/openstack/common.py	2011-03-07 23:19:15 +0000
@@ -36,15 +36,18 @@
     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
-
-    if offset < 0 or limit < 0:
-        raise webob.exc.HTTPBadRequest()
+        raise webob.exc.HTTPBadRequest(_('limit param must be an integer'))
+
+    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

=== modified file 'nova/tests/api/openstack/test_common.py'
--- nova/tests/api/openstack/test_common.py	2011-02-28 19:49:03 +0000
+++ nova/tests/api/openstack/test_common.py	2011-03-07 23:19:15 +0000
@@ -79,20 +79,14 @@
         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 @@
         """
         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)

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-03-02 17:39:30 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-03-07 23:19:15 +0000
@@ -188,6 +188,34 @@
             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': ''}