From 2b2ed02fe531313a6a98673534b97f53a9f00edc Mon Sep 17 00:00:00 2001 From: Neha Alhat Date: Sun, 14 Jan 2018 15:57:06 +0530 Subject: [PATCH] Fix combination of parameters for update APIs For group_type and volume_type update APIs, If user passes description or is_public parameter in the request body then name can be null. But after schema validation [1][2], it is failing with BadRequest. This patch allows users to pass 'name' as None if "description" or is_public" parameter is passed in the request body. [1]: https://review.openstack.org/#/c/520561/ [2]: https://review.openstack.org/#/c/519643/ Change-Id: Id5d5f5ed8b449c9f1059528de241417f231f90b5 Closes-Bug: #1742940 --- cinder/api/contrib/types_manage.py | 12 ++++++ cinder/api/schemas/group_types.py | 7 +--- cinder/api/schemas/volume_types.py | 7 +--- cinder/api/v3/group_types.py | 11 ++++++ .../unit/api/contrib/test_types_manage.py | 39 ++++++++++++++++++- cinder/tests/unit/api/v3/test_group_types.py | 37 ++++++++++++++++++ 6 files changed, 99 insertions(+), 14 deletions(-) diff --git a/cinder/api/contrib/types_manage.py b/cinder/api/contrib/types_manage.py index 4afbf0f34fa..d302ff9217f 100644 --- a/cinder/api/contrib/types_manage.py +++ b/cinder/api/contrib/types_manage.py @@ -99,6 +99,18 @@ class VolumeTypesManageController(wsgi.Controller): if is_public is not None: is_public = strutils.bool_from_string(is_public, strict=True) + # If name specified, name can not be empty. + if name and len(name.strip()) == 0: + msg = _("Volume type name can not be empty.") + raise webob.exc.HTTPBadRequest(explanation=msg) + + # Name, description and is_public can not be None. + # Specify one of them, or a combination thereof. + if name is None and description is None and is_public is None: + msg = _("Specify volume type name, description, is_public or " + "a combination thereof.") + raise webob.exc.HTTPBadRequest(explanation=msg) + try: volume_types.update(context, id, name, description, is_public=is_public) diff --git a/cinder/api/schemas/group_types.py b/cinder/api/schemas/group_types.py index f39a3a826c1..5878c341305 100644 --- a/cinder/api/schemas/group_types.py +++ b/cinder/api/schemas/group_types.py @@ -48,16 +48,11 @@ update = { 'group_type': { 'type': 'object', 'properties': { - 'name': parameter_types.name, + 'name': parameter_types.name_allow_zero_min_length, 'description': parameter_types.description, 'is_public': parameter_types.boolean, }, 'additionalProperties': False, - 'anyOf': [ - {'required': ['name']}, - {'required': ['description']}, - {'required': ['is_public']}, - ] }, }, 'required': ['group_type'], diff --git a/cinder/api/schemas/volume_types.py b/cinder/api/schemas/volume_types.py index e8f47adbfe9..4dfe7067af4 100644 --- a/cinder/api/schemas/volume_types.py +++ b/cinder/api/schemas/volume_types.py @@ -44,16 +44,11 @@ update = { 'volume_type': { 'type': 'object', 'properties': { - 'name': parameter_types.name, + 'name': parameter_types.name_allow_zero_min_length, 'description': parameter_types.description, 'is_public': parameter_types.boolean, }, 'additionalProperties': False, - 'anyOf': [ - {'required': ['name']}, - {'required': ['description']}, - {'required': ['is_public']} - ] }, }, 'required': ['volume_type'], diff --git a/cinder/api/v3/group_types.py b/cinder/api/v3/group_types.py index 02ba737f492..1f691d69364 100644 --- a/cinder/api/v3/group_types.py +++ b/cinder/api/v3/group_types.py @@ -102,6 +102,17 @@ class GroupTypesController(wsgi.Controller): if is_public is not None: is_public = strutils.bool_from_string(is_public, strict=True) + # If name specified, name can not be empty. + if name and len(name.strip()) == 0: + msg = _("Group type name can not be empty.") + raise webob.exc.HTTPBadRequest(explanation=msg) + + # Name, description and is_public can not be None. + # Specify one of them, or a combination thereof. + if name is None and description is None and is_public is None: + msg = _("Specify group type name, description or " + "a combination thereof.") + raise webob.exc.HTTPBadRequest(explanation=msg) try: group_types.update(context, id, name, description, is_public=is_public) diff --git a/cinder/tests/unit/api/contrib/test_types_manage.py b/cinder/tests/unit/api/contrib/test_types_manage.py index 80cabf4dfa7..aee6397cd5b 100644 --- a/cinder/tests/unit/api/contrib/test_types_manage.py +++ b/cinder/tests/unit/api/contrib/test_types_manage.py @@ -502,7 +502,7 @@ class VolumeTypesManageApiTest(test.TestCase): fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) req.method = 'PUT' - self.assertRaises(exception.ValidationError, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller._update, req, DEFAULT_VOLUME_TYPE, body=body) @@ -513,7 +513,7 @@ class VolumeTypesManageApiTest(test.TestCase): fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) req.method = 'PUT' - self.assertRaises(exception.ValidationError, + self.assertRaises(webob.exc.HTTPBadRequest, self.controller._update, req, DEFAULT_VOLUME_TYPE, body=body) @@ -699,3 +699,38 @@ class VolumeTypesManageApiTest(test.TestCase): if expected_results.get('is_public') is not None: self.assertEqual(expected_results['is_public'], results['volume_type']['is_public']) + + def test_update_with_name_null(self): + body = {"volume_type": {"name": None}} + req = fakes.HTTPRequest.blank('/v2/%s/types/%s' % ( + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + req.method = 'PUT' + + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._update, req, + DEFAULT_VOLUME_TYPE, body=body) + + @ddt.data({"volume_type": {"name": None, "description": "description"}}, + {"volume_type": {"name": None, "is_public": True}}, + {"volume_type": {"description": "description", + "is_public": True}}) + def test_update_volume_type(self, body): + req = fakes.HTTPRequest.blank('/v2/%s/types/%s' % ( + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + req.method = 'PUT' + ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True) + req.environ['cinder.context'] = ctxt + volume_type_1 = volume_types.create(ctxt, 'volume_type') + res = self.controller._update(req, volume_type_1.get('id'), body=body) + + expected_name = body['volume_type'].get('name') + if expected_name is not None: + self.assertEqual(expected_name, res['volume_type']['name']) + + expected_is_public = body['volume_type'].get('is_public') + if expected_is_public is not None: + self.assertEqual(expected_is_public, + res['volume_type']['is_public']) + + self.assertEqual(body['volume_type'].get('description'), + res['volume_type']['description']) diff --git a/cinder/tests/unit/api/v3/test_group_types.py b/cinder/tests/unit/api/v3/test_group_types.py index cb9bfbf8acf..9f6d7d25c25 100644 --- a/cinder/tests/unit/api/v3/test_group_types.py +++ b/cinder/tests/unit/api/v3/test_group_types.py @@ -357,6 +357,43 @@ class GroupTypesApiTest(test.TestCase): self.ctxt, type_id, 'group_type1', None, is_public=boolean_is_public) + def test_update_group_type_with_name_null(self): + req = fakes.HTTPRequest.blank( + '/v3/%s/types/%s' % (fake.PROJECT_ID, fake.GROUP_TYPE_ID), + version=mv.GROUP_TYPE) + req.environ['cinder.context'] = self.ctxt + body = {"group_type": {"name": None}} + self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, + req, fake.GROUP_TYPE_ID, body=body) + + @ddt.data({"group_type": {"name": None, + "description": "description"}}, + {"group_type": {"name": "test", + "is_public": True}}, + {"group_type": {"description": None, + "is_public": True}}) + def test_update_group_type(self, body): + req = fakes.HTTPRequest.blank( + '/v3/%s/types/%s' % (fake.PROJECT_ID, fake.GROUP_TYPE_ID), + version=mv.GROUP_TYPE) + + group_type_1 = group_types.create(self.ctxt, 'group_type') + + req.environ['cinder.context'] = self.ctxt + res = self.controller.update(req, group_type_1.get('id'), body=body) + + expected_name = body['group_type'].get('name') + if expected_name is not None: + self.assertEqual(expected_name, res['group_type']['name']) + + expected_is_public = body['group_type'].get('is_public') + if expected_is_public is not None: + self.assertEqual(expected_is_public, + res['group_type']['is_public']) + + self.assertEqual(body['group_type'].get('description'), + res['group_type']['description']) + def test_group_types_show(self): self.mock_object(group_types, 'get_group_type', return_group_types_get_group_type)