Remove the useless next link for volumes, transfers and backups

Two cases this patch will resolve:
1) Currently if the number of items equals the osapi_max_limit or
the last page of items equals the osapi_max_limit without the
parameter limit set in the user request, a next link is generated
in the response, though this next link will return empty volume
list. In fact it is unnecessary to generate the next link in
this case.

2) If the number of items equals the osapi_max_limit and limit is
greater than osapi_max_limit, a next link is generated. Actually,
the next link does not need to be generated, because it is certain
that there is no more volumes left in the database.

The method _get_collection_links has been called in volumes,
volume_transfers and backups. The patch can only affect the next
link generation for three of them. However, other lists like
consistency groups, qos specs, cgsnapshots have not implemented the
generation for the next link. Potentially this can be a wishlist
item for them.

Change-Id: I0f1f449c73d51675281497a095d869c1e72c889f
closes-bug: #1350558

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

cinder/api/contrib/backups.py

@@ -219,13 +219,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

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
 

cinder/api/v2/views/volumes.py

@@ -29,13 +29,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):
@@ -116,12 +118,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
@@ -129,7 +133,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:

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

cinder/api/views/backups.py

@@ -29,13 +29,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."""
@@ -76,12 +78,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:

cinder/api/views/transfers.py

@@ -29,13 +29,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."""
@@ -74,13 +76,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:

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)

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),