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

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.
 

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()

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

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)

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,

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,
+def node_list_convert_with_links(nodes, limit, url, fields=None, **kwargs):
-                                 **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)
+    @args.validate(detail=args.boolean, marker=args.uuid, limit=args.integer)
-    def get_all(self, **kwargs):
+    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'):
+        fields = self.detail_fields if detail else self.standard_fields
-            detail = True
-            fields = self.detail_fields
-        else:
-            detail = False
-            fields = 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,

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')

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,
+def list_convert_with_links(rpc_portgroups, limit, url, fields=None, **kwargs):
-                            **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()

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

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

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('/allocations', data['next'])
-        self.assertIn('fields', 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'

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('/conductors', data['next'])
-        self.assertIn('fields', data['next'])
+        self.assertIn(f'marker={next_marker}', data['next'])
+        self.assertIn(f'fields={fields}', data['next'])
 
     def test_sort_key(self):
         conductors = []

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('/deploy_templates', data['next'])
-        self.assertIn('fields', 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):

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'])

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)
@@ -1104,7 +1105,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' %

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.