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

diff --git a/cinder/api/common.py b/cinder/api/common.py
index 52802295b25ebee2f14a6e40a3be4b80f8fedce5..736599f67785199facdc922727e84734eae86321 100644 (file)
--- 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 778be7767359dc2af6adf88fccf86b5a0b2ac69f..9a034d327444e2975abcce2b0e2f971eb43b3eb7 100644 (file)
--- 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 fab7d98b6d1d627edf26194a6a2f863b6d6e483f..d10be3d61f22f24e06287d5c9703154ad22260f3 100644 (file)
--- 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.