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
(cherry picked from commit e36f72d36da53ff5439d0e5a19561bed9e792b06)

20 files changed, 200 insertions, 9 deletions

diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py
index d461f6d4f..0ec07ba96 100644
--- a/ironic/api/controllers/v1/allocation.py
+++ b/ironic/api/controllers/v1/allocation.py
@@ -181,7 +181,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)
diff --git a/ironic/api/controllers/v1/chassis.py b/ironic/api/controllers/v1/chassis.py
index 752135bd6..d8fc6f73a 100644
--- a/ironic/api/controllers/v1/chassis.py
+++ b/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
diff --git a/ironic/api/controllers/v1/collection.py b/ironic/api/controllers/v1/collection.py
index 032819441..c53fed830 100644
--- a/ironic/api/controllers/v1/collection.py
+++ b/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,
diff --git a/ironic/api/controllers/v1/conductor.py b/ironic/api/controllers/v1/conductor.py
index b405e3e10..f71bbc60b 100644
--- a/ironic/api/controllers/v1/conductor.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/deploy_template.py b/ironic/api/controllers/v1/deploy_template.py
index 29c1e279b..90a12ea6c 100644
--- a/ironic/api/controllers/v1/deploy_template.py
+++ b/ironic/api/controllers/v1/deploy_template.py
@@ -237,7 +237,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)
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index 68efff831..0b600ea4b 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -1346,7 +1346,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)
diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py
index 2f7507830..5ed25b311 100644
--- a/ironic/api/controllers/v1/port.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py
index 1e2544f83..890ddf9e9 100644
--- a/ironic/api/controllers/v1/portgroup.py
+++ b/ironic/api/controllers/v1/portgroup.py
@@ -232,7 +232,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)
diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py
index d6e77f1a8..f378acf62 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 144d06146..e1c50d555 100644
--- a/ironic/api/controllers/v1/volume_target.py
+++ b/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
diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py
index 05d78faa2..eb2c9e45c 100644
--- a/ironic/tests/unit/api/controllers/v1/test_allocation.py
+++ b/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
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_conductor.py b/ironic/tests/unit/api/controllers/v1/test_conductor.py
index a09a18d59..1a5905320 100644
--- a/ironic/tests/unit/api/controllers/v1/test_conductor.py
+++ b/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):
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 eae07c5d3..049a21f7b 100644
--- a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py
+++ b/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
diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py
index f03b943e8..18ef76c42 100644
--- a/ironic/tests/unit/api/controllers/v1/test_node.py
+++ b/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
diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py
index ac385171f..73f3db109 100644
--- a/ironic/tests/unit/api/controllers/v1/test_port.py
+++ b/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):
diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py
index df025cdbd..de2c13e6f 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.