openstack/cinder
Code Issues Proposed changes

Merge "Fix 500 error if boolean is_public is passed as string"

Browse Source
This commit is contained in:
Jenkins 2017-03-13 19:26:01 +00:00 committed by Gerrit Code Review
commit 0453694d0a
6 changed files with 111 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
cinder/api/contrib/types_manage.py
View File

@ -19,8 +19,6 @@ import six
from six.moves import http_client
import webob
from oslo_utils import strutils
from cinder.api import extensions
from cinder.api.openstack import wsgi
from cinder.api.views import types as views_types
@ -64,7 +62,8 @@ class VolumeTypesManageController(wsgi.Controller):
description = vol_type.get('description')
specs = vol_type.get('extra_specs', {})
utils.validate_dictionary_string_length(specs)
is_public = vol_type.get('os-volume-type-access:is_public', True)
is_public = utils.get_bool_param('os-volume-type-access:is_public',
vol_type, True)
if name is None or len(name.strip()) == 0:
msg = _("Volume type name can not be empty.")
@ -77,11 +76,6 @@ class VolumeTypesManageController(wsgi.Controller):
utils.check_string_length(description, 'Type description',
min_length=0, max_length=255)
if not strutils.is_valid_boolstr(is_public):
msg = _("Invalid value '%s' for is_public. Accepted values: "
"True or False.") % is_public
raise webob.exc.HTTPBadRequest(explanation=msg)
try:
volume_types.create(context,
name,
@ -129,10 +123,8 @@ class VolumeTypesManageController(wsgi.Controller):
"a combination thereof.")
raise webob.exc.HTTPBadRequest(explanation=msg)
if is_public is not None and not strutils.is_valid_boolstr(is_public):
msg = _("Invalid value '%s' for is_public. Accepted values: "
"True or False.") % is_public
raise webob.exc.HTTPBadRequest(explanation=msg)
if is_public is not None:
is_public = utils.get_bool_param('is_public', vol_type)
if name:
utils.check_string_length(name, 'Type name',

8
cinder/api/v3/group_types.py
View File

@ -68,7 +68,7 @@ class GroupTypesController(wsgi.Controller):
name = grp_type.get('name', None)
description = grp_type.get('description')
specs = grp_type.get('group_specs', {})
is_public = grp_type.get('is_public', True)
is_public = utils.get_bool_param('is_public', grp_type, True)
if name is None or len(name.strip()) == 0:
msg = _("Group type name can not be empty.")
@ -127,10 +127,8 @@ class GroupTypesController(wsgi.Controller):
"a combination thereof.")
raise webob.exc.HTTPBadRequest(explanation=msg)
if is_public is not None and not strutils.is_valid_boolstr(is_public):
msg = _("Invalid value '%s' for is_public. Accepted values: "
"True or False.") % is_public
raise webob.exc.HTTPBadRequest(explanation=msg)
if is_public is not None:
is_public = utils.get_bool_param('is_public', grp_type)
if name:
utils.check_string_length(name, 'Type name',

49
cinder/tests/unit/api/contrib/test_types_manage.py
View File

@ -18,6 +18,7 @@ import six
import webob
import ddt
from oslo_utils import strutils
from cinder.api.contrib import types_manage
from cinder import context
@ -284,9 +285,31 @@ class VolumeTypesManageApiTest(test.TestCase):
"description": "test description",
"extra_specs": {"key1": "value1"}}}
req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
self.assertRaises(webob.exc.HTTPBadRequest,
self.assertRaises(exception.InvalidParameterValue,
self.controller._create, req, body)
@ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
'y', 'yes')
@mock.patch.object(volume_types, "get_volume_type_by_name")
@mock.patch.object(volume_types, "create")
@mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
@mock.patch("cinder.api.views.types.ViewBuilder.show")
def test_create_type_with_valid_is_public_in_string(
self, is_public, mock_show, mock_cache_resource,
mock_create, mock_get):
boolean_is_public = strutils.bool_from_string(is_public)
ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True)
body = {"volume_type": {"name": "vol_type_1",
"os-volume-type-access:is_public":
is_public,
"extra_specs": {"key1": "value1"}}}
req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
req.environ['cinder.context'] = ctxt
self.controller._create(req, body)
mock_create.assert_called_once_with(
ctxt, 'vol_type_1', {'key1': 'value1'},
boolean_is_public, description=None)
def _create_volume_type_bad_body(self, body):
req = fakes.HTTPRequest.blank('/v2/%s/types' % fake.PROJECT_ID)
req.method = 'POST'
@ -374,6 +397,28 @@ class VolumeTypesManageApiTest(test.TestCase):
(DEFAULT_VOLUME_TYPE, DEFAULT_VOLUME_TYPE),
'is_public': False})
@ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
'y', 'yes')
@mock.patch('cinder.volume.volume_types.update')
@mock.patch('cinder.volume.volume_types.get_volume_type')
@mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
@mock.patch("cinder.api.views.types.ViewBuilder.show")
def test_update_with_valid_is_public_in_string(
self, is_public, mock_show, mock_cache_resource,
mock_get, mock_update):
body = {"volume_type": {"is_public": is_public}}
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
boolean_is_public = strutils.bool_from_string(is_public)
self.controller._update(req, DEFAULT_VOLUME_TYPE, body)
mock_update.assert_called_once_with(
ctxt, DEFAULT_VOLUME_TYPE, None, None,
is_public=boolean_is_public)
@mock.patch('cinder.volume.volume_types.update')
@mock.patch('cinder.volume.volume_types.get_volume_type')
def test_update_type_with_description_having_length_zero(
@ -550,7 +595,7 @@ class VolumeTypesManageApiTest(test.TestCase):
fake.PROJECT_ID, DEFAULT_VOLUME_TYPE))
req.method = 'PUT'
self.assertRaises(webob.exc.HTTPBadRequest,
self.assertRaises(exception.InvalidParameterValue,
self.controller._update, req,
DEFAULT_VOLUME_TYPE, body)

45
cinder/tests/unit/api/v3/test_group_types.py
View File

@ -15,7 +15,9 @@
import uuid
import ddt
import mock
from oslo_utils import strutils
from oslo_utils import timeutils
import six
import webob
@ -86,6 +88,7 @@ def return_group_types_get_default_not_found():
return {}
@ddt.ddt
class GroupTypesApiTest(test.TestCase):
def _create_group_type(self, group_type_name, group_specs=None,
@ -112,6 +115,27 @@ class GroupTypesApiTest(test.TestCase):
[fake.PROJECT_ID])
self.type_id0 = group_types.get_default_cgsnapshot_type()['id']
@ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
'y', 'yes')
@mock.patch.object(group_types, "get_group_type_by_name")
@mock.patch.object(group_types, "create")
@mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
@mock.patch("cinder.api.views.types.ViewBuilder.show")
def test_create_group_type_with_valid_is_public_in_string(
self, is_public, mock_show, mock_cache_resource,
mock_create, mock_get):
boolean_is_public = strutils.bool_from_string(is_public)
req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID,
version=GROUP_TYPE_MICRO_VERSION)
req.environ['cinder.context'] = self.ctxt
body = {"group_type": {"is_public": is_public, "name": "group_type1",
"description": None}}
self.controller.create(req, body)
mock_create.assert_called_once_with(
self.ctxt, 'group_type1', {},
boolean_is_public, description=None)
def test_group_types_index(self):
self.mock_object(group_types, 'get_all_group_types',
return_group_types_get_all_types)
@ -260,6 +284,27 @@ class GroupTypesApiTest(test.TestCase):
self.assertEqual(expect_result[2], res['group_types'][2]['id'])
self.assertEqual(expect_result[3], res['group_types'][3]['id'])
@ddt.data('0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on',
'y', 'yes')
@mock.patch.object(group_types, "get_group_type")
@mock.patch.object(group_types, "update")
@mock.patch("cinder.api.openstack.wsgi.Request.cache_resource")
@mock.patch("cinder.api.views.types.ViewBuilder.show")
def test_update_group_type_with_valid_is_public_in_string(
self, is_public, mock_show, mock_cache_resource,
mock_update, mock_get):
boolean_is_public = strutils.bool_from_string(is_public)
type_id = six.text_type(uuid.uuid4())
req = fakes.HTTPRequest.blank(
'/v3/%s/types/%s' % (fake.PROJECT_ID, type_id),
version=GROUP_TYPE_MICRO_VERSION)
req.environ['cinder.context'] = self.ctxt
body = {"group_type": {"is_public": is_public, "name": "group_type1"}}
self.controller.update(req, type_id, body)
mock_update.assert_called_once_with(
self.ctxt, type_id, 'group_type1', None,
is_public=boolean_is_public)
def test_group_types_show(self):
self.mock_object(group_types, 'get_group_type',
return_group_types_get_group_type)

8
cinder/utils.py
View File

@ -592,11 +592,11 @@ def _get_disk_of_partition(devpath, st=None):
return (devpath, st)
def get_bool_param(param_string, params):
param = params.get(param_string, False)
def get_bool_param(param_string, params, default=False):
param = params.get(param_string, default)
if not strutils.is_valid_boolstr(param):
msg = _('Value %(param)s for %(param_string)s is not a '
'boolean.') % {'param': param, 'param_string': param_string}
msg = _("Value '%(param)s' for '%(param_string)s' is not "
"a boolean.") % {'param': param, 'param_string': param_string}
raise exception.InvalidParameterValue(err=msg)
return strutils.bool_from_string(param, strict=True)

8
releasenotes/notes/bug-1670260-fix-boolean-is_public-d16e1957c0f09d65.yaml Normal file
View File

@ -0,0 +1,8 @@
---
fixes:
- |
Fixed issue where ``create`` and ``update`` api's of ``volume-type`` and
``group_type`` were returning 500 error if boolean 'is_public' value
passed in the form of string. Now user can pass following valid boolean
values to these api's:
'0', 'f', 'false', 'off', 'n', 'no', '1', 't', 'true', 'on', 'y', 'yes'