From f5615dc815f80c3830a7ddfe341856533223dff5 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 24 Jan 2022 18:33:22 +0100 Subject: Fix resource_url in the remaining resources Node history was particularly affected: limit was not converted from string to integer, so "next" link was never added. Add some safeguards to the generic API code. Change-Id: I1328e2f07621bf7e39b96eb4a7ddb66c9a2b65bb (cherry picked from commit 55144d3bd262be35c7a034fea083c3ed73fd63d8) --- ironic/api/controllers/v1/allocation.py | 4 ++-- ironic/api/controllers/v1/chassis.py | 8 ++++---- ironic/api/controllers/v1/collection.py | 8 ++++++-- ironic/api/controllers/v1/conductor.py | 2 +- ironic/api/controllers/v1/deploy_template.py | 1 + ironic/api/controllers/v1/node.py | 24 ++++++++-------------- ironic/api/controllers/v1/port.py | 9 ++++---- ironic/api/controllers/v1/portgroup.py | 9 +++----- ironic/api/controllers/v1/volume_connector.py | 2 +- ironic/api/controllers/v1/volume_target.py | 2 +- .../unit/api/controllers/v1/test_allocation.py | 14 +++++++++---- .../unit/api/controllers/v1/test_conductor.py | 12 +++++++---- .../api/controllers/v1/test_deploy_template.py | 13 ++++++++---- ironic/tests/unit/api/controllers/v1/test_node.py | 10 +++++++++ ironic/tests/unit/api/controllers/v1/test_port.py | 5 +++-- releasenotes/notes/api-none-3fdca1ccbb64d9b0.yaml | 12 +++++++++++ 16 files changed, 83 insertions(+), 52 deletions(-) create mode 100644 releasenotes/notes/api-none-3fdca1ccbb64d9b0.yaml diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index 73aca3a51..3e208b0c8 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -104,7 +104,7 @@ def allocation_sanitize(allocation, fields): api_utils.sanitize_dict(allocation, fields) -def list_convert_with_links(rpc_allocations, limit, url=None, fields=None, +def list_convert_with_links(rpc_allocations, limit, url, fields=None, **kwargs): return collection.list_convert_with_links( items=[convert_with_links(p, fields=fields, @@ -136,7 +136,7 @@ class AllocationsController(pecan.rest.RestController): def _get_allocations_collection(self, node_ident=None, resource_class=None, state=None, owner=None, marker=None, limit=None, sort_key='id', sort_dir='asc', - resource_url=None, fields=None, + resource_url='allocations', fields=None, parent_node=None): """Return allocations collection. diff --git a/ironic/api/controllers/v1/chassis.py b/ironic/api/controllers/v1/chassis.py index 9c280fa58..e04a37557 100644 --- a/ironic/api/controllers/v1/chassis.py +++ b/ironic/api/controllers/v1/chassis.py @@ -78,7 +78,7 @@ def convert_with_links(rpc_chassis, fields=None, sanitize=True): return chassis -def list_convert_with_links(rpc_chassis_list, limit, url=None, fields=None, +def list_convert_with_links(rpc_chassis_list, limit, url, fields=None, **kwargs): return collection.list_convert_with_links( items=[convert_with_links(ch, fields=fields, @@ -164,7 +164,8 @@ class ChassisController(rest.RestController): DEFAULT_RETURN_FIELDS) return self._get_chassis_collection(marker, limit, sort_key, sort_dir, - fields=fields, detail=detail) + fields=fields, detail=detail, + resource_url='chassis') @METRICS.timer('ChassisController.detail') @method.expose() @@ -188,9 +189,8 @@ class ChassisController(rest.RestController): if parent != "chassis": raise exception.HTTPNotFound() - resource_url = '/'.join(['chassis', 'detail']) return self._get_chassis_collection(marker, limit, sort_key, sort_dir, - resource_url) + resource_url='chassis/detail') @METRICS.timer('ChassisController.get_one') @method.expose() diff --git a/ironic/api/controllers/v1/collection.py b/ironic/api/controllers/v1/collection.py index cdcf13712..342efb873 100644 --- a/ironic/api/controllers/v1/collection.py +++ b/ironic/api/controllers/v1/collection.py @@ -22,7 +22,7 @@ def has_next(collection, limit): return len(collection) and len(collection) == limit -def list_convert_with_links(items, item_name, limit, url=None, fields=None, +def list_convert_with_links(items, item_name, limit, url, fields=None, sanitize_func=None, key_field='uuid', sanitizer_args=None, **kwargs): """Build a collection dict including the next link for paging support. @@ -49,6 +49,10 @@ def list_convert_with_links(items, item_name, limit, url=None, fields=None, :returns: A dict containing ``item_name`` and ``next`` values """ + assert url, "BUG: collections require a base URL" + assert limit is None or isinstance(limit, int), \ + f"BUG: limit must be None or int, got {type(limit)}" + items_dict = { item_name: items } @@ -68,7 +72,7 @@ def list_convert_with_links(items, item_name, limit, url=None, fields=None, return items_dict -def get_next(collection, limit, url=None, key_field='uuid', **kwargs): +def get_next(collection, limit, url, key_field='uuid', **kwargs): """Return a link to the next subset of the collection.""" if not has_next(collection, limit): return None diff --git a/ironic/api/controllers/v1/conductor.py b/ironic/api/controllers/v1/conductor.py index 61cbba78a..d421e835e 100644 --- a/ironic/api/controllers/v1/conductor.py +++ b/ironic/api/controllers/v1/conductor.py @@ -71,7 +71,7 @@ class ConductorsController(rest.RestController): invalid_sort_key_list = ['alive', 'drivers'] def _get_conductors_collection(self, marker, limit, sort_key, sort_dir, - resource_url=None, fields=None, + resource_url='conductors', fields=None, detail=None): limit = api_utils.validate_limit(limit) diff --git a/ironic/api/controllers/v1/deploy_template.py b/ironic/api/controllers/v1/deploy_template.py index e54b0d63b..dcf7122e0 100644 --- a/ironic/api/controllers/v1/deploy_template.py +++ b/ironic/api/controllers/v1/deploy_template.py @@ -140,6 +140,7 @@ def list_convert_with_links(rpc_templates, limit, fields=None, **kwargs): items=[convert_with_links(t, fields=fields, sanitize=False) for t in rpc_templates], item_name='deploy_templates', + url='deploy_templates', limit=limit, fields=fields, sanitize_func=template_sanitize, diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 4831361cf..bfba203d1 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1650,8 +1650,7 @@ def _node_sanitize_extended(node, node_keys, target_dict, cdict): 'driver_internal_info permission. **'} -def node_list_convert_with_links(nodes, limit, url=None, fields=None, - **kwargs): +def node_list_convert_with_links(nodes, limit, url, fields=None, **kwargs): cdict = api.request.context.to_policy_values() target_dict = dict(cdict) sanitizer_args = { @@ -1890,25 +1889,19 @@ class NodeHistoryController(rest.RestController): @METRICS.timer('NodeHistoryController.get_all') @method.expose() - @args.validate(details=args.boolean, marker=args.uuid, limit=args.integer) - def get_all(self, **kwargs): + @args.validate(detail=args.boolean, marker=args.uuid, limit=args.integer) + def get_all(self, detail=False, marker=None, limit=None): """List node history.""" node = api_utils.check_node_policy_and_retrieve( 'baremetal:node:history:get', self.node_ident) - if kwargs.get('detail'): - detail = True - fields = self.detail_fields - else: - detail = False - fields = self.standard_fields + fields = self.detail_fields if detail else self.standard_fields marker_obj = None - marker = kwargs.get('marker') if marker: marker_obj = objects.NodeHistory.get_by_uuid(api.request.context, marker) - limit = kwargs.get('limit') + limit = api_utils.validate_limit(limit) events = objects.NodeHistory.list_by_node_id(api.request.context, node.id, @@ -1921,6 +1914,7 @@ class NodeHistoryController(rest.RestController): node.uuid, event, detail=detail) for event in events ], item_name='history', + url=f'nodes/{self.node_ident}/history', fields=fields, marker=marker_obj, limit=limit, @@ -2288,7 +2282,6 @@ class NodesController(rest.RestController): fields = api_utils.get_request_return_fields(fields, detail, _DEFAULT_RETURN_FIELDS) - resource_url = 'nodes' extra_args = {'description_contains': description_contains} return self._get_nodes_collection(chassis_uuid, instance_uuid, associated, maintenance, retired, @@ -2296,7 +2289,7 @@ class NodesController(rest.RestController): limit, sort_key, sort_dir, driver=driver, resource_class=resource_class, - resource_url=resource_url, + resource_url='nodes', fields=fields, fault=fault, conductor_group=conductor_group, detail=detail, @@ -2379,7 +2372,6 @@ class NodesController(rest.RestController): api_utils.check_allow_filter_by_conductor(conductor) - resource_url = '/'.join(['nodes', 'detail']) extra_args = {'description_contains': description_contains} return self._get_nodes_collection(chassis_uuid, instance_uuid, associated, maintenance, retired, @@ -2387,7 +2379,7 @@ class NodesController(rest.RestController): limit, sort_key, sort_dir, driver=driver, resource_class=resource_class, - resource_url=resource_url, + resource_url='nodes/detail', fault=fault, conductor_group=conductor_group, conductor=conductor, diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index 0658fbf3f..adc21ebc2 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -160,7 +160,7 @@ def port_sanitize(port, fields=None): api_utils.sanitize_dict(port, fields) -def list_convert_with_links(rpc_ports, limit, url=None, fields=None, **kwargs): +def list_convert_with_links(rpc_ports, limit, url, fields=None, **kwargs): ports = [] for rpc_port in rpc_ports: try: @@ -407,10 +407,9 @@ class PortsController(rest.RestController): and not uuidutils.is_uuid_like(node)): raise exception.NotAcceptable() - resource_url = 'ports' return self._get_ports_collection(node_uuid or node, address, portgroup, marker, limit, sort_key, - sort_dir, resource_url=resource_url, + sort_dir, resource_url='ports', fields=fields, detail=detail, project=project) @@ -466,10 +465,10 @@ class PortsController(rest.RestController): if parent != "ports": raise exception.HTTPNotFound() - resource_url = '/'.join(['ports', 'detail']) return self._get_ports_collection(node_uuid or node, address, portgroup, marker, limit, sort_key, - sort_dir, resource_url, + sort_dir, + resource_url='ports/detail', project=project) @METRICS.timer('PortsController.get_one') diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 7900c4683..6c68e07ba 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -113,8 +113,7 @@ def convert_with_links(rpc_portgroup, fields=None, sanitize=True): return portgroup -def list_convert_with_links(rpc_portgroups, limit, url=None, fields=None, - **kwargs): +def list_convert_with_links(rpc_portgroups, limit, url, fields=None, **kwargs): return collection.list_convert_with_links( items=[convert_with_links(p, fields=fields, sanitize=False) for p in rpc_portgroups], @@ -283,12 +282,11 @@ class PortgroupsController(pecan.rest.RestController): fields = api_utils.get_request_return_fields(fields, detail, _DEFAULT_RETURN_FIELDS) - resource_url = 'portgroups' return self._get_portgroups_collection(node, address, marker, limit, sort_key, sort_dir, fields=fields, - resource_url=resource_url, + resource_url='portgroups', detail=detail, project=project) @@ -332,10 +330,9 @@ class PortgroupsController(pecan.rest.RestController): if parent != "portgroups": raise exception.HTTPNotFound() - resource_url = '/'.join(['portgroups', 'detail']) return self._get_portgroups_collection( node, address, marker, limit, sort_key, sort_dir, - resource_url=resource_url, project=project) + resource_url='portgroups/detail', project=project) @METRICS.timer('PortgroupsController.get_one') @method.expose() diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py index 100822029..dd003e867 100644 --- a/ironic/api/controllers/v1/volume_connector.py +++ b/ironic/api/controllers/v1/volume_connector.py @@ -83,7 +83,7 @@ def convert_with_links(rpc_connector, fields=None, sanitize=True): return connector -def list_convert_with_links(rpc_connectors, limit, url=None, fields=None, +def list_convert_with_links(rpc_connectors, limit, url, fields=None, detail=None, **kwargs): if detail: kwargs['detail'] = detail diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index d98f461ed..2fe1afb03 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -95,7 +95,7 @@ def convert_with_links(rpc_target, fields=None, sanitize=True): return target -def list_convert_with_links(rpc_targets, limit, url=None, fields=None, +def list_convert_with_links(rpc_targets, limit, url, fields=None, detail=None, **kwargs): if detail: kwargs['detail'] = detail diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index 39a12784e..60fcca0dc 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -192,7 +192,9 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual(3, len(data['allocations'])) next_marker = data['allocations'][-1]['uuid'] - self.assertIn(next_marker, data['next']) + self.assertIn('/allocations', data['next']) + self.assertIn('limit=3', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_default_limit(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -207,7 +209,10 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual(3, len(data['allocations'])) next_marker = data['allocations'][-1]['uuid'] - self.assertIn(next_marker, data['next']) + self.assertIn('/allocations', data['next']) + # FIXME(dtantsur): IMO this should not pass, but it does now + self.assertIn('limit=3', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_custom_fields(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -227,8 +232,9 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual(3, len(data['allocations'])) next_marker = data['allocations'][-1]['uuid'] - self.assertIn(next_marker, data['next']) - self.assertIn('fields', data['next']) + self.assertIn('/allocations', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) + self.assertIn(f'fields={fields}', data['next']) def test_get_collection_pagination_no_uuid(self): fields = 'node_uuid' diff --git a/ironic/tests/unit/api/controllers/v1/test_conductor.py b/ironic/tests/unit/api/controllers/v1/test_conductor.py index caf85eb4c..d5e54ee1b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_conductor.py +++ b/ironic/tests/unit/api/controllers/v1/test_conductor.py @@ -188,7 +188,9 @@ class TestListConductors(test_api_base.BaseApiTest): self.assertEqual(3, len(data['conductors'])) next_marker = data['conductors'][-1]['hostname'] - self.assertIn(next_marker, data['next']) + self.assertIn('/conductors', data['next']) + self.assertIn('limit=3', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_default_limit(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -204,7 +206,8 @@ class TestListConductors(test_api_base.BaseApiTest): self.assertEqual(3, len(data['conductors'])) next_marker = data['conductors'][-1]['hostname'] - self.assertIn(next_marker, data['next']) + self.assertIn('/conductors', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_custom_fields(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -221,8 +224,9 @@ class TestListConductors(test_api_base.BaseApiTest): self.assertEqual(3, len(data['conductors'])) next_marker = data['conductors'][-1]['hostname'] - self.assertIn(next_marker, data['next']) - self.assertIn('fields', data['next']) + self.assertIn('/conductors', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) + self.assertIn(f'fields={fields}', data['next']) def test_sort_key(self): conductors = [] diff --git a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py index ed7239d5c..b86fb0b1d 100644 --- a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py +++ b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py @@ -210,7 +210,9 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest): self.assertEqual(3, len(data['deploy_templates'])) next_marker = data['deploy_templates'][-1]['uuid'] - self.assertIn(next_marker, data['next']) + self.assertIn('/deploy_templates', data['next']) + self.assertIn('limit=3', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_default_limit(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -224,7 +226,8 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest): self.assertEqual(3, len(data['deploy_templates'])) next_marker = data['deploy_templates'][-1]['uuid'] - self.assertIn(next_marker, data['next']) + self.assertIn('/deploy_templates', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) def test_collection_links_custom_fields(self): cfg.CONF.set_override('max_limit', 3, 'api') @@ -240,8 +243,9 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest): headers=self.headers) self.assertEqual(3, len(data['deploy_templates'])) next_marker = data['deploy_templates'][-1]['uuid'] - self.assertIn(next_marker, data['next']) - self.assertIn('fields', data['next']) + self.assertIn('/deploy_templates', data['next']) + self.assertIn(f'marker={next_marker}', data['next']) + self.assertIn(f'fields={fields}', data['next']) def test_get_collection_pagination_no_uuid(self): fields = 'name' @@ -259,6 +263,7 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest): headers=self.headers) self.assertEqual(limit, len(data['deploy_templates'])) + self.assertIn('/deploy_templates', data['next']) self.assertIn('marker=%s' % templates[limit - 1].uuid, data['next']) def test_sort_key(self): diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index ee957178c..3c913834e 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -7841,6 +7841,10 @@ class TestNodeHistory(test_api_base.BaseApiTest): self.assertEqual(1, len(entries)) result_uuid = entries[0]['uuid'] self.assertEqual(self.event1.uuid, result_uuid) + self.assertIn('next', ret) + self.assertIn('nodes/%s/history' % self.node.uuid, ret['next']) + self.assertIn('limit=1', ret['next']) + self.assertIn('marker=%s' % result_uuid, ret['next']) # Second request ret = self.get_json('/nodes/%s/history?limit=1&marker=%s' % (self.node.uuid, result_uuid), @@ -7850,6 +7854,9 @@ class TestNodeHistory(test_api_base.BaseApiTest): self.assertEqual(1, len(entries)) result_uuid = entries[0]['uuid'] self.assertEqual(self.event2.uuid, result_uuid) + self.assertIn('nodes/%s/history' % self.node.uuid, ret['next']) + self.assertIn('limit=1', ret['next']) + self.assertIn('marker=%s' % result_uuid, ret['next']) # Third request ret = self.get_json('/nodes/%s/history?limit=1&marker=%s' % (self.node.uuid, result_uuid), @@ -7859,3 +7866,6 @@ class TestNodeHistory(test_api_base.BaseApiTest): self.assertEqual(1, len(entries)) result_uuid = entries[0]['uuid'] self.assertEqual(self.event3.uuid, result_uuid) + self.assertIn('nodes/%s/history' % self.node.uuid, ret['next']) + self.assertIn('limit=1', ret['next']) + self.assertIn('marker=%s' % result_uuid, ret['next']) diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index 1f3322acc..36c024158 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -194,7 +194,8 @@ class TestPortsController__GetPortsCollection(base.TestCase): mock_request.context = 'fake-context' mock_list.return_value = [] self.controller._get_ports_collection(None, None, None, None, None, - None, 'asc') + None, 'asc', + resource_url='ports') mock_list.assert_called_once_with('fake-context', 1000, None, project=None, sort_dir='asc', sort_key=None) @@ -1093,7 +1094,7 @@ class TestListPorts(test_api_base.BaseApiTest): autospec=True) def test_detail_with_incorrect_api_usage(self, mock_gpc): mock_gpc.return_value = api_port.list_convert_with_links( - [], 0) + [], 0, 'port') # GET /v1/ports/detail specifying node and node_uuid. In this case # we expect the node_uuid interface to be used. self.get_json('/ports/detail?node=%s&node_uuid=%s' % diff --git a/releasenotes/notes/api-none-3fdca1ccbb64d9b0.yaml b/releasenotes/notes/api-none-3fdca1ccbb64d9b0.yaml new file mode 100644 index 000000000..d816be60d --- /dev/null +++ b/releasenotes/notes/api-none-3fdca1ccbb64d9b0.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Fixes pagination for the following collections:: + + /v1/allocations + /v1/chassis + /v1/conductors + /v1/deploy_templates + /v1/nodes/{node}/history + + The ``next`` link now contains a valid URL. -- cgit v1.2.1