From d82d8b24203d4f762ad15a69035f2845586ae183 Mon Sep 17 00:00:00 2001 From: yatin karel Date: Sun, 31 Jul 2016 15:34:01 +0530 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 baymodel.server_type) 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. This patch is copied from ironic [1]. [1] https://review.openstack.org/#/c/240202/9 Change-Id: Ifcfc4e48a05d75b919a33ef463754c199da94a8e Close-Bug: #1530771 --- magnum/api/controllers/v1/bay.py | 29 ++++++------- magnum/api/controllers/v1/baymodel.py | 17 ++++---- magnum/api/controllers/v1/cluster.py | 28 ++++++------ magnum/api/controllers/v1/cluster_template.py | 17 ++++---- magnum/api/controllers/v1/types.py | 38 ++++++++++++---- .../tests/unit/api/controllers/v1/test_bay.py | 24 ++++------- .../unit/api/controllers/v1/test_baymodel.py | 42 +++++++++--------- .../unit/api/controllers/v1/test_cluster.py | 25 ++++------- .../controllers/v1/test_cluster_template.py | 43 +++++++++---------- .../unit/api/controllers/v1/test_types.py | 34 ++++++++++++--- 10 files changed, 158 insertions(+), 139 deletions(-) diff --git a/magnum/api/controllers/v1/bay.py b/magnum/api/controllers/v1/bay.py index 813955d954..b658f06739 100644 --- a/magnum/api/controllers/v1/bay.py +++ b/magnum/api/controllers/v1/bay.py @@ -40,22 +40,6 @@ from magnum.objects import fields LOG = logging.getLogger(__name__) -class BayPatchType(types.JsonPatchType): - - @staticmethod - def mandatory_attrs(): - return ['/baymodel_id'] - - @staticmethod - def internal_attrs(): - internal_attrs = ['/api_address', '/node_addresses', - '/master_addresses', '/stack_id', - '/ca_cert_ref', '/magnum_cert_ref', - '/trust_id', '/trustee_user_name', - '/trustee_password', '/trustee_user_id'] - return types.JsonPatchType.internal_attrs() + internal_attrs - - class BayID(wtypes.Base): uuid = types.uuid @@ -185,6 +169,19 @@ class Bay(base.APIBase): return cls._convert_with_links(sample, 'http://localhost:9511', expand) +class BayPatchType(types.JsonPatchType): + _api_base = Bay + + @staticmethod + def internal_attrs(): + internal_attrs = ['/api_address', '/node_addresses', + '/master_addresses', '/stack_id', + '/ca_cert_ref', '/magnum_cert_ref', + '/trust_id', '/trustee_user_name', + '/trustee_password', '/trustee_user_id'] + return types.JsonPatchType.internal_attrs() + internal_attrs + + class BayCollection(collection.Collection): """API representation of a collection of bays.""" diff --git a/magnum/api/controllers/v1/baymodel.py b/magnum/api/controllers/v1/baymodel.py index bb25c8a0b2..386418768c 100644 --- a/magnum/api/controllers/v1/baymodel.py +++ b/magnum/api/controllers/v1/baymodel.py @@ -33,15 +33,6 @@ from magnum import objects from magnum.objects import fields -class BayModelPatchType(types.JsonPatchType): - - @staticmethod - def mandatory_attrs(): - return ['/image_id', '/keypair_id', '/external_network_id', '/coe', - '/tls_disabled', '/public', '/registry_enabled', - '/server_type', '/cluster_distro', '/network_driver'] - - class BayModel(base.APIBase): """API representation of a baymodel. @@ -206,6 +197,14 @@ class BayModel(base.APIBase): return cls._convert_with_links(sample, 'http://localhost:9511') +class BayModelPatchType(types.JsonPatchType): + _api_base = BayModel + _extra_non_removable_attrs = {'/network_driver', '/external_network_id', + '/tls_disabled', '/public', '/server_type', + '/coe', '/registry_enabled', + '/cluster_distro'} + + class BayModelCollection(collection.Collection): """API representation of a collection of baymodels.""" diff --git a/magnum/api/controllers/v1/cluster.py b/magnum/api/controllers/v1/cluster.py index 26c1c727cf..7acc56ebf8 100644 --- a/magnum/api/controllers/v1/cluster.py +++ b/magnum/api/controllers/v1/cluster.py @@ -40,21 +40,6 @@ from magnum.objects import fields LOG = logging.getLogger(__name__) -class ClusterPatchType(types.JsonPatchType): - @staticmethod - def mandatory_attrs(): - return ['/cluster_template_id'] - - @staticmethod - def internal_attrs(): - internal_attrs = ['/api_address', '/node_addresses', - '/master_addresses', '/stack_id', - '/ca_cert_ref', '/magnum_cert_ref', - '/trust_id', '/trustee_user_name', - '/trustee_password', '/trustee_user_id'] - return types.JsonPatchType.internal_attrs() + internal_attrs - - class ClusterID(wtypes.Base): """API representation of a cluster ID @@ -237,6 +222,19 @@ class Cluster(base.APIBase): return d +class ClusterPatchType(types.JsonPatchType): + _api_base = Cluster + + @staticmethod + def internal_attrs(): + internal_attrs = ['/api_address', '/node_addresses', + '/master_addresses', '/stack_id', + '/ca_cert_ref', '/magnum_cert_ref', + '/trust_id', '/trustee_user_name', + '/trustee_password', '/trustee_user_id'] + return types.JsonPatchType.internal_attrs() + internal_attrs + + class ClusterCollection(collection.Collection): """API representation of a collection of clusters.""" diff --git a/magnum/api/controllers/v1/cluster_template.py b/magnum/api/controllers/v1/cluster_template.py index e80ab81c81..b2b1cf1547 100644 --- a/magnum/api/controllers/v1/cluster_template.py +++ b/magnum/api/controllers/v1/cluster_template.py @@ -33,15 +33,6 @@ from magnum import objects from magnum.objects import fields -class ClusterTemplatePatchType(types.JsonPatchType): - - @staticmethod - def mandatory_attrs(): - return ['/image_id', '/keypair_id', '/external_network_id', '/coe', - '/tls_disabled', '/public', '/registry_enabled', - '/server_type', '/cluster_distro', '/network_driver'] - - class ClusterTemplate(base.APIBase): """API representation of a clustertemplate. @@ -209,6 +200,14 @@ class ClusterTemplate(base.APIBase): return cls._convert_with_links(sample, 'http://localhost:9511') +class ClusterTemplatePatchType(types.JsonPatchType): + _api_base = ClusterTemplate + _extra_non_removable_attrs = {'/network_driver', '/external_network_id', + '/tls_disabled', '/public', '/server_type', + '/coe', '/registry_enabled', + '/cluster_distro'} + + class ClusterTemplateCollection(collection.Collection): """API representation of a collection of clustertemplates.""" diff --git a/magnum/api/controllers/v1/types.py b/magnum/api/controllers/v1/types.py index 00a8d9ebac..9d5389d0ce 100644 --- a/magnum/api/controllers/v1/types.py +++ b/magnum/api/controllers/v1/types.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import inspect + from oslo_utils import strutils from oslo_utils import uuidutils import wsme @@ -143,6 +145,17 @@ class JsonPatchType(wtypes.Base): mandatory=True) value = MultiType(wtypes.text, int) + # The class of the objects being patched. Override this in subclasses. + # Should probably be a subclass of magnum.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. @@ -154,15 +167,24 @@ class JsonPatchType(wtypes.Base): return ['/created_at', '/id', '/links', '/updated_at', '/uuid', '/project_id', '/user_id'] - @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(self): + """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 self._non_removable_attrs is None: + self._non_removable_attrs = self._extra_non_removable_attrs.copy() + if self._api_base: + fields = inspect.getmembers(self._api_base, + lambda a: not inspect.isroutine(a)) + for name, field in fields: + if getattr(field, 'mandatory', False): + self._non_removable_attrs.add('/%s' % name) + return self._non_removable_attrs @staticmethod def validate(patch): @@ -170,7 +192,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/magnum/tests/unit/api/controllers/v1/test_bay.py b/magnum/tests/unit/api/controllers/v1/test_bay.py index 31b28559e8..d25b991945 100644 --- a/magnum/tests/unit/api/controllers/v1/test_bay.py +++ b/magnum/tests/unit/api/controllers/v1/test_bay.py @@ -384,21 +384,15 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual(self.bay.name, response['name']) self.assertEqual(self.bay.master_count, response['master_count']) - def test_remove_uuid(self): - response = self.patch_json('/bays/%s' % self.bay.uuid, - [{'path': '/uuid', 'op': 'remove'}], - expect_errors=True) - self.assertEqual(400, response.status_int) - self.assertEqual('application/json', response.content_type) - self.assertTrue(response.json['errors']) - - def test_remove_baymodel_id(self): - response = self.patch_json('/bays/%s' % self.bay.uuid, - [{'path': '/baymodel_id', 'op': 'remove'}], - expect_errors=True) - self.assertEqual(400, response.status_int) - self.assertEqual('application/json', response.content_type) - self.assertTrue(response.json['errors']) + def test_remove_mandatory_property_fail(self): + mandatory_properties = ('/uuid', '/baymodel_id') + for p in mandatory_properties: + response = self.patch_json('/bays/%s' % self.bay.uuid, + [{'path': p, 'op': 'remove'}], + expect_errors=True) + self.assertEqual(400, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['errors']) def test_remove_non_existent_property(self): response = self.patch_json( diff --git a/magnum/tests/unit/api/controllers/v1/test_baymodel.py b/magnum/tests/unit/api/controllers/v1/test_baymodel.py index caa0292668..649f2ea5bf 100644 --- a/magnum/tests/unit/api/controllers/v1/test_baymodel.py +++ b/magnum/tests/unit/api/controllers/v1/test_baymodel.py @@ -368,39 +368,37 @@ class TestPatch(api_base.FunctionalTest): self.assertTrue(response.json['errors']) def test_remove_singular(self): - baymodel = obj_utils.create_test_baymodel( - self.context, - uuid=uuidutils.generate_uuid()) - response = self.get_json('/baymodels/%s' % baymodel.uuid) + response = self.get_json('/baymodels/%s' % self.baymodel.uuid) self.assertIsNotNone(response['dns_nameserver']) - response = self.patch_json('/baymodels/%s' % baymodel.uuid, + response = self.patch_json('/baymodels/%s' % self.baymodel.uuid, [{'path': '/dns_nameserver', 'op': 'remove'}]) self.assertEqual('application/json', response.content_type) self.assertEqual(200, response.status_code) - response = self.get_json('/baymodels/%s' % baymodel.uuid) + response = self.get_json('/baymodels/%s' % self.baymodel.uuid) self.assertIsNone(response['dns_nameserver']) # Assert nothing else was changed - self.assertEqual(baymodel.uuid, response['uuid']) - self.assertEqual(baymodel.name, response['name']) - self.assertEqual(baymodel.apiserver_port, response['apiserver_port']) - self.assertEqual(baymodel.image_id, + self.assertEqual(self.baymodel.uuid, response['uuid']) + self.assertEqual(self.baymodel.name, response['name']) + self.assertEqual(self.baymodel.apiserver_port, + response['apiserver_port']) + self.assertEqual(self.baymodel.image_id, response['image_id']) - self.assertEqual(baymodel.fixed_network, + self.assertEqual(self.baymodel.fixed_network, response['fixed_network']) - self.assertEqual(baymodel.network_driver, + self.assertEqual(self.baymodel.network_driver, response['network_driver']) - self.assertEqual(baymodel.volume_driver, + self.assertEqual(self.baymodel.volume_driver, response['volume_driver']) - self.assertEqual(baymodel.docker_volume_size, + self.assertEqual(self.baymodel.docker_volume_size, response['docker_volume_size']) - self.assertEqual(baymodel.coe, response['coe']) - self.assertEqual(baymodel.http_proxy, response['http_proxy']) - self.assertEqual(baymodel.https_proxy, response['https_proxy']) - self.assertEqual(baymodel.no_proxy, response['no_proxy']) - self.assertEqual(baymodel.labels, response['labels']) + self.assertEqual(self.baymodel.coe, response['coe']) + self.assertEqual(self.baymodel.http_proxy, response['http_proxy']) + self.assertEqual(self.baymodel.https_proxy, response['https_proxy']) + self.assertEqual(self.baymodel.no_proxy, response['no_proxy']) + self.assertEqual(self.baymodel.labels, response['labels']) def test_remove_non_existent_property_fail(self): response = self.patch_json('/baymodels/%s' % self.baymodel.uuid, @@ -411,10 +409,10 @@ class TestPatch(api_base.FunctionalTest): self.assertTrue(response.json['errors']) def test_remove_mandatory_property_fail(self): - mandatory_properties = ('/image_id', '/keypair_id', - '/external_network_id', '/coe', + mandatory_properties = ('/image_id', '/keypair_id', '/coe', + '/external_network_id', '/server_type', '/tls_disabled', '/public', - '/registry_enabled', '/server_type', + '/registry_enabled', '/cluster_distro', '/network_driver') for p in mandatory_properties: response = self.patch_json('/baymodels/%s' % self.baymodel.uuid, diff --git a/magnum/tests/unit/api/controllers/v1/test_cluster.py b/magnum/tests/unit/api/controllers/v1/test_cluster.py index 8cdb0fee6d..0d281c5398 100644 --- a/magnum/tests/unit/api/controllers/v1/test_cluster.py +++ b/magnum/tests/unit/api/controllers/v1/test_cluster.py @@ -391,22 +391,15 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual(self.cluster_obj.master_count, response['master_count']) - def test_remove_uuid(self): - response = self.patch_json('/clusters/%s' % self.cluster_obj.uuid, - [{'path': '/uuid', 'op': 'remove'}], - expect_errors=True) - self.assertEqual(400, response.status_int) - self.assertEqual('application/json', response.content_type) - self.assertTrue(response.json['errors']) - - def test_remove_cluster_template_id(self): - response = self.patch_json('/clusters/%s' % self.cluster_obj.uuid, - [{'path': '/cluster_template_id', - 'op': 'remove'}], - expect_errors=True) - self.assertEqual(400, response.status_int) - self.assertEqual('application/json', response.content_type) - self.assertTrue(response.json['errors']) + def test_remove_mandatory_property_fail(self): + mandatory_properties = ('/uuid', '/cluster_template_id') + for p in mandatory_properties: + response = self.patch_json('/clusters/%s' % self.cluster_obj.uuid, + [{'path': p, 'op': 'remove'}], + expect_errors=True) + self.assertEqual(400, response.status_int) + self.assertEqual('application/json', response.content_type) + self.assertTrue(response.json['errors']) def test_remove_non_existent_property(self): response = self.patch_json( diff --git a/magnum/tests/unit/api/controllers/v1/test_cluster_template.py b/magnum/tests/unit/api/controllers/v1/test_cluster_template.py index 85ffb822df..e578b68919 100644 --- a/magnum/tests/unit/api/controllers/v1/test_cluster_template.py +++ b/magnum/tests/unit/api/controllers/v1/test_cluster_template.py @@ -394,43 +394,42 @@ class TestPatch(api_base.FunctionalTest): self.assertTrue(response.json['errors']) def test_remove_singular(self): - cluster_template = obj_utils.create_test_cluster_template( - self.context, - uuid=uuidutils.generate_uuid()) response = self.get_json('/clustertemplates/%s' % - cluster_template.uuid) + self.cluster_template.uuid) self.assertIsNotNone(response['dns_nameserver']) response = self.patch_json('/clustertemplates/%s' % - cluster_template.uuid, + self.cluster_template.uuid, [{'path': '/dns_nameserver', 'op': 'remove'}]) self.assertEqual('application/json', response.content_type) self.assertEqual(200, response.status_code) response = self.get_json('/clustertemplates/%s' % - cluster_template.uuid) + self.cluster_template.uuid) self.assertIsNone(response['dns_nameserver']) # Assert nothing else was changed - self.assertEqual(cluster_template.uuid, response['uuid']) - self.assertEqual(cluster_template.name, response['name']) - self.assertEqual(cluster_template.apiserver_port, + self.assertEqual(self.cluster_template.uuid, response['uuid']) + self.assertEqual(self.cluster_template.name, response['name']) + self.assertEqual(self.cluster_template.apiserver_port, response['apiserver_port']) - self.assertEqual(cluster_template.image_id, + self.assertEqual(self.cluster_template.image_id, response['image_id']) - self.assertEqual(cluster_template.fixed_network, + self.assertEqual(self.cluster_template.fixed_network, response['fixed_network']) - self.assertEqual(cluster_template.network_driver, + self.assertEqual(self.cluster_template.network_driver, response['network_driver']) - self.assertEqual(cluster_template.volume_driver, + self.assertEqual(self.cluster_template.volume_driver, response['volume_driver']) - self.assertEqual(cluster_template.docker_volume_size, + self.assertEqual(self.cluster_template.docker_volume_size, response['docker_volume_size']) - self.assertEqual(cluster_template.coe, response['coe']) - self.assertEqual(cluster_template.http_proxy, response['http_proxy']) - self.assertEqual(cluster_template.https_proxy, response['https_proxy']) - self.assertEqual(cluster_template.no_proxy, response['no_proxy']) - self.assertEqual(cluster_template.labels, response['labels']) + self.assertEqual(self.cluster_template.coe, response['coe']) + self.assertEqual(self.cluster_template.http_proxy, + response['http_proxy']) + self.assertEqual(self.cluster_template.https_proxy, + response['https_proxy']) + self.assertEqual(self.cluster_template.no_proxy, response['no_proxy']) + self.assertEqual(self.cluster_template.labels, response['labels']) def test_remove_non_existent_property_fail(self): response = self.patch_json('/clustertemplates/%s' % @@ -443,10 +442,10 @@ class TestPatch(api_base.FunctionalTest): self.assertTrue(response.json['errors']) def test_remove_mandatory_property_fail(self): - mandatory_properties = ('/image_id', '/keypair_id', - '/external_network_id', '/coe', + mandatory_properties = ('/image_id', '/keypair_id', '/coe', + '/external_network_id', '/server_type', '/tls_disabled', '/public', - '/registry_enabled', '/server_type', + '/registry_enabled', '/cluster_distro', '/network_driver') for p in mandatory_properties: response = self.patch_json('/clustertemplates/%s' % diff --git a/magnum/tests/unit/api/controllers/v1/test_types.py b/magnum/tests/unit/api/controllers/v1/test_types.py index 70d3bc33a4..3cfadbd261 100644 --- a/magnum/tests/unit/api/controllers/v1/test_types.py +++ b/magnum/tests/unit/api/controllers/v1/test_types.py @@ -18,6 +18,7 @@ import mock import six import webtest import wsme +from wsme import types as wtypes from magnum.api.controllers.v1 import types from magnum.common import exception @@ -61,12 +62,15 @@ class TestUuidType(base.FunctionalTest): types.UuidType.validate, 'invalid-uuid') +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(): @@ -108,17 +112,33 @@ class TestJsonPatchType(base.FunctionalTest): ret = self._patch_json(patch, True) self.assertEqual(400, ret.status_int) - def test_mandatory_attr(self): - patch = [{'op': 'replace', 'path': '/mandatory', 'value': 'foo'}] + def test_cannot_remove_internal_attr(self): + patch = [{'path': '/internal', 'op': 'remove'}] + ret = self._patch_json(patch, True) + self.assertEqual(400, ret.status_int) + + def test_cannot_add_internal_attr(self): + patch = [{'path': '/internal', 'op': 'add', 'value': 'foo'}] + ret = self._patch_json(patch, True) + self.assertEqual(400, ret.status_int) + + def test_update_mandatory_attr(self): + patch = [{'path': '/mandatory', 'op': 'replace', 'value': 'foo'}] ret = self._patch_json(patch, False) self.assertEqual(200, ret.status_int) self.assertEqual(patch, ret.json) def test_cannot_remove_mandatory_attr(self): - patch = [{'op': 'remove', 'path': '/mandatory'}] + patch = [{'path': '/mandatory', 'op': 'remove'}] ret = self._patch_json(patch, True) self.assertEqual(400, ret.status_int) + def test_cannot_remove_extra_non_removable_attr(self): + patch = [{'path': '/non_removable', 'op': 'remove'}] + ret = self._patch_json(patch, True) + self.assertEqual(400, 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)