[IMP] core: view validation error reporting

* allow chaining tracebacks of exceptions when using view validation
  in order to provide better issue location
* provide actual error messages in get_domain_identifiers to pinpoint
  issues better than "expression is not a valid domain" which
  depending on the domain might not make it entirely clear *why* the
  expression is not a valid domain

closes odoo/odoo#40468

Signed-off-by: Raphael Collet (rco) <rco@openerp.com>

2 files changed, 31 insertions, 18 deletions

diff --git a/odoo/addons/base/models/ir_ui_view.py b/odoo/addons/base/models/ir_ui_view.py
index 1dbccb9ad57..6ddcfe2acf6 100644
--- a/odoo/addons/base/models/ir_ui_view.py
+++ b/odoo/addons/base/models/ir_ui_view.py
@@ -582,9 +582,20 @@ actual arch.
 
         return self.browse(view_ids).sudo().filtered(accessible)
 
-    def handle_view_error(self, message, raise_exception=True):
+    def handle_view_error(self, message, *, raise_exception=True, from_exception=None, from_traceback=None):
         """ Handle a view error by raising an exception or logging a warning,
         depending on the value of `raise_exception`.
+
+        :param str message: message to raise or log, augmented with contextual
+                            view information
+        :param bool raise_exception:
+            whether to raise an exception (the default) or just log a warning
+        :param BaseException from_exception:
+            when raising an exception, chain it to the provided one (default:
+            disable chaining)
+        :param types.TracebackType from_traceback:
+            when raising an exception, start with this traceback (default: start
+            at exception creation)
         """
         lines = [message]
         if self.name:
@@ -605,7 +616,7 @@ actual arch.
         formatted_message = "\n".join(lines)
         if raise_exception:
             _logger.info(formatted_message)
-            raise ValueError(formatted_message) from None
+            raise ValueError(formatted_message).with_traceback(from_traceback) from from_exception
         else:
             _logger.warning(formatted_message)
 
@@ -1387,9 +1398,9 @@ actual arch.
         """
         try:
             (field_names, var_names) = get_domain_identifiers(domain)
-        except ValueError:
+        except ValueError as e:
             msg = _('Invalid domain format while checking %s in %s')
-            self.handle_view_error(msg % (domain, key))
+            self.handle_view_error(msg % (domain, key), from_traceback=e.__traceback__)
 
         # checking field names
         for name_seq in field_names:
diff --git a/odoo/tools/view_validation.py b/odoo/tools/view_validation.py
index 8ea0ccfc1e0..b1329844515 100644
--- a/odoo/tools/view_validation.py
+++ b/odoo/tools/view_validation.py
@@ -89,6 +89,11 @@ def get_dict_asts(expr):
     return {key.s: val for key, val in zip(expr.keys, expr.values)}
 
 
+def _check(condition, explanation):
+    if not condition:
+        raise ValueError("Expression is not a valid domain: %s" % explanation)
+
+
 def get_domain_identifiers(expr):
     """ Check that the given string or AST node represents a domain expression,
     and return a pair of sets ``(fields, vars)`` where ``fields`` are the field
@@ -100,31 +105,28 @@ def get_domain_identifiers(expr):
     if isinstance(expr, str):
         expr = ast.parse(expr.strip(), mode='eval').body
 
-    def check(condition):
-        if not condition:
-            raise ValueError("Expression is not a valid domain")
-
     fnames = set()
     vnames = set()
 
     if isinstance(expr, ast.List):
-        leaves = list(ast.iter_child_nodes(expr))
-        check(isinstance(leaves.pop(), ast.Load))
-        for leaf in leaves:
-            if isinstance(leaf, ast.Str):
+        for elem in expr.elts:
+            if isinstance(elem, ast.Str):
                 # note: this doesn't check the and/or structure
-                check(leaf.s in ('&', '|', '!'))
+                _check(elem.s in ('&', '|', '!'),
+                       f"logical operators should be '&', '|', or '!', found {elem.s!r}")
             else:
-                check(isinstance(leaf, (ast.List, ast.Tuple)))
-                tuple_ = list(ast.iter_child_nodes(leaf))
-                check(isinstance(tuple_.pop(), ast.Load) and len(tuple_) == 3)
-                lhs, operator, rhs = tuple_
+                _check(isinstance(elem, (ast.List, ast.Tuple)),
+                       f"segment should be a list or tuple, found {type(elem).__name__}")
+                _check(len(elem.elts) == 3,
+                       f"segments should have 3 elements, found {len(elem.elts)}")
+                lhs, operator, rhs = elem.elts
                 if isinstance(lhs, ast.Str):
                     fnames.add(lhs.s)
                 else:
                     # limitation: we do not list fnames in this case
                     vnames.update(get_variable_names(lhs))
-                check(isinstance(operator, ast.Str))
+                _check(isinstance(operator, ast.Str),
+                       f"operator should be a string, found {type(operator).__name__}")
                 vnames.update(get_variable_names(rhs))
     else:
         # limitation: we do not list fnames in this case