Fixes Cinder REST API /volumes issue

Issue #1
Once GET variable 'offset' is specified, the API /volume will get an empty
output.

Issue #2
Should validate the GET variable 'limit' before query database to get a
consistent message with Cinder REST API v1.
By current implement, error message is as below if the variable 'limit'
is invalid:
--------------------------------------------------------------------------
{"computeFault": {"message": "The server has either erred or is incapable
of performing the requested operation.", "code": 500}}

After this change, the new message is as below:
--------------------------------------------------------------------------
{"badRequest": {"message": "limit param must be an integer", "code": 400}}

Fixes Bug: 1154454

Change-Id: Ifb155477bae0ea3e39877737ee9019e7d8a104a7

diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index bd2e4e91b3851615f72b1a39c7ef8a3e01345138..76ec83d81b09abd84bb3bee8422c551993023e33 100644 (file)
--- a/cinder/api/v2/volumes.py
+++ b/cinder/api/v2/volumes.py
@@ -179,6 +179,7 @@ class VolumeController(wsgi.Controller):
         limit = params.pop('limit', None)
         sort_key = params.pop('sort_key', 'created_at')
         sort_dir = params.pop('sort_dir', 'desc')
+        params.pop('offset', None)
         filters = params
 
         remove_invalid_options(context,

diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py
index 78a9046c25b5679d7fa9aba353fa923d30927473..d176f92f5ae9a0fa569395f15fdee783f9050262 100644 (file)
--- a/cinder/tests/api/v2/test_volumes.py
+++ b/cinder/tests/api/v2/test_volumes.py
@@ -419,13 +419,13 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_index_limit_negative(self):
         req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1')
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.Invalid,
                           self.controller.index,
                           req)
 
     def test_volume_index_limit_non_int(self):
         req = fakes.HTTPRequest.blank('/v2/volumes?limit=a')
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.Invalid,
                           self.controller.index,
                           req)
 
@@ -436,6 +436,31 @@ class VolumeApiTest(test.TestCase):
         self.assertEquals(len(volumes), 1)
         self.assertEquals(volumes[0]['id'], '1')
 
+    def test_volume_index_limit_offset(self):
+        def stub_volume_get_all_by_project(context, project_id, marker, limit,
+                                           sort_key, sort_dir):
+            return [
+                stubs.stub_volume(1, display_name='vol1'),
+                stubs.stub_volume(2, display_name='vol2'),
+            ]
+        self.stubs.Set(db, 'volume_get_all_by_project',
+                       stub_volume_get_all_by_project)
+        req = fakes.HTTPRequest.blank('/v2/volumes?limit=2&offset=1')
+        res_dict = self.controller.index(req)
+        volumes = res_dict['volumes']
+        self.assertEquals(len(volumes), 1)
+        self.assertEquals(volumes[0]['id'], 2)
+
+        req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1&offset=1')
+        self.assertRaises(exception.InvalidInput,
+                          self.controller.index,
+                          req)
+
+        req = fakes.HTTPRequest.blank('/v2/volumes?limit=a&offset=1')
+        self.assertRaises(exception.InvalidInput,
+                          self.controller.index,
+                          req)
+
     def test_volume_detail_with_marker(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
                                            sort_key, sort_dir):
@@ -460,13 +485,13 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_detail_limit_negative(self):
         req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1')
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.Invalid,
                           self.controller.index,
                           req)
 
     def test_volume_detail_limit_non_int(self):
         req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a')
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.Invalid,
                           self.controller.index,
                           req)
 
@@ -477,6 +502,31 @@ class VolumeApiTest(test.TestCase):
         self.assertEquals(len(volumes), 1)
         self.assertEquals(volumes[0]['id'], '1')
 
+    def test_volume_detail_limit_offset(self):
+        def stub_volume_get_all_by_project(context, project_id, marker, limit,
+                                           sort_key, sort_dir):
+            return [
+                stubs.stub_volume(1, display_name='vol1'),
+                stubs.stub_volume(2, display_name='vol2'),
+            ]
+        self.stubs.Set(db, 'volume_get_all_by_project',
+                       stub_volume_get_all_by_project)
+        req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1')
+        res_dict = self.controller.index(req)
+        volumes = res_dict['volumes']
+        self.assertEquals(len(volumes), 1)
+        self.assertEquals(volumes[0]['id'], 2)
+
+        req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1&offset=1')
+        self.assertRaises(exception.InvalidInput,
+                          self.controller.index,
+                          req)
+
+        req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a&offset=1')
+        self.assertRaises(exception.InvalidInput,
+                          self.controller.index,
+                          req)
+
     def test_volume_list_by_name(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
                                            sort_key, sort_dir):

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index a6fb584b18d005c631381dd7414ff2eefe873288..f57b6b8604935738ea9ef14bd2a483f86cba9cc2 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -330,6 +330,16 @@ class API(base.Base):
                 sort_dir='desc', filters={}):
         check_policy(context, 'get_all')
 
+        try:
+            if limit is not None:
+                limit = int(limit)
+                if limit < 0:
+                    msg = _('limit param must be positive')
+                    raise exception.InvalidInput(reason=msg)
+        except ValueError:
+            msg = _('limit param must be an integer')
+            raise exception.InvalidInput(reason=msg)
+
         if (context.is_admin and 'all_tenants' in filters):
             # Need to remove all_tenants to pass the filtering below.
             del filters['all_tenants']