Marten: Negating a predicate that contains an and operator results in an incorrect query

[kasparurban]

When negating a predicate that contains an and operator results in an incorrect query.

Example: query.Query<Player>().Where(c => !(c.Name == "Tony" && c.Level == 10)).ToArray();

Result: select d.data, d.id, d.mt_version from public.mt_doc_bug_negating_predicate_with_and_operator_player as d where not CAST(d.data ->> 'Level' as integer) = :arg0

Expected: select d.data, d.id, d.mt_version from public.mt_doc_bug_negating_predicate_with_and_operator_player as d where (not d.data ->> 'Name' = :arg0 or not CAST(d.data ->> 'Level' as integer) = :arg1)

When adding && true to the end of the predicate: query.Query<Player>().Where(c => !(c.Name == "Tony" && c.Level == 10) && true)

Results into: select d.data, d.id, d.mt_version from public.mt_doc_bug_negating_predicate_with_and_operator_player as d where (not d.data ->> 'Name' = :arg0 and not CAST(d.data ->> 'Level' as integer) = :arg1 and true) which is wrong because it should have or operator instead of and.

This is currently broken in 3.13 but works correctly in master.

Repro test here: https://gist.github.com/kasparurban/82e8c1482a19c6a9a43cb74815631c34

[jokokko]

Yikes. The scariest of LINQ related bugs, as they don't yield a runtime exception, but generate a "faulty" (incorrectly translated) but working (syntactically correct) query behind the scenes (not the first of its kind, but luckily there have been only a few [discovered]). Guess these should be given some space in docs or release notes at the very least, since they can pose quite a risk for anybody in prod & with prod data.

Edit: Going from memory, this fails in an area that I've worked with. I'm interested in how it was discovered & how it could have been avoinded @kasparurban (on avoidance, yeah theorem proving gonna be OOS).

[kasparurban]

We're using specification pattern and specs contain predicates @jokokko.
For example spec IsExpired might contain something like: entity => entity.ValidUntil != null && entity.ValidUntil <= Now.
Specifications can be combined which can result in complex predicates.
Since specifications are also negatable then !IsExpired produces:
entity => !(entity.ValidUntil != null && entity.ValidUntil <= Now).
Taking this into account then simple query like query.Find(!new IsExpired()) would produce incorrect results.

On avoidance, I quess more and better tests wouldn't hurt :). Tests should cover different and complex cases.
The more tests the easier for mistakes like this to be caught.

[kasparurban]

As I understand this is going to be fixed in version 4 but right now it is a big issue for us.
It can cause unexpected and dangerous behaviour in pretty much everywhere.

We found out that the problematic code is in _WhereClauseVisitor.cs_:

protected override Expression VisitUnary(UnaryExpression node)
{
    switch (node.NodeType)
    {
        case ExpressionType.Not:
            if (node.Operand is SubQueryExpression)
            {
                var nested = new WhereClauseVisitor(_parent, _mapping);
                nested.Visit(node.Operand);

                var @where = new NotWhereFragment(nested.ToWhereFragment());
                _register.Peek()(@where);
            }
            else
            {
                var visitor = new NotVisitor(this, _mapping, _register.Peek(), _parent._serializer);
                visitor.Visit(node);
            }

            return null;
    }

    return base.VisitUnary(node);
}

For example something like this should fix the problem:

if (node.Operand is SubQueryExpression
    || node.Operand.NodeType == ExpressionType.AndAlso
    || node.Operand.NodeType == ExpressionType.OrElse
    || node.Operand.NodeType == ExpressionType.And
    || node.Operand.NodeType == ExpressionType.Or
)

Could someone with a better knowlege of the codebase take a look at this problem?
We would really appreciate if this gets fixed in version 3.

[mysticmind]

@kasparurban we will take a look and see if we could do a bug fix release on 3.x

[mysticmind]

Quick note, this issue is not there in 4.x branch and the new linq related implementation changes have sorted this out. I will add the unit tests to 4.x branch.

[mysticmind]

Closed via https://github.com/JasperFx/marten/commit/dc817a908a817a208811a5786004a1be1751229b

[mysticmind]

FYI, bug fix release 3.13.2 is available in NuGet with fixes for this issue.