From 2b7fb6ff08a49618ee441f80b07bd622e1ee0b1b Mon Sep 17 00:00:00 2001 From: Gary Kotton Date: Thu, 29 Oct 2015 07:40:05 -0700 Subject: [PATCH] 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. This code was taken from commit 0bea84ac20fe498bd08f7212a0017196c8cb0812 in Nova. Change-Id: I36d07cade687690dc02a8f6cc3d70f5d00caf112 Co-Authored-By: ChangBo Guo(gcb) --- HACKING.rst | 1 + neutron/agent/linux/interface.py | 4 ++-- neutron/agent/linux/iptables_firewall.py | 3 ++- neutron/api/api_common.py | 3 ++- neutron/api/extensions.py | 5 ++++- neutron/hacking/checks.py | 8 ++++++++ .../ml2/drivers/mech_sriov/agent/sriov_nic_agent.py | 3 ++- neutron/tests/unit/agent/linux/test_interface.py | 4 ++-- neutron/tests/unit/api/test_extensions.py | 3 ++- neutron/tests/unit/api/v2/test_base.py | 3 ++- neutron/tests/unit/db/test_db_base_plugin_v2.py | 4 +++- neutron/tests/unit/extension_stubs.py | 3 ++- neutron/tests/unit/extensions/test_dns.py | 7 +++++-- neutron/tests/unit/extensions/test_l3.py | 3 ++- neutron/tests/unit/extensions/test_securitygroup.py | 3 ++- neutron/tests/unit/hacking/test_checks.py | 13 +++++++++++++ .../ml2/drivers/freescale/test_mechanism_fslsdn.py | 3 ++- 17 files changed, 56 insertions(+), 17 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 6b9df2590..0eade2778 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -18,6 +18,7 @@ Neutron Specific Commandments - [N326] Python 3: do not use basestring. - [N327] Python 3: do not use dict.iteritems. - [N328] Detect wrong usage with assertEqual +- [N329] Method's default argument shouldn't be mutable Creating Unit Tests ------------------- diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index bcdf06c59..ad235be89 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -105,8 +105,7 @@ class LinuxInterfaceDriver(object): return False def init_l3(self, device_name, ip_cidrs, namespace=None, - preserve_ips=[], - clean_connections=False): + preserve_ips=None, clean_connections=False): """Set the L3 settings for the interface using data from the port. ip_cidrs: list of 'X.X.X.X/YY' strings @@ -114,6 +113,7 @@ class LinuxInterfaceDriver(object): clean_connections: Boolean to indicate if we should cleanup connections associated to removed ips """ + preserve_ips = preserve_ips or [] device = ip_lib.IPDevice(device_name, namespace=namespace) # The LLA generated by the operating system is not known to diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 2770e0842..4165ed7a6 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -128,7 +128,8 @@ class IptablesFirewallDriver(firewall.FirewallDriver): self.devices_with_udpated_sg_members[sg_id].append(device) def security_group_updated(self, action_type, sec_group_ids, - device_ids=[]): + device_ids=None): + device_ids = device_ids or [] if action_type == 'sg_rule': self.updated_rule_sg_ids.update(sec_group_ids) elif action_type == 'sg_member': diff --git a/neutron/api/api_common.py b/neutron/api/api_common.py index 5b7032092..659e1ee1a 100644 --- a/neutron/api/api_common.py +++ b/neutron/api/api_common.py @@ -29,7 +29,7 @@ from neutron.i18n import _LW LOG = logging.getLogger(__name__) -def get_filters(request, attr_info, skips=[]): +def get_filters(request, attr_info, skips=None): """Extracts the filters from the request string. Returns a dict of lists for the filters: @@ -37,6 +37,7 @@ def get_filters(request, attr_info, skips=[]): becomes: {'check': [u'a', u'b'], 'name': [u'Bob']} """ + skips = skips or [] res = {} for key, values in six.iteritems(request.GET.dict_of_lists()): if key in skips: diff --git a/neutron/api/extensions.py b/neutron/api/extensions.py index 442dfc9aa..f5fb29b2e 100644 --- a/neutron/api/extensions.py +++ b/neutron/api/extensions.py @@ -641,7 +641,10 @@ class ResourceExtension(object): """Add top level resources to the OpenStack API in Neutron.""" def __init__(self, collection, controller, parent=None, path_prefix="", - collection_actions={}, member_actions={}, attr_map={}): + collection_actions=None, member_actions=None, attr_map=None): + collection_actions = collection_actions or {} + member_actions = member_actions or {} + attr_map = attr_map or {} self.collection = collection self.controller = controller self.parent = parent diff --git a/neutron/hacking/checks.py b/neutron/hacking/checks.py index 383c64e9f..a1d2ae4f3 100644 --- a/neutron/hacking/checks.py +++ b/neutron/hacking/checks.py @@ -38,6 +38,7 @@ _all_log_levels = { 'exception': '_LE', } _all_hints = set(_all_log_levels.values()) +mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") def _regex_for_level(level, hint): @@ -186,6 +187,12 @@ def check_asserttrue(logical_line, filename): yield (0, msg) +def no_mutable_default_args(logical_line): + msg = "N329: Method's default argument shouldn't be mutable!" + if mutable_default_args.match(logical_line): + yield (0, msg) + + def factory(register): register(validate_log_translations) register(use_jsonutils) @@ -197,3 +204,4 @@ def factory(register): register(check_no_basestring) register(check_python3_no_iteritems) register(check_asserttrue) + register(no_mutable_default_args) diff --git a/neutron/plugins/ml2/drivers/mech_sriov/agent/sriov_nic_agent.py b/neutron/plugins/ml2/drivers/mech_sriov/agent/sriov_nic_agent.py index 1918b4950..4815a0ac8 100644 --- a/neutron/plugins/ml2/drivers/mech_sriov/agent/sriov_nic_agent.py +++ b/neutron/plugins/ml2/drivers/mech_sriov/agent/sriov_nic_agent.py @@ -155,7 +155,8 @@ class SriovNicSwitchAgent(object): mgr.initialize(connection, 'sriov') return mgr - def setup_eswitch_mgr(self, device_mappings, exclude_devices={}): + def setup_eswitch_mgr(self, device_mappings, exclude_devices=None): + exclude_devices = exclude_devices or {} self.eswitch_mgr = esm.ESwitchManager() self.eswitch_mgr.discover_devices(device_mappings, exclude_devices) diff --git a/neutron/tests/unit/agent/linux/test_interface.py b/neutron/tests/unit/agent/linux/test_interface.py index 130abf185..86f542ae1 100644 --- a/neutron/tests/unit/agent/linux/test_interface.py +++ b/neutron/tests/unit/agent/linux/test_interface.py @@ -361,9 +361,9 @@ class TestOVSInterfaceDriver(TestBase): 'aa:bb:cc:dd:ee:ff', internal=True) - def _test_plug(self, additional_expectation=[], bridge=None, + def _test_plug(self, additional_expectation=None, bridge=None, namespace=None): - + additional_expectation = additional_expectation or [] if not bridge: bridge = 'br-int' diff --git a/neutron/tests/unit/api/test_extensions.py b/neutron/tests/unit/api/test_extensions.py index c8fea09c9..60efe6db6 100644 --- a/neutron/tests/unit/api/test_extensions.py +++ b/neutron/tests/unit/api/test_extensions.py @@ -51,7 +51,8 @@ extensions_path = ':'.join(neutron.tests.unit.extensions.__path__) class ExtensionsTestApp(base_wsgi.Router): - def __init__(self, options={}): + def __init__(self, options=None): + options = options or {} mapper = routes.Mapper() controller = ext_stubs.StubBaseAppController() mapper.resource("dummy_resource", "/dummy_resources", diff --git a/neutron/tests/unit/api/v2/test_base.py b/neutron/tests/unit/api/v2/test_base.py index 8507df9f9..2e062014f 100644 --- a/neutron/tests/unit/api/v2/test_base.py +++ b/neutron/tests/unit/api/v2/test_base.py @@ -146,7 +146,8 @@ class APIv2TestCase(APIv2TestBase): fields.extend(policy_attrs) return fields - def _get_collection_kwargs(self, skipargs=[], **kwargs): + def _get_collection_kwargs(self, skipargs=None, **kwargs): + skipargs = skipargs or [] args_list = ['filters', 'fields', 'sorts', 'limit', 'marker', 'page_reverse'] args_dict = dict( diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index 6d57248dc..588c7509f 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -1220,7 +1220,9 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s self, expected_status=webob.exc.HTTPOk.code, expected_error='StateInvalid', subnet=None, device_owner=DEVICE_OWNER_COMPUTE, updated_fixed_ips=None, - host_arg={}, arg_list=[]): + host_arg=None, arg_list=None): + host_arg = host_arg or {} + arg_list = arg_list or [] with self.port(device_owner=device_owner, subnet=subnet, arg_list=arg_list, **host_arg) as port: self.assertIn('mac_address', port['port']) diff --git a/neutron/tests/unit/extension_stubs.py b/neutron/tests/unit/extension_stubs.py index f98d21491..b2a0748af 100644 --- a/neutron/tests/unit/extension_stubs.py +++ b/neutron/tests/unit/extension_stubs.py @@ -39,7 +39,8 @@ class StubExtension(object): class StubPlugin(object): - def __init__(self, supported_extensions=[]): + def __init__(self, supported_extensions=None): + supported_extensions = supported_extensions or [] self.supported_extension_aliases = supported_extensions diff --git a/neutron/tests/unit/extensions/test_dns.py b/neutron/tests/unit/extensions/test_dns.py index 683cb65b0..48f799055 100644 --- a/neutron/tests/unit/extensions/test_dns.py +++ b/neutron/tests/unit/extensions/test_dns.py @@ -151,9 +151,12 @@ class DnsExtensionTestCase(test_db_base_plugin_v2.TestNetworksV2): self._verify_dns_assigment(res['port'], ips_list=['10.0.0.2']) - def _verify_dns_assigment(self, port, ips_list=[], exp_ips_ipv4=0, - exp_ips_ipv6=0, ipv4_cidrs=[], ipv6_cidrs=[], + def _verify_dns_assigment(self, port, ips_list=None, exp_ips_ipv4=0, + exp_ips_ipv6=0, ipv4_cidrs=None, ipv6_cidrs=None, dns_name=''): + ips_list = ips_list or [] + ipv4_cidrs = ipv4_cidrs or [] + ipv6_cidrs = ipv6_cidrs or [] self.assertEqual(port['dns_name'], dns_name) dns_assignment = port['dns_assignment'] if ips_list: diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index bfe0f1303..771830817 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -353,7 +353,8 @@ class L3NatTestCaseMixin(object): def _add_external_gateway_to_router(self, router_id, network_id, expected_code=exc.HTTPOk.code, - neutron_context=None, ext_ips=[]): + neutron_context=None, ext_ips=None): + ext_ips = ext_ips or [] body = {'router': {'external_gateway_info': {'network_id': network_id}}} if ext_ips: diff --git a/neutron/tests/unit/extensions/test_securitygroup.py b/neutron/tests/unit/extensions/test_securitygroup.py index 9ce50f2de..bf9be380d 100644 --- a/neutron/tests/unit/extensions/test_securitygroup.py +++ b/neutron/tests/unit/extensions/test_securitygroup.py @@ -213,8 +213,9 @@ class SecurityGroupTestPlugin(db_base_plugin_v2.NeutronDbPluginV2, network) def get_ports(self, context, filters=None, fields=None, - sorts=[], limit=None, marker=None, + sorts=None, limit=None, marker=None, page_reverse=False): + sorts = sorts or [] neutron_lports = super(SecurityGroupTestPlugin, self).get_ports( context, filters, sorts=sorts, limit=limit, marker=marker, page_reverse=page_reverse) diff --git a/neutron/tests/unit/hacking/test_checks.py b/neutron/tests/unit/hacking/test_checks.py index d6e1d5f8d..97193bf28 100644 --- a/neutron/tests/unit/hacking/test_checks.py +++ b/neutron/tests/unit/hacking/test_checks.py @@ -177,3 +177,16 @@ class HackingTestCase(base.BaseTestCase): self.assertEqual( 0, len(list(checks.check_asserttrue(pass_code, "neutron/tests/test_assert.py")))) + + 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 = [], {}")))) diff --git a/neutron/tests/unit/plugins/ml2/drivers/freescale/test_mechanism_fslsdn.py b/neutron/tests/unit/plugins/ml2/drivers/freescale/test_mechanism_fslsdn.py index f4ac15f30..a34f5eaf3 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/freescale/test_mechanism_fslsdn.py +++ b/neutron/tests/unit/plugins/ml2/drivers/freescale/test_mechanism_fslsdn.py @@ -221,7 +221,8 @@ class TestFslSdnMechanismDriver(base.BaseTestCase): return FakeContext(subnet) def _get_port_context(self, tenant_id, net_id, port_id, - fixed_ips=[]): + fixed_ips=None): + fixed_ips = fixed_ips or [] # sample data for testing purpose only port = {'device_id': '1234', 'name': 'FakePort', -- 2.45.2