[FIX] base_import_module: extract only used files and thread-safe

For performance reasons, instead of extracting the whole
zip, extract only the files which are actually used
during in the `_import_module`, in case people
put additional crap in the archive which are ignored
during the import. That way, we avoid useless I/O.

Also, adding the temporary directory in the addons
path wasn't thread-safe.
This revision changes this to make the module
import feature thread-safe.

closes odoo/odoo#80120

Signed-off-by: Pierre Masereel <pim@odoo.com>

addons/base_import_module/models/ir_module.py

@@ -7,18 +7,28 @@ import os
 import sys
 import tempfile
 import zipfile
+from collections import defaultdict
 from os.path import join as opj
 
 import odoo
 from odoo import api, fields, models, _
 from odoo.exceptions import UserError
-from odoo.modules.module import load_manifest
-from odoo.tools import convert_file, exception_to_unicode
+from odoo.modules.module import MANIFEST_NAMES
+from odoo.tools import convert_csv_import, convert_sql_import, convert_xml_import, exception_to_unicode
 
 _logger = logging.getLogger(__name__)
 
 MAX_FILE_SIZE = 100 * 1024 * 1024  # in megabytes
 
+__import_paths__ = {}
+
+def _file_open(env, path):
+    path = os.path.normcase(os.path.normpath(path))
+    import_path = __import_paths__.get(env)
+    if import_path and path.startswith(import_path):
+        return open(path, 'rb')
+    return odoo.tools.file_open(path, 'rb')
+
 
 class IrModule(models.Model):
     _inherit = "ir.module.module"
@@ -41,14 +51,18 @@ class IrModule(models.Model):
         known_mods_names = {m.name: m for m in known_mods}
         installed_mods = [m.name for m in known_mods if m.state == 'installed']
 
-        terp = load_manifest(module, mod_path=path)
+        terp = {}
+        manifest_path = next((opj(path, name) for name in MANIFEST_NAMES if os.path.exists(opj(path, name))), None)
+        if manifest_path:
+            with _file_open(self.env, manifest_path) as f:
+                terp.update(ast.literal_eval(f.read().decode()))
         if not terp:
             return False
         values = self.get_values_from_terp(terp)
         if 'version' in terp:
             values['latest_version'] = terp['version']
 
-        unmet_dependencies = set(terp['depends']).difference(installed_mods)
+        unmet_dependencies = set(terp.get('depends', [])).difference(installed_mods)
 
         if unmet_dependencies:
             if (unmet_dependencies == set(['web_studio']) and
@@ -72,7 +86,7 @@ class IrModule(models.Model):
             mode = 'init'
 
         for kind in ['data', 'init_xml', 'update_xml']:
-            for filename in terp[kind]:
+            for filename in terp.get(kind, []):
                 ext = os.path.splitext(filename)[1].lower()
                 if ext not in ('.xml', '.csv', '.sql'):
                     _logger.info("module %s: skip unsupported file %s", module, filename)
@@ -83,7 +97,13 @@ class IrModule(models.Model):
                     noupdate = True
                 pathname = opj(path, filename)
                 idref = {}
-                convert_file(self.env, module, filename, idref, mode=mode, noupdate=noupdate, kind=kind, pathname=pathname)
+                with _file_open(self.env, pathname) as fp:
+                    if ext == '.csv':
+                        convert_csv_import(self.env, module, pathname, fp.read(), idref, mode, noupdate)
+                    elif ext == '.sql':
+                        convert_sql_import(self.env, fp)
+                    elif ext == '.xml':
+                        convert_xml_import(self.env, module, fp, idref, mode, noupdate)
 
         path_static = opj(path, 'static')
         IrAttachment = self.env['ir.attachment']
@@ -91,7 +111,7 @@ class IrModule(models.Model):
             for root, dirs, files in os.walk(path_static):
                 for static_file in files:
                     full_path = opj(root, static_file)
-                    with open(full_path, 'rb') as fp:
+                    with _file_open(self.env, full_path) as fp:
                         data = base64.b64encode(fp.read())
                     url_path = '/{}{}'.format(module, full_path.split(path)[1].replace(os.path.sep, '/'))
                     if not isinstance(url_path, str):
@@ -173,10 +193,35 @@ class IrModule(models.Model):
                     raise UserError(_("File '%s' exceed maximum allowed file size", zf.filename))
 
             with tempfile.TemporaryDirectory() as module_dir:
-                import odoo.modules.module as module
                 try:
-                    odoo.addons.__path__.append(module_dir)
-                    z.extractall(module_dir)
+                    __import_paths__[self.env] = module_dir
+                    manifest_files = [
+                        file
+                        for file in z.filelist
+                        if file.filename.count('/') == 1
+                        and file.filename.split('/')[1] in MANIFEST_NAMES
+                    ]
+                    module_data_files = defaultdict(list)
+                    for manifest in manifest_files:
+                        manifest_path = z.extract(manifest, module_dir)
+                        mod_name = manifest.filename.split('/')[0]
+                        try:
+                            with _file_open(self.env, manifest_path) as f:
+                                terp = ast.literal_eval(f.read().decode())
+                        except Exception:
+                            continue
+                        for filename in terp.get('data', []) + terp.get('init_xml', []) + terp.get('update_xml', []):
+                            if os.path.splitext(filename)[1].lower() not in ('.xml', '.csv', '.sql'):
+                                continue
+                            module_data_files[mod_name].append('%s/%s' % (mod_name, filename))
+                    for file in z.filelist:
+                        filename = file.filename
+                        mod_name = filename.split('/')[0]
+                        is_data_file = filename in module_data_files[mod_name]
+                        is_static = filename.startswith('%s/static' % mod_name)
+                        if is_data_file or is_static:
+                            z.extract(file, module_dir)
+
                     dirs = [d for d in os.listdir(module_dir) if os.path.isdir(opj(module_dir, d))]
                     for mod_name in dirs:
                         module_names.append(mod_name)
@@ -189,7 +234,7 @@ class IrModule(models.Model):
                             _logger.exception('Error while importing module')
                             errors[mod_name] = exception_to_unicode(e)
                 finally:
-                    odoo.addons.__path__.remove(module_dir)
+                    __import_paths__.pop(self.env)
         r = ["Successfully imported module '%s'" % mod for mod in success]
         for mod, error in errors.items():
             r.append("Error while importing module '%s'.\n\n %s \n Make sure those modules are installed and try again." % (mod, error))

addons/base_import_module/tests/__init__.py

@@ -2,3 +2,4 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 from . import test_import_module
 from . import test_cloc
+from . import test_import_module

addons/base_import_module/tests/test_import_module.py

@@ -1,15 +1,204 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
+import base64
 import json
+import os
+import tempfile
+
 from io import BytesIO
 from zipfile import ZipFile
 
 import odoo.tests
 from odoo.tests import new_test_user
 
+
+from unittest.mock import patch
+
+from odoo.addons import __path__ as __addons_path__
+from odoo.tools import mute_logger
+
+
 @odoo.tests.tagged('post_install', '-at_install')
 class TestImportModule(odoo.tests.TransactionCase):
 
+    def import_zipfile(self, files):
+        archive = BytesIO()
+        with ZipFile(archive, 'w') as zipf:
+            for path, data in files:
+                zipf.writestr(path, data)
+        return self.env['ir.module.module'].import_zipfile(archive)
+
+    def test_import_zip(self):
+        """Assert the behaviors expected by the module import feature using a ZIP archive"""
+        files = [
+            ('foo/__manifest__.py', b"{'data': ['data.xml', 'res.partner.csv', 'data.sql']}"),
+            ('foo/data.xml', b"""
+                <data>
+                    <record id="foo" model="res.partner">
+                        <field name="name">foo</field>
+                    </record>
+                </data>
+            """),
+            ('foo/res.partner.csv',
+                b'"id","name"\n' \
+                b'bar,bar'
+            ),
+            ('foo/data.sql', b"INSERT INTO res_partner (active, name) VALUES (true, 'baz');"),
+            ('foo/static/css/style.css', b".foo{color: black;}"),
+            ('foo/static/js/foo.js', b"console.log('foo')"),
+            ('bar/__manifest__.py', b"{'data': ['data.xml']}"),
+            ('bar/data.xml', b"""
+                <data>
+                    <record id="foo" model="res.country">
+                        <field name="name">foo</field>
+                        <field name="code">XX</field>
+                    </record>
+                </data>
+            """),
+        ]
+        self.import_zipfile(files)
+        self.assertEqual(self.env.ref('foo.foo')._name, 'res.partner')
+        self.assertEqual(self.env.ref('foo.foo').name, 'foo')
+        self.assertEqual(self.env.ref('foo.bar')._name, 'res.partner')
+        self.assertEqual(self.env.ref('foo.bar').name, 'bar')
+        self.assertEqual(self.env['res.partner'].search_count([('name', '=', 'baz')]), 1)
+
+        self.assertEqual(self.env.ref('bar.foo')._name, 'res.country')
+        self.assertEqual(self.env.ref('bar.foo').name, 'foo')
+
+        for path, data in files:
+            if path.split('/')[1] == 'static':
+                static_attachment = self.env['ir.attachment'].search([('url', '=', '/%s' % path)])
+                self.assertEqual(static_attachment.name, os.path.basename(path))
+                self.assertEqual(static_attachment.datas, base64.b64encode(data))
+
+    def test_import_zip_invalid_manifest(self):
+        """Assert the expected behavior when import a ZIP module with an invalid manifest"""
+        files = [
+            ('foo/__manifest__.py', b"foo")
+        ]
+        with mute_logger("odoo.addons.base_import_module.models.ir_module"):
+            result = self.import_zipfile(files)
+        self.assertIn("Error while importing module 'foo'", result[0])
+
+    def test_import_zip_data_not_in_manifest(self):
+        """Assert a data file not mentioned in the manifest is not imported"""
+        files = [
+            ('foo/__manifest__.py', b"{'data': ['foo.xml']}"),
+            ('foo/foo.xml', b"""
+                <data>
+                    <record id="foo" model="res.partner">
+                        <field name="name">foo</field>
+                    </record>
+                </data>
+            """),
+            ('foo/bar.xml', b"""
+                <data>
+                    <record id="bar" model="res.partner">
+                        <field name="name">bar</field>
+                    </record>
+                </data>
+            """),
+        ]
+        self.import_zipfile(files)
+        self.assertEqual(self.env.ref('foo.foo').name, 'foo')
+        self.assertFalse(self.env.ref('foo.bar', raise_if_not_found=False))
+
+    def test_import_zip_ignore_unexpected_data_extension(self):
+        """Assert data files using an unexpected extensions are correctly ignored"""
+        files = [
+            ('foo/__manifest__.py', b"{'data': ['res.partner.xls']}"),
+            ('foo/res.partner.xls',
+                b'"id","name"\n' \
+                b'foo,foo'
+            ),
+        ]
+        with self.assertLogs('odoo.addons.base_import_module.models.ir_module') as log_catcher:
+            self.import_zipfile(files)
+            self.assertEqual(len(log_catcher.output), 1)
+            self.assertIn('module foo: skip unsupported file res.partner.xls', log_catcher.output[0])
+            self.assertFalse(self.env.ref('foo.foo', raise_if_not_found=False))
+
+    def test_import_zip_extract_only_useful(self):
+        """Assert only the data and static files are extracted of the ZIP archive during the module import"""
+        files = [
+            ('foo/__manifest__.py', b"{'data': ['data.xml', 'res.partner.xls']}"),
+            ('foo/data.xml', b"""
+                <data>
+                    <record id="foo" model="res.partner">
+                        <field name="name">foo</field>
+                    </record>
+                </data>
+            """),
+            ('foo/res.partner.xls',
+                b'"id","name"\n' \
+                b'foo,foo'
+            ),
+            ('foo/static/css/style.css', b".foo{color: black;}"),
+            ('foo/foo.py', b"foo = 42"),
+        ]
+        extracted_files = []
+        addons_path = []
+        origin_import_module = type(self.env['ir.module.module'])._import_module
+        def _import_module(self, *args, **kwargs):
+            _module, path = args
+            for root, _dirs, files in os.walk(path):
+                for file in files:
+                    extracted_files.append(os.path.relpath(os.path.join(root, file), path))
+            addons_path.extend(__addons_path__)
+            return origin_import_module(self, *args, **kwargs)
+        with patch.object(type(self.env['ir.module.module']), '_import_module', _import_module):
+            self.import_zipfile(files)
+        self.assertIn(
+            '__manifest__.py', extracted_files,
+            "__manifest__.py must be in the extracted files")
+        self.assertIn(
+            'data.xml', extracted_files,
+            "data.xml must be in the extracted files as its in the manifest's data")
+        self.assertIn(
+            'static/css/style.css', extracted_files,
+            "style.css must be in the extracted files as its in the static folder")
+        self.assertNotIn(
+            'res.partner.xls', extracted_files,
+            "res.partner.xls must not be in the extracted files as it uses an unsupported extension of data file")
+        self.assertNotIn(
+            'foo.py', extracted_files,
+            "foo.py must not be in the extracted files as its not the manifest's data")
+        self.assertFalse(
+            set(addons_path).difference(__addons_path__),
+            'No directory must be added in the addons path during import')
+
+    def test_import_module_addons_path(self):
+        """Assert it's possible to import a module using directly `_import_module` without zip from the addons path"""
+        files = [
+            ('foo/__manifest__.py', b"{'data': ['data.xml']}"),
+            ('foo/data.xml', b"""
+                <data>
+                    <record id="foo" model="res.partner">
+                        <field name="name">foo</field>
+                    </record>
+                </data>
+            """),
+            ('foo/static/css/style.css', b".foo{color: black;}"),
+        ]
+        with tempfile.TemporaryDirectory() as module_dir:
+            for path, data in files:
+                os.makedirs(os.path.join(module_dir, os.path.dirname(path)), exist_ok=True)
+                with open(os.path.join(module_dir, path), 'wb') as fp:
+                    fp.write(data)
+            try:
+                __addons_path__.append(module_dir)
+                self.env['ir.module.module']._import_module('foo', os.path.join(module_dir, 'foo'))
+            finally:
+                __addons_path__.remove(module_dir)
+
+        self.assertEqual(self.env.ref('foo.foo')._name, 'res.partner')
+        self.assertEqual(self.env.ref('foo.foo').name, 'foo')
+        static_path, static_data = files[2]
+        static_attachment = self.env['ir.attachment'].search([('url', '=', '/%s' % static_path)])
+        self.assertEqual(static_attachment.name, os.path.basename(static_path))
+        self.assertEqual(static_attachment.datas, base64.b64encode(static_data))
+
     def test_import_and_uninstall_module(self):
         bundle = 'web.assets_backend'
         path = '/test_module/static/src/js/test.js'