From 18e2c3542a6db98b1a71e50ed77b337ab179a0ba Mon Sep 17 00:00:00 2001 From: Fei Long Wang Date: Thu, 14 Mar 2013 14:35:10 +0800 Subject: [PATCH] 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 (cherry picked from commit 7c760581d80b2ee5cd1e01a1d0a007770c9aa807) --- cinder/api/v2/volumes.py | 1 + cinder/tests/api/v2/test_volumes.py | 58 +++++++++++++++++++++++++++-- cinder/volume/api.py | 10 +++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index e78d0207c..b88fa10d4 100644 --- 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 baba2bdc8..5d5d1fff5 100644 --- 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 8c3d394b0..911664bf8 100644 --- 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'] -- 2.45.2