From 34b3589ea3c9e87b8ba370f122a37ac4eb2d9afd Mon Sep 17 00:00:00 2001 From: Miles Gould Date: Wed, 28 Oct 2015 17:59:42 +0000 Subject: [PATCH] Get mandatory patch attrs from WSME properties Attributes which are mandatory (ie, required for object creation) should not be removable. However, some attributes (such as Node.chassis_uuid) are not required for object creation, but should not be removable if they are set. This commit does the following: - rename JsonPatchType.mandatory_attrs to non_removable_attrs to better describe its meaning, - change its return type to set-of-strings for faster lookup - ensure all mandatory attributes on the type being patched are included in the set of non-removable attributes, - add a new field, JsonPatchType._extra_non_removable_attrs, which should be a set of attributes that are not required for creation but should not be removed if set. Since the object to be patched does not exist at patch-validation time, we leave the validation logic in methods of JsonPatchType and subclasses. This means introspecting the types to be patched. Closes-Bug: #1284781 Change-Id: I2bb7fed2c776c8d63535c5ee19cdc218e57806e3 --- ironic/api/controllers/v1/chassis.py | 9 +++--- ironic/api/controllers/v1/node.py | 40 +++++++++++++------------- ironic/api/controllers/v1/port.py | 11 +++---- ironic/api/controllers/v1/types.py | 37 ++++++++++++++++++------ ironic/tests/unit/api/v1/test_ports.py | 1 + ironic/tests/unit/api/v1/test_types.py | 17 ++++++++--- 6 files changed, 72 insertions(+), 43 deletions(-) diff --git a/ironic/api/controllers/v1/chassis.py b/ironic/api/controllers/v1/chassis.py index 89e7933ca7..819278faec 100644 --- a/ironic/api/controllers/v1/chassis.py +++ b/ironic/api/controllers/v1/chassis.py @@ -36,10 +36,6 @@ from ironic import objects _DEFAULT_RETURN_FIELDS = ('uuid', 'description') -class ChassisPatchType(types.JsonPatchType): - pass - - class Chassis(base.APIBase): """API representation of a chassis. @@ -122,6 +118,11 @@ class Chassis(base.APIBase): fields=fields) +class ChassisPatchType(types.JsonPatchType): + + _api_base = Chassis + + class ChassisCollection(collection.Collection): """API representation of a collection of chassis.""" diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index d95298f712..474dd826cb 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -110,26 +110,6 @@ def check_allow_management_verbs(verb): raise exception.NotAcceptable() -class NodePatchType(types.JsonPatchType): - - @staticmethod - def internal_attrs(): - defaults = types.JsonPatchType.internal_attrs() - # TODO(lucasagomes): Include maintenance once the endpoint - # v1/nodes//maintenance do more things than updating the DB. - return defaults + ['/console_enabled', '/last_error', - '/power_state', '/provision_state', '/reservation', - '/target_power_state', '/target_provision_state', - '/provision_updated_at', '/maintenance_reason', - '/driver_internal_info', '/inspection_finished_at', - '/inspection_started_at', '/clean_step', - '/raid_config', '/target_raid_config'] - - @staticmethod - def mandatory_attrs(): - return ['/chassis_uuid', '/driver'] - - class BootDeviceController(rest.RestController): _custom_actions = { @@ -735,6 +715,26 @@ class Node(base.APIBase): fields=fields) +class NodePatchType(types.JsonPatchType): + + _api_base = Node + + _extra_non_removable_attrs = {'/chassis_uuid'} + + @staticmethod + def internal_attrs(): + defaults = types.JsonPatchType.internal_attrs() + # TODO(lucasagomes): Include maintenance once the endpoint + # v1/nodes//maintenance do more things than updating the DB. + return defaults + ['/console_enabled', '/last_error', + '/power_state', '/provision_state', '/reservation', + '/target_power_state', '/target_provision_state', + '/provision_updated_at', '/maintenance_reason', + '/driver_internal_info', '/inspection_finished_at', + '/inspection_started_at', '/clean_step', + '/raid_config', '/target_raid_config'] + + class NodeCollection(collection.Collection): """API representation of a collection of nodes.""" diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index 16b9ab5025..962d03bc56 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -36,13 +36,6 @@ from ironic import objects _DEFAULT_RETURN_FIELDS = ('uuid', 'address') -class PortPatchType(types.JsonPatchType): - - @staticmethod - def mandatory_attrs(): - return ['/address', '/node_uuid'] - - class Port(base.APIBase): """API representation of a port. @@ -155,6 +148,10 @@ class Port(base.APIBase): fields=fields) +class PortPatchType(types.JsonPatchType): + _api_base = Port + + class PortCollection(collection.Collection): """API representation of a collection of ports.""" diff --git a/ironic/api/controllers/v1/types.py b/ironic/api/controllers/v1/types.py index 1dc4ed6188..b5fdd7f21c 100644 --- a/ironic/api/controllers/v1/types.py +++ b/ironic/api/controllers/v1/types.py @@ -15,6 +15,7 @@ # License for the specific language governing permissions and limitations # under the License. +import inspect import json from oslo_utils import strutils @@ -194,6 +195,17 @@ class JsonPatchType(wtypes.Base): mandatory=True) value = wsme.wsattr(jsontype, default=wtypes.Unset) + # The class of the objects being patched. Override this in subclasses. + # Should probably be a subclass of ironic.api.controllers.base.APIBase. + _api_base = None + + # Attributes that are not required for construction, but which may not be + # removed if set. Override in subclasses if needed. + _extra_non_removable_attrs = set() + + # Set of non-removable attributes, calculated lazily. + _non_removable_attrs = None + @staticmethod def internal_attrs(): """Returns a list of internal attributes. @@ -204,15 +216,24 @@ class JsonPatchType(wtypes.Base): """ return ['/created_at', '/id', '/links', '/updated_at', '/uuid'] - @staticmethod - def mandatory_attrs(): - """Retruns a list of mandatory attributes. - - Mandatory attributes can't be removed from the document. This - method should be overwritten by derived class. + @classmethod + def non_removable_attrs(cls): + """Returns a set of names of attributes that may not be removed. + Attributes whose 'mandatory' property is True are automatically added + to this set. To add additional attributes to the set, override the + field _extra_non_removable_attrs in subclasses, with a set of the form + {'/foo', '/bar'}. """ - return [] + if cls._non_removable_attrs is None: + cls._non_removable_attrs = cls._extra_non_removable_attrs.copy() + if cls._api_base: + fields = inspect.getmembers(cls._api_base, + lambda a: not inspect.isroutine(a)) + for name, field in fields: + if getattr(field, 'mandatory', False): + cls._non_removable_attrs.add('/%s' % name) + return cls._non_removable_attrs @staticmethod def validate(patch): @@ -221,7 +242,7 @@ class JsonPatchType(wtypes.Base): msg = _("'%s' is an internal attribute and can not be updated") raise wsme.exc.ClientSideError(msg % patch.path) - if patch.path in patch.mandatory_attrs() and patch.op == 'remove': + if patch.path in patch.non_removable_attrs() and patch.op == 'remove': msg = _("'%s' is a mandatory attribute and can not be removed") raise wsme.exc.ClientSideError(msg % patch.path) diff --git a/ironic/tests/unit/api/v1/test_ports.py b/ironic/tests/unit/api/v1/test_ports.py index e8c3ea12a4..2427a16ec9 100644 --- a/ironic/tests/unit/api/v1/test_ports.py +++ b/ironic/tests/unit/api/v1/test_ports.py @@ -562,6 +562,7 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.BAD_REQUEST, response.status_code) self.assertTrue(response.json['error_message']) + self.assertIn('mandatory attribute', response.json['error_message']) self.assertFalse(mock_upd.called) def test_add_root(self, mock_upd): diff --git a/ironic/tests/unit/api/v1/test_types.py b/ironic/tests/unit/api/v1/test_types.py index 9feca3bfe4..1bc28ab201 100644 --- a/ironic/tests/unit/api/v1/test_types.py +++ b/ironic/tests/unit/api/v1/test_types.py @@ -88,12 +88,15 @@ class TestUuidOrNameType(base.TestCase): types.UuidOrNameType.validate, 'inval#uuid%or*name') +class MyBaseType(object): + """Helper class, patched by objects of type MyPatchType""" + mandatory = wsme.wsattr(wtypes.text, mandatory=True) + + class MyPatchType(types.JsonPatchType): """Helper class for TestJsonPatchType tests.""" - - @staticmethod - def mandatory_attrs(): - return ['/mandatory'] + _api_base = MyBaseType + _extra_non_removable_attrs = {'/non_removable'} @staticmethod def internal_attrs(): @@ -162,6 +165,12 @@ class TestJsonPatchType(base.TestCase): self.assertEqual(http_client.BAD_REQUEST, ret.status_int) self.assertTrue(ret.json['faultstring']) + def test_cannot_remove_extra_non_removable_attr(self): + patch = [{'op': 'remove', 'path': '/non_removable'}] + ret = self._patch_json(patch, True) + self.assertEqual(http_client.BAD_REQUEST, ret.status_int) + self.assertTrue(ret.json['faultstring']) + def test_missing_required_fields_path(self): missing_path = [{'op': 'remove'}] ret = self._patch_json(missing_path, True)