Conversation
The RSQL parser appended the logical separators and the groups only to the WHERE string while sending computed property comparisons to a separate HAVING string. Two computed comparisons then ended up next to each other in HAVING with no operator between them, and a computed comparison inside a group left an empty pair of parentheses in WHERE. Both produce invalid SQL and a 500. An OR between a regular and a computed comparison could also not be represented once the expression was split. Build the whole expression, with its separators and groups, as a single string and keep it in the WHERE clause, which preserves index usage for the common case. Only when the filter references a computed property is the whole expression moved to the HAVING clause, so the comparison can resolve against the computed column and the operator precedence stays intact.
Code Review by Qodo🔴 High 1. Mixed joined filters miss assets
|
|
@cconard96 wanted your steer on this before reworking, since it is your area. The automated review above raises a fair point: moving the whole expression to HAVING when a computed field is present changes the meaning of predicates on multi-valued joined properties (like the asset groups), which need to be evaluated in WHERE before the group by. So it regresses a mixed filter such as a group condition AND a custom field, which works today. Two directions I see. One, render computed comparisons from their computation expression instead of the SELECT alias, so the whole expression stays in WHERE and nothing moves to HAVING; this keeps joined semantics and index usage, but the operator callables pre-quote the field name, so each would need to emit the RAW criterion form for computed fields. Two, keep regular and joined predicates in WHERE and computed ones in HAVING as today, but track the operators and groups in both clauses and emit a neutral 1 into the clause a predicate does not belong to; this is smaller and safe for joined semantics, but a top-level OR between a regular and a computed field would over-return rather than error, since that cannot be expressed once split. Which would you prefer? I have marked this draft meanwhile. The separator case, an unknown property between two valid comparisons, reproduces on the current code with plain fields too, so I will handle that separately. |
1 participant
Fixes #25565.
The RSQL parser built the SQL as two strings, WHERE for regular fields and HAVING for computed ones, but appended the logical separators (AND/OR) and the group parentheses only to the WHERE string. So a filter that put two computed comparisons together left them next to each other in HAVING with no operator between them, and a computed comparison inside a group left an empty pair of parentheses in WHERE. Both are invalid SQL and return a 500. On custom assets the computed fields are the custom fields, so filtering on two of them, or wrapping one in parentheses, hit this. An OR between a regular field and a computed field also could not be represented once the expression was split between the two clauses.
The parser now builds the whole expression, with its separators and groups, as a single string. It stays in the WHERE clause, so index usage is unchanged for the common all-regular case. Only when the filter references at least one computed property is the whole expression moved to the HAVING clause, where the computed columns are available and the operator precedence and grouping stay intact. This is the trade discussed in the issue: the extra cost of evaluating regular comparisons in HAVING is only paid when a computed field is actually part of the filter.
Added tests covering two computed comparisons with AND and with OR, a computed comparison in a group, and a regular field combined with a computed field with both AND and OR. They fail without the change and pass with it.