Mercurial > p > roundup > code
changeset 8241:741ea8a86012
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 | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Mon, 30 Dec 2024 20:22:55 -0500 |
| parents | 1189c742e4b3 |
| children | 393dfc750d8b |
| files | CHANGES.txt roundup/backends/back_anydbm.py roundup/backends/rdbms_common.py roundup/cgi/client.py roundup/mlink_expr.py test/db_test_base.py test/test_liveserver.py |
| diffstat | 7 files changed, 252 insertions(+), 30 deletions(-) [+] |
line wrap: on
line diff
--- 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:
--- 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:
--- 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 = ''
--- 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
--- 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):
--- 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')
--- 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. """