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

@@ -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
 -----------------------

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'])

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,

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

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)

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:

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)

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,

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 = [], {}"))))