Fix possible double import of cubes modules
authorPhilippe Pepiot <philippe.pepiot@logilab.fr>
Tue, 07 Feb 2017 13:47:03 +0100
changeset 11955 f85ec84355db
parent 11954 e0d708fb20e8
child 11956 3199a40db419
Fix possible double import of cubes modules When cubes using the new layout are imported with 'cubicweb_<cube>' and with 'cubes.<cube>', the same module is imported twice. Handle this by adding 'cubes.<cube>' to sys.modules when importing from 'cubicweb_<cube>'. Move load_module() to a sub class _CubesLoader to share informations computed in find_modules(). Don't handle subpackages in _CubesImporter and rely on normal import machinery instead. Add a test and use unittest from cubicweb.devtools.testlib which resolve to unittest2 on PY2 with assertLogs() method.
cubicweb/__init__.py
cubicweb/test/unittest_cubes.py
--- a/cubicweb/__init__.py	Wed Feb 08 10:31:26 2017 +0100
+++ b/cubicweb/__init__.py	Tue Feb 07 13:47:03 2017 +0100
@@ -24,7 +24,6 @@
 import logging
 import os
 import pickle
-import pkgutil
 import sys
 import types
 import warnings
@@ -283,6 +282,37 @@
 
 # Import hook for "legacy" cubes ##############################################
 
+class _CubesLoader(object):
+
+    def __init__(self, *modinfo):
+        self.modinfo = modinfo
+
+    def load_module(self, fullname):
+        try:
+            # If there is an existing module object named 'fullname' in
+            # sys.modules , the loader must use that existing module.
+            # Otherwise, the reload() builtin will not work correctly.
+            return sys.modules[fullname]
+        except KeyError:
+            pass
+        if fullname == 'cubes':
+            mod = sys.modules[fullname] = types.ModuleType(
+                fullname, doc='CubicWeb cubes')
+        else:
+            modname, file, pathname, description = self.modinfo
+            try:
+                mod = sys.modules[fullname] = imp.load_module(
+                    modname, file, pathname, description)
+            finally:
+                # https://docs.python.org/2/library/imp.html#imp.load_module
+                # Important: the caller is responsible for closing the file
+                # argument, if it was not None, even when an exception is
+                # raised. This is best done using a try ... finally statement
+                if file is not None:
+                    file.close()
+        return mod
+
+
 class _CubesImporter(object):
     """Module finder handling redirection of import of "cubes.<name>"
     to "cubicweb_<name>".
@@ -296,25 +326,12 @@
 
     def find_module(self, fullname, path=None):
         if fullname == 'cubes':
-            return self
-        elif fullname.startswith('cubes.'):
+            return _CubesLoader()
+        elif fullname.startswith('cubes.') and fullname.count('.') == 1:
             modname = 'cubicweb_' + fullname.split('.', 1)[1]
             try:
                 modinfo = imp.find_module(modname)
             except ImportError:
                 return None
             else:
-                return pkgutil.ImpLoader(fullname, *modinfo)
-
-    def load_module(self, fullname):
-        try:
-            # If there is an existing module object named 'fullname' in
-            # sys.modules , the loader must use that existing module.
-            # Otherwise, the reload() builtin will not work correctly.
-            return sys.modules[fullname]
-        except KeyError:
-            pass
-        if fullname != 'cubes':
-            raise ImportError('No module named {0}'.format(fullname))
-        mod = sys.modules[fullname] = types.ModuleType(fullname, doc='CubicWeb cubes')
-        return mod
+                return _CubesLoader(modname, *modinfo)
--- a/cubicweb/test/unittest_cubes.py	Wed Feb 08 10:31:26 2017 +0100
+++ b/cubicweb/test/unittest_cubes.py	Tue Feb 07 13:47:03 2017 +0100
@@ -21,13 +21,12 @@
 import os
 from os import path
 import sys
-import unittest
 
 from six import PY2
 
 from cubicweb import _CubesImporter
 from cubicweb.cwconfig import CubicWebConfiguration
-from cubicweb.devtools.testlib import TemporaryDirectory
+from cubicweb.devtools.testlib import TemporaryDirectory, TestCase
 
 
 @contextmanager
@@ -37,17 +36,20 @@
             libdir = path.join(tempdir, 'libpython')
             cubedir = path.join(libdir, 'cubicweb_foo')
             os.makedirs(cubedir)
+            check_code = ("import logging\n"
+                          "logging.getLogger('cubicweb_foo')"
+                          ".warn('imported %s', __name__)\n")
             with open(path.join(cubedir, '__init__.py'), 'w') as f:
-                f.write('"""cubicweb_foo application package"""')
+                f.write("'cubicweb_foo application package'\n" + check_code)
             with open(path.join(cubedir, 'bar.py'), 'w') as f:
-                f.write('baz = 1')
+                f.write(check_code + 'baz = 1\n')
             sys.path.append(libdir)
             yield cubedir
         finally:
             sys.path.remove(libdir)
 
 
-class CubesImporterTC(unittest.TestCase):
+class CubesImporterTC(TestCase):
 
     def setUp(self):
         # During discovery, CubicWebConfiguration.cls_adjust_sys_path may be
@@ -104,6 +106,27 @@
             new = importlib.reload(cubes)
         self.assertIs(new, cubes)
 
+    def test_no_double_import(self):
+        """Check new and legacy import the same module once"""
+        with temp_cube():
+            CubicWebConfiguration.cls_adjust_sys_path()
+            with self.assertLogs('cubicweb_foo', 'WARNING') as cm:
+                from cubes.foo import bar
+                from cubicweb_foo import bar as bar2
+                self.assertIs(bar, bar2)
+                self.assertIs(sys.modules['cubes.foo'],
+                              sys.modules['cubicweb_foo'])
+            self.assertEqual(cm.output, [
+                'WARNING:cubicweb_foo:imported cubicweb_foo',
+                # module __name__ for subpackage differ along python version
+                # for PY2 it's based on how the module was imported "from
+                # cubes.foo import bar" and for PY3 based on __name__ of parent
+                # module "cubicweb_foo". Not sure if it's an issue, but PY3
+                # behavior looks better.
+                'WARNING:cubicweb_foo:imported ' + (
+                    'cubes.foo.bar' if PY2 else 'cubicweb_foo.bar')
+            ])
+
     def test_import_legacy_cube(self):
         """Check that importing a legacy cube works when sys.path got adjusted.
         """