From b4c5a136642bb87816bfbbad81b79efd4551a05e Mon Sep 17 00:00:00 2001 From: Brian Cline Date: Sat, 10 May 2014 05:15:12 -0500 Subject: [PATCH] Uses None instead of mutables for function param defaults 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 --- swift/common/internal_client.py | 6 +- test/functional/swift_test_client.py | 174 +++++++++++++++--- test/unit/account/test_reaper.py | 7 +- .../common/middleware/test_account_quotas.py | 8 +- .../unit/common/middleware/test_gatekeeper.py | 5 +- .../common/middleware/test_keystoneauth.py | 4 +- .../common/middleware/test_proxy_logging.py | 10 +- test/unit/common/test_internal_client.py | 3 +- test/unit/common/test_manager.py | 4 +- test/unit/common/test_utils.py | 9 +- test/unit/common/test_wsgi.py | 6 +- 11 files changed, 200 insertions(+), 36 deletions(-) diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py index c477a4960e..ad876a38ed 100644 --- a/swift/common/internal_client.py +++ b/swift/common/internal_client.py @@ -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 diff --git a/test/functional/swift_test_client.py b/test/functional/swift_test_client.py index 142daae556..d19edaedc7 100644 --- a/test/functional/swift_test_client.py +++ b/test/functional/swift_test_client.py @@ -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) diff --git a/test/unit/account/test_reaper.py b/test/unit/account/test_reaper.py index e122fdb612..3b83fa9640 100644 --- a/test/unit/account/test_reaper.py +++ b/test/unit/account/test_reaper.py @@ -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 diff --git a/test/unit/common/middleware/test_account_quotas.py b/test/unit/common/middleware/test_account_quotas.py index e040dd6a6c..8660cf5e48 100644 --- a/test/unit/common/middleware/test_account_quotas.py +++ b/test/unit/common/middleware/test_account_quotas.py @@ -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): diff --git a/test/unit/common/middleware/test_gatekeeper.py b/test/unit/common/middleware/test_gatekeeper.py index 846baecb76..e4109b0c3d 100644 --- a/test/unit/common/middleware/test_gatekeeper.py +++ b/test/unit/common/middleware/test_gatekeeper.py @@ -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 diff --git a/test/unit/common/middleware/test_keystoneauth.py b/test/unit/common/middleware/test_keystoneauth.py index 8f5bbec27a..70a4b33262 100644 --- a/test/unit/common/middleware/test_keystoneauth.py +++ b/test/unit/common/middleware/test_keystoneauth.py @@ -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, diff --git a/test/unit/common/middleware/test_proxy_logging.py b/test/unit/common/middleware/test_proxy_logging.py index c577fd07e0..4fa5b2b8f7 100644 --- a/test/unit/common/middleware/test_proxy_logging.py +++ b/test/unit/common/middleware/test_proxy_logging.py @@ -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): diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py index 8a10106737..b8628074d1 100644 --- a/test/unit/common/test_internal_client.py +++ b/test/unit/common/test_internal_client.py @@ -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) diff --git a/test/unit/common/test_manager.py b/test/unit/common/test_manager.py index 512e98f87c..0468cb701b 100644 --- a/test/unit/common/test_manager.py +++ b/test/unit/common/test_manager.py @@ -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) diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index 4208d16387..b6e7b7a1b9 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -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) diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index 06e3c65553..425383aea6 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -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