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

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

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)

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)
 

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

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

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)

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"

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)
 

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)