fix: issue2551374. Error handling for filter expressions.

Errors in filter expressions are now reported. The UI needs some work
but even the current code is helpful when debugging filter
expressions.

mlink_expr:

  defines/raises ExpressionError(error string template,
     context=dict())

  raises ExpressionError when it detects errors when popping arguments
  off stack

  raises ExpressionError when more than one element left on the stack
  before returning

  also ruff fix to group boolean expression with parens

back_anydbm.py, rdbms_common.py:

  catches ExpressionError, augments context with class and
  attribute being searched. raises the exception

  for both link and multilink relations

client.py

  catches ExpressionError returning a basic error page. The page is a
  dead end. There are no links or anything for the user to move
  forward. The user has to go back, possibly refresh the page (because
  the submit button may be disalbled) re-enter the query and try
  again.

  This needs to be improved.

test_liveserver.py

  test the error page generated by client.py

db_test_base

  unit tests for filter with too few arguments, too many arguments,
  check all repr and str formats.

Author: rouilj

CHANGES.txt

@@ -58,6 +58,10 @@ Fixed:
   it the default. (John Rouillard)
 - fixed a crash with roundup-admin perftest password when rounds not set
   on command line. (John Rouillard)
+- issue2551374 - Add error handling for filter expressions. Filter
+  expression errors are now reported. The UI needs some work but
+  even the current code is helpful when debugging filter
+  expressions. (John Rouillard)
 
 Features:
 

roundup/backends/back_anydbm.py

@@ -34,7 +34,7 @@
 from roundup.anypy.strings import b2s, bs2b, repr_export, eval_import, is_us
 
 from roundup import hyperdb, date, password, roundupdb, security, support
-from roundup.mlink_expr import Expression
+from roundup.mlink_expr import Expression, ExpressionError
 from roundup.backends import locking
 from roundup.i18n import _
 
@@ -1880,7 +1880,12 @@ def _filter(self, search_matches, filterspec, proptree,
                     if t == LINK:
                         # link - if this node's property doesn't appear in the
                         # filterspec's nodeid list, skip it
-                        expr = Expression(v, is_link=True)
+                        try:
+                            expr = Expression(v, is_link=True)
+                        except ExpressionError as e:
+                            e.context['class'] = cn
+                            e.context['attr'] = k
+                            raise
                         if expr.evaluate(nv):
                             match = 1
                     elif t == MULTILINK:
@@ -1895,7 +1900,12 @@ def _filter(self, search_matches, filterspec, proptree,
                         else:
                             # otherwise, make sure this node has each of the
                             # required values
-                            expr = Expression(v)
+                            try:
+                                expr = Expression(v)
+                            except ExpressionError as e:
+                                e.context['class'] = cn
+                                e.context['attr'] = k
+                                raise
                             if expr.evaluate(nv):
                                 match = 1
                     elif t == STRING:

roundup/backends/rdbms_common.py

@@ -72,7 +72,7 @@
 from roundup.hyperdb import String, Password, Date, Interval, Link, \
     Multilink, DatabaseError, Boolean, Number, Integer
 from roundup.i18n import _
-from roundup.mlink_expr import compile_expression
+from roundup.mlink_expr import compile_expression, ExpressionError
 
 # dummy value meaning "argument not passed"
 _marker = []
@@ -2594,6 +2594,10 @@ def collect_values(n):
                     values.append(n.x)
             expr.visit(collect_values)
             return w, values
+        except ExpressionError as e:
+            e.context['class'] = pln
+            e.context['attr'] = prp
+            raise
         except:
             pass
         # Fallback to original code
@@ -2671,6 +2675,10 @@ def collect_values(n):
             expr.visit(collect_values)
 
             return intron, values
+        except ExpressionError as e:
+            e.context['class'] = classname
+            e.context['attr'] = proptree.name
+            raise
         except:
             # fallback behavior when expression parsing above fails
             orclause = ''

roundup/cgi/client.py

@@ -54,6 +54,9 @@ class SysCallError(Exception):
     Unauthorised,
     UsageError,
 )
+
+from roundup.mlink_expr import ExpressionError
+
 from roundup.mailer import Mailer, MessageSendError
 
 logger = logging.getLogger('roundup')
@@ -2216,6 +2219,9 @@ def renderContext(self):
                 result = pt.render(self, None, None, **args)
             except IndexerQueryError as e:
                 result = self.renderError(e.args[0])
+            except ExpressionError as e:
+                self.add_error_message(str(e))
+                result = self.renderError(str(e))
 
             if 'Content-Type' not in self.additional_headers:
                 self.additional_headers['Content-Type'] = pt.content_type

roundup/mlink_expr.py

@@ -7,6 +7,57 @@
 # see the COPYING.txt file coming with Roundup.
 #
 
+from roundup.exceptions import RoundupException
+from roundup.i18n import _
+
+opcode_names = {
+    -2: "not",
+    -3: "and",
+    -4: "or",
+}
+
+
+class ExpressionError(RoundupException):
+    """Takes two arguments.
+
+        ExpressionError(template, context={})
+
+    The repr of ExpressionError is:
+
+        template % context
+
+    """
+
+    # only works on python 3
+    #def __init__(self, *args, context=None):
+    #    super().__init__(*args)
+    #    self.context = context if isinstance(context, dict) else {}
+
+    # works python 2 and 3
+    def __init__(self, *args, **kwargs):
+        super(RoundupException, self).__init__(*args)
+        self.context = {}
+        if 'context' in kwargs and isinstance(kwargs['context'], dict):
+            self.context = kwargs['context']
+
+        # Skip testing for a bad call to ExpressionError
+        # keywords = [x for x in list(kwargs) if x != "context"]
+        #if len(keywords) != 0:
+        #        raise ValueError("unknown keyword argument(s) passed to ExpressionError: %s" % keywords)
+
+    def __str__(self):
+        try:
+            return self.args[0] % self.context
+        except KeyError:
+            return "%s: context=%s" % (self.args[0], self.context)
+
+    def __repr__(self):
+        try:
+            return self.args[0] % self.context
+        except KeyError:
+            return "%s: context=%s" % (self.args[0], self.context)
+
+
 class Binary:
 
     def __init__(self, x, y):
@@ -38,6 +89,9 @@ def evaluate(self, v):
     def visit(self, visitor):
         visitor(self)
 
+    def __repr__(self):
+        return "Value %s" % self.x
+
 
 class Empty(Unary):
 
@@ -47,6 +101,9 @@ def evaluate(self, v):
     def visit(self, visitor):
         visitor(self)
 
+    def __repr__(self):
+        return "ISEMPTY(-1)"
+
 
 class Not(Unary):
 
@@ -56,6 +113,9 @@ def evaluate(self, v):
     def generate(self, atom):
         return "NOT(%s)" % self.x.generate(atom)
 
+    def __repr__(self):
+        return "NOT(%s)" % self.x
+
 
 class Or(Binary):
 
@@ -67,6 +127,9 @@ def generate(self, atom):
             self.x.generate(atom),
             self.y.generate(atom))
 
+    def __repr__(self):
+        return "(%s OR %s)" % (self.y, self.x)
+
 
 class And(Binary):
 
@@ -78,17 +141,44 @@ def generate(self, atom):
             self.x.generate(atom),
             self.y.generate(atom))
 
+    def __repr__(self):
+        return "(%s AND %s)" % (self.y, self.x)
+
 
 def compile_expression(opcodes):
 
     stack = []
     push, pop = stack.append, stack.pop
-    for opcode in opcodes:
-        if   opcode == -1: push(Empty(opcode))      # noqa: E271,E701
-        elif opcode == -2: push(Not(pop()))         # noqa: E701
-        elif opcode == -3: push(And(pop(), pop()))  # noqa: E701
-        elif opcode == -4: push(Or(pop(), pop()))   # noqa: E701
-        else:              push(Equals(opcode))     # noqa: E701
+    try:
+        for position, opcode in enumerate(opcodes):     # noqa: B007
+            if   opcode == -1: push(Empty(opcode))      # noqa: E271,E701
+            elif opcode == -2: push(Not(pop()))         # noqa: E701
+            elif opcode == -3: push(And(pop(), pop()))  # noqa: E701
+            elif opcode == -4: push(Or(pop(), pop()))   # noqa: E701
+            else:              push(Equals(opcode))     # noqa: E701
+    except IndexError:
+        raise ExpressionError(
+            _("There was an error searching %(class)s by %(attr)s using: "
+              "%(opcodes)s. "
+              "The operator %(opcode)s (%(opcodename)s) at position "
+              "%(position)d has too few arguments."),
+            context={
+                "opcode": opcode,
+                "opcodename": opcode_names[opcode],
+                "position": position + 1,
+                "opcodes": opcodes,
+            })
+    if len(stack) != 1:
+        # Too many arguments - I don't think stack can be zero length
+        raise ExpressionError(
+            _("There was an error searching %(class)s by %(attr)s using: "
+              "%(opcodes)s. "
+              "There are too many arguments for the existing operators. The "
+              "values on the stack are: %(stack)s"),
+            context={
+                "opcodes": opcodes,
+                "stack": stack,
+            })
 
     return pop()
 
@@ -104,7 +194,7 @@ def __init__(self, v, is_link=False):
             compiled = compile_expression(opcodes)
             if is_link:
                 self.evaluate = lambda x: compiled.evaluate(
-                    x and [int(x)] or [])
+                    (x and [int(x)]) or [])
             else:
                 self.evaluate = lambda x: compiled.evaluate([int(y) for y in x])
         except (ValueError, TypeError):

test/db_test_base.py

@@ -41,6 +41,7 @@
 from roundup.cgi.engine_zopetal import RoundupPageTemplate
 from roundup.cgi.templating import HTMLItem
 from roundup.exceptions import UsageError, Reject
+from roundup.mlink_expr import ExpressionError
 
 from roundup.anypy.strings import b2s, s2b, u2s
 from roundup.anypy.cmp_ import NoneAndDictComparable
@@ -2048,6 +2049,43 @@ def testFilteringLink(self):
             ae(filt(None, {a: ['-1', None]}, ('+','id'), grp), ['3','4'])
             ae(filt(None, {a: ['1', None]}, ('+','id'), grp), ['1', '3','4'])
 
+    def testFilteringBrokenLinkExpression(self):
+        ae, iiter = self.filteringSetup()
+        a = 'assignedto'
+        for filt in iiter():
+            with self.assertRaises(ExpressionError) as e:
+                filt(None, {a: ['1', '-3']}, ('+','status'))
+            self.assertIn("position 2", str(e.exception))
+            # verify all tokens are consumed
+            self.assertNotIn("%", str(e.exception))
+            self.assertNotIn("%", repr(e.exception))
+
+            with self.assertRaises(ExpressionError) as e:
+                filt(None, {a: ['0', '1', '2', '3', '-3', '-4']},
+                     ('+','status'))
+            self.assertIn("values on the stack are: [Value 0,",
+                          str(e.exception))
+            self.assertNotIn("%", str(e.exception))
+            self.assertNotIn("%", repr(e.exception))
+
+            with self.assertRaises(ExpressionError) as e:
+                # expression tests _repr_ for every operator and
+                # three values
+                filt(None, {a: ['-1', '1', '2', '3', '-3', '-2', '-4']},
+                     ('+','status'))
+            result = str(e.exception)
+            self.assertIn(" AND ", result)
+            self.assertIn(" OR ", result)
+            self.assertIn("NOT(", result)
+            self.assertIn("ISEMPTY(-1)", result)
+            # trigger a KeyError and verify fallback format
+            # is correct. It includes the template with %(name)s tokens,
+            del(e.exception.context['class'])
+            self.assertIn("values on the stack are: %(stack)s",
+                          str(e.exception))
+            self.assertIn("values on the stack are: %(stack)s",
+                          repr(e.exception))
+
     def testFilteringLinkExpression(self):
         ae, iiter = self.filteringSetup()
         a = 'assignedto'
@@ -2074,25 +2112,6 @@ def testFilteringLinkExpression(self):
             ae(filt(None, {a: ['1','-2','2','-2','-4']}, ('+',a)),
                 ['3','4','1','2'])
 
-    def NOtestFilteringLinkInvalid(self):
-        """Test invalid filter expressions.
-           Code must be written to generate an exception for these.
-           Then this code must be changed to test the exception.
-           See issue 2551374 in the roundup tracker.
-        """
-        ae, iiter = self.filteringSetup()
-        a = 'assignedto'
-        # filt(??, filter expression, sort order)
-        # ae = self.assertEqual(val1, val2)
-        for filt in iiter():
-            # -2 is missing an operand
-            ae(filt(None, {a: ['-2','2','-2','-4']},
-                    ('+',a)), [])
-            # 3 is left on the stack
-            ae(filt(None, {a: ['3','2','-2']},
-                    ('+',a)), [])
-
-
     def testFilteringRevLink(self):
         ae, iiter = self.filteringSetupTransitiveSearch('user')
         # We have
@@ -2228,6 +2247,56 @@ def testFilteringMultilink(self):
             ae(filt(None, {'nosy': ['1','2']}, ('+', 'status'),
                 ('-', 'deadline')), ['4', '3'])
 
+    def testFilteringBrokenMultilinkExpression(self):
+        ae, iiter = self.filteringSetup()
+        kw1 = self.db.keyword.create(name='Key1')
+        kw2 = self.db.keyword.create(name='Key2')
+        kw3 = self.db.keyword.create(name='Key3')
+        kw4 = self.db.keyword.create(name='Key4')
+        self.db.issue.set('1', keywords=[kw1, kw2])
+        self.db.issue.set('2', keywords=[kw1, kw3])
+        self.db.issue.set('3', keywords=[kw2, kw3, kw4])
+        self.db.issue.set('4', keywords=[kw1, kw2, kw4])
+        self.db.commit()
+        kw = 'keywords'
+        for filt in iiter():
+            with self.assertRaises(ExpressionError) as e:
+                filt(None, {kw: ['1', '-3']}, ('+','status'))
+            self.assertIn("searching issue by keywords", str(e.exception))
+            self.assertIn("position 2", str(e.exception))
+            # verify all tokens are consumed
+            self.assertNotIn("%", str(e.exception))
+            self.assertNotIn("%", repr(e.exception))
+
+            with self.assertRaises(ExpressionError) as e:
+                filt(None, {kw: ['0', '1', '2', '3', '-3', '-4']},
+                     ('+','status'))
+            self.assertIn("searching issue by keywords", str(e.exception))
+            self.assertIn("values on the stack are: [Value 0,",
+                          str(e.exception))
+            self.assertNotIn("%", str(e.exception))
+            self.assertNotIn("%", repr(e.exception))
+
+            with self.assertRaises(ExpressionError) as e:
+                # expression tests _repr_ for every operator and
+                # three values
+                filt(None, {kw: ['-1', '1', '2', '3', '-3', '-2', '-4']},
+                     ('+','status'))
+            result = str(e.exception)
+            self.assertIn("searching issue by keywords", result)
+            self.assertIn(" AND ", result)
+            self.assertIn(" OR ", result)
+            self.assertIn("NOT(", result)
+            self.assertIn("ISEMPTY(-1)", result)
+            # trigger a KeyError and verify fallback format
+            # is correct. It includes the template with %(name)s tokens,
+            del(e.exception.context['class'])
+            self.assertIn("values on the stack are: %(stack)s",
+                          str(e.exception))
+            self.assertIn("values on the stack are: %(stack)s",
+                          repr(e.exception))
+
+
     def testFilteringMultilinkExpression(self):
         ae, iiter = self.filteringSetup()
         kw1 = self.db.keyword.create(name='Key1')