# HG changeset patch # User John Rouillard # Date 1735608175 18000 # Node ID 741ea8a86012d3216274d437d72198b432dc6b8c # Parent 1189c742e4b3e5b39675efa30542566ec6195874 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. diff -r 1189c742e4b3 -r 741ea8a86012 CHANGES.txt --- a/CHANGES.txt Mon Dec 30 02:59:27 2024 -0500 +++ b/CHANGES.txt Mon Dec 30 20:22:55 2024 -0500 @@ -58,6 +58,10 @@ 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: diff -r 1189c742e4b3 -r 741ea8a86012 roundup/backends/back_anydbm.py --- a/roundup/backends/back_anydbm.py Mon Dec 30 02:59:27 2024 -0500 +++ b/roundup/backends/back_anydbm.py Mon Dec 30 20:22:55 2024 -0500 @@ -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 @@ 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 @@ 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: diff -r 1189c742e4b3 -r 741ea8a86012 roundup/backends/rdbms_common.py --- a/roundup/backends/rdbms_common.py Mon Dec 30 02:59:27 2024 -0500 +++ b/roundup/backends/rdbms_common.py Mon Dec 30 20:22:55 2024 -0500 @@ -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 @@ 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 @@ 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 = '' diff -r 1189c742e4b3 -r 741ea8a86012 roundup/cgi/client.py --- a/roundup/cgi/client.py Mon Dec 30 02:59:27 2024 -0500 +++ b/roundup/cgi/client.py Mon Dec 30 20:22:55 2024 -0500 @@ -54,6 +54,9 @@ Unauthorised, UsageError, ) + +from roundup.mlink_expr import ExpressionError + from roundup.mailer import Mailer, MessageSendError logger = logging.getLogger('roundup') @@ -2216,6 +2219,9 @@ 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 diff -r 1189c742e4b3 -r 741ea8a86012 roundup/mlink_expr.py --- a/roundup/mlink_expr.py Mon Dec 30 02:59:27 2024 -0500 +++ b/roundup/mlink_expr.py Mon Dec 30 20:22:55 2024 -0500 @@ -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 visit(self, visitor): visitor(self) + def __repr__(self): + return "Value %s" % self.x + class Empty(Unary): @@ -47,6 +101,9 @@ def visit(self, visitor): visitor(self) + def __repr__(self): + return "ISEMPTY(-1)" + class Not(Unary): @@ -56,6 +113,9 @@ 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 @@ 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 @@ 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 @@ 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): diff -r 1189c742e4b3 -r 741ea8a86012 test/db_test_base.py --- a/test/db_test_base.py Mon Dec 30 02:59:27 2024 -0500 +++ b/test/db_test_base.py Mon Dec 30 20:22:55 2024 -0500 @@ -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 @@ 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 @@ 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 @@ 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') diff -r 1189c742e4b3 -r 741ea8a86012 test/test_liveserver.py --- a/test/test_liveserver.py Mon Dec 30 02:59:27 2024 -0500 +++ b/test/test_liveserver.py Mon Dec 30 20:22:55 2024 -0500 @@ -351,6 +351,41 @@ # of the footer is missing. self.assertNotIn('out of', f.text) + def test_broken_query(self): + # query link item + current_user_query = ( + "@columns=title,id,activity,status,assignedto&" + "@sort=activity&@group=priority&@filter=creator&" + "@pagesize=50&@startwith=0&creator=-2&" + "@dispname=Test1") + + session, _response = self.create_login_session() + f = session.get(self.url_base()+'/issue?' + current_user_query) + + # verify the query has run by looking for the query name + # print(f.text) + self.assertIn('Error when searching issue by creator using: ' + '[-2]. The operator -2 (not) at position 1 has ' + 'too few arguments.', + f.text) + + def test_broken_multiink_query(self): + # query multilink item + current_user_query = ( + "@columns=title,id,activity,status,assignedto" + "&keyword=-3&@sort=activity&@group=priority" + "&@pagesize=50&@startwith=0&@template=index|search" + "&@action=search") + session, _response = self.create_login_session() + f = session.get(self.url_base()+'/issue?' + current_user_query) + + # verify the query has run by looking for the query name + print(f.text) + self.assertIn('Error when searching issue by keyword using: ' + '[-3]. The operator -3 (and) at position 1 has ' + 'too few arguments.', + f.text) + def test_start_page(self): """ simple test that verifies that the server can serve a start page. """