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.

Change-Id: I62b59e8148171c72de0ccf63a1517e754b520c76
Story: 2006721
Task: 37093

ironic/api/controllers/v1/allocation.py

@@ -186,7 +186,8 @@ class AllocationCollection(collection.Collection):
             Allocation.convert_with_links(p, fields=fields, sanitize=False)
             for p in rpc_allocations
         ]
-        collection.next = collection.get_next(limit, url=url, **kwargs)
+        collection.next = collection.get_next(limit, url=url, fields=fields,
+                                              **kwargs)
 
         for item in collection.allocations:
             item.sanitize(fields=fields)

ironic/api/controllers/v1/chassis.py

@@ -157,7 +157,8 @@ 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)
         return collection

ironic/api/controllers/v1/collection.py

@@ -43,6 +43,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,

ironic/api/controllers/v1/conductor.py

@@ -140,7 +140,8 @@ class ConductorCollection(collection.Collection):
         collection = ConductorCollection()
         collection.conductors = [Conductor.convert_with_links(c, fields=fields)
                                  for c in conductors]
-        collection.next = collection.get_next(limit, url=url, **kwargs)
+        collection.next = collection.get_next(limit, url=url, fields=fields,
+                                              **kwargs)
 
         for conductor in collection.conductors:
             conductor.sanitize(fields)

ironic/api/controllers/v1/deploy_template.py

@@ -238,7 +238,7 @@ class DeployTemplateCollection(collection.Collection):
         collection.deploy_templates = [
             DeployTemplate.convert_with_links(t, fields=fields, sanitize=False)
             for t in templates]
-        collection.next = collection.get_next(limit, **kwargs)
+        collection.next = collection.get_next(limit, fields=fields, **kwargs)
 
         for template in collection.deploy_templates:
             template.sanitize(fields)

ironic/api/controllers/v1/node.py

@@ -1347,7 +1347,8 @@ 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)
+        collection.next = collection.get_next(limit, url=url, fields=fields,
+                                              **kwargs)
 
         for node in collection.nodes:
             node.sanitize(fields)

ironic/api/controllers/v1/port.py

@@ -306,7 +306,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)

ironic/api/controllers/v1/portgroup.py

@@ -233,7 +233,8 @@ 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)

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

ironic/api/controllers/v1/volume_target.py

@@ -217,7 +217,8 @@ 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)
         return collection

ironic/tests/unit/api/controllers/v1/test_allocation.py

@@ -217,6 +217,27 @@ class TestListAllocations(test_api_base.BaseApiTest):
         next_marker = data['allocations'][-1]['uuid']
         self.assertIn(next_marker, data['next'])
 
+    def test_collection_links_custom_fields(self):
+        cfg.CONF.set_override('max_limit', 3, 'api')
+        fields = 'uuid,extra'
+        allocations = []
+        for i in range(5):
+            allocation = obj_utils.create_test_allocation(
+                self.context,
+                node_id=self.node.id,
+                uuid=uuidutils.generate_uuid(),
+                name='allocation%s' % i)
+            allocations.append(allocation.uuid)
+
+        data = self.get_json(
+            '/allocations?fields=%s' % fields,
+            headers=self.headers)
+        self.assertEqual(3, len(data['allocations']))
+
+        next_marker = data['allocations'][-1]['uuid']
+        self.assertIn(next_marker, data['next'])
+        self.assertIn('fields', data['next'])
+
     def test_get_collection_pagination_no_uuid(self):
         fields = 'node_uuid'
         limit = 2

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

ironic/tests/unit/api/controllers/v1/test_conductor.py

@@ -206,6 +206,24 @@ class TestListConductors(test_api_base.BaseApiTest):
         next_marker = data['conductors'][-1]['hostname']
         self.assertIn(next_marker, data['next'])
 
+    def test_collection_links_custom_fields(self):
+        cfg.CONF.set_override('max_limit', 3, 'api')
+        conductors = []
+        fields = 'hostname,alive'
+        for id in range(5):
+            hostname = uuidutils.generate_uuid()
+            conductor = obj_utils.create_test_conductor(self.context,
+                                                        hostname=hostname)
+            conductors.append(conductor.hostname)
+        data = self.get_json(
+            '/conductors?fields=%s' % fields,
+            headers={api_base.Version.string: str(api_v1.max_version())})
+        self.assertEqual(3, len(data['conductors']))
+
+        next_marker = data['conductors'][-1]['hostname']
+        self.assertIn(next_marker, data['next'])
+        self.assertIn('fields', data['next'])
+
     def test_sort_key(self):
         conductors = []
         for id in range(5):

ironic/tests/unit/api/controllers/v1/test_deploy_template.py

@@ -250,6 +250,23 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest):
         next_marker = data['deploy_templates'][-1]['uuid']
         self.assertIn(next_marker, data['next'])
 
+    def test_collection_links_custom_fields(self):
+        cfg.CONF.set_override('max_limit', 3, 'api')
+        templates = []
+        fields = 'uuid,steps'
+        for i in range(5):
+            template = obj_utils.create_test_deploy_template(
+                self.context,
+                uuid=uuidutils.generate_uuid(),
+                name='CUSTOM_DT%s' % i)
+            templates.append(template.uuid)
+        data = self.get_json('/deploy_templates?fields=%s' % fields,
+                             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'])
+
     def test_get_collection_pagination_no_uuid(self):
         fields = 'name'
         limit = 2

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -907,6 +907,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

ironic/tests/unit/api/controllers/v1/test_port.py

@@ -641,6 +641,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):

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

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

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

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.