From e36f72d36da53ff5439d0e5a19561bed9e792b06 Mon Sep 17 00:00:00 2001
From: Arun S A G <sagarun@gmail.com>
Date: Mon, 14 Oct 2019 16:00:34 -0700
Subject: [PATCH] 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       |  3 ++-
 ironic/api/controllers/v1/chassis.py          |  3 ++-
 ironic/api/controllers/v1/collection.py       |  5 +++++
 ironic/api/controllers/v1/conductor.py        |  3 ++-
 ironic/api/controllers/v1/deploy_template.py  |  2 +-
 ironic/api/controllers/v1/node.py             |  3 ++-
 ironic/api/controllers/v1/port.py             |  3 ++-
 ironic/api/controllers/v1/portgroup.py        |  3 ++-
 ironic/api/controllers/v1/volume_connector.py |  3 ++-
 ironic/api/controllers/v1/volume_target.py    |  3 ++-
 .../api/controllers/v1/test_allocation.py     | 21 ++++++++++++++++++
 .../unit/api/controllers/v1/test_chassis.py   | 16 ++++++++++++++
 .../unit/api/controllers/v1/test_conductor.py | 18 +++++++++++++++
 .../controllers/v1/test_deploy_template.py    | 17 ++++++++++++++
 .../unit/api/controllers/v1/test_node.py      | 19 ++++++++++++++++
 .../unit/api/controllers/v1/test_port.py      | 19 ++++++++++++++++
 .../unit/api/controllers/v1/test_portgroup.py | 20 +++++++++++++++++
 .../controllers/v1/test_volume_connector.py   | 22 +++++++++++++++++++
 .../api/controllers/v1/test_volume_target.py  | 18 +++++++++++++++
 ...issing-from-next-url-fd9fddf8e70b65ea.yaml |  8 +++++++
 20 files changed, 200 insertions(+), 9 deletions(-)
 create mode 100644 releasenotes/notes/fix-fields-missing-from-next-url-fd9fddf8e70b65ea.yaml

diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py
index 22c20947de..6e7916d61e 100644
--- a/ironic/api/controllers/v1/allocation.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/chassis.py b/ironic/api/controllers/v1/chassis.py
index 09b0d0f8be..b446d815e7 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 2ff98aedb1..335e874c1d 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 23931cc117..6ce7fa912a 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 a43e222e39..bc713d97c9 100644
--- a/ironic/api/controllers/v1/deploy_template.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index 82665508ce..71fa734951 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py
index 6d39e58dd9..e824be8b14 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 e2d6742446..27a21e089b 100644
--- a/ironic/api/controllers/v1/portgroup.py
+++ b/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)
diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py
index 405987e790..8dcb36619c 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 28349670ff..63afd35af7 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 b51ad73b9a..3399fb5a48 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 4718f30763..a46c3e48a5 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 a09a18d592..1a5905320f 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 eae07c5d36..049a21f7bc 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 f87b98d903..f5c5440a8b 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 3cc53355e6..630f66b9a9 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 df025cdbdd..de2c13e6f0 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 e1ed61bbed..3f58fae418 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 abffd3799b..cdd014af2d 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 0000000000..dc10ea8c2b
--- /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.