Removes the use of mutables as default args

Passing mutable objects as default args is a known Python pitfall.
We'd better avoid this. This commit changes mutable default args with
None, then use 'arg = arg or {}', 'arg = arg or []'. For unit code which
doesn't use the args , just set with None. This commit also adds hacking
check.

Closes-Bug: #1327473
Change-Id: I5a8492bf8ffef8e000b13b6bdfaef1968b96f816

HACKING.rst

@@ -34,6 +34,7 @@ Nova Specific Commandments
 - [N320] Setting CONF.* attributes directly in tests is forbidden. Use
   self.flags(option=value) instead.
 - [N321] Validate that LOG messages, except debug ones, have translations
+- [N322] Method's default argument shouldn't be mutable
 
 Creating Unit Tests
 -------------------

nova/api/openstack/xmlutil.py

@@ -411,7 +411,7 @@ class TemplateElement(object):
         # We have fully rendered the element; return it
         return elem
 
-    def render(self, parent, obj, patches=[], nsmap=None):
+    def render(self, parent, obj, patches=None, nsmap=None):
         """Render an object.
 
         Renders an object against this template node.  Returns a list
@@ -428,6 +428,7 @@ class TemplateElement(object):
                       the etree.Element instances.
         """
 
+        patches = patches or []
         # First, get the datum we're rendering
         data = None if obj is None else self.selector(obj)
 

nova/hacking/checks.py

@@ -54,6 +54,7 @@ asse_equal_start_with_none_re = re.compile(
 conf_attribute_set_re = re.compile(r"CONF\.[a-z0-9_.]+\s*=\s*\w")
 log_translation = re.compile(
     r"(.)*LOG\.(audit|error|info|warn|warning|critical|exception)\(\s*('|\")")
+mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
 
 
 def import_no_db_in_virt(logical_line, filename):
@@ -249,6 +250,12 @@ def validate_log_translations(logical_line, physical_line, filename):
         yield (0, msg)
 
 
+def no_mutable_default_args(logical_line):
+    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_db_in_virt)
     register(no_db_session_in_public_api)
@@ -264,3 +271,4 @@ def factory(register):
     register(no_translate_debug_logs)
     register(no_setting_conf_directly_in_tests)
     register(validate_log_translations)
+    register(no_mutable_default_args)

nova/tests/api/openstack/compute/plugins/v3/admin_only_action_common.py

@@ -70,10 +70,12 @@ class CommonMixin(object):
         self.mox.UnsetStubs()
 
     def _test_action(self, action, body=None, method=None,
-                     compute_api_args_map={}):
+                     compute_api_args_map=None):
         if method is None:
             method = action
 
+        compute_api_args_map = compute_api_args_map or {}
+
         instance = self._stub_instance_get()
 
         args, kwargs = compute_api_args_map.get(action, ((), {}))
@@ -121,10 +123,11 @@ class CommonMixin(object):
         self.mox.UnsetStubs()
 
     def _test_locked_instance(self, action, method=None, body=None,
-                              compute_api_args_map={}):
+                              compute_api_args_map=None):
         if method is None:
             method = action
 
+        compute_api_args_map = compute_api_args_map or {}
         instance = self._stub_instance_get()
 
         args, kwargs = compute_api_args_map.get(action, ((), {}))
@@ -143,10 +146,12 @@ class CommonMixin(object):
         self.mox.UnsetStubs()
 
     def _test_instance_not_found_in_compute_api(self, action,
-                         method=None, body=None, compute_api_args_map={}):
+                         method=None, body=None, compute_api_args_map=None):
         if method is None:
             method = action
 
+        compute_api_args_map = compute_api_args_map or {}
+
         instance = self._stub_instance_get()
 
         args, kwargs = compute_api_args_map.get(action, ((), {}))
@@ -166,8 +171,11 @@ class CommonMixin(object):
 
 
 class CommonTests(CommonMixin, test.NoDBTestCase):
-    def _test_actions(self, actions, method_translations={}, body_map={},
+    def _test_actions(self, actions, method_translations=None, body_map=None,
-                      args_map={}):
+                      args_map=None):
+        method_translations = method_translations or {}
+        body_map = body_map or {}
+        args_map = args_map or {}
         for action in actions:
             method = method_translations.get(action)
             body = body_map.get(action)
@@ -178,7 +186,11 @@ class CommonTests(CommonMixin, test.NoDBTestCase):
             self.mox.StubOutWithMock(self.compute_api, 'get')
 
     def _test_actions_instance_not_found_in_compute_api(self,
-                  actions, method_translations={}, body_map={}, args_map={}):
+                  actions, method_translations=None, body_map=None,
+                  args_map=None):
+        method_translations = method_translations or {}
+        body_map = body_map or {}
+        args_map = args_map or {}
         for action in actions:
             method = method_translations.get(action)
             body = body_map.get(action)
@@ -189,7 +201,8 @@ class CommonTests(CommonMixin, test.NoDBTestCase):
             # Re-mock this.
             self.mox.StubOutWithMock(self.compute_api, 'get')
 
-    def _test_actions_with_non_existed_instance(self, actions, body_map={}):
+    def _test_actions_with_non_existed_instance(self, actions, body_map=None):
+        body_map = body_map or {}
         for action in actions:
             self._test_non_existing_instance(action,
                                              body_map=body_map)
@@ -197,7 +210,11 @@ class CommonTests(CommonMixin, test.NoDBTestCase):
             self.mox.StubOutWithMock(self.compute_api, 'get')
 
     def _test_actions_raise_conflict_on_invalid_state(
-            self, actions, method_translations={}, body_map={}, args_map={}):
+            self, actions, method_translations=None, body_map=None,
+            args_map=None):
+        method_translations = method_translations or {}
+        body_map = body_map or {}
+        args_map = args_map or {}
         for action in actions:
             method = method_translations.get(action)
             self.mox.StubOutWithMock(self.compute_api, method or action)
@@ -208,8 +225,11 @@ class CommonTests(CommonMixin, test.NoDBTestCase):
             self.mox.StubOutWithMock(self.compute_api, 'get')
 
     def _test_actions_with_locked_instance(self, actions,
-                                           method_translations={},
+                                           method_translations=None,
-                                           body_map={}, args_map={}):
+                                           body_map=None, args_map=None):
+        method_translations = method_translations or {}
+        body_map = body_map or {}
+        args_map = args_map or {}
         for action in actions:
             method = method_translations.get(action)
             body = body_map.get(action)

nova/tests/api/openstack/compute/plugins/v3/test_console_output.py

@@ -63,7 +63,8 @@ class ConsoleOutputExtensionTest(test.NoDBTestCase):
         self.app = fakes.wsgi_app_v3(init_only=('servers',
                                                 'os-console-output'))
 
-    def _create_request(self, length_dict={}):
+    def _create_request(self, length_dict=None):
+        length_dict = length_dict or {}
         body = {'get_console_output': length_dict}
         req = fakes.HTTPRequestV3.blank('/v3/servers/1/action')
         req.method = "POST"

nova/tests/api/openstack/compute/plugins/v3/test_servers.py

@@ -652,7 +652,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             db_list = [fakes.stub_instance(100, uuid=server_uuid)]
             return instance_obj._make_instance_list(
                 context, objects.InstanceList(), db_list, FIELDS)
@@ -671,7 +671,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('image', search_opts)
             self.assertEqual(search_opts['image'], '12345')
@@ -691,7 +691,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(filters)
             self.assertEqual(filters['project_id'], 'newfake')
             self.assertFalse(filters.get('tenant_id'))
@@ -711,7 +711,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotEqual(filters, None)
             self.assertEqual(filters['project_id'], 'fake')
             return [fakes.stub_instance(100)]
@@ -727,7 +727,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotEqual(filters, None)
             # The project_id assertion checks that the project_id
             # filter is set to that specified in the request url and
@@ -749,7 +749,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotIn('project_id', filters)
             return [fakes.stub_instance(100)]
 
@@ -766,7 +766,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotIn('project_id', filters)
             return [fakes.stub_instance(100)]
 
@@ -783,7 +783,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotIn('all_tenants', filters)
             return [fakes.stub_instance(100)]
 
@@ -800,7 +800,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotIn('all_tenants', filters)
             return [fakes.stub_instance(100)]
 
@@ -817,7 +817,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertNotIn('all_tenants', filters)
             return [fakes.stub_instance(100)]
 
@@ -833,7 +833,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(filters)
             self.assertEqual(filters['project_id'], 'fake')
             return [fakes.stub_instance(100)]
@@ -851,7 +851,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(context, filters=None, sort_key=None,
                          sort_dir='desc', limit=None, marker=None,
                          columns_to_join=None, use_slave=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(filters)
             self.assertNotIn('project_id', filters)
             return [fakes.stub_instance(100)]
@@ -901,7 +901,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('flavor', search_opts)
             # flavor is an integer ID
@@ -936,7 +936,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('vm_state', search_opts)
             self.assertEqual(search_opts['vm_state'], [vm_states.ACTIVE])
@@ -959,7 +959,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('task_state', search_opts)
             self.assertEqual([task_states.REBOOT_PENDING,
@@ -986,7 +986,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIn('vm_state', search_opts)
             self.assertEqual(search_opts['vm_state'],
                              [vm_states.ACTIVE, vm_states.STOPPED])
@@ -1022,7 +1022,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIn('vm_state', search_opts)
             self.assertEqual(search_opts['vm_state'], ['deleted'])
 
@@ -1045,7 +1045,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('name', search_opts)
             self.assertEqual(search_opts['name'], 'whee.*')
@@ -1067,7 +1067,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('changes-since', search_opts)
             changes_since = datetime.datetime(2011, 1, 24, 17, 8, 1,
@@ -1102,7 +1102,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             # Allowed by user
             self.assertIn('name', search_opts)
@@ -1134,7 +1134,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             # Allowed by user
             self.assertIn('name', search_opts)
@@ -1165,7 +1165,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('ip', search_opts)
             self.assertEqual(search_opts['ip'], '10\..*')
@@ -1190,7 +1190,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.assertIsNotNone(search_opts)
             self.assertIn('ip6', search_opts)
             self.assertEqual(search_opts['ip6'], 'ffff.*')
@@ -1272,7 +1272,7 @@ class ServersControllerTest(ControllerTest):
         def fake_get_all(compute_self, context, search_opts=None,
                          sort_key=None, sort_dir='desc',
                          limit=None, marker=None, want_objects=False,
-                         expected_attrs=[]):
+                         expected_attrs=None):
             self.expected_attrs = expected_attrs
             return []
 

nova/tests/api/openstack/compute/test_v3_extensions.py

@@ -33,7 +33,7 @@ class fake_bad_extension(object):
 
 class fake_stevedore_enabled_extensions(object):
     def __init__(self, namespace, check_func, invoke_on_load=False,
-                 invoke_args=(), invoke_kwds={}):
+                 invoke_args=(), invoke_kwds=None):
         self.extensions = []
 
     def map(self, func, *args, **kwds):

nova/tests/compute/test_compute.py

@@ -3345,8 +3345,9 @@ class ComputeTestCase(BaseTestCase):
         self.compute.terminate_instance(self.context,
                 self._objectify(instance), [], [])
 
-    def test_run_instance_usage_notification(self, request_spec={}):
+    def test_run_instance_usage_notification(self, request_spec=None):
         # Ensure run instance generates appropriate usage notification.
+        request_spec = request_spec or {}
         instance = jsonutils.to_primitive(self._create_fake_instance())
         instance_uuid = instance['uuid']
         expected_image_name = request_spec.get('image', {}).get('name', '')

nova/tests/compute/test_compute_utils.py

@@ -416,8 +416,9 @@ class UsageInfoTestCase(test.TestCase):
         fake_network.set_stub_network_methods(self.stubs)
         fake_server_actions.stub_out_action_events(self.stubs)
 
-    def _create_instance(self, params={}):
+    def _create_instance(self, params=None):
         """Create a test instance."""
+        params = params or {}
         flavor = flavors.get_flavor_by_name('m1.tiny')
         sys_meta = flavors.save_flavor_info({}, flavor)
         inst = {}

nova/tests/integrated/test_api_samples.py

@@ -477,7 +477,8 @@ class LimitsSampleXmlTest(LimitsSampleJsonTest):
 
 class ServersActionsJsonTest(ServersSampleBase):
     def _test_server_action(self, uuid, action,
-                            subs={}, resp_tpl=None, code=202):
+                            subs=None, resp_tpl=None, code=202):
+        subs = subs or {}
         subs.update({'action': action})
         response = self._do_post('servers/%s/action' % uuid,
                                  'server-action-%s' % action.lower(),
@@ -4173,7 +4174,8 @@ class PreserveEphemeralOnRebuildJsonTest(ServersSampleBase):
                       'Preserve_ephemeral_rebuild')
 
     def _test_server_action(self, uuid, action,
-                            subs={}, resp_tpl=None, code=202):
+                            subs=None, resp_tpl=None, code=202):
+        subs = subs or {}
         subs.update({'action': action})
         response = self._do_post('servers/%s/action' % uuid,
                                  'server-action-%s' % action.lower(),

nova/tests/integrated/v3/test_servers.py

@@ -72,7 +72,8 @@ class ServersActionsJsonTest(ServersSampleBase):
     sample_dir = 'servers'
 
     def _test_server_action(self, uuid, action,
-                            subs={}, resp_tpl=None, code=202):
+                            subs=None, resp_tpl=None, code=202):
+        subs = subs or {}
         subs.update({'action': action,
                      'glance_host': self._get_glance_host()})
         response = self._do_post('servers/%s/action' % uuid,

nova/tests/test_hacking.py

@@ -175,3 +175,16 @@ class HackingTestCase(test.NoDBTestCase):
                 self.assertEqual(0,
                     len(list(
                         checks.validate_log_translations(ok, ok, 'f'))))
+
+    def test_no_mutable_default_args(self):
+        self.assertEqual(1, len(list(checks.no_mutable_default_args(
+            " def fake_suds_context(calls={}):"))))
+
+        self.assertEqual(1, len(list(checks.no_mutable_default_args(
+            "def get_info_from_bdm(virt_type, bdm, mapping=[])"))))
+
+        self.assertEqual(0, len(list(checks.no_mutable_default_args(
+            "defined = []"))))
+
+        self.assertEqual(0, len(list(checks.no_mutable_default_args(
+            "defined, undefined = [], {}"))))

nova/tests/test_metadata.py

@@ -97,9 +97,10 @@ def return_non_existing_address(*args, **kwarg):
 
 
 def fake_InstanceMetadata(stubs, inst_data, address=None,
-                          sgroups=None, content=[], extra_md={},
+                          sgroups=None, content=None, extra_md=None,
                           vd_driver=None, network_info=None):
-
+    content = content or []
+    extra_md = extra_md or {}
     if sgroups is None:
         sgroups = [dict(test_security_group.fake_secgroup,
                         name='default')]

nova/tests/virt/libvirt/test_driver.py

@@ -8010,7 +8010,8 @@ class NWFilterTestCase(test.TestCase):
 
         self.fw.setup_basic_filtering(instance, network_info)
 
-        def assert_filterref(instance, vif, expected=[]):
+        def assert_filterref(instance, vif, expected=None):
+            expected = expected or []
             nic_id = vif['address'].replace(':', '')
             filter_name = self.fw._instance_filter_name(instance, nic_id)
             f = fakefilter.nwfilterLookupByName(filter_name)

nova/tests/virt/vmwareapi/stubs.py

@@ -73,7 +73,7 @@ def set_stubs(stubs):
               fake_is_vim_object)
 
 
-def fake_suds_context(calls={}):
+def fake_suds_context(calls=None):
     """Generate a suds client which automatically mocks all SOAP method calls.
 
     Calls are stored in <calls>, indexed by the name of the call. If you need
@@ -81,6 +81,8 @@ def fake_suds_context(calls={}):
     with appropriate Mock objects.
     """
 
+    calls = calls or {}
+
     class fake_factory:
         def create(self, name):
             return mock.NonCallableMagicMock(name=name)

nova/tests/virt/vmwareapi/test_configdrive.py

@@ -118,9 +118,10 @@ class ConfigDriveTestCase(test.NoDBTestCase):
         vmwareapi_fake.cleanup()
         nova.tests.image.fake.FakeImageService_reset()
 
-    def _spawn_vm(self, injected_files=[], admin_password=None,
+    def _spawn_vm(self, injected_files=None, admin_password=None,
                   block_device_info=None):
 
+        injected_files = injected_files or []
         read_file_handle = mock.MagicMock()
         write_file_handle = mock.MagicMock()
         self.image_ref = self.instance['image_ref']

nova/virt/libvirt/blockinfo.py

@@ -345,9 +345,10 @@ def get_config_drive_type():
     return config_drive_type
 
 
-def get_info_from_bdm(virt_type, bdm, mapping={}, disk_bus=None,
+def get_info_from_bdm(virt_type, bdm, mapping=None, disk_bus=None,
                       dev_type=None, allowed_types=None,
                       assigned_devices=None):
+    mapping = mapping or {}
     allowed_types = allowed_types or SUPPORTED_DEVICE_TYPES
     device_name = block_device.strip_dev(get_device_name(bdm))
 

nova/volume/cinder.py

@@ -221,7 +221,8 @@ class API(object):
         item = cinderclient(context).volumes.get(volume_id)
         return _untranslate_volume_summary_view(context, item)
 
-    def get_all(self, context, search_opts={}):
+    def get_all(self, context, search_opts=None):
+        search_opts = search_opts or {}
         items = cinderclient(context).volumes.list(detailed=True)
         rval = []