From 1d35f464364df4c26127ee953d182c4106fd3464 Mon Sep 17 00:00:00 2001 From: Arun S A G Date: Mon, 14 Oct 2019 16:00:34 -0700 Subject: Do not ignore 'fields' query parameter when building next url When an user calls the GET on an ironic resource it returns MAX_LIMIT number of resources at a time along with a next url. The default MAX_LIMIT is 1000. If the user requested specific set of fields from ironic API using the fields query parameter (eg: /v1/resource?fields=f1,f2,f3) The next url returned by the API ignores fields query parameter. This results in fields missing from the results after MAX_LIMIT is reached. This change fixes this problem by passing the fields as parameter to collections.get_next method and using the fields argument to build the query parameter. NOTE: Removed changes from deploy_templates, allocation, and conductor API endpoints which were added after Rocky. Change-Id: I62b59e8148171c72de0ccf63a1517e754b520c76 Story: 2006721 Task: 37093 (cherry picked from commit e36f72d36da53ff5439d0e5a19561bed9e792b06) (cherry picked from commit 0785477a6304bf9280dde503cdd18a467ddced13) --- ironic/api/controllers/v1/chassis.py | 5 +++-- ironic/api/controllers/v1/collection.py | 5 +++++ ironic/api/controllers/v1/node.py | 8 ++++---- ironic/api/controllers/v1/port.py | 4 ++-- ironic/api/controllers/v1/portgroup.py | 3 ++- ironic/api/controllers/v1/volume_connector.py | 3 ++- ironic/api/controllers/v1/volume_target.py | 5 +++-- .../tests/unit/api/controllers/v1/test_chassis.py | 16 ++++++++++++++++ ironic/tests/unit/api/controllers/v1/test_node.py | 19 +++++++++++++++++++ ironic/tests/unit/api/controllers/v1/test_port.py | 19 +++++++++++++++++++ .../unit/api/controllers/v1/test_portgroup.py | 20 ++++++++++++++++++++ .../api/controllers/v1/test_volume_connector.py | 22 ++++++++++++++++++++++ .../unit/api/controllers/v1/test_volume_target.py | 18 ++++++++++++++++++ ...lds-missing-from-next-url-fd9fddf8e70b65ea.yaml | 8 ++++++++ 14 files changed, 143 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/fix-fields-missing-from-next-url-fd9fddf8e70b65ea.yaml diff --git a/ironic/api/controllers/v1/chassis.py b/ironic/api/controllers/v1/chassis.py index 8f665a340..2d9022008 100644 --- a/ironic/api/controllers/v1/chassis.py +++ b/ironic/api/controllers/v1/chassis.py @@ -157,9 +157,10 @@ class ChassisCollection(collection.Collection): sanitize=False) for ch in chassis] url = url or None - collection.next = collection.get_next(limit, url=url, **kwargs) + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) for item in collection.chassis: - item.sanitize(fields) + item.sanitize(fields) return collection @classmethod diff --git a/ironic/api/controllers/v1/collection.py b/ironic/api/controllers/v1/collection.py index 9f82bd1bf..58243c9c1 100644 --- a/ironic/api/controllers/v1/collection.py +++ b/ironic/api/controllers/v1/collection.py @@ -39,6 +39,11 @@ class Collection(base.APIBase): return wtypes.Unset resource_url = url or self._type + fields = kwargs.pop('fields', None) + # NOTE(saga): If fields argument is present in kwargs and not None. It + # is a list so convert it into a comma seperated string. + if fields: + kwargs['fields'] = ','.join(fields) q_args = ''.join(['%s=%s&' % (key, kwargs[key]) for key in kwargs]) next_args = '?%(args)slimit=%(limit)d&marker=%(marker)s' % { 'args': q_args, 'limit': limit, diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 9f512028f..c3998dd2d 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1319,10 +1319,10 @@ class NodeCollection(collection.Collection): collection.nodes = [Node.convert_with_links(n, fields=fields, sanitize=False) for n in nodes] - collection.next = collection.get_next(limit, url=url, **kwargs) - - for node in collection.nodes: - node.sanitize(fields) + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) + for item in collection.nodes: + item.sanitize(fields) return collection diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index b72f0ddbf..019e16075 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -299,8 +299,8 @@ class PortCollection(collection.Collection): collection.ports.append(port) - collection.next = collection.get_next(limit, url=url, **kwargs) - + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) for item in collection.ports: item.sanitize(fields=fields) diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 0091c4c03..e45c92c59 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -232,8 +232,9 @@ class PortgroupCollection(collection.Collection): collection.portgroups = [Portgroup.convert_with_links(p, fields=fields, sanitize=False) for p in rpc_portgroups] - collection.next = collection.get_next(limit, url=url, **kwargs) + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) for item in collection.portgroups: item.sanitize(fields=fields) diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py index ede708f0a..398e56023 100644 --- a/ironic/api/controllers/v1/volume_connector.py +++ b/ironic/api/controllers/v1/volume_connector.py @@ -201,7 +201,8 @@ class VolumeConnectorCollection(collection.Collection): for p in rpc_connectors] if detail: kwargs['detail'] = detail - collection.next = collection.get_next(limit, url=url, **kwargs) + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) for connector in collection.connectors: connector.sanitize(fields) return collection diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index 7c97a41d6..cb7581a6d 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -217,9 +217,10 @@ class VolumeTargetCollection(collection.Collection): for p in rpc_targets] if detail: kwargs['detail'] = detail - collection.next = collection.get_next(limit, url=url, **kwargs) + collection.next = collection.get_next(limit, url=url, fields=fields, + **kwargs) for target in collection.targets: - target.sanitize(fields) + target.sanitize(fields=fields) return collection @classmethod diff --git a/ironic/tests/unit/api/controllers/v1/test_chassis.py b/ironic/tests/unit/api/controllers/v1/test_chassis.py index 4718f3076..a46c3e48a 100644 --- a/ironic/tests/unit/api/controllers/v1/test_chassis.py +++ b/ironic/tests/unit/api/controllers/v1/test_chassis.py @@ -230,6 +230,22 @@ class TestListChassis(test_api_base.BaseApiTest): next_marker = data['chassis'][-1]['uuid'] self.assertIn(next_marker, data['next']) + def test_collection_links_custom_fields(self): + fields = 'extra,uuid' + cfg.CONF.set_override('max_limit', 3, 'api') + for i in range(5): + obj_utils.create_test_chassis( + self.context, uuid=uuidutils.generate_uuid()) + + data = self.get_json( + '/chassis?fields=%s' % fields, + headers={api_base.Version.string: str(api_v1.max_version())}) + + self.assertEqual(3, len(data['chassis'])) + next_marker = data['chassis'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('fields', data['next']) + def test_get_collection_pagination_no_uuid(self): fields = 'extra' limit = 2 diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 9cce45dc3..e5bf31575 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -736,6 +736,25 @@ class TestListNodes(test_api_base.BaseApiTest): next_marker = data['nodes'][-1]['uuid'] self.assertIn(next_marker, data['next']) + def test_collection_links_custom_fields(self): + fields = 'driver_info,uuid' + cfg.CONF.set_override('max_limit', 3, 'api') + nodes = [] + for id in range(5): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + driver_info={'fake': 'value'}, + properties={'fake': 'bar'}) + nodes.append(node.uuid) + data = self.get_json( + '/nodes?fields=%s' % fields, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(3, len(data['nodes'])) + + next_marker = data['nodes'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('fields', data['next']) + def test_get_collection_pagination_no_uuid(self): fields = 'name' limit = 2 diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index 67467b932..ea3da1f67 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -609,6 +609,25 @@ class TestListPorts(test_api_base.BaseApiTest): next_marker = data['ports'][-1]['uuid'] self.assertIn(next_marker, data['next']) + def test_collection_links_custom_fields(self): + fields = 'address,uuid' + cfg.CONF.set_override('max_limit', 3, 'api') + for i in range(5): + obj_utils.create_test_port( + self.context, + uuid=uuidutils.generate_uuid(), + node_id=self.node.id, + address='52:54:00:cf:2d:3%s' % i) + + data = self.get_json( + '/ports?fields=%s' % fields, + headers={api_base.Version.string: str(api_v1.max_version())}) + + self.assertEqual(3, len(data['ports'])) + next_marker = data['ports'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('fields', data['next']) + def test_port_by_address(self): address_template = "aa:bb:cc:dd:ee:f%d" for id_ in range(3): diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py index 8591873b1..6e0c01b9b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_portgroup.py +++ b/ironic/tests/unit/api/controllers/v1/test_portgroup.py @@ -327,6 +327,26 @@ class TestListPortgroups(test_api_base.BaseApiTest): next_marker = data['portgroups'][-1]['uuid'] self.assertIn(next_marker, data['next']) + def test_collection_links_custom_fields(self): + fields = 'address,uuid' + cfg.CONF.set_override('max_limit', 3, 'api') + for i in range(5): + obj_utils.create_test_portgroup( + self.context, + uuid=uuidutils.generate_uuid(), + node_id=self.node.id, + name='portgroup%s' % i, + address='52:54:00:cf:2d:3%s' % i) + + data = self.get_json( + '/portgroups?fields=%s' % fields, + headers={api_base.Version.string: str(api_v1.max_version())}) + + self.assertEqual(3, len(data['portgroups'])) + next_marker = data['portgroups'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('fields', data['next']) + def test_get_collection_pagination_no_uuid(self): fields = 'address' limit = 2 diff --git a/ironic/tests/unit/api/controllers/v1/test_volume_connector.py b/ironic/tests/unit/api/controllers/v1/test_volume_connector.py index e1ed61bbe..3f58fae41 100644 --- a/ironic/tests/unit/api/controllers/v1/test_volume_connector.py +++ b/ironic/tests/unit/api/controllers/v1/test_volume_connector.py @@ -273,6 +273,28 @@ class TestListVolumeConnectors(test_api_base.BaseApiTest): next_marker = data['connectors'][-1]['uuid'] self.assertIn(next_marker, data['next']) + def test_collection_links_custom_fields(self): + cfg.CONF.set_override('max_limit', 3, 'api') + connectors = [] + fields = 'uuid,extra' + for i in range(5): + connector = obj_utils.create_test_volume_connector( + self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + connector_id='test-connector_id-%s' % i) + connectors.append(connector.uuid) + + data = self.get_json( + '/volume/connectors?fields=%s' % fields, + headers=self.headers) + + self.assertEqual(3, len(data['connectors'])) + self.assertIn('volume/connectors', data['next']) + + next_marker = data['connectors'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('fields', data['next']) + def test_get_collection_pagination_no_uuid(self): fields = 'connector_id' limit = 2 diff --git a/ironic/tests/unit/api/controllers/v1/test_volume_target.py b/ironic/tests/unit/api/controllers/v1/test_volume_target.py index abffd3799..cdd014af2 100644 --- a/ironic/tests/unit/api/controllers/v1/test_volume_target.py +++ b/ironic/tests/unit/api/controllers/v1/test_volume_target.py @@ -258,6 +258,24 @@ class TestListVolumeTargets(test_api_base.BaseApiTest): self.assertIn(next_marker, data['next']) self.assertIn('volume/targets', data['next']) + def test_collection_links_custom_fields(self): + fields = 'uuid,extra' + cfg.CONF.set_override('max_limit', 3, 'api') + targets = [] + for id_ in range(5): + target = obj_utils.create_test_volume_target( + self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), boot_index=id_) + targets.append(target.uuid) + data = self.get_json('/volume/targets?fields=%s' % fields, + headers=self.headers) + self.assertEqual(3, len(data['targets'])) + + next_marker = data['targets'][-1]['uuid'] + self.assertIn(next_marker, data['next']) + self.assertIn('volume/targets', data['next']) + self.assertIn('fields', data['next']) + def test_get_collection_pagination_no_uuid(self): fields = 'boot_index' limit = 2 diff --git a/releasenotes/notes/fix-fields-missing-from-next-url-fd9fddf8e70b65ea.yaml b/releasenotes/notes/fix-fields-missing-from-next-url-fd9fddf8e70b65ea.yaml new file mode 100644 index 000000000..dc10ea8c2 --- /dev/null +++ b/releasenotes/notes/fix-fields-missing-from-next-url-fd9fddf8e70b65ea.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes issue where the resource list API returned results with requested + fields only until the API MAX_LIMIT. After the API MAX_LIMIT is reached the + API started ignoring user requested fields. This fix will make sure that + the next url generated by the pagination code will include the user + requested fields as query parameter. -- cgit v1.2.1