[FIX] *: domains should not have as many binary operator as there are terms

* modify client-side domain normalisation to error if the domain is
  invalid (not enough terms / segments for the number of operators)
* add better error reporting to attrs / modifiers parsing
* add a few layered error augmentation to provide clearer context
  e.g. "error: invalid domain <thing>" is helpful but "error while
  parsing modifiers for field foo: modifier invisible: invalid domain
  <thing>" is much more helpful
* rework _evalModifiers to deduplicate it in order to more easily
  implement this contextual augmentation
* test that improper domains are properly found improper
* fix a bunch of incorrect attrs domains
* also removed an apparently undefined (& unused) "options" argument
  to a _applyModifiers call

closes odoo/odoo#44642

Related: odoo/enterprise#8175
Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>

3 files changed, 34 insertions, 22 deletions

diff --git a/addons/web/static/src/js/core/domain.js b/addons/web/static/src/js/core/domain.js
index b54dd54d0b6..0db9847645d 100644
--- a/addons/web/static/src/js/core/domain.js
+++ b/addons/web/static/src/js/core/domain.js
@@ -383,8 +383,10 @@ var Domain = collections.Tree.extend({
      *                       to normalize (! will be normalized in-place)
      * @returns {Array} the normalized JS prefix-array representation of the
      *                  given domain
+     * @throws {Error} if the domain is invalid and can't be normalised
      */
     normalizeArray: function (domain) {
+        if (domain.length === 0) { return domain; }
         var expected = 1;
         _.each(domain, function (item) {
             if (item === "&" || item === "|") {
@@ -395,6 +397,11 @@ var Domain = collections.Tree.extend({
         });
         if (expected < 0) {
             domain.unshift.apply(domain, _.times(Math.abs(expected), _.constant("&")));
+        } else if (expected > 0) {
+            throw new Error(_.str.sprintf(
+                "invalid domain %s (missing %d segment(s))",
+                JSON.stringify(domain), expected
+            ));
         }
         return domain;
     },
diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js
index 1ed8c36bb98..532af65744c 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -2259,31 +2259,27 @@ var BasicModel = AbstractModel.extend({
      *   context.
      * @param {Object} modifiers
      * @returns {Object}
+     * @throws {Error} if one of the modifier domains is invalid
      */
     _evalModifiers: function (element, modifiers) {
-        var result = {};
-        var self = this;
-        var evalContext;
-        function evalModifier(mod) {
+        let evalContext = null;
+        const evaluated = {};
+        for (const k of ['invisible', 'column_invisible', 'readonly', 'required']) {
+            const mod = modifiers[k];
             if (mod === undefined || mod === false || mod === true) {
-                return !!mod;
+                if (k in modifiers) {
+                    evaluated[k] = !!mod;
+                }
+                continue;
+            }
+            try {
+                evalContext = evalContext || this._getEvalContext(element);
+                evaluated[k] = new Domain(mod, evalContext).compute(evalContext);
+            } catch (e) {
+                throw new Error(_.str.sprintf('for modifier "%s": %s', k, e.message));
             }
-            evalContext = evalContext || self._getEvalContext(element);
-            return new Domain(mod, evalContext).compute(evalContext);
-        }
-        if ('invisible' in modifiers) {
-            result.invisible = evalModifier(modifiers.invisible);
-        }
-        if ('column_invisible' in modifiers) {
-            result.column_invisible = evalModifier(modifiers.column_invisible);
-        }
-        if ('readonly' in modifiers) {
-            result.readonly = evalModifier(modifiers.readonly);
-        }
-        if ('required' in modifiers) {
-            result.required = evalModifier(modifiers.required);
         }
-        return result;
+        return evaluated;
     },
     /**
      * Fetch all name_gets for the many2ones in a group
diff --git a/addons/web/static/src/js/views/basic/basic_renderer.js b/addons/web/static/src/js/views/basic/basic_renderer.js
index 046d10a741f..ba407b766c1 100644
--- a/addons/web/static/src/js/views/basic/basic_renderer.js
+++ b/addons/web/static/src/js/views/basic/basic_renderer.js
@@ -547,6 +547,7 @@ var BasicRenderer = AbstractRenderer.extend(WidgetAdapterMixin, {
      *   value (if not given, it is set to this.mode, the mode of the renderer)
      * @returns {Object} for code efficiency, returns the last evaluated
      *   modifiers for the given node and record.
+     * @throws {Error} if one of the modifier domains is not valid
      */
     _registerModifiers: function (node, record, element, options) {
         options = options || {};
@@ -575,7 +576,15 @@ var BasicRenderer = AbstractRenderer.extend(WidgetAdapterMixin, {
 
         // Evaluate if necessary
         if (!modifiersData.evaluatedModifiers[record.id]) {
-            modifiersData.evaluatedModifiers[record.id] = record.evalModifiers(modifiersData.modifiers);
+            try {
+                modifiersData.evaluatedModifiers[record.id] = record.evalModifiers(modifiersData.modifiers);
+            } catch (e) {
+                throw new Error(_.str.sprintf(
+                    "While parsing modifiers for %s%s: %s",
+                    node.tag, node.tag === 'field' ? ' ' + node.attrs.name : '',
+                    e.message
+                ));
+            }
         }
 
         // Element might not be given yet (a second call to the function can
@@ -597,7 +606,7 @@ var BasicRenderer = AbstractRenderer.extend(WidgetAdapterMixin, {
             }
             modifiersData.elementsByRecord[record.id].push(newElement);
 
-            this._applyModifiers(modifiersData, record, newElement, options);
+            this._applyModifiers(modifiersData, record, newElement);
         }
 
         return modifiersData.evaluatedModifiers[record.id];