openstack
/
magnum
Code Issues Proposed changes

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
This commit is contained in:
yatin karel 2016-07-31 15:34:01 +05:30 committed by yatin
parent 43ea1ab6f2
commit d82d8b2420
10 changed files with 158 additions and 139 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
magnum/api/controllers/v1/bay.py
View File

@ -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."""

17
magnum/api/controllers/v1/baymodel.py
View File

@ -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."""

28
magnum/api/controllers/v1/cluster.py
View File

@ -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."""

17
magnum/api/controllers/v1/cluster_template.py
View File

@ -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."""

38
magnum/api/controllers/v1/types.py
View File

@ -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)

24
magnum/tests/unit/api/controllers/v1/test_bay.py
View File

@ -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(

42
magnum/tests/unit/api/controllers/v1/test_baymodel.py
View File

@ -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,

25
magnum/tests/unit/api/controllers/v1/test_cluster.py
View File

@ -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(

43
magnum/tests/unit/api/controllers/v1/test_cluster_template.py
View File

@ -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' %

34
magnum/tests/unit/api/controllers/v1/test_types.py
View File

@ -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)