On Mon, Dec 02, 2024 at 05:15:20PM +0000, John Rouillard wrote:
> We still need tests right? (From discussion below, the answer is yes.)

I *did* make a lot of tests for this feature quite some time ago.
But, yes, we probably need to at least make sure that the documented
behavior is tested.
In particular it doesn't have many tests with malformed expressions.

> >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)

> It looks like rdbms_common.py:_filter_multilink_expression has a bare
> except statement.
> 
>         except:
>             # fallback behavior when expression parsing above fails
>             orclause = ''
> 
> When IndexError is raised by popping a non-existent operand to the
> leading '-2' operator, the error is caught here and suppressed. It
> looks like this except statement also handles the case where there are
> no operators as it seems to be building a default 'or' expression.

Ah yes, I remember.

> 
> 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.
I'm not sure the HTML for the expressions can only return valid
expressions, I've never used that.

> 
> Also a more interesting example could be:
> 
>   keyword=-1,-2,1,8,-3,-2,-3

Nice!

Thanks
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com