From 2981cdbb03c6f1239a58fedb260796667b8154ab Mon Sep 17 00:00:00 2001 From: Steven Kaufer Date: Mon, 31 Mar 2014 20:32:39 +0000 Subject: [PATCH] GET details REST API next link missing 'details' When executing a pagination query a "next" link is included in the API reply when there are more items then the specified limit. See pagination documentation for more information: http://docs.openstack.org/api/openstack-compute/2/content/ Paginated_Collections-d1e664.html The caller should be able to invoke the "next" link (without having to re-format it) in order to get the next page of data. The documentation states "Subsequent links will honor the initial page size. Thus, a client may follow links to traverse a paginated collection without having to input the marker parameter." The problem is that the "next" link is always scoped to the non- detailed query. For example, if you execute "/v2//volumes/detail?limit=1", the "next" link does not have the URL for a detailed query and is formatted as "/v2//volumes?limit=1&marker=". In this case the "next" link needs to be scoped to "/v2//volumes/detail". The user could work around this issue my manually inserting '/details' into the "next" link URL. Test code is included to verify that the '/details' URL is correctly added when the "next" link is included in a detailed pagination query. Also, existing tests were changed to ensure that the correct controller function (ie, 'index' vs. 'detail) are invoked for the appropriate query (ie, non-detailed or detailed) -- 'index' was previously alwayed invoked for detailed URL requests. Change-Id: Ib00d6deb25255fac1db0f7bf4ecd3c8d30e1c39d Closes-bug: 1299247 --- cinder/api/common.py | 22 ++++++--- cinder/api/v2/views/volumes.py | 17 +++++-- cinder/tests/api/v2/test_volumes.py | 69 ++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 29 deletions(-) diff --git a/cinder/api/common.py b/cinder/api/common.py index 52802295b25..736599f6778 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -216,7 +216,7 @@ class ViewBuilder(object): {"rel": "bookmark", "href": self._get_bookmark_link(request, identifier), }] - def _get_next_link(self, request, identifier): + def _get_next_link(self, request, identifier, collection_name): """Return href string with proper limit and marker params.""" params = request.params.copy() params["marker"] = identifier @@ -224,7 +224,7 @@ class ViewBuilder(object): CONF.osapi_volume_base_URL) url = os.path.join(prefix, request.environ["cinder.context"].project_id, - self._collection_name) + collection_name) return "%s?%s" % (url, dict_to_query_str(params)) def _get_href_link(self, request, identifier): @@ -246,13 +246,24 @@ class ViewBuilder(object): self._collection_name, str(identifier)) - def _get_collection_links(self, request, items, id_key="uuid"): - """Retrieve 'next' link, if applicable. This is included if: + def _get_collection_links(self, request, items, collection_name, + id_key="uuid"): + """Retrieve 'next' link, if applicable. + + The next link is included if: 1) 'limit' param is specified and equals the number of volumes. 2) 'limit' param is specified but it exceeds CONF.osapi_max_limit, in this case the number of volumes is CONF.osapi_max_limit. 3) 'limit' param is NOT specified but the number of volumes is CONF.osapi_max_limit. + + :param request: API request + :param items: List of collection items + :param collection_name: Name of collection, used to generate the + next link for a pagination query + :param id_key: Attribute key used to retrieve the unique ID, used + to generate the next link marker for a pagination query + :returns links """ links = [] max_items = min( @@ -266,7 +277,8 @@ class ViewBuilder(object): last_item_id = last_item["id"] links.append({ "rel": "next", - "href": self._get_next_link(request, last_item_id), + "href": self._get_next_link(request, last_item_id, + collection_name), }) return links diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py index 778be776735..9a034d32744 100644 --- a/cinder/api/v2/views/volumes.py +++ b/cinder/api/v2/views/volumes.py @@ -35,7 +35,8 @@ class ViewBuilder(common.ViewBuilder): def detail_list(self, request, volumes): """Detailed view of a list of volumes.""" - return self._list_view(self.detail, request, volumes) + return self._list_view(self.detail, request, volumes, + coll_name=self._collection_name + '/detail') def summary(self, request, volume): """Generic, non-detailed view of an volume.""" @@ -114,12 +115,20 @@ class ViewBuilder(common.ViewBuilder): else: return volume['volume_type_id'] - def _list_view(self, func, request, volumes): - """Provide a view for a list of volumes.""" + def _list_view(self, func, request, volumes, coll_name=_collection_name): + """Provide a view for a list of volumes. + + :param func: Function used to format the volume data + :param request: API request + :param servers: List of volumes in dictionary format + :param coll_name: Name of collection, used to generate the next link + for a pagination query + :returns: Volume data in dictionary format + """ volumes_list = [func(request, volume)['volume'] for volume in volumes] volumes_links = self._get_collection_links(request, volumes, - self._collection_name) + coll_name) volumes_dict = dict(volumes=volumes_list) if volumes_links: diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index fab7d98b6d1..d10be3d61f2 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/cinder/tests/api/v2/test_volumes.py @@ -18,6 +18,7 @@ import datetime from lxml import etree from oslo.config import cfg +import six.moves.urllib.parse as urlparse import webob from cinder.api import extensions @@ -609,6 +610,15 @@ class VolumeApiTest(test.TestCase): volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) + # Ensure that the next link is correctly formatted + links = res_dict['volumes_links'] + self.assertEqual(links[0]['rel'], 'next') + href_parts = urlparse.urlparse(links[0]['href']) + self.assertEqual('/v2/fake/volumes', href_parts.path) + params = urlparse.parse_qs(href_parts.query) + self.assertTrue('marker' in params) + self.assertEqual('1', params['limit'][0]) + def test_volume_index_limit_negative(self): req = fakes.HTTPRequest.blank('/v2/volumes?limit=-1') self.assertRaises(exception.Invalid, @@ -671,7 +681,7 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) req = fakes.HTTPRequest.blank('/v2/volumes/detail?marker=1') - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 2) self.assertEqual(volumes[0]['id'], 1) @@ -683,20 +693,29 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=1') - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) + # Ensure that the next link is correctly formatted + links = res_dict['volumes_links'] + self.assertEqual(links[0]['rel'], 'next') + href_parts = urlparse.urlparse(links[0]['href']) + self.assertEqual('/v2/fake/volumes/detail', href_parts.path) + params = urlparse.parse_qs(href_parts.query) + self.assertTrue('marker' in params) + self.assertEqual('1', params['limit'][0]) + def test_volume_detail_limit_negative(self): req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1') self.assertRaises(exception.Invalid, - self.controller.index, + self.controller.detail, req) def test_volume_detail_limit_non_int(self): req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a') self.assertRaises(exception.Invalid, - self.controller.index, + self.controller.detail, req) def test_volume_detail_limit_marker(self): @@ -705,7 +724,7 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) req = fakes.HTTPRequest.blank('/v2/volumes/detail?marker=1&limit=1') - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['id'], '1') @@ -722,26 +741,26 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1') - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['id'], 2) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=2&offset=1', use_admin_context=True) - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) volumes = res_dict['volumes'] self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0]['id'], 2) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=-1&offset=1') self.assertRaises(exception.InvalidInput, - self.controller.index, + self.controller.detail, req) req = fakes.HTTPRequest.blank('/v2/volumes/detail?limit=a&offset=1') self.assertRaises(exception.InvalidInput, - self.controller.index, + self.controller.detail, req) def test_volume_with_limit_zero(self): @@ -757,6 +776,16 @@ class VolumeApiTest(test.TestCase): def test_volume_default_limit(self): self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) + def _verify_links(links, url_key): + '''Verify next link and url.''' + self.assertEqual(links[0]['rel'], 'next') + href_parts = urlparse.urlparse(links[0]['href']) + self.assertEqual('/v2/fake/%s' % key, href_parts.path) + + # Verify both the index and detail queries + api_keys = ['volumes', 'volumes/detail'] + fns = [self.controller.index, self.controller.detail] + # Number of volumes equals the max, include next link def stub_volume_get_all(context, marker, limit, sort_key, sort_dir, @@ -767,13 +796,13 @@ class VolumeApiTest(test.TestCase): return vols return vols[:limit] self.stubs.Set(db, 'volume_get_all', stub_volume_get_all) - for key in ['volumes', 'volumes/detail']: + for key, fn in zip(api_keys, fns): req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, use_admin_context=True) - res_dict = self.controller.index(req) + res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) volumes_links = res_dict['volumes_links'] - self.assertEqual(volumes_links[0]['rel'], 'next') + _verify_links(volumes_links, key) # Number of volumes less then max, do not include def stub_volume_get_all2(context, marker, limit, @@ -785,10 +814,10 @@ class VolumeApiTest(test.TestCase): return vols return vols[:limit] self.stubs.Set(db, 'volume_get_all', stub_volume_get_all2) - for key in ['volumes', 'volumes/detail']: + for key, fn in zip(api_keys, fns): req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, use_admin_context=True) - res_dict = self.controller.index(req) + res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), 100) self.assertFalse('volumes_links' in res_dict) @@ -802,24 +831,24 @@ class VolumeApiTest(test.TestCase): return vols return vols[:limit] self.stubs.Set(db, 'volume_get_all', stub_volume_get_all3) - for key in ['volumes', 'volumes/detail']: + for key, fn in zip(api_keys, fns): req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1' % key, use_admin_context=True) - res_dict = self.controller.index(req) + res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) volumes_links = res_dict['volumes_links'] - self.assertEqual(volumes_links[0]['rel'], 'next') + _verify_links(volumes_links, key) # Pass a limit that is greater then the max and the total number of # volumes, ensure only the maximum is returned and that the next # link is present - for key in ['volumes', 'volumes/detail']: + for key, fn in zip(api_keys, fns): req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1&limit=%d' % (key, CONF.osapi_max_limit * 2), use_admin_context=True) - res_dict = self.controller.index(req) + res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) volumes_links = res_dict['volumes_links'] - self.assertEqual(volumes_links[0]['rel'], 'next') + _verify_links(volumes_links, key) def test_volume_list_default_filters(self): """Tests that the default filters from volume.api.API.get_all are set.