From a0292478c1b1682aa02c3911d0ee1a3899b63202 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Sat, 13 Aug 2022 15:34:55 +0200 Subject: [PATCH] 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 | 8 ++-- openstack/cloud/_security_group.py | 3 +- openstack/compute/v2/flavor.py | 10 +++-- openstack/proxy.py | 24 +++++++++-- openstack/resource.py | 48 ++++++++++++++++++++-- openstack/tests/unit/test_proxy.py | 32 +++++++++++++++ openstack/tests/unit/test_resource.py | 58 ++++++++++++++++----------- 7 files changed, 142 insertions(+), 41 deletions(-) diff --git a/openstack/cloud/_compute.py b/openstack/cloud/_compute.py index d9a85f1ac..a33529c44 100644 --- a/openstack/cloud/_compute.py +++ b/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, - **filters)) + return list(self.compute.keypairs(**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): diff --git a/openstack/cloud/_security_group.py b/openstack/cloud/_security_group.py index 46427496d..b63959701 100644 --- a/openstack/cloud/_security_group.py +++ b/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, - **filters)) + self.network.security_groups(**filters)) # Handle nova security groups else: diff --git a/openstack/compute/v2/flavor.py b/openstack/compute/v2/flavor.py index f2ad48386..55555873c 100644 --- a/openstack/compute/v2/flavor.py +++ b/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', - allow_unknown_params=False, **params): + def list( + 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): diff --git a/openstack/proxy.py b/openstack/proxy.py index cd2fc5e73..7790eaac3 100644 --- a/openstack/proxy.py +++ b/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 diff --git a/openstack/resource.py b/openstack/resource.py index 1d7724154..d802af09e 100644 --- a/openstack/resource.py +++ b/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: diff --git a/openstack/tests/unit/test_proxy.py b/openstack/tests/unit/test_proxy.py index f2f55a54a..c4b1c437b 100644 --- a/openstack/tests/unit/test_proxy.py +++ b/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): diff --git a/openstack/tests/unit/test_resource.py b/openstack/tests/unit/test_resource.py index 2edb9e380..49ea8439b 100644 --- a/openstack/tests/unit/test_resource.py +++ b/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!"