Convert list and find to use params parameter

We were doing a last minute switch to params in the page
method, but we need to leave it up to the user to set limit,
marker, fields, and search criteria.  What is supported by the
servers varies a lot.

Closes-Bug: #1466175

Change-Id: I6b5932a812e2d0821f0ae5ed2d93b44dbd10b19a

examples/list.py

@@ -22,7 +22,7 @@ def run_list(opts):
     path_args = None
     if opts.data:
         path_args = common.get_data_option(opts)
-    for obj in cls.list(sess, path_args=path_args):
+    for obj in cls.list(sess, path_args=path_args, paginated=True):
         print(str(obj))
     return
 

openstack/network/v2/floating_ip.py

@@ -50,11 +50,11 @@ class FloatingIP(resource.Resource):
 
     @classmethod
     def find_available(cls, session):
-        args = {
+        params = {
             'port_id': '',
             'fields': cls.id_attribute,
         }
-        info = cls.list(session, **args)
+        info = cls.list(session, params=params)
         try:
             return next(info)
         except StopIteration:

openstack/resource.py

@@ -36,7 +36,6 @@ import itertools
 import time
 
 import six
-from six.moves.urllib import parse as url_parse
 
 from openstack import exceptions
 from openstack import utils
@@ -813,29 +812,14 @@ class Resource(collections.MutableMapping):
         self.delete_by_id(session, self.id, path_args=self)
 
     @classmethod
-    def list(cls, session, limit=None, marker=None, path_args=None,
+    def list(cls, session, path_args=None, paginated=False, params=None):
-             paginated=False, **params):
+        """This method is a generator which yields resource objects.
-        """Get a response that is a list of objects.
 
-        This method starts at ``limit`` and ``marker`` (both defaulting to
+        This resource object list generator handles pagination and takes query
-        None), advances the marker to the last item received in each response,
+        params for response filtering.
-        and continues making requests for more resources until no results
-        are returned.
-
-        When no ``limit`` is provided, the server will return its maximum
-        amount of items per page. After that response, the limit will be
-        determined and sent in subsequent requests to the server. That limit
-        will be used to calculate when all items have been received.
 
         :param session: The session to use for making this request.
         :type session: :class:`~openstack.session.Session`
-        :param limit: The maximum amount of results to retrieve.
-                      The default is ``None``, which means no limit will be
-                      set, and the underlying API will return its default
-                      amount of responses.
-        :param marker: The position in the list to begin requests from.
-                       The type of value to use for ``marker`` depends on
-                       the API being called.
         :param dict path_args: A dictionary of arguments to construct
                                a compound URL.
                                See `How path_args are used`_ for details.
@@ -845,8 +829,10 @@ class Resource(collections.MutableMapping):
                                **When paginated is False only one
                                page of data will be returned regardless
                                of the API's support of pagination.**
-        :param dict params: Parameters to be passed into the underlying
+        :param dict params: Query parameters to be passed into the underlying
                             :meth:`~openstack.session.Session.get` method.
+                            Values that the server may support include `limit`
+                            and `marker`.
 
         :return: A generator of :class:`Resource` objects.
         :raises: :exc:`~openstack.exceptions.MethodNotSupported` if
@@ -856,9 +842,12 @@ class Resource(collections.MutableMapping):
             raise exceptions.MethodNotSupported(cls, 'list')
 
         more_data = True
-
+        params = {} if params is None else params
+        url = cls._get_url(path_args)
         while more_data:
-            resp = cls.page(session, limit, marker, path_args, **params)
+            resp = session.get(url, service=cls.service, params=params).body
+            if cls.resources_key:
+                resp = resp[cls.resources_key]
 
             if not resp:
                 more_data = False
@@ -869,55 +858,16 @@ class Resource(collections.MutableMapping):
             yielded = 0
             for data in resp:
                 value = cls.existing(**data)
-                marker = value.id
+                new_marker = value.id
                 yielded += 1
                 yield value
 
-            if not paginated or limit and yielded < limit:
+            if not paginated:
-                more_data = False
+                return
-
+            if 'limit' in params and yielded < params['limit']:
-            limit = yielded
+                return
-
+            params['limit'] = yielded
-    @classmethod
+            params['marker'] = new_marker
-    def page(cls, session, limit=None, marker=None, path_args=None, **params):
-        """Get a one page response.
-
-        This method gets starting at ``marker`` with a maximum of ``limit``
-        records.
-
-        :param session: The session to use for making this request.
-        :type session: :class:`~openstack.session.Session`
-        :param limit: The maximum amount of results to retrieve. The default
-                      is to retrieve as many results as the service allows.
-        :param marker: The position in the list to begin requests from.
-                       The type of value to use for ``marker`` depends on
-                       the API being called.
-        :param dict path_args: A dictionary of arguments to construct
-                               a compound URL.
-                               See `How path_args are used`_ for details.
-        :param dict params: Parameters to be passed into the underlying
-                            :meth:`~openstack.session.Session.get` method.
-
-        :return: A list of dictionaries returned in the response body.
-        """
-
-        filters = {}
-
-        if limit:
-            filters['limit'] = limit
-        if marker:
-            filters['marker'] = marker
-
-        url = cls._get_url(path_args)
-        if filters:
-            url = '%s?%s' % (url, url_parse.urlencode(filters))
-
-        resp = session.get(url, service=cls.service, params=params).body
-
-        if cls.resources_key:
-            resp = resp[cls.resources_key]
-
-        return resp
 
     @classmethod
     def find(cls, session, name_or_id, path_args=None, ignore_missing=True):
@@ -943,9 +893,18 @@ class Resource(collections.MutableMapping):
         :raises: :class:`openstack.exceptions.ResourceNotFound` if nothing
                  is found and ignore_missing is ``False``.
         """
-        def get_one_match(data, attr):
+        def get_one_match(generator, attr, raise_exception):
-            if len(data) == 1:
+            try:
-                result = cls.existing(**data[0])
+                first = next(generator)
+                if any(generator):
+                    if raise_exception:
+                        msg = "More than one %s exists with the name '%s'."
+                        msg = (msg % (cls.get_resource_name(), name_or_id))
+                        raise exceptions.DuplicateResource(msg)
+                    return None
+            except StopIteration:
+                return None
+            result = cls.existing(**first)
             value = getattr(result, attr, False)
             if value == name_or_id:
                 return result
@@ -954,24 +913,20 @@ class Resource(collections.MutableMapping):
         try:
             if cls.allow_retrieve:
                 return cls.get_by_id(session, name_or_id, path_args=path_args)
-            args = {cls.id_attribute: name_or_id}
+            params = {'limit': 2}
-            info = cls.page(session, limit=2, path_args=path_args, **args)
+            params[cls.id_attribute] = name_or_id
-            result = get_one_match(info, cls.id_attribute)
+            info = cls.list(session, path_args=path_args, params=params)
+            result = get_one_match(info, cls.id_attribute, False)
             if result is not None:
                 return result
         except exceptions.HttpException:
             pass
 
         if cls.name_attribute:
-            params = {cls.name_attribute: name_or_id}
+            params = {'limit': 2}
-            info = cls.page(session, limit=2, path_args=path_args, **params)
+            params[cls.name_attribute] = name_or_id
-
+            info = cls.list(session, path_args=path_args, params=params)
-            if len(info) > 1:
+            result = get_one_match(info, cls.name_attribute, True)
-                msg = "More than one %s exists with the name '%s'."
-                msg = (msg % (cls.get_resource_name(), name_or_id))
-                raise exceptions.DuplicateResource(msg)
-
-            result = get_one_match(info, cls.name_attribute)
             if result is not None:
                 return result
 

openstack/tests/unit/test_resource.py

@@ -737,7 +737,6 @@ class ResourceTests(base.TestTransportBase):
         results = [fake_data.copy(), fake_data.copy(), fake_data.copy()]
         for i in range(len(results)):
             results[i]['id'] = fake_id + i
-
         if resource_class.resources_key is not None:
             body = {resource_class.resources_key:
                     self._get_expected_results()}
@@ -745,19 +744,15 @@ class ResourceTests(base.TestTransportBase):
         else:
             body = self._get_expected_results()
             sentinel = []
-
-        marker = "marker=%d" % results[-1]['id']
-
         self.session.get.side_effect = [mock.Mock(body=body),
                                         mock.Mock(body=sentinel)]
 
-        objs = resource_class.list(self.session, limit=1,
+        objs = list(resource_class.list(self.session, path_args=fake_arguments,
-                                   path_args=fake_arguments,
+                                        paginated=True))
-                                   paginated=True)
-        objs = list(objs)
-        self.assertIn(marker, self.session.get.call_args[0][0])
-        self.assertEqual(3, len(objs))
 
+        params = {'limit': 3, 'marker': results[-1]['id']}
+        self.assertEqual(params, self.session.get.call_args[1]['params'])
+        self.assertEqual(3, len(objs))
         for obj in objs:
             self.assertIn(obj.id, range(fake_id, fake_id + 3))
             self.assertEqual(fake_name, obj['name'])
@@ -783,7 +778,7 @@ class ResourceTests(base.TestTransportBase):
         attrs = {"get.return_value": body}
         session = mock.Mock(**attrs)
 
-        list(FakeResource.list(session, limit=len(results) + 1,
+        list(FakeResource.list(session, params={'limit': len(results) + 1},
                                path_args=fake_arguments,
                                paginated=paginated))
 
@@ -798,53 +793,26 @@ class ResourceTests(base.TestTransportBase):
         # When we call with paginated=False, make sure we made one call
         self._test_list_call_count(False)
 
-    @mock.patch("openstack.resource.Resource.page")
+    def test_determine_limit(self):
-    def test_determine_limit(self, fake_page):
         full_page = [fake_data.copy(), fake_data.copy(), fake_data.copy()]
         last_page = [fake_data.copy()]
 
         session = mock.MagicMock()
-        pages = [full_page, full_page, last_page]
+        session.get = mock.MagicMock()
-        fake_page.side_effect = pages
+        full_response = mock.MagicMock()
+        full_response.body = {FakeResource.resources_key: full_page}
+        last_response = mock.MagicMock()
+        last_response.body = {FakeResource.resources_key: last_page}
+        pages = [full_response, full_response, last_response]
+        session.get.side_effect = pages
 
         # Don't specify a limit. Resource.list will determine the limit
         # is 3 based on the first `full_page`.
         results = list(FakeResource.list(session, path_args=fake_arguments,
                        paginated=True))
 
-        self.assertEqual(fake_page.call_count, len(pages))
+        self.assertEqual(session.get.call_count, len(pages))
-        self.assertEqual(len(results), sum([len(page) for page in pages]))
+        self.assertEqual(len(full_page + full_page + last_page), len(results))
-
-    def test_page(self):
-        session = mock.Mock()
-        session.get = mock.Mock()
-        records = [{'id': 'squid'}]
-        response = FakeResponse({FakeResource.resources_key: records})
-        session.get.return_value = response
-
-        objs = FakeResource.page(session, 1, None, path_args=fake_arguments)
-
-        self.assertEqual(records, objs)
-        path = fake_path + '?limit=1'
-        session.get.assert_called_with(path, params={}, service=None)
-
-        objs = FakeResource.page(session, None, 'a', path_args=fake_arguments)
-
-        self.assertEqual(records, objs)
-        path = fake_path + '?marker=a'
-        session.get.assert_called_with(path, params={}, service=None)
-
-        objs = FakeResource.page(session, None, None, path_args=fake_arguments)
-
-        self.assertEqual(records, objs)
-        path = fake_path
-        session.get.assert_called_with(path, params={}, service=None)
-
-        objs = FakeResource.page(session)
-
-        self.assertEqual(records, objs)
-        path = fake_base_path
-        session.get.assert_called_with(path, params={}, service=None)
 
     def test_attrs_name(self):
         obj = FakeResource()
@@ -1200,9 +1168,8 @@ class TestFind(base.TestCase):
         self.assertEqual(self.NAME, result.name)
         self.assertEqual(self.PROP, result.prop)
 
-        p = {'name': self.NAME}
+        p = {'limit': 2, 'name': self.NAME}
-        path = fake_path + "?limit=2"
+        self.assertEqual(p, self.mock_get.call_args[1]['params'])
-        self.mock_get.assert_any_call(path, params=p, service=None)
 
     def test_id(self):
         self.mock_get.side_effect = [
@@ -1232,9 +1199,8 @@ class TestFind(base.TestCase):
         self.assertEqual(self.ID, result.id)
         self.assertEqual(self.PROP, result.prop)
 
-        p = {'id': self.ID}
+        p = {'id': self.ID, 'limit': 2}
-        path = fake_path + "?limit=2"
+        self.assertEqual(p, self.mock_get.call_args[1]['params'])
-        self.mock_get.assert_any_call(path, params=p, service=None)
 
     def test_dups(self):
         dup = {'id': 'Larry'}