Merge "Remove the useless next link for volumes, transfers and backups"

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

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

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

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

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

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

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:

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