From 934658dab4fea90981e7e1d1680e2c34a85aef6a Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Thu, 29 Feb 2024 17:04:46 +0100
Subject: [PATCH] Support more standard way of passing lists via query strings

Currently, arguments like "fields", "shared" or the new "device_types"
only accept comma-separated strings. While there is no single standard,
the most common approach is to repeat the arguments, i.e.

 NOT /nodes?fields=uuid,name
 BUT /nodes?fields=uuid&fields=name

Unfortunately, at least GopherCloud already relies [1] on the more common
(but not currently working in Ironic) behavior. Let's make it work.

[1] https://github.com/gophercloud/gophercloud/blob/8455d013430a26431d80976ec03829a4c0a5e5c1/openstack/baremetal/v1/nodes/testing/requests_test.go#L87

Change-Id: Ia780b10986929d79dc4f334d278bcb00a9984fd0
---
 ironic/common/args.py                         |  5 ++-
 .../unit/api/controllers/v1/test_node.py      | 44 +++++++++++++++++++
 .../notes/string-list-6098010bfdce9149.yaml   |  7 +++
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/string-list-6098010bfdce9149.yaml

diff --git a/ironic/common/args.py b/ironic/common/args.py
index 5ad865d33f..13d59398d6 100755
--- a/ironic/common/args.py
+++ b/ironic/common/args.py
@@ -128,9 +128,12 @@ def string_list(name, value):
               same order, or None if value is None
     :raises: InvalidParameterValue if the value is not a string
     """
-    value = string(name, value)
     if value is None:
         return
+    if isinstance(value, list):
+        return [string(name, item) for item in value]
+
+    value = string(name, value)
     items = []
     for v in str(value).split(','):
         v_norm = v.strip().lower()
diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py
index 4456284148..e12f99eb72 100644
--- a/ironic/tests/unit/api/controllers/v1/test_node.py
+++ b/ironic/tests/unit/api/controllers/v1/test_node.py
@@ -523,6 +523,15 @@ class TestListNodes(test_api_base.BaseApiTest):
         # We always append "links"
         self.assertCountEqual(['extra', 'instance_info', 'links'], data)
 
+    def test_get_one_custom_fields_as_list(self):
+        node = obj_utils.create_test_node(self.context,
+                                          chassis_id=self.chassis.id)
+        data = self.get_json(
+            '/nodes/%s?fields=extra&fields=instance_info' % node.uuid,
+            headers={api_base.Version.string: str(api_v1.max_version())})
+        # We always append "links"
+        self.assertCountEqual(['extra', 'instance_info', 'links'], data)
+
     def test_get_collection_custom_fields(self):
         fields = 'uuid,instance_info'
         for i in range(3):
@@ -539,6 +548,21 @@ class TestListNodes(test_api_base.BaseApiTest):
             # We always append "links"
             self.assertCountEqual(['uuid', 'instance_info', 'links'], node)
 
+    def test_get_collection_custom_fields_as_list(self):
+        for i in range(3):
+            obj_utils.create_test_node(self.context,
+                                       uuid=uuidutils.generate_uuid(),
+                                       instance_uuid=uuidutils.generate_uuid())
+
+        data = self.get_json(
+            '/nodes?fields=uuid&fields=instance_info',
+            headers={api_base.Version.string: str(api_v1.max_version())})
+
+        self.assertEqual(3, len(data['nodes']))
+        for node in data['nodes']:
+            # We always append "links"
+            self.assertCountEqual(['uuid', 'instance_info', 'links'], node)
+
     def test_get_collection_fields_for_nova(self):
         # Unit test which explicitly attempts to request traits along with
         # all columns which nova would normally request as part of it's sync
@@ -8301,6 +8325,13 @@ class TestNodeShardGets(test_api_base.BaseApiTest):
             '/nodes?shard=foo,bar', headers=self.headers)
         self.assertEqual(2, len(result['nodes']))
 
+    def test_filtering_by_multi_shard_as_list(self):
+        obj_utils.create_test_node(
+            self.context, uuid=uuid.uuid4(), shard='bar')
+        result = self.get_json(
+            '/nodes?shard=foo&shard=bar', headers=self.headers)
+        self.assertEqual(2, len(result['nodes']))
+
     def test_filtering_by_shard_detail_fails_wrong_version(self):
         headers = {api_base.Version.string: '1.80'}
 
@@ -8763,6 +8794,19 @@ class TestNodeVmedia(test_api_base.BaseApiTest):
             mock.ANY, mock.ANY, self.node.uuid,
             device_types=[boot_devices.CDROM], topic='test-topic')
 
+    @mock.patch.object(rpcapi.ConductorAPI, 'detach_virtual_media',
+                       autospec=True)
+    def test_detach_several_via_argument(self, mock_detach):
+        ret = self.delete('/nodes/%s/vmedia?device_types=%s&device_types=%s'
+                          % (self.node.uuid,
+                             boot_devices.CDROM, boot_devices.DISK),
+                          headers={api_base.Version.string: self.version})
+        self.assertEqual(http_client.NO_CONTENT, ret.status_int)
+        mock_detach.assert_called_once_with(
+            mock.ANY, mock.ANY, self.node.uuid,
+            device_types=[boot_devices.CDROM, boot_devices.DISK],
+            topic='test-topic')
+
     def test_detach_wrong_device_types(self):
         ret = self.delete('/nodes/%s/vmedia?device_types=cdrom,cat'
                           % self.node.uuid,
diff --git a/releasenotes/notes/string-list-6098010bfdce9149.yaml b/releasenotes/notes/string-list-6098010bfdce9149.yaml
new file mode 100644
index 0000000000..65c7665275
--- /dev/null
+++ b/releasenotes/notes/string-list-6098010bfdce9149.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Query parameters in the API that expect lists now accept repeated arguments
+    (``param=value1&param=value2``) in addition to comma-separated strings
+    (``param=value1,value2``). The former seems to be more common and is
+    actually (incorrectly) used in GopherCloud.