diff --git a/cinder/api/common.py b/cinder/api/common.py index 4434b7d016b..ceffef5b099 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -287,39 +287,64 @@ class ViewBuilder(object): str(identifier)) def _get_collection_links(self, request, items, collection_name, - id_key="uuid"): + item_count, 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. + 1) 'limit' param is specified and equal to or less than the + number of items. + 2) 'limit' param is specified and both the limit and the number + of items are greater than CONF.osapi_max_limit, even if limit + is greater than the number of items. + 3) 'limit' param is NOT specified and the number of items is + greater than CONF.osapi_max_limit. + Notes: The case limit equals to 0 or CONF.osapi_max_limit equals to 0 + is not included in the above conditions. :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 item_count: Length of the list of the original collection + items :param id_key: Attribute key used to retrieve the unique ID, used to generate the next link marker for a pagination query :returns links """ + limit = request.params.get("limit", None) + if limit is None or int(limit) > CONF.osapi_max_limit: + # If limit is not set in the request or greater than + # osapi_max_limit, len(items) < item_count means the items + # are limited by osapi_max_limit and we need to generate the + # next link. Otherwise, all the items will be returned in + # the response, no next link is generated. + if len(items) < item_count: + return self._generate_next_link(items, id_key, request, + collection_name) + else: + # If limit is set in the request and not more than + # osapi_max_limit, int(limit) == len(items) means it is possible + # that the DB still have more items left. In this case, + # we generate the next link. + limit = int(limit) + if limit and limit == len(items): + return self._generate_next_link(items, id_key, request, + collection_name) + return [] + + def _generate_next_link(self, items, id_key, request, + collection_name): links = [] - max_items = min( - int(request.params.get("limit", CONF.osapi_max_limit)), - CONF.osapi_max_limit) - if max_items and max_items == len(items): - last_item = items[-1] - if id_key in last_item: - last_item_id = last_item[id_key] - else: - last_item_id = last_item["id"] - links.append({ - "rel": "next", - "href": self._get_next_link(request, last_item_id, - collection_name), - }) + last_item = items[-1] + if id_key in last_item: + last_item_id = last_item[id_key] + else: + last_item_id = last_item["id"] + links.append({ + "rel": "next", + "href": self._get_next_link(request, last_item_id, + collection_name), + }) return links def _update_link_prefix(self, orig_url, prefix): diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index 5d0ffdde5fa..51d3fb496a9 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -218,13 +218,16 @@ class BackupsController(wsgi.Controller): del filters['name'] backups = self.backup_api.get_all(context, search_opts=filters) + backup_count = len(backups) limited_list = common.limited(backups, req) req.cache_db_backups(limited_list) if is_detail: - backups = self._view_builder.detail_list(req, limited_list) + backups = self._view_builder.detail_list(req, limited_list, + backup_count) else: - backups = self._view_builder.summary_list(req, limited_list) + backups = self._view_builder.summary_list(req, limited_list, + backup_count) return backups # TODO(frankm): Add some checks here including diff --git a/cinder/api/contrib/volume_transfer.py b/cinder/api/contrib/volume_transfer.py index 9cc35351b1c..132e122af4b 100644 --- a/cinder/api/contrib/volume_transfer.py +++ b/cinder/api/contrib/volume_transfer.py @@ -131,12 +131,15 @@ class VolumeTransferController(wsgi.Controller): filters = req.params.copy() LOG.debug('Listing volume transfers') transfers = self.transfer_api.get_all(context, filters=filters) + transfer_count = len(transfers) limited_list = common.limited(transfers, req) if is_detail: - transfers = self._view_builder.detail_list(req, limited_list) + transfers = self._view_builder.detail_list(req, limited_list, + transfer_count) else: - transfers = self._view_builder.summary_list(req, limited_list) + transfers = self._view_builder.summary_list(req, limited_list, + transfer_count) return transfers diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py index 6ea89a78d17..54cbcd7c448 100644 --- a/cinder/api/v2/views/volumes.py +++ b/cinder/api/v2/views/volumes.py @@ -30,13 +30,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, volumes): + def summary_list(self, request, volumes, volume_count): """Show a list of volumes without many details.""" - return self._list_view(self.summary, request, volumes) + return self._list_view(self.summary, request, volumes, + volume_count) - def detail_list(self, request, volumes): + def detail_list(self, request, volumes, volume_count): """Detailed view of a list of volumes.""" return self._list_view(self.detail, request, volumes, + volume_count, coll_name=self._collection_name + '/detail') def summary(self, request, volume): @@ -117,12 +119,14 @@ class ViewBuilder(common.ViewBuilder): else: return volume['volume_type_id'] - def _list_view(self, func, request, volumes, coll_name=_collection_name): + def _list_view(self, func, request, volumes, volume_count, + 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 volumes: List of volumes in dictionary format + :param volume_count: Length of the original list of volumes :param coll_name: Name of collection, used to generate the next link for a pagination query :returns: Volume data in dictionary format @@ -130,7 +134,8 @@ class ViewBuilder(common.ViewBuilder): volumes_list = [func(request, volume)['volume'] for volume in volumes] volumes_links = self._get_collection_links(request, volumes, - coll_name) + coll_name, + volume_count) volumes_dict = dict(volumes=volumes_list) if volumes_links: diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index 26eb42a8023..13a86009cf5 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -249,12 +249,15 @@ class VolumeController(wsgi.Controller): utils.add_visible_admin_metadata(volume) limited_list = common.limited(volumes, req) + volume_count = len(volumes) req.cache_db_volumes(limited_list) if is_detail: - volumes = self._view_builder.detail_list(req, limited_list) + volumes = self._view_builder.detail_list(req, limited_list, + volume_count) else: - volumes = self._view_builder.summary_list(req, limited_list) + volumes = self._view_builder.summary_list(req, limited_list, + volume_count) return volumes def _image_uuid_from_ref(self, image_ref, context): diff --git a/cinder/api/views/backups.py b/cinder/api/views/backups.py index ce6bd53974c..fff90d251fe 100644 --- a/cinder/api/views/backups.py +++ b/cinder/api/views/backups.py @@ -30,13 +30,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, backups): + def summary_list(self, request, backups, origin_backup_count): """Show a list of backups without many details.""" - return self._list_view(self.summary, request, backups) + return self._list_view(self.summary, request, backups, + origin_backup_count) - def detail_list(self, request, backups): + def detail_list(self, request, backups, origin_backup_count): """Detailed view of a list of backups .""" - return self._list_view(self.detail, request, backups) + return self._list_view(self.detail, request, backups, + origin_backup_count) def summary(self, request, backup): """Generic, non-detailed view of a backup.""" @@ -77,12 +79,13 @@ class ViewBuilder(common.ViewBuilder): } } - def _list_view(self, func, request, backups): + def _list_view(self, func, request, backups, origin_backup_count): """Provide a view for a list of backups.""" backups_list = [func(request, backup)['backup'] for backup in backups] backups_links = self._get_collection_links(request, backups, - self._collection_name) + self._collection_name, + origin_backup_count) backups_dict = dict(backups=backups_list) if backups_links: diff --git a/cinder/api/views/transfers.py b/cinder/api/views/transfers.py index dbacb2c19e8..7a4a5ac1f90 100644 --- a/cinder/api/views/transfers.py +++ b/cinder/api/views/transfers.py @@ -30,13 +30,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, transfers): + def summary_list(self, request, transfers, origin_transfer_count): """Show a list of transfers without many details.""" - return self._list_view(self.summary, request, transfers) + return self._list_view(self.summary, request, transfers, + origin_transfer_count) - def detail_list(self, request, transfers): + def detail_list(self, request, transfers, origin_transfer_count): """Detailed view of a list of transfers .""" - return self._list_view(self.detail, request, transfers) + return self._list_view(self.detail, request, transfers, + origin_transfer_count) def summary(self, request, transfer): """Generic, non-detailed view of a transfer.""" @@ -75,13 +77,14 @@ class ViewBuilder(common.ViewBuilder): } } - def _list_view(self, func, request, transfers): + def _list_view(self, func, request, transfers, origin_transfer_count): """Provide a view for a list of transfers.""" transfers_list = [func(request, transfer)['transfer'] for transfer in transfers] transfers_links = self._get_collection_links(request, transfers, - self._collection_name) + self._collection_name, + origin_transfer_count) transfers_dict = dict(transfers=transfers_list) if transfers_links: diff --git a/cinder/tests/api/test_common.py b/cinder/tests/api/test_common.py index c5be2bdbcfe..75ef32894a8 100644 --- a/cinder/tests/api/test_common.py +++ b/cinder/tests/api/test_common.py @@ -17,6 +17,8 @@ Test suites for 'common' code used throughout the OpenStack HTTP API. """ +import mock +from testtools import matchers import webob import webob.exc @@ -347,3 +349,201 @@ class MiscFunctionsTest(test.TestCase): self.assertRaises(ValueError, common.remove_version_from_href, fixture) + + +class TestCollectionLinks(test.TestCase): + """Tests the _get_collection_links method.""" + + def _validate_next_link(self, href_link_mock, item_count, + osapi_max_limit, limit, should_link_exist): + req = mock.MagicMock() + href_link_mock.return_value = [{"rel": "next", + "href": "fake_link"}] + self.flags(osapi_max_limit=osapi_max_limit) + if limit is None: + params = mock.PropertyMock(return_value=dict()) + limited_list_size = min(item_count, osapi_max_limit) + else: + params = mock.PropertyMock(return_value=dict(limit=limit)) + limited_list_size = min(item_count, osapi_max_limit, + limit) + limited_list = [{"uuid": str(i)} for i in range(limited_list_size)] + type(req).params = params + builder = common.ViewBuilder() + results = builder._get_collection_links(req, limited_list, + mock.sentinel.coll_key, + item_count, "uuid") + if should_link_exist: + href_link_mock.assert_called_once_with(limited_list, "uuid", + req, + mock.sentinel.coll_key) + self.assertThat(results, matchers.HasLength(1)) + else: + self.assertFalse(href_link_mock.called) + self.assertThat(results, matchers.HasLength(0)) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = None + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_greater_than_limit(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_equals_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_less_than_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 6 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = None + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_less_than_items_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_equals_items_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_limit_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 6 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_equals_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 7 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_less_than_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 8 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = None + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_less_than_items_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 2 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_osapi_max_equals_limit(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 3 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_limit_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_limit_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_greater_than_items_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 6 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index a6bb9ef4182..921639890f5 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/cinder/tests/api/v2/test_volumes.py @@ -1111,7 +1111,7 @@ class VolumeApiTest(test.TestCase): api_keys = ['volumes', 'volumes/detail'] fns = [self.controller.index, self.controller.detail] - # Number of volumes equals the max, include next link + # Number of volumes equals the max, next link not included def stub_volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, @@ -1127,8 +1127,7 @@ class VolumeApiTest(test.TestCase): use_admin_context=True) res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) - volumes_links = res_dict['volumes_links'] - _verify_links(volumes_links, key) + self.assertFalse('volumes_links' in res_dict) # Number of volumes less than max, do not include def stub_volume_get_all2(context, marker, limit, @@ -1168,7 +1167,7 @@ class VolumeApiTest(test.TestCase): _verify_links(volumes_links, key) # Pass a limit that is greater than the max and the total number of # volumes, ensure only the maximum is returned and that the next - # link is present + # link is present. 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),