openstack/swift
Code Issues Proposed changes

Uses None instead of mutables for function param defaults

Browse Source 
As seen on #1174809, changes use of mutable types as default
arguments and defaults them within the method. Otherwise, those
defaults can be unexpectedly persisted with the function between
invocations and erupt into mass hysteria on the streets.

There was indeed a test (TestSimpleClient.test_get_with_retries)
that was erroneously relying on this behavior. Since previous tests
had populated their own instantiations with a token, this test only
passed because the modified headers dict from previous tests was
being overridden. As expected, with the mutable defaults fix in
SimpleClient, this test begain to fail since it never specified any
token, yet it has always passed anyway. This change also now provides
the expected token.

Change-Id: If95f11d259008517dab511e88acfe9731e5a99b5
Related-Bug: #1174809
This commit is contained in:
Brian Cline 2014-05-10 05:15:12 -05:00
parent 1dfe518654
commit b4c5a13664
11 changed files with 200 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
swift/common/internal_client.py
View File

@ -711,10 +711,14 @@ class SimpleClient(object):
self.retries = retries
def base_request(self, method, container=None, name=None, prefix=None,
headers={}, proxy=None, contents=None, full_listing=None):
headers=None, proxy=None, contents=None,
full_listing=None):
# Common request method
url = self.url
if headers is None:
headers = {}
if self.token:
headers['X-Auth-Token'] = self.token

174
test/functional/swift_test_client.py
View File

@ -197,7 +197,12 @@ class Connection(object):
port=self.storage_port)
#self.connection.set_debuglevel(3)
def make_path(self, path=[], cfg={}):
def make_path(self, path=None, cfg=None):
if path is None:
path = []
if cfg is None:
cfg = {}
if cfg.get('version_only_path'):
return '/' + self.storage_url.split('/')[1]
@ -210,7 +215,9 @@ class Connection(object):
else:
return self.storage_url
def make_headers(self, hdrs, cfg={}):
def make_headers(self, hdrs, cfg=None):
if cfg is None:
cfg = {}
headers = {}
if not cfg.get('no_auth_token'):
@ -220,8 +227,16 @@ class Connection(object):
headers.update(hdrs)
return headers
def make_request(self, method, path=[], data='', hdrs={}, parms={},
cfg={}):
def make_request(self, method, path=None, data='', hdrs=None, parms=None,
cfg=None):
if path is None:
path = []
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
if not cfg.get('absolute_path'):
# Set absolute_path=True to make a request to exactly the given
# path, not storage path + given path. Useful for
@ -279,7 +294,16 @@ class Connection(object):
'Attempts: %s, Failures: %s' %
(request, len(fail_messages), fail_messages))
def put_start(self, path, hdrs={}, parms={}, cfg={}, chunked=False):
def put_start(self, path, hdrs=None, parms=None, cfg=None, chunked=False):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
self.http_connect()
path = self.make_path(path, cfg)
headers = self.make_headers(hdrs, cfg=cfg)
@ -322,7 +346,10 @@ class Base(object):
def __str__(self):
return self.name
def header_fields(self, required_fields, optional_fields=()):
def header_fields(self, required_fields, optional_fields=None):
if optional_fields is None:
optional_fields = ()
headers = dict(self.conn.response.getheaders())
ret = {}
@ -352,7 +379,11 @@ class Account(Base):
self.conn = conn
self.name = str(name)
def update_metadata(self, metadata={}, cfg={}):
def update_metadata(self, metadata=None, cfg=None):
if metadata is None:
metadata = {}
if cfg is None:
cfg = {}
headers = dict(("X-Account-Meta-%s" % k, v)
for k, v in metadata.items())
@ -365,7 +396,14 @@ class Account(Base):
def container(self, container_name):
return Container(self.conn, self.name, container_name)
def containers(self, hdrs={}, parms={}, cfg={}):
def containers(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
format_type = parms.get('format', None)
if format_type not in [None, 'json', 'xml']:
raise RequestError('Invalid format: %s' % format_type)
@ -411,7 +449,13 @@ class Account(Base):
return listing_empty(self.containers)
def info(self, hdrs={}, parms={}, cfg={}):
def info(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
if self.conn.make_request('HEAD', self.path, hdrs=hdrs,
parms=parms, cfg=cfg) != 204:
@ -435,11 +479,21 @@ class Container(Base):
self.account = str(account)
self.name = str(name)
def create(self, hdrs={}, parms={}, cfg={}):
def create(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
return self.conn.make_request('PUT', self.path, hdrs=hdrs,
parms=parms, cfg=cfg) in (201, 202)
def delete(self, hdrs={}, parms={}):
def delete(self, hdrs=None, parms=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
return self.conn.make_request('DELETE', self.path, hdrs=hdrs,
parms=parms) == 204
@ -457,7 +511,13 @@ class Container(Base):
def file(self, file_name):
return File(self.conn, self.account, self.name, file_name)
def files(self, hdrs={}, parms={}, cfg={}):
def files(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
format_type = parms.get('format', None)
if format_type not in [None, 'json', 'xml']:
raise RequestError('Invalid format: %s' % format_type)
@ -507,7 +567,13 @@ class Container(Base):
raise ResponseError(self.conn.response, 'GET',
self.conn.make_path(self.path))
def info(self, hdrs={}, parms={}, cfg={}):
def info(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
self.conn.make_request('HEAD', self.path, hdrs=hdrs,
parms=parms, cfg=cfg)
@ -538,7 +604,9 @@ class File(Base):
self.size = None
self.metadata = {}
def make_headers(self, cfg={}):
def make_headers(self, cfg=None):
if cfg is None:
cfg = {}
headers = {}
if not cfg.get('no_content_length'):
if cfg.get('set_content_length'):
@ -575,7 +643,13 @@ class File(Base):
data.seek(0)
return checksum.hexdigest()
def copy(self, dest_cont, dest_file, hdrs={}, parms={}, cfg={}):
def copy(self, dest_cont, dest_file, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
if 'destination' in cfg:
headers = {'Destination': cfg['destination']}
elif cfg.get('no_destination'):
@ -590,7 +664,11 @@ class File(Base):
return self.conn.make_request('COPY', self.path, hdrs=headers,
parms=parms) == 201
def delete(self, hdrs={}, parms={}):
def delete(self, hdrs=None, parms=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if self.conn.make_request('DELETE', self.path, hdrs=hdrs,
parms=parms) != 204:
@ -599,7 +677,13 @@ class File(Base):
return True
def info(self, hdrs={}, parms={}, cfg={}):
def info(self, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
if self.conn.make_request('HEAD', self.path, hdrs=hdrs,
parms=parms, cfg=cfg) != 200:
@ -615,7 +699,11 @@ class File(Base):
header_fields['etag'] = header_fields['etag'].strip('"')
return header_fields
def initialize(self, hdrs={}, parms={}):
def initialize(self, hdrs=None, parms=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if not self.name:
return False
@ -660,7 +748,11 @@ class File(Base):
return data
def read(self, size=-1, offset=0, hdrs=None, buffer=None,
callback=None, cfg={}, parms={}):
callback=None, cfg=None, parms=None):
if cfg is None:
cfg = {}
if parms is None:
parms = {}
if size > 0:
range_string = 'bytes=%d-%d' % (offset, (offset + size) - 1)
@ -717,7 +809,12 @@ class File(Base):
finally:
fobj.close()
def sync_metadata(self, metadata={}, cfg={}):
def sync_metadata(self, metadata=None, cfg=None):
if metadata is None:
metadata = {}
if cfg is None:
cfg = {}
self.metadata.update(metadata)
if self.metadata:
@ -737,7 +834,14 @@ class File(Base):
return True
def chunked_write(self, data=None, hdrs={}, parms={}, cfg={}):
def chunked_write(self, data=None, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
if data is not None and self.chunked_write_in_progress:
self.conn.put_data(data, True)
elif data is not None:
@ -756,8 +860,15 @@ class File(Base):
else:
raise RuntimeError
def write(self, data='', hdrs={}, parms={}, callback=None, cfg={},
def write(self, data='', hdrs=None, parms=None, callback=None, cfg=None,
return_resp=False):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
block_size = 2 ** 20
if isinstance(data, file):
@ -808,7 +919,14 @@ class File(Base):
return True
def write_random(self, size=None, hdrs={}, parms={}, cfg={}):
def write_random(self, size=None, hdrs=None, parms=None, cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
data = self.random_data(size)
if not self.write(data, hdrs=hdrs, parms=parms, cfg=cfg):
raise ResponseError(self.conn.response, 'PUT',
@ -816,7 +934,15 @@ class File(Base):
self.md5 = self.compute_md5sum(StringIO.StringIO(data))
return data
def write_random_return_resp(self, size=None, hdrs={}, parms={}, cfg={}):
def write_random_return_resp(self, size=None, hdrs=None, parms=None,
cfg=None):
if hdrs is None:
hdrs = {}
if parms is None:
parms = {}
if cfg is None:
cfg = {}
data = self.random_data(size)
resp = self.write(data, hdrs=hdrs, parms=parms, cfg=cfg,
return_resp=True)

7
test/unit/account/test_reaper.py
View File

@ -190,7 +190,12 @@ class TestReaper(unittest.TestCase):
fd.write('')
return devices_path
def init_reaper(self, conf={}, myips=['10.10.10.1'], fakelogger=False):
def init_reaper(self, conf=None, myips=None, fakelogger=False):
if conf is None:
conf = {}
if myips is None:
myips = ['10.10.10.1']
r = reaper.AccountReaper(conf)
r.stats_return_codes = {}
r.stats_containers_deleted = 0

8
test/unit/common/middleware/test_account_quotas.py
View File

@ -34,7 +34,9 @@ class FakeCache(object):
class FakeBadApp(object):
def __init__(self, headers=[]):
def __init__(self, headers=None):
if headers is None:
headers = []
self.headers = headers
def __call__(self, env, start_response):
@ -43,7 +45,9 @@ class FakeBadApp(object):
class FakeApp(object):
def __init__(self, headers=[]):
def __init__(self, headers=None):
if headers is None:
headers = []
self.headers = headers
def __call__(self, env, start_response):

5
test/unit/common/middleware/test_gatekeeper.py
View File

@ -20,7 +20,10 @@ from swift.common.middleware import gatekeeper
class FakeApp(object):
def __init__(self, headers={}):
def __init__(self, headers=None):
if headers is None:
headers = {}
self.headers = headers
self.req = None

4
test/unit/common/middleware/test_keystoneauth.py
View File

@ -188,7 +188,9 @@ class TestAuthorize(unittest.TestCase):
identity['HTTP_X_TENANT_ID'])
def _get_identity(self, tenant_id='tenant_id', tenant_name='tenant_name',
user_id='user_id', user_name='user_name', roles=[]):
user_id='user_id', user_name='user_name', roles=None):
if roles is None:
roles = []
if isinstance(roles, list):
roles = ','.join(roles)
return {'HTTP_X_USER_ID': user_id,

10
test/unit/common/middleware/test_proxy_logging.py
View File

@ -27,7 +27,10 @@ from swift.common.swob import Request, Response
class FakeApp(object):
def __init__(self, body=['FAKE APP'], response_str='200 OK'):
def __init__(self, body=None, response_str='200 OK'):
if body is None:
body = ['FAKE APP']
self.body = body
self.response_str = response_str
@ -48,7 +51,10 @@ class FakeAppThatExcepts(object):
class FakeAppNoContentLengthNoTransferEncoding(object):
def __init__(self, body=['FAKE APP']):
def __init__(self, body=None):
if body is None:
body = ['FAKE APP']
self.body = body
def __call__(self, env, start_response):

3
test/unit/common/test_internal_client.py
View File

@ -1045,7 +1045,8 @@ class TestSimpleClient(unittest.TestCase):
urlopen.return_value.read.return_value = ''
req = urllib2.Request('http://127.0.0.1', method='GET')
request.side_effect = [urllib2.URLError(''), req]
sc = internal_client.SimpleClient(url='http://127.0.0.1', retries=1)
sc = internal_client.SimpleClient(url='http://127.0.0.1', retries=1,
token='token')
retval = sc.retry_request('GET')
self.assertEqual(request.call_count, 3)

4
test/unit/common/test_manager.py
View File

@ -147,7 +147,9 @@ class TestManagerModule(unittest.TestCase):
class MockOs(object):
WNOHANG = os.WNOHANG
def __init__(self, pid_map={}):
def __init__(self, pid_map=None):
if pid_map is None:
pid_map = {}
self.pid_map = {}
for pid, v in pid_map.items():
self.pid_map[pid] = (x for x in v)

9
test/unit/common/test_utils.py
View File

@ -62,7 +62,14 @@ from test.unit import FakeLogger
class MockOs(object):
def __init__(self, pass_funcs=[], called_funcs=[], raise_funcs=[]):
def __init__(self, pass_funcs=None, called_funcs=None, raise_funcs=None):
if pass_funcs is None:
pass_funcs = []
if called_funcs is None:
called_funcs = []
if raise_funcs is None:
raise_funcs = []
self.closed_fds = []
for func in pass_funcs:
setattr(self, func, self.pass_func)

6
test/unit/common/test_wsgi.py
View File

@ -464,7 +464,11 @@ class TestWSGI(unittest.TestCase):
def test_pre_auth_req(self):
class FakeReq(object):
@classmethod
def fake_blank(cls, path, environ={}, body='', headers={}):
def fake_blank(cls, path, environ=None, body='', headers=None):
if environ is None:
environ = {}
if headers is None:
headers = {}
self.assertEquals(environ['swift.authorize']('test'), None)
self.assertFalse('HTTP_X_TRANS_ID' in environ)
was_blank = Request.blank