Eliminate mutable default arguments

The best practice in Python is not to use mutable object (i.e. list,
dictionary, or instances of most classes) as value of default argument.

See: https://docs.python.org/2/tutorial/controlflow.html#default-argument-values

This patch also added a hacking rule to enforce this practice.

Change-Id: I4aa8aede57d6fd31b4b30c3f7535b819e591165d
Closes-Bug: 1471349

HACKING.rst

@@ -9,3 +9,4 @@ Magnum Specific Commandments
 ---------------------------
 
 - [M301] policy.enforce_wsgi decorator must be the first decorator on a method.
+- [M322] Method's default argument shouldn't be mutable.

magnum/api/middleware/auth_token.py

@@ -29,7 +29,9 @@ class AuthTokenMiddleware(auth_token.AuthProtocol):
     for public routes in the API.
 
     """
-    def __init__(self, app, conf, public_api_routes=[]):
+    def __init__(self, app, conf, public_api_routes=None):
+        if public_api_routes is None:
+            public_api_routes = []
         route_pattern_tpl = '%s(\.json|\.xml)?$'
 
         try:

magnum/common/service.py

@@ -16,7 +16,9 @@ from oslo_config import cfg
 from oslo_log import log as logging
 
 
-def prepare_service(argv=[]):
+def prepare_service(argv=None):
+    if argv is None:
+        argv = []
     logging.register_options(cfg.CONF)
     cfg.CONF(argv[1:], project='magnum')
     logging.setup(cfg.CONF, 'magnum')

magnum/db/sqlalchemy/api.py

@@ -106,7 +106,9 @@ class Connection(api.Connection):
     def __init__(self):
         pass
 
-    def _add_tenant_filters(self, context, query, opts={}):
+    def _add_tenant_filters(self, context, query, opts=None):
+        if opts is None:
+            opts = {}
 
         all_tenants = opts.get('get_all_tenants', False)
 
@@ -146,7 +148,9 @@ class Connection(api.Connection):
         return query
 
     def get_bay_list(self, context, filters=None, limit=None, marker=None,
-                     sort_key=None, sort_dir=None, opts={}):
+                     sort_key=None, sort_dir=None, opts=None):
+        if opts is None:
+            opts = {}
         query = model_query(models.Bay)
         query = self._add_tenant_filters(context, query, opts=opts)
         query = self._add_bays_filters(query, filters)

magnum/hacking/checks.py

@@ -32,6 +32,7 @@ Guidelines for writing new hacking checks
 
 enforce_re = re.compile(r"@policy.enforce_wsgi*")
 decorator_re = re.compile(r"@.*")
+mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
 
 
 def check_policy_enforce_decorator(logical_line,
@@ -44,5 +45,12 @@ def check_policy_enforce_decorator(logical_line,
         yield(0, msg)
 
 
+def no_mutable_default_args(logical_line):
+    msg = "M322: Method's default argument shouldn't be mutable!"
+    if mutable_default_args.match(logical_line):
+        yield (0, msg)
+
+
 def factory(register):
     register(check_policy_enforce_decorator)
+    register(no_mutable_default_args)

magnum/tests/unit/api/base.py

@@ -189,7 +189,8 @@ class FunctionalTest(base.DbTestCase):
         return response
 
     def get_json(self, path, expect_errors=False, headers=None,
-                 extra_environ=None, q=[], path_prefix=PATH_PREFIX, **params):
+                 extra_environ=None, q=None, path_prefix=PATH_PREFIX,
+                 **params):
         """Sends simulated HTTP GET request to Pecan test app.
 
         :param path: url path of target service
@@ -203,6 +204,8 @@ class FunctionalTest(base.DbTestCase):
         :param path_prefix: prefix of the url path
         :param params: content for wsgi.input of request
         """
+        if q is None:
+            q = []
         full_path = path_prefix + path
         query_params = {'q.field': [],
                         'q.value': [],

magnum/tests/unit/test_hacking.py

@@ -83,3 +83,13 @@ class HackingTestCase(base.TestCase):
                """
         self._assert_has_errors(code, checks.check_policy_enforce_decorator,
                                 expected_errors=[(2, 0, "M301")])
+
+    def test_no_mutable_default_args(self):
+        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 = [], {}"))))