openstack/swift
Code Issues Proposed changes

Simplify policy-name validation slightly

Browse Source 
_validate_policy_name always either returns True or raises an exception.
Simplify it to just being a callable that may raise an exception.

Also, move the check for blank/None names into _validate_policy_name, so
it will be applied in more cases.

Change-Id: I7832a0c9c895cd75ba4c6d0e8b5568a3c8a0ea25
This commit is contained in:
Tim Burke 2016-02-11 16:00:38 -08:00
parent 45a7a15674
commit 7be55acf1b
2 changed files with 22 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
swift/common/storage_policy.py
View File

@ -170,16 +170,13 @@ class BaseStoragePolicy(object):
if self.idx < 0:
raise PolicyError('Invalid index', idx)
self.alias_list = []
if not name or not self._validate_policy_name(name):
raise PolicyError('Invalid name %r' % name, idx)
self.alias_list.append(name)
self.add_name(name)
if aliases:
names_list = list_from_csv(aliases)
for alias in names_list:
if alias == name:
continue
self._validate_policy_name(alias)
self.alias_list.append(alias)
self.add_name(alias)
self.is_deprecated = config_true_value(is_deprecated)
self.is_default = config_true_value(is_default)
if self.policy_type not in BaseStoragePolicy.policy_type_to_policy_cls:
@ -288,14 +285,16 @@ class BaseStoragePolicy(object):
to check policy names before setting them.
:param name: a name string for a single policy name.
:returns: true if the name is valid.
:raises: PolicyError if the policy name is invalid.
"""
if not name:
raise PolicyError('Invalid name %r' % name, self.idx)
# this is defensively restrictive, but could be expanded in the future
if not all(c in VALID_CHARS for c in name):
raise PolicyError('Names are used as HTTP headers, and can not '
'reliably contain any characters not in %r. '
'Invalid name %r' % (VALID_CHARS, name))
msg = 'Names are used as HTTP headers, and can not ' \
'reliably contain any characters not in %r. ' \
'Invalid name %r' % (VALID_CHARS, name)
raise PolicyError(msg, self.idx)
if name.upper() == LEGACY_POLICY_NAME.upper() and self.idx != 0:
msg = 'The name %s is reserved for policy index 0. ' \
'Invalid name %r' % (LEGACY_POLICY_NAME, name)
@ -305,8 +304,6 @@ class BaseStoragePolicy(object):
msg = 'The name %s is already assigned to this policy.' % name
raise PolicyError(msg, self.idx)
return True
def add_name(self, name):
"""
Adds an alias name to the storage policy. Shouldn't be called
@ -316,7 +313,7 @@ class BaseStoragePolicy(object):
:param name: a new alias for the storage policy
"""
if self._validate_policy_name(name):
self._validate_policy_name(name)
self.alias_list.append(name)
def remove_name(self, name):

12
test/unit/common/test_storage_policy.py
View File

@ -374,6 +374,15 @@ class TestStoragePolicies(unittest.TestCase):
# but only because automated testing requires it.
policies = parse_storage_policies(name_repeat_conf)
extra_commas_conf = self._conf("""
[storage-policy:0]
name = one
aliases = ,,one, ,
default = yes
""")
# Extra blank entries should be silently dropped
policies = parse_storage_policies(extra_commas_conf)
bad_conf = self._conf("""
[storage-policy:0]
name = one
@ -499,6 +508,9 @@ class TestStoragePolicies(unittest.TestCase):
self.assertRaisesWithMessage(PolicyError, 'Invalid name',
policies.add_policy_alias, 2, 'double\n')
self.assertRaisesWithMessage(PolicyError, 'Invalid name',
policies.add_policy_alias, 2, '')
# try to add existing name
self.assertRaisesWithMessage(PolicyError, 'Duplicate name',
policies.add_policy_alias, 2, 'two')