From 8f68a146bfd41865acfee5b7a141440fefb7d6f5 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Fri, 28 Jul 2023 07:36:04 +0000
Subject: [PATCH] [IMP] web: check for undesirable module dependencies

Previously, we have accidentally made some core modules depend on some
modules outside of the core folder (eg views), this is undesirable as it
can make assets bundle hard to write using globs, and can make it easy
to introduce circular dependencies, it also doesn't make sense from a
code-organisation perspective.

This commit introduces a QUnit test that checks the modules in some
folders only depend on other whitelisted folders.

closes odoo/odoo#129982

Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
---
 addons/web/__manifest__.py                    |  2 +
 addons/web/static/src/boot.js                 | 21 +++---
 addons/web/static/tests/dependencies_tests.js | 65 +++++++++++++++++++
 3 files changed, 77 insertions(+), 11 deletions(-)
 create mode 100644 addons/web/static/tests/dependencies_tests.js

diff --git a/addons/web/__manifest__.py b/addons/web/__manifest__.py
index ed5c07cad23..65d154d9989 100644
--- a/addons/web/__manifest__.py
+++ b/addons/web/__manifest__.py
@@ -620,8 +620,10 @@ This module provides the core of the Odoo Web Client.
         ],
         'web.qunit_suite_tests': [
             'web/static/tests/env_tests.js',
+            'web/static/tests/dependencies_tests.js',
             'web/static/tests/core/**/*.js',
             'web/static/tests/search/**/*.js',
+            'web/static/tests/model/**/*.js',
             ('remove', 'web/static/tests/search/helpers.js'),
             'web/static/tests/views/**/*.js',
             ('remove', 'web/static/tests/views/helpers.js'),
diff --git a/addons/web/static/src/boot.js b/addons/web/static/src/boot.js
index a87fd3ebdf6..19bac157053 100644
--- a/addons/web/static/src/boot.js
+++ b/addons/web/static/src/boot.js
@@ -49,18 +49,17 @@
     odoo.__DEBUG__ = {
         didLogInfo: didLogInfoPromise,
         getDependencies: function (name, transitive) {
-            var deps = name instanceof Array ? name : [name];
-            var changed;
+            const deps = new Set();
+            let queue = [name];
             do {
-                changed = false;
-                jobDeps.forEach(function (dep) {
-                    if (deps.indexOf(dep.to) >= 0 && deps.indexOf(dep.from) < 0) {
-                        deps.push(dep.from);
-                        changed = true;
-                    }
-                });
-            } while (changed && transitive);
-            return deps;
+                queue = queue.flatMap((job) =>
+                    jobDeps.filter((dep) => dep.to === job).map((dep) => dep.from)
+                );
+                for (const dep of queue) {
+                    deps.add(dep);
+                }
+            } while (queue.length && transitive);
+            return [...deps];
         },
         getDependents: function (name) {
             return jobDeps
diff --git a/addons/web/static/tests/dependencies_tests.js b/addons/web/static/tests/dependencies_tests.js
new file mode 100644
index 00000000000..bb7b8881834
--- /dev/null
+++ b/addons/web/static/tests/dependencies_tests.js
@@ -0,0 +1,65 @@
+/** @odoo-module **/
+
+/**
+ * @param {string} folder folder that can only import from `allowedFolders`
+ * @param {string[]} allowedFolders folders from which `folder` can import
+ * @returns {{[key: string]: string[]}} an object where the keys are modules and
+ *  the values are an array of imports that the module is not allowed to import
+ */
+function invalidImportsFrom(folder, allowedFolders) {
+    // modules within a folder can always depend on one another
+    allowedFolders.push(folder);
+    const modulesToCheck = Object.keys(odoo.__DEBUG__.services).filter((module) =>
+        module.startsWith(`@web/${folder}/`)
+    );
+    const invalidDeps = {};
+    for (const module of modulesToCheck) {
+        const invalid = odoo.__DEBUG__.getDependencies(module).filter((dep) => {
+            // owl and @web/session are allowed everywhere
+            if (dep === "@odoo/owl" || dep === "@web/session") {
+                return false;
+            }
+            return !allowedFolders.some((allowed) => dep.startsWith(`@web/${allowed}/`));
+        });
+        if (invalid.length) {
+            invalidDeps[module] = invalid;
+        }
+    }
+    return invalidDeps;
+}
+
+QUnit.module("module dependencies", {}, () => {
+    QUnit.test("modules only import from allowed folders", (assert) => {
+        // Using deepEqual with {} in this test to get a good diff view when failing
+        assert.deepEqual(
+            invalidImportsFrom("core", []),
+            // FIXME: this dependency should not exist. Temporarily whitelist it so we don't add more, and remove ASAP
+            { "@web/core/utils/hooks": ["@web/env"] },
+            "Core modules don't depend on any other folder"
+        );
+        assert.deepEqual(
+            invalidImportsFrom("search", ["core"]),
+            // FIXME: this dependency should not exist. Temporarily whitelist it so we don't add more, and remove ASAP
+            { "@web/search/with_search/with_search": ["@web/webclient/actions/action_hook"] },
+            "Search modules only depend on core"
+        );
+        assert.deepEqual(
+            invalidImportsFrom("model", ["core", "search"]),
+            // FIXME: this dependency should not exist. Temporarily whitelist it so we don't add more, and remove ASAP
+            { "@web/model/model": ["@web/views/view_hook"] },
+            "Model modules only depend on core and search"
+        );
+        assert.deepEqual(
+            invalidImportsFrom("views", ["core", "search", "model"]),
+            // FIXME: these dependencies should not exist. Temporarily whitelist them so we don't add more, and remove ASAP
+            {
+                "@web/views/fields/many2one/many2one_field": [
+                    "@web/webclient/barcode/barcode_scanner",
+                ],
+                "@web/views/pivot/pivot_renderer": ["@web/legacy/js/fields/field_utils"],
+                "@web/views/view_hook": ["@web/webclient/actions/action_hook"],
+            },
+            "View modules only depend on core, search and model"
+        );
+    });
+});