Properly convert baremetal fields to server-side values

Currently we pass the 'fields' argument as it is. If a caller provides
a client-side name instead of a server-side one (e.g. id instead of uuid),
it results in a failure. This patch fixes it.

Also stops requiring redundant 'name' for QueryParameters that are
specified as a dictionary (like fields).

Related-Bug: #1842989
Change-Id: Ie39067b6e9217eed63f5ae46331ab8a408f0ace6

openstack/baremetal/v1/_common.py

@@ -10,6 +10,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from openstack import resource
+
 
 RETRIABLE_STATUS_CODES = [
     # HTTP Conflict - happens if a node is locked
@@ -91,3 +93,16 @@ def comma_separated_list(value):
         return None
     else:
         return ','.join(value)
+
+
+def fields_type(value, resource_type):
+    if value is None:
+        return None
+
+    resource_mapping = {
+        key: value.name
+        for key, value in resource_type.__dict__.items()
+        if isinstance(value, resource.Body)
+    }
+
+    return comma_separated_list(resource_mapping.get(x, x) for x in value)

openstack/baremetal/v1/_proxy.py

@@ -41,7 +41,7 @@ class Proxy(proxy.Proxy):
         res = self._get_resource(resource_type, value)
         kwargs = {}
         if fields:
-            kwargs['fields'] = _common.comma_separated_list(fields)
+            kwargs['fields'] = _common.fields_type(fields, resource_type)
         return res.fetch(
             self,
             error_message="No {resource_type} found for {value}".format(

openstack/baremetal/v1/allocation.py

@@ -33,7 +33,7 @@ class Allocation(_common.ListMixin, resource.Resource):
 
     _query_mapping = resource.QueryParameters(
         'node', 'resource_class', 'state',
-        fields={'name': 'fields', 'type': _common.comma_separated_list},
+        fields={'type': _common.fields_type},
     )
 
     # Allocation update is available since 1.57

openstack/baremetal/v1/chassis.py

@@ -33,7 +33,7 @@ class Chassis(_common.ListMixin, resource.Resource):
     commit_jsonpatch = True
 
     _query_mapping = resource.QueryParameters(
-        fields={'name': 'fields', 'type': _common.comma_separated_list},
+        fields={'type': _common.fields_type},
     )
 
     #: Timestamp at which the chassis was created.

openstack/baremetal/v1/node.py

@@ -48,7 +48,7 @@ class Node(_common.ListMixin, resource.Resource):
     _query_mapping = resource.QueryParameters(
         'associated', 'conductor_group', 'driver', 'fault',
         'provision_state', 'resource_class',
-        fields={'name': 'fields', 'type': _common.comma_separated_list},
+        fields={'type': _common.fields_type},
         instance_id='instance_uuid',
         is_maintenance='maintenance',
     )

openstack/baremetal/v1/port.py

@@ -31,7 +31,7 @@ class Port(_common.ListMixin, resource.Resource):
 
     _query_mapping = resource.QueryParameters(
         'address', 'node', 'portgroup',
-        fields={'name': 'fields', 'type': _common.comma_separated_list},
+        fields={'type': _common.fields_type},
         node_id='node_uuid',
     )
 

openstack/baremetal/v1/port_group.py

@@ -31,7 +31,7 @@ class PortGroup(_common.ListMixin, resource.Resource):
 
     _query_mapping = resource.QueryParameters(
         'node', 'address',
-        fields={'name': 'fields', 'type': _common.comma_separated_list},
+        fields={'type': _common.fields_type},
     )
 
     # The mode and properties field introduced in 1.26.

openstack/message/v2/message.py

@@ -86,7 +86,7 @@ class Message(resource.Resource):
                                        ) or session.get_project_id()
         }
 
-        query_params = cls._query_mapping._transpose(params)
+        query_params = cls._query_mapping._transpose(params, cls)
         while more_data:
             resp = session.get(uri,
                                headers=headers, params=query_params)

openstack/message/v2/queue.py

@@ -74,7 +74,7 @@ class Queue(resource.Resource):
         and `X-PROJECT-ID` fields which are required by Zaqar v2 API.
         """
         more_data = True
-        query_params = cls._query_mapping._transpose(params)
+        query_params = cls._query_mapping._transpose(params, cls)
 
         if base_path is None:
             base_path = cls.base_path

openstack/message/v2/subscription.py

@@ -93,7 +93,7 @@ class Subscription(resource.Resource):
                                        ) or session.get_project_id()
         }
 
-        query_params = cls._query_mapping._transpose(params)
+        query_params = cls._query_mapping._transpose(params, cls)
         while more_data:
             resp = session.get(uri,
                                headers=headers, params=query_params)

openstack/resource.py

@@ -32,6 +32,7 @@ and then returned to the caller.
 """
 
 import collections
+import inspect
 import itertools
 
 import jsonpatch
@@ -303,8 +304,8 @@ class QueryParameters(object):
         """
         expected_params = list(self._mapping.keys())
         expected_params.extend(
-            value['name'] if isinstance(value, dict) else value
+            value.get('name', key) if isinstance(value, dict) else value
-            for value in self._mapping.values())
+            for key, value in self._mapping.items())
 
         if base_path:
             expected_params += utils.get_string_format_keys(base_path)
@@ -323,7 +324,7 @@ class QueryParameters(object):
                     set(expected_params))
                 return {k: query[k] for k in known_keys}
 
-    def _transpose(self, query):
+    def _transpose(self, query, resource_type):
         """Transpose the keys in query based on the mapping
 
         If a query is supplied with its server side name, we will still use
@@ -332,16 +333,25 @@ class QueryParameters(object):
         :param dict query: Collection of key-value pairs where each key is the
                            client-side parameter name to be transposed to its
                            server side name.
+        :param resource_type: Class of a resource.
         """
         result = {}
         for client_side, server_side in self._mapping.items():
             if isinstance(server_side, dict):
-                name = server_side['name']
+                name = server_side.get('name', client_side)
                 type_ = server_side.get('type')
             else:
                 name = server_side
                 type_ = None
 
+            # NOTE(dtantsur): a small hack to be compatible with both
+            # single-argument (like int) and double-argument type functions.
+            try:
+                provide_resource_type = (
+                    len(inspect.getargspec(type_).args) > 1)
+            except TypeError:
+                provide_resource_type = False
+
             if client_side in query:
                 value = query[client_side]
             elif name in query:
@@ -350,7 +360,10 @@ class QueryParameters(object):
                 continue
 
             if type_ is not None:
-                result[name] = type_(value)
+                if provide_resource_type:
+                    result[name] = type_(value, resource_type)
+                else:
+                    result[name] = type_(value)
             else:
                 result[name] = value
         return result
@@ -1568,7 +1581,7 @@ class Resource(dict):
         params = cls._query_mapping._validate(
             params, base_path=base_path,
             allow_unknown_params=allow_unknown_params)
-        query_params = cls._query_mapping._transpose(params)
+        query_params = cls._query_mapping._transpose(params, cls)
         uri = base_path % params
 
         limit = query_params.get('limit')

openstack/tests/functional/baremetal/test_baremetal_node.py

@@ -34,8 +34,9 @@ class TestBareMetalNode(base.BaseBaremetalTest):
             self.assertEqual(node.id, found.id)
             self.assertEqual(node.name, found.name)
 
-        with_fields = self.conn.baremetal.get_node('node-name',
+        with_fields = self.conn.baremetal.get_node(
-                                                   fields=['uuid', 'driver'])
+            'node-name',
+            fields=['uuid', 'driver', 'instance_id'])
         self.assertEqual(node.id, with_fields.id)
         self.assertEqual(node.driver, with_fields.driver)
         self.assertIsNone(with_fields.name)
@@ -275,7 +276,8 @@ class TestBareMetalNodeFields(base.BaseBaremetalTest):
 
     def test_node_fields(self):
         self.create_node()
-        result = self.conn.baremetal.nodes(fields=['uuid', 'name'])
+        result = self.conn.baremetal.nodes(
+            fields=['uuid', 'name', 'instance_id'])
         for item in result:
             self.assertIsNotNone(item.id)
             self.assertIsNone(item.driver)

openstack/tests/functional/baremetal/test_baremetal_port.py

@@ -33,8 +33,8 @@ class TestBareMetalPort(base.BaseBaremetalTest):
         self.assertEqual(loaded.id, port.id)
         self.assertIsNotNone(loaded.address)
 
-        with_fields = self.conn.baremetal.get_port(port.id,
+        with_fields = self.conn.baremetal.get_port(
-                                                   fields=['uuid', 'extra'])
+            port.id, fields=['uuid', 'extra', 'node_id'])
         self.assertEqual(port.id, with_fields.id)
         self.assertIsNone(with_fields.address)
 
@@ -120,7 +120,7 @@ class TestBareMetalPortFields(base.BaseBaremetalTest):
     def test_port_fields(self):
         self.create_node()
         self.create_port(address='11:22:33:44:55:66')
-        result = self.conn.baremetal.ports(fields=['uuid'])
+        result = self.conn.baremetal.ports(fields=['uuid', 'node_id'])
         for item in result:
             self.assertIsNotNone(item.id)
             self.assertIsNone(item.address)

openstack/tests/unit/baremetal/v1/test_proxy.py

@@ -188,13 +188,25 @@ class TestBaremetalProxy(test_proxy_base.TestProxyBase):
                                            error_message=mock.ANY)
 
     @mock.patch.object(node.Node, 'fetch', autospec=True)
-    def test__get_with_fields(self, mock_fetch):
+    def test__get_with_fields_node(self, mock_fetch):
-        result = self.proxy._get_with_fields(node.Node, 'value',
+        result = self.proxy._get_with_fields(
-                                             fields=['a', 'b'])
+            # Mix of server-side and client-side fields
+            node.Node, 'value', fields=['maintenance', 'id', 'instance_id'])
         self.assertIs(result, mock_fetch.return_value)
-        mock_fetch.assert_called_once_with(mock.ANY, self.proxy,
+        mock_fetch.assert_called_once_with(
-                                           error_message=mock.ANY,
+            mock.ANY, self.proxy, error_message=mock.ANY,
-                                           fields='a,b')
+            # instance_id converted to server-side instance_uuid
+            fields='maintenance,uuid,instance_uuid')
+
+    @mock.patch.object(port.Port, 'fetch', autospec=True)
+    def test__get_with_fields_port(self, mock_fetch):
+        result = self.proxy._get_with_fields(
+            port.Port, 'value', fields=['address', 'id', 'node_id'])
+        self.assertIs(result, mock_fetch.return_value)
+        mock_fetch.assert_called_once_with(
+            mock.ANY, self.proxy, error_message=mock.ANY,
+            # node_id converted to server-side node_uuid
+            fields='address,uuid,node_uuid')
 
 
 @mock.patch('time.sleep', lambda _sec: None)

openstack/tests/unit/test_resource.py

@@ -377,21 +377,28 @@ class TestQueryParameters(base.TestCase):
                          sot._mapping)
 
     def test_transpose_unmapped(self):
+        def _type(value, rtype):
+            self.assertIs(rtype, mock.sentinel.resource_type)
+            return value * 10
+
         location = "location"
         mapping = {"first_name": "first-name",
                    "pet_name": {"name": "pet"},
-                   "answer": {"name": "answer", "type": int}}
+                   "answer": {"name": "answer", "type": int},
+                   "complex": {"type": _type}}
 
         sot = resource.QueryParameters(location, **mapping)
         result = sot._transpose({"location": "Brooklyn",
                                  "first_name": "Brian",
                                  "pet_name": "Meow",
                                  "answer": "42",
-                                 "last_name": "Curtin"})
+                                 "last_name": "Curtin",
+                                 "complex": 1},
+                                mock.sentinel.resource_type)
 
         # last_name isn't mapped and shouldn't be included
         self.assertEqual({"location": "Brooklyn", "first-name": "Brian",
-                          "pet": "Meow", "answer": 42},
+                          "pet": "Meow", "answer": 42, "complex": 10},
                          result)
 
     def test_transpose_not_in_query(self):
@@ -401,7 +408,8 @@ class TestQueryParameters(base.TestCase):
                    "answer": {"name": "answer", "type": int}}
 
         sot = resource.QueryParameters(location, **mapping)
-        result = sot._transpose({"location": "Brooklyn"})
+        result = sot._transpose({"location": "Brooklyn"},
+                                mock.sentinel.resource_type)
 
         # first_name not being in the query shouldn't affect results
         self.assertEqual({"location": "Brooklyn"},

releasenotes/notes/baremetal-fields-convert-857b8804327f1e86.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes conversion of the bare metal ``fields`` argument from SDK to
+    server-side field names (e.g. ``instance_id`` to ``instance_uuid``).