From 2a180b83982442275505f275ba1a4d16662e78b3 Mon Sep 17 00:00:00 2001 From: Ghanshyam Date: Mon, 16 Jun 2014 13:54:22 +0900 Subject: [PATCH] Fix "mutable" object as default value This patch fix the coding issue where "mutable" objects like list, dict are being used as default value of param. Current functions with mutable object as default value can create issues if they will be called in more random order. For example - first call with non default value and then with default value. In this case original default will be overridden by the first function call (as mutable object keep their state among function calls). This commit also add the hacking check for the same and its corresponding test. Closes-Bug: #1330322 Change-Id: I251b316ef6d37f4b95c5e8d73a20a39005c22870 --- HACKING.rst | 1 + tempest/api/compute/base.py | 4 +- tempest/api/orchestration/base.py | 4 +- tempest/common/rest_client.py | 8 +++- tempest/hacking/checks.py | 12 ++++++ tempest/scenario/manager.py | 41 ++++++++++++++----- .../services/object_storage/account_client.py | 8 +++- .../json/orchestration_client.py | 20 ++++++--- tempest/tests/test_hacking.py | 13 ++++++ 9 files changed, 89 insertions(+), 22 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 8652971836..025bf7460a 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -12,6 +12,7 @@ Tempest Specific Commandments - [T104] Scenario tests require a services decorator - [T105] Unit tests cannot use setUpClass - [T106] vim configuration should not be kept in source files. +- [N322] Method's default argument shouldn't be mutable Test Data/Configuration ----------------------- diff --git a/tempest/api/compute/base.py b/tempest/api/compute/base.py index 343a39a02f..47d12540ac 100644 --- a/tempest/api/compute/base.py +++ b/tempest/api/compute/base.py @@ -271,10 +271,10 @@ class BaseComputeTest(tempest.test.BaseTestCase): return resp, body @classmethod - def create_test_server_group(cls, name="", policy=[]): + def create_test_server_group(cls, name="", policy=None): if not name: name = data_utils.rand_name(cls.__name__ + "-Server-Group") - if not policy: + if policy is None: policy = ['affinity'] resp, body = cls.servers_client.create_server_group(name, policy) cls.server_groups.append(body['id']) diff --git a/tempest/api/orchestration/base.py b/tempest/api/orchestration/base.py index d0fb8254d8..0b22de596a 100644 --- a/tempest/api/orchestration/base.py +++ b/tempest/api/orchestration/base.py @@ -64,8 +64,10 @@ class BaseOrchestrationTest(tempest.test.BaseTestCase): return admin_client @classmethod - def create_stack(cls, stack_name, template_data, parameters={}, + def create_stack(cls, stack_name, template_data, parameters=None, environment=None, files=None): + if parameters is None: + parameters = {} resp, body = cls.client.create_stack( stack_name, template=template_data, diff --git a/tempest/common/rest_client.py b/tempest/common/rest_client.py index ff92b67281..132d0a69d7 100644 --- a/tempest/common/rest_client.py +++ b/tempest/common/rest_client.py @@ -248,8 +248,10 @@ class RestClient(object): return resp[i] return "" - def _log_request_start(self, method, req_url, req_headers={}, + def _log_request_start(self, method, req_url, req_headers=None, req_body=None): + if req_headers is None: + req_headers = {} caller_name = misc_utils.find_test_caller() trace_regex = CONF.debug.trace_requests if trace_regex and re.search(trace_regex, caller_name): @@ -257,8 +259,10 @@ class RestClient(object): (caller_name, method, req_url)) def _log_request(self, method, req_url, resp, - secs="", req_headers={}, + secs="", req_headers=None, req_body=None, resp_body=None): + if req_headers is None: + req_headers = {} # if we have the request id, put it in the right part of the log extra = dict(request_id=self._get_request_id(resp)) # NOTE(sdague): while we still have 6 callers to this function diff --git a/tempest/hacking/checks.py b/tempest/hacking/checks.py index cef010ea16..abc60cb384 100644 --- a/tempest/hacking/checks.py +++ b/tempest/hacking/checks.py @@ -27,6 +27,7 @@ TEST_DEFINITION = re.compile(r'^\s*def test.*') SETUPCLASS_DEFINITION = re.compile(r'^\s*def setUpClass') SCENARIO_DECORATOR = re.compile(r'\s*@.*services\((.*)\)') VI_HEADER_RE = re.compile(r"^#\s+vim?:.+") +mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") def import_no_clients_in_api(physical_line, filename): @@ -119,6 +120,16 @@ def no_official_client_manager_in_api_tests(physical_line, filename): 'tests') +def no_mutable_default_args(logical_line): + """Check that mutable object isn't used as default argument + + N322: Method's default argument shouldn't be mutable + """ + msg = "N322: Method's default argument shouldn't be mutable!" + if mutable_default_args.match(logical_line): + yield (0, msg) + + def factory(register): register(import_no_clients_in_api) register(scenario_tests_need_service_tags) @@ -126,3 +137,4 @@ def factory(register): register(no_vi_headers) register(service_tags_not_in_module_path) register(no_official_client_manager_in_api_tests) + register(no_mutable_default_args) diff --git a/tempest/scenario/manager.py b/tempest/scenario/manager.py index 0f14c940a8..c7236ce654 100644 --- a/tempest/scenario/manager.py +++ b/tempest/scenario/manager.py @@ -136,8 +136,8 @@ class ScenarioTest(tempest.test.BaseTestCase): pass def addCleanup_with_wait(self, waiter_callable, thing_id, thing_id_param, - cleanup_callable, cleanup_args=[], - cleanup_kwargs={}, ignore_error=True): + cleanup_callable, cleanup_args=None, + cleanup_kwargs=None, ignore_error=True): """Adds wait for ansyc resource deletion at the end of cleanups @param waiter_callable: callable to wait for the resource to delete @@ -147,6 +147,10 @@ class ScenarioTest(tempest.test.BaseTestCase): the following *cleanup_args, **cleanup_kwargs. usually a delete method. """ + if cleanup_args is None: + cleanup_args = [] + if cleanup_kwargs is None: + cleanup_kwargs = {} self.addCleanup(cleanup_callable, *cleanup_args, **cleanup_kwargs) wait_dict = { 'waiter_callable': waiter_callable, @@ -181,7 +185,7 @@ class ScenarioTest(tempest.test.BaseTestCase): def create_server(self, name=None, image=None, flavor=None, wait_on_boot=True, wait_on_delete=True, - create_kwargs={}): + create_kwargs=None): """Creates VM instance. @param image: image from which to create the instance @@ -196,6 +200,8 @@ class ScenarioTest(tempest.test.BaseTestCase): image = CONF.compute.image_ref if flavor is None: flavor = CONF.compute.flavor_ref + if create_kwargs is None: + create_kwargs = {} fixed_network_name = CONF.compute.fixed_network_name if 'nics' not in create_kwargs and fixed_network_name: @@ -338,7 +344,9 @@ class ScenarioTest(tempest.test.BaseTestCase): return linux_client - def _image_create(self, name, fmt, path, properties={}): + def _image_create(self, name, fmt, path, properties=None): + if properties is None: + properties = {} name = data_utils.rand_name('%s-' % name) image_file = open(path, 'rb') self.addCleanup(image_file.close) @@ -531,8 +539,8 @@ class OfficialClientTest(tempest.test.BaseTestCase): def addCleanup_with_wait(self, things, thing_id, error_status='ERROR', exc_type=nova_exceptions.NotFound, - cleanup_callable=None, cleanup_args=[], - cleanup_kwargs={}): + cleanup_callable=None, cleanup_args=None, + cleanup_kwargs=None): """Adds wait for ansyc resource deletion at the end of cleanups @param things: type of the resource to delete @@ -544,6 +552,10 @@ class OfficialClientTest(tempest.test.BaseTestCase): usually a delete method. if not used, will try to use: things.delete(thing_id) """ + if cleanup_args is None: + cleanup_args = [] + if cleanup_kwargs is None: + cleanup_kwargs = {} if cleanup_callable is None: LOG.debug("no delete method passed. using {rclass}.delete({id}) as" " default".format(rclass=things, id=thing_id)) @@ -725,7 +737,7 @@ class OfficialClientTest(tempest.test.BaseTestCase): def create_server(self, client=None, name=None, image=None, flavor=None, wait_on_boot=True, wait_on_delete=True, - create_kwargs={}): + create_kwargs=None): """Creates VM instance. @param client: compute client to create the instance @@ -743,6 +755,8 @@ class OfficialClientTest(tempest.test.BaseTestCase): image = CONF.compute.image_ref if flavor is None: flavor = CONF.compute.flavor_ref + if create_kwargs is None: + create_kwargs = {} fixed_network_name = CONF.compute.fixed_network_name if 'nics' not in create_kwargs and fixed_network_name: @@ -872,7 +886,9 @@ class OfficialClientTest(tempest.test.BaseTestCase): self.status_timeout( self.volume_client.volumes, volume_id, status) - def _image_create(self, name, fmt, path, properties={}): + def _image_create(self, name, fmt, path, properties=None): + if properties is None: + properties = {} name = data_utils.rand_name('%s-' % name) image_file = open(path, 'rb') self.addCleanup(image_file.close) @@ -1866,12 +1882,17 @@ class SwiftScenarioTest(ScenarioTest): self._list_and_check_container_objects(container_name, not_present_obj=[filename]) - def _list_and_check_container_objects(self, container_name, present_obj=[], - not_present_obj=[]): + def _list_and_check_container_objects(self, container_name, + present_obj=None, + not_present_obj=None): """ List objects for a given container and assert which are present and which are not. """ + if present_obj is None: + present_obj = [] + if not_present_obj is None: + not_present_obj = [] _, object_list = self.container_client.list_container_contents( container_name) if present_obj: diff --git a/tempest/services/object_storage/account_client.py b/tempest/services/object_storage/account_client.py index be0f888112..eca57c001a 100644 --- a/tempest/services/object_storage/account_client.py +++ b/tempest/services/object_storage/account_client.py @@ -32,11 +32,15 @@ class AccountClient(rest_client.RestClient): def create_account(self, data=None, params=None, - metadata={}, - remove_metadata={}, + metadata=None, + remove_metadata=None, metadata_prefix='X-Account-Meta-', remove_metadata_prefix='X-Remove-Account-Meta-'): """Create an account.""" + if metadata is None: + metadata = {} + if remove_metadata is None: + remove_metadata = {} url = '' if params: url += '?%s' % urllib.urlencode(params) diff --git a/tempest/services/orchestration/json/orchestration_client.py b/tempest/services/orchestration/json/orchestration_client.py index dd166dd903..3b8e81e06d 100644 --- a/tempest/services/orchestration/json/orchestration_client.py +++ b/tempest/services/orchestration/json/orchestration_client.py @@ -45,9 +45,11 @@ class OrchestrationClient(rest_client.RestClient): body = json.loads(body) return resp, body['stacks'] - def create_stack(self, name, disable_rollback=True, parameters={}, + def create_stack(self, name, disable_rollback=True, parameters=None, timeout_mins=60, template=None, template_url=None, environment=None, files=None): + if parameters is None: + parameters = {} headers, body = self._prepare_update_create( name, disable_rollback, @@ -63,8 +65,10 @@ class OrchestrationClient(rest_client.RestClient): return resp, body def update_stack(self, stack_identifier, name, disable_rollback=True, - parameters={}, timeout_mins=60, template=None, + parameters=None, timeout_mins=60, template=None, template_url=None, environment=None, files=None): + if parameters is None: + parameters = {} headers, body = self._prepare_update_create( name, disable_rollback, @@ -80,9 +84,11 @@ class OrchestrationClient(rest_client.RestClient): return resp, body def _prepare_update_create(self, name, disable_rollback=True, - parameters={}, timeout_mins=60, + parameters=None, timeout_mins=60, template=None, template_url=None, environment=None, files=None): + if parameters is None: + parameters = {} post_body = { "stack_name": name, "disable_rollback": disable_rollback, @@ -264,16 +270,20 @@ class OrchestrationClient(rest_client.RestClient): body = json.loads(body) return resp, body - def validate_template(self, template, parameters={}): + def validate_template(self, template, parameters=None): """Returns the validation result for a template with parameters.""" + if parameters is None: + parameters = {} post_body = { 'template': template, 'parameters': parameters, } return self._validate_template(post_body) - def validate_template_url(self, template_url, parameters={}): + def validate_template_url(self, template_url, parameters=None): """Returns the validation result for a template with parameters.""" + if parameters is None: + parameters = {} post_body = { 'template_url': template_url, 'parameters': parameters, diff --git a/tempest/tests/test_hacking.py b/tempest/tests/test_hacking.py index 52fdf7ea6f..9c13013ed7 100644 --- a/tempest/tests/test_hacking.py +++ b/tempest/tests/test_hacking.py @@ -107,3 +107,16 @@ class HackingTestCase(base.TestCase): self.assertFalse(checks.no_official_client_manager_in_api_tests( "cls.official_client = clients.OfficialClientManager(credentials)", "tempest/scenario/fake_test.py")) + + def test_no_mutable_default_args(self): + self.assertEqual(1, len(list(checks.no_mutable_default_args( + " def function1(para={}):")))) + + self.assertEqual(1, len(list(checks.no_mutable_default_args( + "def function2(para1, para2, para3=[])")))) + + self.assertEqual(0, len(list(checks.no_mutable_default_args( + "defined = []")))) + + self.assertEqual(0, len(list(checks.no_mutable_default_args( + "defined, undefined = [], {}"))))