Merge "Convert portgroups endpoint to plain JSON"

4 files changed, 175 insertions, 281 deletions

diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py
index 2697ec4e7..e1e49d652 100644
--- a/ironic/api/controllers/v1/portgroup.py
+++ b/ironic/api/controllers/v1/portgroup.py
@@ -10,7 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import datetime
+import copy
 from http import client as http_client
 
 from ironic_lib import metrics_utils
@@ -18,15 +18,13 @@ from oslo_utils import uuidutils
 import pecan
 
 from ironic import api
-from ironic.api.controllers import base
 from ironic.api.controllers import link
 from ironic.api.controllers.v1 import collection
 from ironic.api.controllers.v1 import notification_utils as notify
 from ironic.api.controllers.v1 import port
-from ironic.api.controllers.v1 import types
 from ironic.api.controllers.v1 import utils as api_utils
-from ironic.api import expose
-from ironic.api import types as atypes
+from ironic.api import method
+from ironic.common import args
 from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import policy
@@ -35,217 +33,103 @@ from ironic import objects
 
 METRICS = metrics_utils.get_metrics_logger(__name__)
 
-_DEFAULT_RETURN_FIELDS = ('uuid', 'address', 'name')
-
-
-class Portgroup(base.APIBase):
-    """API representation of a portgroup.
-
-    This class enforces type checking and value constraints, and converts
-    between the internal object model and the API representation of a
-    portgroup.
-    """
-
-    _node_uuid = None
-
-    def _get_node_uuid(self):
-        return self._node_uuid
-
-    def _set_node_uuid(self, value):
-        if value and self._node_uuid != value:
-            if not api_utils.allow_portgroups():
-                self._node_uuid = atypes.Unset
-                return
-            try:
-                node = objects.Node.get(api.request.context, value)
-                self._node_uuid = node.uuid
-                # NOTE: Create the node_id attribute on-the-fly
-                #       to satisfy the api -> rpc object
-                #       conversion.
-                self.node_id = node.id
-            except exception.NodeNotFound as e:
-                # Change error code because 404 (NotFound) is inappropriate
-                # response for a POST request to create a Portgroup
-                e.code = http_client.BAD_REQUEST
-                raise e
-        elif value == atypes.Unset:
-            self._node_uuid = atypes.Unset
-
-    uuid = types.uuid
-    """Unique UUID for this portgroup"""
-
-    address = atypes.wsattr(types.macaddress)
-    """MAC Address for this portgroup"""
-
-    extra = {str: types.jsontype}
-    """This portgroup's meta data"""
-
-    internal_info = atypes.wsattr({str: types.jsontype}, readonly=True)
-    """This portgroup's internal info"""
-
-    node_uuid = atypes.wsproperty(types.uuid, _get_node_uuid, _set_node_uuid,
-                                  mandatory=True)
-    """The UUID of the node this portgroup belongs to"""
-
-    name = atypes.wsattr(str)
-    """The logical name for this portgroup"""
-
-    links = None
-    """A list containing a self link and associated portgroup links"""
-
-    standalone_ports_supported = types.boolean
-    """Indicates whether ports of this portgroup may be used as
-       single NIC ports"""
-
-    mode = atypes.wsattr(str)
-    """The mode for this portgroup. See linux bonding
-    documentation for details:
-    https://www.kernel.org/doc/Documentation/networking/bonding.txt"""
-
-    properties = {str: types.jsontype}
-    """This portgroup's properties"""
-
-    ports = None
-    """Links to the collection of ports of this portgroup"""
-
-    def __init__(self, **kwargs):
-        self.fields = []
-        fields = list(objects.Portgroup.fields)
-        # NOTE: node_uuid is not part of objects.Portgroup.fields
-        #       because it's an API-only attribute
-        fields.append('node_uuid')
-        for field in fields:
-            # Skip fields we do not expose.
-            if not hasattr(self, field):
-                continue
-            self.fields.append(field)
-            setattr(self, field, kwargs.get(field, atypes.Unset))
-
-        # NOTE: node_id is an attribute created on-the-fly
-        # by _set_node_uuid(), it needs to be present in the fields so
-        # that as_dict() will contain node_id field when converting it
-        # before saving it in the database.
-        self.fields.append('node_id')
-        setattr(self, 'node_uuid', kwargs.get('node_id', atypes.Unset))
-
-    @staticmethod
-    def _convert_with_links(portgroup, url, fields=None):
-        """Add links to the portgroup."""
-        if fields is None:
-            portgroup.ports = [
-                link.make_link('self', url, 'portgroups',
-                               portgroup.uuid + "/ports"),
-                link.make_link('bookmark', url, 'portgroups',
-                               portgroup.uuid + "/ports", bookmark=True)
-            ]
-
-        # never expose the node_id attribute
-        portgroup.node_id = atypes.Unset
-
-        portgroup.links = [link.make_link('self', url,
-                                          'portgroups', portgroup.uuid),
-                           link.make_link('bookmark', url,
-                                          'portgroups', portgroup.uuid,
-                                          bookmark=True)
-                           ]
+_DEFAULT_RETURN_FIELDS = ['uuid', 'address', 'name']
+
+PORTGROUP_SCHEMA = {
+    'type': 'object',
+    'properties': {
+        'address': {'type': ['string', 'null']},
+        'extra': {'type': ['object', 'null']},
+        'mode': {'type': ['string', 'null']},
+        'name': {'type': ['string', 'null']},
+        'node_uuid': {'type': 'string'},
+        'properties': {'type': ['object', 'null']},
+        'standalone_ports_supported': {'type': ['string', 'boolean', 'null']},
+        'uuid': {'type': ['string', 'null']},
+    },
+    'required': ['node_uuid'],
+    'additionalProperties': False,
+}
+
+PORTGROUP_PATCH_SCHEMA = copy.deepcopy(PORTGROUP_SCHEMA)
+# patch supports patching some internal_info values
+PORTGROUP_PATCH_SCHEMA['properties']['internal_info'] = {
+    'type': ['null', 'object']}
+
+PORTGROUP_VALIDATOR_EXTRA = args.dict_valid(
+    address=args.mac_address,
+    node_uuid=args.uuid,
+    standalone_ports_supported=args.boolean,
+    uuid=args.uuid
+)
+PORTGROUP_VALIDATOR = args.and_valid(
+    args.schema(PORTGROUP_SCHEMA),
+    PORTGROUP_VALIDATOR_EXTRA
+)
+
+PORTGROUP_PATCH_VALIDATOR = args.and_valid(
+    args.schema(PORTGROUP_PATCH_SCHEMA),
+    PORTGROUP_VALIDATOR_EXTRA
+)
+
+PATCH_ALLOWED_FIELDS = [
+    'address',
+    'extra',
+    'mode',
+    'name',
+    'node_uuid',
+    'properties',
+    'standalone_ports_supported'
+]
+
+
+def convert_with_links(rpc_portgroup, fields=None, sanitize=True):
+    """Add links to the portgroup."""
+    portgroup = api_utils.object_to_dict(
+        rpc_portgroup,
+        link_resource='portgroups',
+        fields=(
+            'address',
+            'extra',
+            'internal_info',
+            'mode',
+            'name',
+            'properties',
+            'standalone_ports_supported'
+        )
+    )
+    api_utils.populate_node_uuid(rpc_portgroup, portgroup)
+    url = api.request.public_url
+    portgroup['ports'] = [
+        link.make_link('self', url, 'portgroups',
+                       rpc_portgroup.uuid + "/ports"),
+        link.make_link('bookmark', url, 'portgroups',
+                       rpc_portgroup.uuid + "/ports", bookmark=True)
+    ]
+
+    if fields is not None:
+        api_utils.check_for_invalid_fields(fields, portgroup)
+
+    if not sanitize:
         return portgroup
 
-    @classmethod
-    def convert_with_links(cls, rpc_portgroup, fields=None, sanitize=True):
-        """Add links to the portgroup."""
-        portgroup = Portgroup(**rpc_portgroup.as_dict())
+    api_utils.sanitize_dict(portgroup, fields)
 
-        if fields is not None:
-            api_utils.check_for_invalid_fields(fields, portgroup.as_dict())
+    return portgroup
 
-        portgroup = cls._convert_with_links(portgroup, api.request.host_url,
-                                            fields=fields)
 
-        if not sanitize:
-            return portgroup
-
-        portgroup.sanitize(fields)
-
-        return portgroup
-
-    def sanitize(self, fields=None):
-        """Removes sensitive and unrequested data.
-
-        Will only keep the fields specified in the ``fields`` parameter.
-
-        :param fields:
-            list of fields to preserve, or ``None`` to preserve them all
-        :type fields: list of str
-        """
-
-        if fields is not None:
-            self.unset_fields_except(fields)
-
-        # never expose the node_id attribute
-        self.node_id = atypes.Unset
-
-    @classmethod
-    def sample(cls, expand=True):
-        """Return a sample of the portgroup."""
-        sample = cls(uuid='a594544a-2daf-420c-8775-17a8c3e0852f',
-                     address='fe:54:00:77:07:d9',
-                     name='node1-portgroup-01',
-                     extra={'foo': 'bar'},
-                     internal_info={'baz': 'boo'},
-                     standalone_ports_supported=True,
-                     mode='active-backup',
-                     properties={},
-                     created_at=datetime.datetime(2000, 1, 1, 12, 0, 0),
-                     updated_at=datetime.datetime(2000, 1, 1, 12, 0, 0))
-        # NOTE(lucasagomes): node_uuid getter() method look at the
-        # _node_uuid variable
-        sample._node_uuid = '7ae81bb3-dec3-4289-8d6c-da80bd8001ae'
-        fields = None if expand else _DEFAULT_RETURN_FIELDS
-        return cls._convert_with_links(sample, 'http://localhost:6385',
-                                       fields=fields)
-
-
-class PortgroupPatchType(types.JsonPatchType):
-
-    _api_base = Portgroup
-    _extra_non_removable_attrs = {'/mode'}
-
-    @staticmethod
-    def internal_attrs():
-        defaults = types.JsonPatchType.internal_attrs()
-        return defaults + ['/internal_info']
-
-
-class PortgroupCollection(collection.Collection):
-    """API representation of a collection of portgroups."""
-
-    portgroups = [Portgroup]
-    """A list containing portgroup objects"""
-
-    def __init__(self, **kwargs):
-        self._type = 'portgroups'
-
-    @staticmethod
-    def convert_with_links(rpc_portgroups, limit, url=None, fields=None,
-                           **kwargs):
-        collection = PortgroupCollection()
-        collection.portgroups = [Portgroup.convert_with_links(p, fields=fields,
-                                                              sanitize=False)
-                                 for p in rpc_portgroups]
-        collection.next = collection.get_next(limit, url=url, fields=fields,
-                                              **kwargs)
-
-        for item in collection.portgroups:
-            item.sanitize(fields=fields)
-
-        return collection
-
-    @classmethod
-    def sample(cls):
-        """Return a sample of the portgroup."""
-        sample = cls()
-        sample.portgroups = [Portgroup.sample(expand=False)]
-        return sample
+def list_convert_with_links(rpc_portgroups, limit, url=None, fields=None,
+                            **kwargs):
+    return collection.list_convert_with_links(
+        items=[convert_with_links(p, fields=fields, sanitize=False)
+               for p in rpc_portgroups],
+        item_name='portgroups',
+        limit=limit,
+        url=url,
+        fields=fields,
+        sanitize_func=api_utils.sanitize_dict,
+        **kwargs
+    )
 
 
 class PortgroupsController(pecan.rest.RestController):
@@ -266,8 +150,8 @@ class PortgroupsController(pecan.rest.RestController):
         if not api_utils.allow_portgroups():
             pecan.abort(http_client.NOT_FOUND)
         try:
-            ident = types.uuid_or_name.validate(ident)
-        except exception.InvalidUuidOrName as e:
+            ident = args.uuid_or_name('portgroup', ident)
+        except exception.InvalidParameterValue as e:
             pecan.abort(http_client.BAD_REQUEST, e.args[0])
         if not remainder:
             return
@@ -333,12 +217,12 @@ class PortgroupsController(pecan.rest.RestController):
         if detail is not None:
             parameters['detail'] = detail
 
-        return PortgroupCollection.convert_with_links(portgroups, limit,
-                                                      url=resource_url,
-                                                      fields=fields,
-                                                      sort_key=sort_key,
-                                                      sort_dir=sort_dir,
-                                                      **parameters)
+        return list_convert_with_links(portgroups, limit,
+                                       url=resource_url,
+                                       fields=fields,
+                                       sort_key=sort_key,
+                                       sort_dir=sort_dir,
+                                       **parameters)
 
     def _get_portgroups_by_address(self, address):
         """Retrieve a portgroup by its address.
@@ -357,9 +241,11 @@ class PortgroupsController(pecan.rest.RestController):
             return []
 
     @METRICS.timer('PortgroupsController.get_all')
-    @expose.expose(PortgroupCollection, types.uuid_or_name, types.macaddress,
-                   types.uuid, int, str, str, types.listtype,
-                   types.boolean)
+    @method.expose()
+    @args.validate(node=args.uuid_or_name, address=args.mac_address,
+                   marker=args.uuid, limit=args.integer, sort_key=args.string,
+                   sort_dir=args.string, fields=args.string_list,
+                   detail=args.boolean)
     def get_all(self, node=None, address=None, marker=None,
                 limit=None, sort_key='id', sort_dir='asc', fields=None,
                 detail=None):
@@ -398,8 +284,10 @@ class PortgroupsController(pecan.rest.RestController):
                                                detail=detail)
 
     @METRICS.timer('PortgroupsController.detail')
-    @expose.expose(PortgroupCollection, types.uuid_or_name, types.macaddress,
-                   types.uuid, int, str, str)
+    @method.expose()
+    @args.validate(node=args.uuid_or_name, address=args.mac_address,
+                   marker=args.uuid, limit=args.integer, sort_key=args.string,
+                   sort_dir=args.string)
     def detail(self, node=None, address=None, marker=None,
                limit=None, sort_key='id', sort_dir='asc'):
         """Retrieve a list of portgroups with detail.
@@ -434,7 +322,8 @@ class PortgroupsController(pecan.rest.RestController):
             resource_url=resource_url)
 
     @METRICS.timer('PortgroupsController.get_one')
-    @expose.expose(Portgroup, types.uuid_or_name, types.listtype)
+    @method.expose()
+    @args.validate(portgroup_ident=args.uuid_or_name, fields=args.string_list)
     def get_one(self, portgroup_ident, fields=None):
         """Retrieve information about the given portgroup.
 
@@ -455,10 +344,12 @@ class PortgroupsController(pecan.rest.RestController):
 
         rpc_portgroup = api_utils.get_rpc_portgroup_with_suffix(
             portgroup_ident)
-        return Portgroup.convert_with_links(rpc_portgroup, fields=fields)
+        return convert_with_links(rpc_portgroup, fields=fields)
 
     @METRICS.timer('PortgroupsController.post')
-    @expose.expose(Portgroup, body=Portgroup, status_code=http_client.CREATED)
+    @method.expose(status_code=http_client.CREATED)
+    @method.body('portgroup')
+    @args.validate(portgroup=PORTGROUP_VALIDATOR)
     def post(self, portgroup):
         """Create a new portgroup.
 
@@ -475,43 +366,43 @@ class PortgroupsController(pecan.rest.RestController):
             raise exception.OperationNotPermitted()
 
         if (not api_utils.allow_portgroup_mode_properties()
-                and (portgroup.mode is not atypes.Unset
-                     or portgroup.properties is not atypes.Unset)):
+                and (portgroup.get('mode') or portgroup.get('properties'))):
             raise exception.NotAcceptable()
 
-        if (portgroup.name
-                and not api_utils.is_valid_logical_name(portgroup.name)):
+        if (portgroup.get('name')
+                and not api_utils.is_valid_logical_name(portgroup['name'])):
             error_msg = _("Cannot create portgroup with invalid name "
-                          "'%(name)s'") % {'name': portgroup.name}
+                          "'%(name)s'") % {'name': portgroup['name']}
             raise exception.ClientSideError(
                 error_msg, status_code=http_client.BAD_REQUEST)
 
-        pg_dict = portgroup.as_dict()
-
-        api_utils.handle_post_port_like_extra_vif(pg_dict)
+        api_utils.handle_post_port_like_extra_vif(portgroup)
 
         # NOTE(yuriyz): UUID is mandatory for notifications payload
-        if not pg_dict.get('uuid'):
-            pg_dict['uuid'] = uuidutils.generate_uuid()
+        if not portgroup.get('uuid'):
+            portgroup['uuid'] = uuidutils.generate_uuid()
 
-        new_portgroup = objects.Portgroup(context, **pg_dict)
+        node = api_utils.replace_node_uuid_with_id(portgroup)
+
+        new_portgroup = objects.Portgroup(context, **portgroup)
 
         notify.emit_start_notification(context, new_portgroup, 'create',
-                                       node_uuid=portgroup.node_uuid)
+                                       node_uuid=node.uuid)
         with notify.handle_error_notification(context, new_portgroup, 'create',
-                                              node_uuid=portgroup.node_uuid):
+                                              node_uuid=node.uuid):
             new_portgroup.create()
         notify.emit_end_notification(context, new_portgroup, 'create',
-                                     node_uuid=portgroup.node_uuid)
+                                     node_uuid=node.uuid)
 
         # Set the HTTP Location Header
         api.response.location = link.build_url('portgroups',
                                                new_portgroup.uuid)
-        return Portgroup.convert_with_links(new_portgroup)
+        return convert_with_links(new_portgroup)
 
     @METRICS.timer('PortgroupsController.patch')
-    @expose.validate(types.uuid_or_name, [PortgroupPatchType])
-    @expose.expose(Portgroup, types.uuid_or_name, body=[PortgroupPatchType])
+    @method.expose()
+    @method.body('patch')
+    @args.validate(portgroup_ident=args.uuid_or_name, patch=args.patch)
     def patch(self, portgroup_ident, patch):
         """Update an existing portgroup.
 
@@ -533,6 +424,8 @@ class PortgroupsController(pecan.rest.RestController):
                      or api_utils.is_path_updated(patch, '/properties'))):
             raise exception.NotAcceptable()
 
+        api_utils.patch_validate_allowed_fields(patch, PATCH_ALLOWED_FIELDS)
+
         rpc_portgroup = api_utils.get_rpc_portgroup_with_suffix(
             portgroup_ident)
 
@@ -547,30 +440,41 @@ class PortgroupsController(pecan.rest.RestController):
                     error_msg, status_code=http_client.BAD_REQUEST)
 
         portgroup_dict = rpc_portgroup.as_dict()
+
         # NOTE:
         # 1) Remove node_id because it's an internal value and
         #    not present in the API object
         # 2) Add node_uuid
-        portgroup_dict['node_uuid'] = portgroup_dict.pop('node_id', None)
-        portgroup = Portgroup(**api_utils.apply_jsonpatch(portgroup_dict,
-                                                          patch))
+        rpc_node = api_utils.replace_node_id_with_uuid(portgroup_dict)
+
+        portgroup_dict = api_utils.apply_jsonpatch(portgroup_dict, patch)
+
+        if 'mode' not in portgroup_dict:
+            msg = _("'mode' is a mandatory attribute and can not be removed")
+            raise exception.ClientSideError(msg)
+
+        try:
+            if portgroup_dict['node_uuid'] != rpc_node.uuid:
+                rpc_node = objects.Node.get(api.request.context,
+                                            portgroup_dict['node_uuid'])
+
+        except exception.NodeNotFound as e:
+            # Change error code because 404 (NotFound) is inappropriate
+            # response for a POST request to patch a Portgroup
+            e.code = http_client.BAD_REQUEST  # BadRequest
+            raise
 
         api_utils.handle_patch_port_like_extra_vif(
-            rpc_portgroup, portgroup.internal_info, patch)
-
-        # Update only the fields that have changed
-        for field in objects.Portgroup.fields:
-            try:
-                patch_val = getattr(portgroup, field)
-            except AttributeError:
-                # Ignore fields that aren't exposed in the API
-                continue
-            if patch_val == atypes.Unset:
-                patch_val = None
-            if rpc_portgroup[field] != patch_val:
-                rpc_portgroup[field] = patch_val
-
-        rpc_node = objects.Node.get_by_id(context, rpc_portgroup.node_id)
+            rpc_portgroup, portgroup_dict.get('internal_info'), patch)
+
+        api_utils.patched_validate_with_schema(
+            portgroup_dict, PORTGROUP_PATCH_SCHEMA, PORTGROUP_PATCH_VALIDATOR)
+
+        api_utils.patch_update_changed_fields(
+            portgroup_dict, rpc_portgroup, fields=objects.Portgroup.fields,
+            schema=PORTGROUP_PATCH_SCHEMA, id_map={'node_id': rpc_node.id}
+        )
+
         if (rpc_node.provision_state == ir_states.INSPECTING
                 and api_utils.allow_inspect_wait_state()):
             msg = _('Cannot update portgroup "%(portgroup)s" on node '
@@ -588,15 +492,15 @@ class PortgroupsController(pecan.rest.RestController):
             new_portgroup = api.request.rpcapi.update_portgroup(
                 context, rpc_portgroup, topic)
 
-        api_portgroup = Portgroup.convert_with_links(new_portgroup)
+        api_portgroup = convert_with_links(new_portgroup)
         notify.emit_end_notification(context, new_portgroup, 'update',
-                                     node_uuid=api_portgroup.node_uuid)
+                                     node_uuid=rpc_node.uuid)
 
         return api_portgroup
 
     @METRICS.timer('PortgroupsController.delete')
-    @expose.expose(None, types.uuid_or_name,
-                   status_code=http_client.NO_CONTENT)
+    @method.expose(status_code=http_client.NO_CONTENT)
+    @args.validate(portgroup_ident=args.uuid_or_name)
     def delete(self, portgroup_ident):
         """Delete a portgroup.
 
diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py
index dfdddc470..b2065e7bb 100644
--- a/ironic/tests/unit/api/controllers/v1/test_port.py
+++ b/ironic/tests/unit/api/controllers/v1/test_port.py
@@ -1112,7 +1112,7 @@ class TestListPorts(test_api_base.BaseApiTest):
                                  headers={api_base.Version.string: '1.24'},
                                  expect_errors=True)
         self.assertEqual(http_client.BAD_REQUEST, response.status_int)
-        self.assertIn('Expected a logical name or UUID',
+        self.assertIn('Expected UUID or name for portgroup',
                       response.json['error_message'])
 
 
diff --git a/ironic/tests/unit/api/controllers/v1/test_portgroup.py b/ironic/tests/unit/api/controllers/v1/test_portgroup.py
index 8d66c414d..1f18564a0 100644
--- a/ironic/tests/unit/api/controllers/v1/test_portgroup.py
+++ b/ironic/tests/unit/api/controllers/v1/test_portgroup.py
@@ -26,16 +26,13 @@ from testtools.matchers import HasLength
 from ironic.api.controllers import base as api_base
 from ironic.api.controllers import v1 as api_v1
 from ironic.api.controllers.v1 import notification_utils
-from ironic.api.controllers.v1 import portgroup as api_portgroup
 from ironic.api.controllers.v1 import utils as api_utils
-from ironic.api import types as atypes
 from ironic.common import exception
 from ironic.common import states
 from ironic.common import utils as common_utils
 from ironic.conductor import rpcapi
 from ironic import objects
 from ironic.objects import fields as obj_fields
-from ironic.tests import base
 from ironic.tests.unit.api import base as test_api_base
 from ironic.tests.unit.api import utils as apiutils
 from ironic.tests.unit.objects import utils as obj_utils
@@ -51,15 +48,6 @@ def _rpcapi_update_portgroup(self, context, portgroup, topic):
     return portgroup
 
 
-class TestPortgroupObject(base.TestCase):
-
-    def test_portgroup_init(self):
-        portgroup_dict = apiutils.portgroup_post_data(node_id=None)
-        del portgroup_dict['extra']
-        portgroup = api_portgroup.Portgroup(**portgroup_dict)
-        self.assertEqual(atypes.Unset, portgroup.extra)
-
-
 class TestListPortgroups(test_api_base.BaseApiTest):
     headers = {api_base.Version.string: str(api_v1.max_version())}
 
@@ -1007,6 +995,7 @@ class TestPatch(test_api_base.BaseApiTest):
         self.assertFalse(mock_upd.called)
 
     def test_update_portgroup_internal_info_not_allowed(self, mock_upd):
+        mock_upd.return_value = self.portgroup
         response = self.patch_json('/portgroups/%s' % self.portgroup.uuid,
                                    [{'path': '/internal_info',
                                      'value': False,
@@ -1052,6 +1041,7 @@ class TestPatch(test_api_base.BaseApiTest):
             [{'path': '/properties/abc', 'op': 'add', 'value': 123}], mock_upd)
 
     def test_remove_mode_not_allowed(self, mock_upd):
+        mock_upd.return_value = self.portgroup
         response = self.patch_json('/portgroups/%s' % self.portgroup.uuid,
                                    [{'path': '/mode',
                                      'op': 'remove'}],
diff --git a/ironic/tests/unit/api/utils.py b/ironic/tests/unit/api/utils.py
index 28566a4c6..ce5d95866 100644
--- a/ironic/tests/unit/api/utils.py
+++ b/ironic/tests/unit/api/utils.py
@@ -167,8 +167,8 @@ def portgroup_post_data(**kw):
         if arg not in kw:
             portgroup.pop(arg)
 
-    internal = portgroup_controller.PortgroupPatchType.internal_attrs()
-    return remove_internal(portgroup, internal)
+    return remove_other_fields(
+        portgroup, portgroup_controller.PORTGROUP_SCHEMA['properties'])
 
 
 def post_get_test_portgroup(**kw):