From ba5384f8656cf4499bc4f45f915cecfe0cdfcfa3 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 27 Jan 2017 19:58:30 +0200 Subject: [PATCH] Fix creation of share group types using share type names Before it was possible to create share group types mapping them to share types using only share type IDs and when we were providing its names we were getting DB error and HTTP 500 as a response. Fix it by properly looking for share type by both its unique values - ID and name. Also, raise proper 404 error when nothing is found. Add functional tests covering this case. Change-Id: I216f935383a87f6d679c431bc46cfa8977a6d8ab Depends-On: Ic555d241f98d0fa027897c69a7115d1be88f6c96 Closes-Bug: #1659625 --- manila/api/v2/share_group_types.py | 2 ++ manila/db/api.py | 5 +++ manila/db/sqlalchemy/api.py | 25 ++++++++++++-- manila/exception.py | 4 +++ manila/tests/api/v2/test_share_group_types.py | 13 +++++++ manila/tests/db/sqlalchemy/test_api.py | 34 +++++++++++++++++++ manila/tests/test_exception.py | 7 ++++ .../tests/api/admin/test_share_group_types.py | 12 ++++--- .../admin/test_share_group_types_negative.py | 11 +++++- 9 files changed, 105 insertions(+), 8 deletions(-) diff --git a/manila/api/v2/share_group_types.py b/manila/api/v2/share_group_types.py index ae2720d4a3..fe9114ca9b 100644 --- a/manila/api/v2/share_group_types.py +++ b/manila/api/v2/share_group_types.py @@ -138,6 +138,8 @@ class ShareGroupTypesController(wsgi.Controller): context, name) except exception.ShareGroupTypeExists as err: raise webob.exc.HTTPConflict(explanation=six.text_type(err)) + except exception.ShareTypeDoesNotExist as err: + raise webob.exc.HTTPNotFound(explanation=six.text_type(err)) except exception.NotFound: raise webob.exc.HTTPNotFound() return self._view_builder.show(req, share_group_type) diff --git a/manila/db/api.py b/manila/db/api.py index 385c88c834..570a02997e 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -894,6 +894,11 @@ def share_type_get_by_name(context, name): return IMPL.share_type_get_by_name(context, name) +def share_type_get_by_name_or_id(context, name_or_id): + """Get share type by name or ID and return None if not found.""" + return IMPL.share_type_get_by_name_or_id(context, name_or_id) + + def share_type_access_get_all(context, type_id): """Get all share type access of a share type.""" return IMPL.share_type_access_get_all(context, type_id) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index db7f36ccc0..bb942bfd95 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3537,10 +3537,24 @@ def _share_type_get_by_name(context, name, session=None): @require_context def share_type_get_by_name(context, name): """Return a dict describing specific share_type.""" - return _share_type_get_by_name(context, name) +@require_context +def share_type_get_by_name_or_id(context, name_or_id): + """Return a dict describing specific share_type using its name or ID. + + :returns: ShareType object or None if not found + """ + try: + return _share_type_get(context, name_or_id) + except exception.ShareTypeNotFound: + try: + return _share_type_get_by_name(context, name_or_id) + except exception.ShareTypeNotFoundByName: + return None + + @require_admin_context def share_type_destroy(context, id): session = get_session() @@ -4201,10 +4215,13 @@ def share_group_type_create(context, values, projects=None): values['group_specs'] = _metadata_refs( values.get('group_specs'), models.ShareGroupTypeSpecs) mappings = [] - for item in values.get('share_types') or []: + for item in values.get('share_types', []): + share_type = share_type_get_by_name_or_id(context, item) + if not share_type: + raise exception.ShareTypeDoesNotExist(share_type=item) mapping = models.ShareGroupTypeShareTypeMapping() mapping['id'] = six.text_type(uuidutils.generate_uuid()) - mapping['share_type_id'] = item + mapping['share_type_id'] = share_type['id'] mapping['share_group_type_id'] = values['id'] mappings.append(mapping) @@ -4214,6 +4231,8 @@ def share_group_type_create(context, values, projects=None): share_group_type_ref.save(session=session) except db_exception.DBDuplicateEntry: raise exception.ShareGroupTypeExists(type_id=values['name']) + except exception.ShareTypeDoesNotExist: + raise except Exception as e: raise db_exception.DBError(e) diff --git a/manila/exception.py b/manila/exception.py index f2959a3b2c..d0b813b3e8 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -619,6 +619,10 @@ class ShareTypeExists(ManilaException): message = _("Share Type %(id)s already exists.") +class ShareTypeDoesNotExist(NotFound): + message = _("Share Type %(share_type)s does not exist.") + + class ShareGroupTypeExists(ManilaException): message = _("Share group type %(type_id)s already exists.") diff --git a/manila/tests/api/v2/test_share_group_types.py b/manila/tests/api/v2/test_share_group_types.py index 319289e5e3..abecc498d5 100644 --- a/manila/tests/api/v2/test_share_group_types.py +++ b/manila/tests/api/v2/test_share_group_types.py @@ -390,6 +390,19 @@ class ShareGroupTypesAPITest(test.TestCase): webob.exc.HTTPBadRequest, self.controller._create, req, fake_body) + def test_create_provided_share_type_does_not_exist(self): + req = fake_request('/v2/fake/share-group-types', admin=True) + fake_body = { + 'share_group_type': { + 'name': GROUP_TYPE_1['name'], + 'share_types': SHARE_TYPE_ID + '_does_not_exist', + } + } + + self.assertRaises( + webob.exc.HTTPNotFound, + self.controller._create, req, fake_body) + class ShareGroupTypeAccessTest(test.TestCase): diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index d989794b18..e783793138 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -2576,3 +2576,37 @@ class PurgeDeletedTest(test.TestCase): type_row = db_api.model_query(self.context, models.ShareTypes).count() self.assertEqual(0, s_row + type_row) + + +class ShareTypeAPITestCase(test.TestCase): + + def setUp(self): + super(self.__class__, self).setUp() + self.ctxt = context.RequestContext( + user_id='user_id', project_id='project_id', is_admin=True) + + def test_share_type_get_by_name_or_id_found_by_id(self): + share_type = db_utils.create_share_type() + + result = db_api.share_type_get_by_name_or_id( + self.ctxt, share_type['id']) + + self.assertIsNotNone(result) + self.assertEqual(share_type['id'], result['id']) + + def test_share_type_get_by_name_or_id_found_by_name(self): + name = uuidutils.generate_uuid() + db_utils.create_share_type(name=name) + + result = db_api.share_type_get_by_name_or_id(self.ctxt, name) + + self.assertIsNotNone(result) + self.assertEqual(name, result['name']) + self.assertNotEqual(name, result['id']) + + def test_share_type_get_by_name_or_id_when_does_not_exist(self): + fake_id = uuidutils.generate_uuid() + + result = db_api.share_type_get_by_name_or_id(self.ctxt, fake_id) + + self.assertIsNone(result) diff --git a/manila/tests/test_exception.py b/manila/tests/test_exception.py index bc1f9c6f6b..d882d6f3b3 100644 --- a/manila/tests/test_exception.py +++ b/manila/tests/test_exception.py @@ -469,6 +469,13 @@ class ManilaExceptionResponseCode404(test.TestCase): self.assertEqual(404, e.code) self.assertIn(share_type_name, e.msg) + def test_share_type_does_not_exist(self): + # verify response code for exception.ShareTypeDoesNotExist + share_type = "fake_share_type_1234" + e = exception.ShareTypeDoesNotExist(share_type=share_type) + self.assertEqual(404, e.code) + self.assertIn(share_type, e.msg) + def test_share_type_extra_specs_not_found(self): # verify response code for exception.ShareTypeExtraSpecsNotFound share_type_id = "fake_share_type_id" diff --git a/manila_tempest_tests/tests/api/admin/test_share_group_types.py b/manila_tempest_tests/tests/api/admin/test_share_group_types.py index a6bda73401..532e773ff0 100644 --- a/manila_tempest_tests/tests/api/admin/test_share_group_types.py +++ b/manila_tempest_tests/tests/api/admin/test_share_group_types.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import ddt from tempest import config from tempest.lib.common.utils import data_utils import testtools @@ -27,6 +28,7 @@ CONF = config.CONF @testtools.skipUnless( CONF.share.run_share_group_tests, 'Share Group tests disabled.') @base.skip_if_microversion_lt(constants.MIN_SHARE_GROUP_MICROVERSION) +@ddt.ddt class ShareGroupTypesTest(base.BaseSharesAdminTest): @classmethod @@ -44,13 +46,14 @@ class ShareGroupTypesTest(base.BaseSharesAdminTest): cls.share_type2 = share_type['share_type'] @tc.attr(base.TAG_POSITIVE, base.TAG_API) - def test_create_get_delete_share_group_type_min(self): + @ddt.data('id', 'name') + def test_create_get_delete_share_group_type_min(self, st_key): name = data_utils.rand_name("tempest-manila") # Create share group type sg_type_c = self.create_share_group_type( name=name, - share_types=self.share_type['id'], + share_types=self.share_type[st_key], cleanup_in_class=False, version=constants.MIN_SHARE_GROUP_MICROVERSION) @@ -76,12 +79,13 @@ class ShareGroupTypesTest(base.BaseSharesAdminTest): share_group_type_id=sg_type_r['id']) @tc.attr(base.TAG_POSITIVE, base.TAG_API) - def test_create_share_group_type_multiple_share_types_min(self): + @ddt.data('id', 'name') + def test_create_share_group_type_multiple_share_types_min(self, st_key): name = data_utils.rand_name("tempest-manila") sg_type = self.create_share_group_type( name=name, - share_types=[self.share_type['id'], self.share_type2['id']], + share_types=[self.share_type[st_key], self.share_type2[st_key]], cleanup_in_class=False, version=constants.MIN_SHARE_GROUP_MICROVERSION) diff --git a/manila_tempest_tests/tests/api/admin/test_share_group_types_negative.py b/manila_tempest_tests/tests/api/admin/test_share_group_types_negative.py index e7f682402d..18e8cdde2f 100644 --- a/manila_tempest_tests/tests/api/admin/test_share_group_types_negative.py +++ b/manila_tempest_tests/tests/api/admin/test_share_group_types_negative.py @@ -40,10 +40,19 @@ class ShareGroupTypesAdminNegativeTest(base.BaseSharesMixedTest): client=cls.admin_shares_v2_client) @tc.attr(base.TAG_NEGATIVE, base.TAG_API) - def test_create_share_ggroup_with_nonexistent_share_type(self): + def test_create_share_group_type_without_name(self): self.assertRaises( lib_exc.BadRequest, self.admin_shares_v2_client.create_share_group_type, + name=None, + share_types=data_utils.rand_name("fake")) + + @tc.attr(base.TAG_NEGATIVE, base.TAG_API) + def test_create_share_group_type_with_nonexistent_share_type(self): + self.assertRaises( + lib_exc.NotFound, + self.admin_shares_v2_client.create_share_group_type, + name=data_utils.rand_name("sgt_name_should_have_not_been_created"), share_types=data_utils.rand_name("fake")) @tc.attr(base.TAG_NEGATIVE, base.TAG_API)