Unify resource list filtering

Extend resource list method to accept all possible filtering parameters.
What is supported by the API are sent to the server. Remaining
parameters are applied to the fetched results. With this
`allow_unknown_params` parameter of the list call is dropped.

Change-Id: Ie9cfb81330d6b98b97b7abad9cf5ae6334ba12e7

openstack/cloud/_compute.py

@@ -153,8 +153,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
         """
         if not filters:
             filters = {}
-        return list(self.compute.keypairs(allow_unknown_params=True,
+        return list(self.compute.keypairs(**filters))
-                                          **filters))
 
     @_utils.cache_on_arguments()
     def list_availability_zone_names(self, unavailable=False):
@@ -353,7 +352,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
         return [
             self._expand_server(server, detailed, bare)
             for server in self.compute.servers(
-                all_projects=all_projects, allow_unknown_params=True,
+                all_projects=all_projects,
                 **filters)
         ]
 
@@ -1504,7 +1503,6 @@ class ComputeCloudMixin(_normalize.Normalizer):
 
         return list(self.compute.hypervisors(
             details=True,
-            allow_unknown_params=True,
             **filters))
 
     def search_aggregates(self, name_or_id=None, filters=None):
@@ -1525,7 +1523,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
 
         :returns: A list of compute ``Aggregate`` objects.
         """
-        return self.compute.aggregates(allow_unknown_params=True, **filters)
+        return self.compute.aggregates(**filters)
 
     # TODO(stephenfin): This shouldn't return a munch
     def get_aggregate(self, name_or_id, filters=None):

openstack/cloud/_security_group.py

@@ -57,8 +57,7 @@ class SecurityGroupCloudMixin(_normalize.Normalizer):
             # pass filters dict to the list to filter as much as possible on
             # the server side
             return list(
-                self.network.security_groups(allow_unknown_params=True,
+                self.network.security_groups(**filters))
-                                             **filters))
 
         # Handle nova security groups
         else:

openstack/compute/v2/flavor.py

@@ -86,8 +86,13 @@ class Flavor(resource.Resource):
             return super().__getattribute__(name)
 
     @classmethod
-    def list(cls, session, paginated=True, base_path='/flavors/detail',
+    def list(
-             allow_unknown_params=False, **params):
+        cls,
+        session,
+        paginated=True,
+        base_path='/flavors/detail',
+        **params
+    ):
         # Find will invoke list when name was passed. Since we want to return
         # flavor with details (same as direct get) we need to swap default here
         # and list with "/flavors" if no details explicitely requested
@@ -98,7 +103,6 @@ class Flavor(resource.Resource):
         return super(Flavor, cls).list(
             session, paginated=paginated,
             base_path=base_path,
-            allow_unknown_params=allow_unknown_params,
             **params)
 
     def _action(self, session, body, microversion=None):

openstack/proxy.py

@@ -21,6 +21,7 @@ try:
 except ImportError:
     JSONDecodeError = ValueError
 import iso8601
+import jmespath
 from keystoneauth1 import adapter
 
 from openstack import _log
@@ -637,7 +638,14 @@ class Proxy(adapter.Adapter):
             ),
         )
 
-    def _list(self, resource_type, paginated=True, base_path=None, **attrs):
+    def _list(
+        self,
+        resource_type,
+        paginated=True,
+        base_path=None,
+        jmespath_filters=None,
+        **attrs
+    ):
         """List a resource
 
         :param resource_type: The type of resource to list. This should
@@ -650,6 +658,9 @@ class Proxy(adapter.Adapter):
         :param str base_path: Base part of the URI for listing resources, if
             different from
             :data:`~openstack.resource.Resource.base_path`.
+        :param str jmespath_filters: A string containing a jmespath expression
+            for further filtering.
+
         :param dict attrs: Attributes to be passed onto the
             :meth:`~openstack.resource.Resource.list` method. These should
             correspond to either :class:`~openstack.resource.URI` values
@@ -660,10 +671,17 @@ class Proxy(adapter.Adapter):
             :class:`~openstack.resource.Resource` that doesn't match
             the ``resource_type``.
         """
-        return resource_type.list(
+
-            self, paginated=paginated, base_path=base_path, **attrs
+        data = resource_type.list(
+            self, paginated=paginated, base_path=base_path,
+            **attrs
         )
 
+        if jmespath_filters and isinstance(jmespath_filters, str):
+            return jmespath.search(jmespath_filters, data)
+
+        return data
+
     def _head(self, resource_type, value=None, base_path=None, **attrs):
         """Retrieve a resource's header
 

openstack/resource.py

@@ -1953,6 +1953,9 @@ class Resource(dict):
             checked against the :data:`~openstack.resource.Resource.base_path`
             format string to see if any path fragments need to be filled in by
             the contents of this argument.
+            Parameters supported as filters by the server side are passed in
+            the API call, remaining parameters are applied as filters to the
+            retrieved results.
 
         :return: A generator of :class:`Resource` objects.
         :raises: :exc:`~openstack.exceptions.MethodNotSupported` if
@@ -1969,12 +1972,24 @@ class Resource(dict):
 
         if base_path is None:
             base_path = cls.base_path
-        params = cls._query_mapping._validate(
+        api_filters = cls._query_mapping._validate(
             params,
             base_path=base_path,
-            allow_unknown_params=allow_unknown_params,
+            allow_unknown_params=True,
         )
-        query_params = cls._query_mapping._transpose(params, cls)
+        client_filters = dict()
+        # Gather query parameters which are not supported by the server
+        for (k, v) in params.items():
+            if (
+                # Known attr
+                hasattr(cls, k)
+                # Is real attr property
+                and isinstance(getattr(cls, k), Body)
+                # not included in the query_params
+                and k not in cls._query_mapping._mapping.keys()
+            ):
+                client_filters[k] = v
+        query_params = cls._query_mapping._transpose(api_filters, cls)
         uri = base_path % params
         uri_params = {}
 
@@ -1985,6 +2000,18 @@ class Resource(dict):
             if hasattr(cls, k) and isinstance(getattr(cls, k), URI):
                 uri_params[k] = v
 
+        def _dict_filter(f, d):
+            """Dict param based filtering"""
+            if not d:
+                return False
+            for key in f.keys():
+                if isinstance(f[key], dict):
+                    if not _dict_filter(f[key], d.get(key, None)):
+                        return False
+                elif d.get(key, None) != f[key]:
+                    return False
+            return True
+
         # Track the total number of resources yielded so we can paginate
         # swift objects
         total_yielded = 0
@@ -2028,7 +2055,20 @@ class Resource(dict):
                     **raw_resource,
                 )
                 marker = value.id
-                yield value
+                filters_matched = True
+                # Iterate over client filters and return only if matching
+                for key in client_filters.keys():
+                    if isinstance(client_filters[key], dict):
+                        if not _dict_filter(
+                                client_filters[key], value.get(key, None)):
+                            filters_matched = False
+                            break
+                    elif value.get(key, None) != client_filters[key]:
+                        filters_matched = False
+                        break
+
+                if filters_matched:
+                    yield value
                 total_yielded += 1
 
             if resources and paginated:

openstack/tests/unit/test_proxy.py

@@ -43,6 +43,16 @@ class ListableResource(resource.Resource):
     allow_list = True
 
 
+class FilterableResource(resource.Resource):
+    allow_list = True
+    base_path = '/fakes'
+
+    _query_mapping = resource.QueryParameters('a')
+    a = resource.Body('a')
+    b = resource.Body('b')
+    c = resource.Body('c')
+
+
 class HeadableResource(resource.Resource):
     allow_head = True
 
@@ -461,6 +471,28 @@ class TestProxyList(base.TestCase):
     def test_list_override_base_path(self):
         self._test_list(False, base_path='dummy')
 
+    def test_list_filters_jmespath(self):
+        fake_response = [
+            FilterableResource(a='a1', b='b1', c='c'),
+            FilterableResource(a='a2', b='b2', c='c'),
+            FilterableResource(a='a3', b='b3', c='c'),
+        ]
+        FilterableResource.list = mock.Mock()
+        FilterableResource.list.return_value = fake_response
+
+        rv = self.sot._list(
+            FilterableResource, paginated=False,
+            base_path=None, jmespath_filters="[?c=='c']"
+        )
+        self.assertEqual(3, len(rv))
+
+        # Test filtering based on unknown attribute
+        rv = self.sot._list(
+            FilterableResource, paginated=False,
+            base_path=None, jmespath_filters="[?d=='c']"
+        )
+        self.assertEqual(0, len(rv))
+
 
 class TestProxyHead(base.TestCase):
 

openstack/tests/unit/test_resource.py

@@ -2333,30 +2333,6 @@ class TestResourceActions(base.TestCase):
         self.assertEqual(self.session.get.call_args_list[0][0][0],
                          Test.base_path % {"something": uri_param})
 
-    def test_invalid_list_params(self):
-        id = 1
-        qp = "query param!"
-        qp_name = "query-param"
-        uri_param = "uri param!"
-
-        mock_response = mock.Mock()
-        mock_response.json.side_effect = [[{"id": id}],
-                                          []]
-
-        self.session.get.return_value = mock_response
-
-        class Test(self.test_class):
-            _query_mapping = resource.QueryParameters(query_param=qp_name)
-            base_path = "/%(something)s/blah"
-            something = resource.URI("something")
-
-        try:
-            list(Test.list(self.session, paginated=True, query_param=qp,
-                           something=uri_param, something_wrong=True))
-            self.assertFail('The above line should fail')
-        except exceptions.InvalidResourceQuery as err:
-            self.assertEqual(str(err), 'Invalid query params: something_wrong')
-
     def test_allow_invalid_list_params(self):
         qp = "query param!"
         qp_name = "query-param"
@@ -2384,6 +2360,40 @@ class TestResourceActions(base.TestCase):
             params={qp_name: qp}
         )
 
+    def test_list_client_filters(self):
+        qp = "query param!"
+        uri_param = "uri param!"
+
+        mock_empty = mock.Mock()
+        mock_empty.status_code = 200
+        mock_empty.links = {}
+        mock_empty.json.return_value = {"resources": [
+            {"a": "1", "b": "1"},
+            {"a": "1", "b": "2"},
+        ]}
+
+        self.session.get.side_effect = [mock_empty]
+
+        class Test(self.test_class):
+            _query_mapping = resource.QueryParameters('a')
+            base_path = "/%(something)s/blah"
+            something = resource.URI("something")
+            a = resource.Body("a")
+            b = resource.Body("b")
+
+        res = list(Test.list(
+            self.session, paginated=True, query_param=qp,
+            allow_unknown_params=True, something=uri_param,
+            a='1', b='2'))
+        self.session.get.assert_called_once_with(
+            "/{something}/blah".format(something=uri_param),
+            headers={'Accept': 'application/json'},
+            microversion=None,
+            params={'a': '1'}
+        )
+        self.assertEqual(1, len(res))
+        self.assertEqual("2", res[0].b)
+
     def test_values_as_list_params(self):
         id = 1
         qp = "query param!"