Merge "Prefer links dicts for pagination"

openstack/resource2.py

@@ -216,6 +216,8 @@ class Resource(object):
     resource_key = None
     #: Plural form of key for resource.
     resources_key = None
+    #: Key used for pagination links
+    pagination_key = None
 
     #: The ID of this resource.
     id = Body("id")
@@ -726,45 +728,111 @@ class Resource(object):
         if not cls.allow_list:
             raise exceptions.MethodNotSupported(cls, "list")
 
-        more_data = True
         query_params = cls._query_mapping._transpose(params)
         uri = cls.base_path % params
 
-        while more_data:
+        limit = query_params.get('limit')
-            resp = session.get(uri,
+
+        # Track the total number of resources yielded so we can paginate
+        # swift objects
+        total_yielded = 0
+        while uri:
+            # Copy query_params due to weird mock unittest interactions
+            response = session.get(
+                uri,
                 headers={"Accept": "application/json"},
-                               params=query_params)
+                params=query_params.copy())
-            resp = resp.json()
+            exceptions.raise_from_response(response)
+            data = response.json()
+
+            # Discard any existing pagination keys
+            query_params.pop('marker', None)
+            query_params.pop('limit', None)
+
             if cls.resources_key:
-                resp = resp[cls.resources_key]
+                resources = data[cls.resources_key]
+            else:
+                resources = data
 
-            if not resp:
+            if not isinstance(resources, list):
-                more_data = False
+                resources = [resources]
 
-            # Keep track of how many items we've yielded. If we yielded
+            # Keep track of how many items we've yielded. The server should
-            # less than our limit, we don't need to do an extra request
+            # handle this, but it's easy for us to as well.
-            # to get back an empty data set, which acts as a sentinel.
             yielded = 0
-            new_marker = None
+            marker = None
-            for data in resp:
+            for raw_resource in resources:
                 # Do not allow keys called "self" through. Glance chose
                 # to name a key "self", so we need to pop it out because
                 # we can't send it through cls.existing and into the
                 # Resource initializer. "self" is already the first
                 # argument and is practically a reserved word.
-                data.pop("self", None)
+                raw_resource.pop("self", None)
 
-                value = cls.existing(**data)
+                value = cls.existing(**raw_resource)
-                new_marker = value.id
+                marker = value.id
-                yielded += 1
                 yield value
+                yielded += 1
+                total_yielded += 1
 
-            if not paginated:
+            # If a limit was given by the user and we have not returned
+            # as many records as the limit, then it stands to reason that
+            # there are no more records to return and we don't need to do
+            # anything else.
+            if limit and yielded < limit:
                 return
-            if "limit" in query_params and yielded < query_params["limit"]:
+
+            if resources and paginated:
+                uri, next_params = cls._get_next_link(
+                    uri, response, data, marker, limit, total_yielded)
+                query_params.update(next_params)
+            else:
                 return
-            query_params["limit"] = yielded
+
-            query_params["marker"] = new_marker
+    @classmethod
+    def _get_next_link(cls, uri, response, data, marker, limit, total_yielded):
+        next_link = None
+        params = {}
+        if isinstance(data, dict):
+            pagination_key = cls.pagination_key
+
+            if not pagination_key and 'links' in data:
+                # api-wg guidelines are for a links dict in the main body
+                pagination_key == 'links'
+            if not pagination_key and cls.resources_key:
+                # Nova has a {key}_links dict in the main body
+                pagination_key = '{key}_links'.format(key=cls.resources_key)
+            if pagination_key:
+                links = data.get(pagination_key, {})
+                for item in links:
+                    if item.get('rel') == 'next' and 'href' in item:
+                        next_link = item['href']
+                        break
+            # Glance has a next field in the main body
+            next_link = next_link or data.get('next')
+        if not next_link and 'next' in response.links:
+            # RFC5988 specifies Link headers and requests parses them if they
+            # are there. We prefer link dicts in resource body, but if those
+            # aren't there and Link headers are, use them.
+            next_link = response.links['next']['uri']
+        # Swift provides a count of resources in a header and a list body
+        if not next_link and cls.pagination_key:
+            total_count = response.headers.get(cls.pagination_key)
+            if total_count:
+                total_count = int(total_count)
+                if total_count > total_yielded:
+                    params['marker'] = marker
+                    if limit:
+                        params['limit'] = limit
+                    next_link = uri
+        # If we still have no link, and limit was given and is non-zero,
+        # and the number of records yielded equals the limit, then the user
+        # is playing pagination ball so we should go ahead and try once more.
+        if not next_link and limit:
+            next_link = uri
+            params['marker'] = marker
+            params['limit'] = limit
+        return next_link, params
 
     @classmethod
     def _get_one_match(cls, name_or_id, results):

openstack/tests/unit/meter/v2/test_sample.py

@@ -65,6 +65,7 @@ class TestSample(testtools.TestCase):
         sess = mock.Mock()
         resp = mock.Mock()
         resp.json = mock.Mock(return_value=[SAMPLE])
+        resp.status_code = 200
         sess.get = mock.Mock(return_value=resp)
 
         found = sample.Sample.list(sess, counter_name='name_of_meter')

openstack/tests/unit/network/v2/test_floating_ip.py

@@ -73,6 +73,7 @@ class TestFloatingIP(testtools.TestCase):
         fake_response = mock.Mock()
         body = {floating_ip.FloatingIP.resources_key: [data]}
         fake_response.json = mock.Mock(return_value=body)
+        fake_response.status_code = 200
         mock_session.get = mock.Mock(return_value=fake_response)
 
         result = floating_ip.FloatingIP.find_available(mock_session)
@@ -88,6 +89,7 @@ class TestFloatingIP(testtools.TestCase):
         fake_response = mock.Mock()
         body = {floating_ip.FloatingIP.resources_key: []}
         fake_response.json = mock.Mock(return_value=body)
+        fake_response.status_code = 200
         mock_session.get = mock.Mock(return_value=fake_response)
 
         self.assertIsNone(floating_ip.FloatingIP.find_available(mock_session))

openstack/tests/unit/test_resource2.py

@@ -938,6 +938,7 @@ class TestResourceActions(base.TestCase):
         class Test(resource2.Resource):
             service = self.service_name
             base_path = self.base_path
+            resources_key = 'resources'
             allow_create = True
             allow_get = True
             allow_head = True
@@ -1096,7 +1097,9 @@ class TestResourceActions(base.TestCase):
     # the generator. Wrap calls to self.sot.list in a `list`
     # and then test the results as a list of responses.
     def test_list_empty_response(self):
-        mock_response = FakeResponse([])
+        mock_response = mock.Mock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {"resources": []}
 
         self.session.get.return_value = mock_response
 
@@ -1112,8 +1115,9 @@ class TestResourceActions(base.TestCase):
     def test_list_one_page_response_paginated(self):
         id_value = 1
         mock_response = mock.Mock()
-        mock_response.json.side_effect = [[{"id": id_value}],
+        mock_response.status_code = 200
-                                          []]
+        mock_response.links = {}
+        mock_response.json.return_value = {"resources": [{"id": id_value}]}
 
         self.session.get.return_value = mock_response
 
@@ -1123,17 +1127,15 @@ class TestResourceActions(base.TestCase):
 
         self.assertEqual(1, len(results))
 
-        # Look at the `params` argument to each of the get calls that
+        self.assertEqual(1, len(self.session.get.call_args_list))
-        # were made.
-        self.session.get.call_args_list[0][1]["params"] = {}
-        self.session.get.call_args_list[1][1]["params"] = {"marker": id_value}
         self.assertEqual(id_value, results[0].id)
         self.assertIsInstance(results[0], self.test_class)
 
     def test_list_one_page_response_not_paginated(self):
         id_value = 1
         mock_response = mock.Mock()
-        mock_response.json.return_value = [{"id": id_value}]
+        mock_response.status_code = 200
+        mock_response.json.return_value = {"resources": [{"id": id_value}]}
 
         self.session.get.return_value = mock_response
 
@@ -1156,6 +1158,7 @@ class TestResourceActions(base.TestCase):
 
         id_value = 1
         mock_response = mock.Mock()
+        mock_response.status_code = 200
         mock_response.json.return_value = {key: [{"id": id_value}]}
 
         self.session.get.return_value = mock_response
@@ -1173,11 +1176,85 @@ class TestResourceActions(base.TestCase):
         self.assertEqual(id_value, results[0].id)
         self.assertIsInstance(results[0], self.test_class)
 
+    def test_list_response_paginated_without_links(self):
+        ids = [1, 2]
+        mock_response = mock.Mock()
+        mock_response.status_code = 200
+        mock_response.links = {}
+        mock_response.json.return_value = {
+            "resources": [{"id": ids[0]}],
+            "resources_links": [{
+                "href": "https://example.com/next-url",
+                "rel": "next",
+            }]
+        }
+        mock_response2 = mock.Mock()
+        mock_response2.status_code = 200
+        mock_response2.links = {}
+        mock_response2.json.return_value = {
+            "resources": [{"id": ids[1]}],
+        }
+
+        self.session.get.side_effect = [mock_response, mock_response2]
+
+        results = list(self.sot.list(self.session, paginated=True))
+
+        self.assertEqual(2, len(results))
+        self.assertEqual(ids[0], results[0].id)
+        self.assertEqual(ids[1], results[1].id)
+        self.assertEqual(
+            mock.call('base_path',
+                      headers={'Accept': 'application/json'}, params={}),
+            self.session.get.mock_calls[0])
+        self.assertEqual(
+            mock.call('https://example.com/next-url',
+                      headers={'Accept': 'application/json'}, params={}),
+            self.session.get.mock_calls[1])
+        self.assertEqual(2, len(self.session.get.call_args_list))
+        self.assertIsInstance(results[0], self.test_class)
+
+    def test_list_response_paginated_with_links(self):
+        ids = [1, 2]
+        mock_response = mock.Mock()
+        mock_response.status_code = 200
+        mock_response.links = {}
+        mock_response.json.side_effect = [
+            {
+                "resources": [{"id": ids[0]}],
+                "resources_links": [{
+                    "href": "https://example.com/next-url",
+                    "rel": "next",
+                }]
+            }, {
+                "resources": [{"id": ids[1]}],
+            }]
+
+        self.session.get.return_value = mock_response
+
+        results = list(self.sot.list(self.session, paginated=True))
+
+        self.assertEqual(2, len(results))
+        self.assertEqual(ids[0], results[0].id)
+        self.assertEqual(ids[1], results[1].id)
+        self.assertEqual(
+            mock.call('base_path',
+                      headers={'Accept': 'application/json'}, params={}),
+            self.session.get.mock_calls[0])
+        self.assertEqual(
+            mock.call('https://example.com/next-url',
+                      headers={'Accept': 'application/json'}, params={}),
+            self.session.get.mock_calls[2])
+        self.assertEqual(2, len(self.session.get.call_args_list))
+        self.assertIsInstance(results[0], self.test_class)
+
     def test_list_multi_page_response_not_paginated(self):
         ids = [1, 2]
         mock_response = mock.Mock()
-        mock_response.json.side_effect = [[{"id": ids[0]}],
+        mock_response.status_code = 200
-                                          [{"id": ids[1]}]]
+        mock_response.json.side_effect = [
+            {"resources": [{"id": ids[0]}]},
+            {"resources": [{"id": ids[1]}]},
+        ]
 
         self.session.get.return_value = mock_response
 
@@ -1194,10 +1271,16 @@ class TestResourceActions(base.TestCase):
         uri_param = "uri param!"
 
         mock_response = mock.Mock()
-        mock_response.json.side_effect = [[{"id": id}],
+        mock_response.status_code = 200
-                                          []]
+        mock_response.links = {}
+        mock_response.json.return_value = {"resources": [{"id": id}]}
 
-        self.session.get.return_value = mock_response
+        mock_empty = mock.Mock()
+        mock_empty.status_code = 200
+        mock_empty.links = {}
+        mock_empty.json.return_value = {"resources": []}
+
+        self.session.get.side_effect = [mock_response, mock_empty]
 
         class Test(self.test_class):
             _query_mapping = resource2.QueryParameters(query_param=qp_name)
@@ -1217,19 +1300,33 @@ class TestResourceActions(base.TestCase):
                          Test.base_path % {"something": uri_param})
 
     def test_list_multi_page_response_paginated(self):
-        # This tests our ability to stop making calls once
-        # we've received all of the data. However, this tests
-        # the case that we always receive full pages of data
-        # and then the signal that there is no more data - an empty list.
-        # In this case, we need to make one extra request beyond
-        # the end of data to ensure we've received it all.
         ids = [1, 2]
         resp1 = mock.Mock()
-        resp1.json.return_value = [{"id": ids[0]}]
+        resp1.status_code = 200
+        resp1.links = {}
+        resp1.json.return_value = {
+            "resources": [{"id": ids[0]}],
+            "resources_links": [{
+                "href": "https://example.com/next-url",
+                "rel": "next",
+            }],
+        }
         resp2 = mock.Mock()
-        resp2.json.return_value = [{"id": ids[1]}]
+        resp2.status_code = 200
+        resp2.links = {}
+        resp2.json.return_value = {
+            "resources": [{"id": ids[1]}],
+            "resources_links": [{
+                "href": "https://example.com/next-url",
+                "rel": "next",
+            }],
+        }
         resp3 = mock.Mock()
-        resp3.json.return_value = []
+        resp3.status_code = 200
+        resp3.links = {}
+        resp3.json.return_value = {
+            "resources": []
+        }
 
         self.session.get.side_effect = [resp1, resp2, resp3]
 
@@ -1245,26 +1342,113 @@ class TestResourceActions(base.TestCase):
         result1 = next(results)
         self.assertEqual(result1.id, ids[1])
         self.session.get.assert_called_with(
-            self.base_path,
+            'https://example.com/next-url',
             headers={"Accept": "application/json"},
-            params={"limit": 1, "marker": 1})
+            params={})
 
         self.assertRaises(StopIteration, next, results)
         self.session.get.assert_called_with(
-            self.base_path,
+            'https://example.com/next-url',
             headers={"Accept": "application/json"},
-            params={"limit": 1, "marker": 2})
+            params={})
 
     def test_list_multi_page_early_termination(self):
         # This tests our ability to be somewhat smart when evaluating
-        # the contents of the responses. When we receive a full page
+        # the contents of the responses. When we request a limit and
-        # of data, we can be smart about terminating our responses
+        # receive less than that limit and there are no next links,
-        # once we see that we've received a page with less data than
+        # we can be pretty sure there are no more pages.
-        # expected, saving one request.
+        ids = [1, 2]
+        resp1 = mock.Mock()
+        resp1.status_code = 200
+        resp1.json.return_value = {
+            "resources": [{"id": ids[0]}, {"id": ids[1]}],
+        }
+
+        self.session.get.return_value = resp1
+
+        results = self.sot.list(self.session, limit=3, paginated=True)
+
+        result0 = next(results)
+        self.assertEqual(result0.id, ids[0])
+        result1 = next(results)
+        self.assertEqual(result1.id, ids[1])
+        self.session.get.assert_called_with(
+            self.base_path,
+            headers={"Accept": "application/json"},
+            params={"limit": 3})
+
+        # Ensure we're done after those two items
+        self.assertRaises(StopIteration, next, results)
+
+        # Ensure we only made one calls to get this done
+        self.assertEqual(1, len(self.session.get.call_args_list))
+
+    def test_list_multi_page_inferred_additional(self):
+        # If we explicitly request a limit and we receive EXACTLY that
+        # amount of results and there is no next link, we make one additional
+        # call to check to see if there are more records and the service is
+        # just sad.
+        # NOTE(mordred) In a perfect world we would not do this. But it's 2018
+        # and I don't think anyone has any illusions that we live in a perfect
+        # world anymore.
         ids = [1, 2, 3]
         resp1 = mock.Mock()
+        resp1.status_code = 200
+        resp1.links = {}
+        resp1.json.return_value = {
+            "resources": [{"id": ids[0]}, {"id": ids[1]}],
+        }
+        resp2 = mock.Mock()
+        resp2.status_code = 200
+        resp2.links = {}
+        resp2.json.return_value = {"resources": [{"id": ids[2]}]}
+
+        self.session.get.side_effect = [resp1, resp2]
+
+        results = self.sot.list(self.session, limit=2, paginated=True)
+
+        # Get the first page's two items
+        result0 = next(results)
+        self.assertEqual(result0.id, ids[0])
+        result1 = next(results)
+        self.assertEqual(result1.id, ids[1])
+        self.session.get.assert_called_with(
+            self.base_path,
+            headers={"Accept": "application/json"},
+            params={"limit": 2})
+
+        result2 = next(results)
+        self.assertEqual(result2.id, ids[2])
+        self.session.get.assert_called_with(
+            self.base_path,
+            headers={"Accept": "application/json"},
+            params={'limit': 2, 'marker': 2})
+
+        # Ensure we're done after those three items
+        self.assertRaises(StopIteration, next, results)
+
+        # Ensure we only made two calls to get this done
+        self.assertEqual(2, len(self.session.get.call_args_list))
+
+    def test_list_multi_page_header_count(self):
+        class Test(self.test_class):
+            resources_key = None
+            pagination_key = 'X-Container-Object-Count'
+        self.sot = Test()
+
+        # Swift returns a total number of objects in a header and we compare
+        # that against the total number returned to know if we need to fetch
+        # more objects.
+        ids = [1, 2, 3]
+        resp1 = mock.Mock()
+        resp1.status_code = 200
+        resp1.links = {}
+        resp1.headers = {'X-Container-Object-Count': 3}
         resp1.json.return_value = [{"id": ids[0]}, {"id": ids[1]}]
         resp2 = mock.Mock()
+        resp2.status_code = 200
+        resp2.links = {}
+        resp2.headers = {'X-Container-Object-Count': 3}
         resp2.json.return_value = [{"id": ids[2]}]
 
         self.session.get.side_effect = [resp1, resp2]
@@ -1281,13 +1465,58 @@ class TestResourceActions(base.TestCase):
             headers={"Accept": "application/json"},
             params={})
 
-        # Second page only has one item
         result2 = next(results)
         self.assertEqual(result2.id, ids[2])
         self.session.get.assert_called_with(
             self.base_path,
             headers={"Accept": "application/json"},
-            params={"limit": 2, "marker": 2})
+            params={'marker': 2})
+
+        # Ensure we're done after those three items
+        self.assertRaises(StopIteration, next, results)
+
+        # Ensure we only made two calls to get this done
+        self.assertEqual(2, len(self.session.get.call_args_list))
+
+    def test_list_multi_page_link_header(self):
+        # Swift returns a total number of objects in a header and we compare
+        # that against the total number returned to know if we need to fetch
+        # more objects.
+        ids = [1, 2, 3]
+        resp1 = mock.Mock()
+        resp1.status_code = 200
+        resp1.links = {
+            'next': {'uri': 'https://example.com/next-url', 'rel': 'next'}}
+        resp1.headers = {}
+        resp1.json.return_value = {
+            "resources": [{"id": ids[0]}, {"id": ids[1]}],
+        }
+        resp2 = mock.Mock()
+        resp2.status_code = 200
+        resp2.links = {}
+        resp2.headers = {}
+        resp2.json.return_value = {"resources": [{"id": ids[2]}]}
+
+        self.session.get.side_effect = [resp1, resp2]
+
+        results = self.sot.list(self.session, paginated=True)
+
+        # Get the first page's two items
+        result0 = next(results)
+        self.assertEqual(result0.id, ids[0])
+        result1 = next(results)
+        self.assertEqual(result1.id, ids[1])
+        self.session.get.assert_called_with(
+            self.base_path,
+            headers={"Accept": "application/json"},
+            params={})
+
+        result2 = next(results)
+        self.assertEqual(result2.id, ids[2])
+        self.session.get.assert_called_with(
+            'https://example.com/next-url',
+            headers={"Accept": "application/json"},
+            params={})
 
         # Ensure we're done after those three items
         self.assertRaises(StopIteration, next, results)