openstack
/
cinder
Code Issues Proposed changes

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/<tenant>/volumes/detail?limit=1",
the "next" link does not have the URL for a detailed query and is
formatted as "/v2/<tenant>/volumes?limit=1&marker=<marker>". In this
case the "next" link needs to be scoped to "/v2/<tenant>/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
This commit is contained in:
Steven Kaufer 2014-03-31 20:32:39 +00:00
parent 6b3d177695
commit 2981cdbb03
3 changed files with 79 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
cinder/api/common.py
View File

@ -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

17
cinder/api/v2/views/volumes.py
View File

@ -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:

69
cinder/tests/api/v2/test_volumes.py
View File

@ -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.