Roundup Tracker - Issues

Issue 2551374

classification
Add error handling for filter expressions
Type: behavior Severity: normal
Components: Database, Web interface Versions:
process
Status: new
:
: : rouilj
Priority: :

Created on 2024-12-02 18:37 by rouilj, last changed 2024-12-02 18:37 by rouilj.

Messages
msg8205 Author: [hidden] (rouilj) Date: 2024-12-02 18:37
Roundup has a search expression language based on an RPN grammar. E.G. -1,-2 finds all
links/multilinks that have a value (-1 -> empty, -2 -> NOT, -3 -> AND, -4 -> OR).

Errors in the RPN filter are not reported to the user. This allows invalid expressions
like: -2,... (-2 needs an argument to NOT) to be sent without an error.

Done for this ticket looks like:

  1) exception is raised for malformed expressions
  2) The exception is reported to the user in some form like:
       Your expression is incorrect. While processing the keyword filter, the
         -2 (not) operator at position 1, of "-2,1,8,-3..." no operand was found.

      that:
         1 identifies the operator,
         2 the location in the filter
         3 the property being filtered
         4 the error
         5 the whole filter

      Another example: status=3,2,-2 leaves the 3 on the stack. Currently this returns
        not(status2), but should report an error like:

         Not all values were used in the filter for status. The remaining values were
          '3' after processing the rest of the expression.

In the referenced messages I suggest creating a new ExpressionError and propagating that
back to the user.

I will add the method: NOtestFilteringLinkInvalid to db_test_base.py as an example of
running the two cases above. Code to trap/verify a raised exception TBD.
Since it starts with NO it won't be run and it currently fails if enabled (by renaming
without the NO).

Initial ideas (see msg8201, msg8202):

  Trap IndexError if a pop fails in mlink_expr.py:compile_expression
     Raise a new exception let's call it ExpressionError with the parse info (1,2,4,5 above).

  Verify that there is only one item on the stack when returning. If not raise
     ExpressionError.

  Propagate any other error up the stack.

  Have the caller trap ExpressionError  and modify the ExpressionError message adding
      the property name (3 above) and reraise.

  Make sure the error is displayed to the user.

  Any other exception operates as it does currently.

Not sure that these changes will work as I haven't run through the code but...

Excerpts from msg8201 msg8202:

====
Ralf:
> >On Mon, Dec 02, 2024 at 03:30:54AM +0000, John Rouillard wrote:
> >[...]
> >> Another example is: ``creator=3,-2,1,-2,-3``. This is the same as the
> >> expression: ``(not user3) and (not user1)``. Using the rules of logic,
> >> this is the same as: ``not (user3 or user1)`` which is expressed in
> >> RPN as ``creator=3,1,-4,-2``. Compare this to ``creator=3,1,-2`` which
> >> returns issues created by user3 or any user other than user1.
> >
> >Have actually tried the last example? I'm not sure the implicit 'OR'
> >applies when other expressions have been matched. So it may well be that
> >creator=3,1,-2 is a syntax error (and would return *all* issues) and you
> >really need the final 'OR' like in creator=3,1,-2,-4
>
> I did test it. Both link (using status) and multilink (using keyword)
> cases. I just retested manually in case I screwed up the URL
> editing. It returns what I said, all entries without user1. The 3 is
> effectively a no-op since '1,-2' includes 3 anyway.
>
> Looking at mlink_expr.py:compile_expression, I'll bet '3' is just left
> on the stack and "1,-2" is popped/returned rather than a syntax
> error. compile_expression probably should check stack depth before
> returning and if it's not 1, raise an exception.

Yes that was my hunch here, too, that when in 'expression mode' it would
not do the implicit OR.
Checking the stack depth is a good idea.

> Also since "keyword=-2,1,8,-3,-2,-3" doesn't throw an error, I suspect
> your intuition is right. (However it acts like the expression was
> "keyword=-1,-2" not sure if that's intentional.)

That would be an operator on an empty stack... we probably should check
that, too. (Ah it is caught by the bare except statement)

[...]
> I wonder if wrapping the opcodes loop in
> mlink_expr.py::compile_expression in a try/except IndexError and
> raising a new ExpressionError exception that propigates through
> rdbms_common.py:_filter_multilink_expression's bare except would work?
...
> This fix would need to be in rdbms and anydbm back ends.
> Also it needs a new issue.

Yes, I also think fixing the expression parsing deserves its own
issue... Or we just document that an expression error is silently
ignored and returns the OR of all valid issue ids.
====

See also: msg8201 msg8202
History
Date User Action Args
2024-12-02 18:56:34rouiljlinkissue2550698 superseder
2024-12-02 18:37:42rouiljcreate