Orm: Condition using "CASE WHEN" not expecting "IN" keyword

[Tony-Esales]

Bug Report

DQL:
SELECT cg FROM \App\Entity\Common\CompanyGroup cg WHERE CASE WHEN cg.id = 1 THEN 0 WHEN cg.id = 2 THEN 1 ELSE 3 END IN (3)
Error:
[Syntax Error] line 0, col 116: Error: Expected =, <, <=, <>, >, >=, !=, got 'IN'

If I change "IN (3)" for "= 3" it works.

Note: the DQL above is not my real need, it's just a simple snippet of code to demonstrate the bug. The entity used can have just one field called "id" to run the example.

BC Break: no (I guess)
Version: doctrine/orm 2.6.3

Summary

Doctrine should accept "IN" operator when the left operand IS a "CASE WHEN" (DQL)

Current behavior

Doctrine is not accepting "IN" when the left operand IS a "CASE WHEN"

How to reproduce

$q = $em->createQuery('SELECT cg FROM \App\Entity\Common\CompanyGroup cg WHERE CASE WHEN cg.id = 1 THEN 0 WHEN cg.id = 2 THEN 1 ELSE 3 END IN (3)');
$cgs = $q->getResult();

Expected behavior

The DQL above run without any error

[lcobucci]

@Tony-Esales this is the expected behaviour and your DQL is invalid.

A CaseExpression is not a SimpleConditionalExpression, thus it can't be used in the WHERE clause.

You can achieve the desired behaviour by using NOT IN() with the field (no CaseExpression):

SELECT 
    cg
FROM \App\Entity\Common\CompanyGroup cg
WHERE cg.id NOT IN (1, 3)

Or use a HIDDEN field:

SELECT 
    cg,
    CASE WHEN cg.id = 1 THEN 0 WHEN cg.id = 2 THEN 1 ELSE 3 END AS HIDDEN some_identifier
FROM \App\Entity\Common\CompanyGroup cg
WHERE some_identifier IN (3)

[Tony-Esales]

@lcobucci a CaseExpression is not a SimpleConditionalExpression, but it can be part of a SimpleConditionalExpression. In this case it's the left operand of it.

Your solution using the HIDDEN field will only work for MySQL because using aliased field defined in the SELECT portion is not SQL standard. In my case, I'm using Postgres and it's not allowed.

Your first solution doesn't fit my need. As I mentioned in the issue description, this DQL is not the real one. It's just a very simple example to reproduce my problem. My real need is to filter by a computed value using a bunch of table fields (that's the why I used a CASE WHEN).

[lcobucci]

@lcobucci a CaseExpression is not a SimpleConditionalExpression, but it can be part of a SimpleConditionalExpression. In this case it's the left operand of it.

From a SQL point of view you're correct but that's not applicable in DQL, that's why I sent you those links.

Your solution using the HIDDEN field will only work for MySQL because using aliased field defined in the SELECT portion is not SQL standard. In my case, I'm using Postgres and it's not allowed.

You're completely correct. Using HAVING is the SQL standard way of handling this, though.

[Tony-Esales]

@lcobucci

SimpleConditionalExpression ::= ComparisonExpression | BetweenExpression | LikeExpression |
                                InExpression | NullComparisonExpression | ExistsExpression |
                                EmptyCollectionComparisonExpression | CollectionMemberExpression |
                                InstanceOfExpression

InExpression ::= SingleValuedPathExpression ["NOT"] "IN" "(" (InParameter {"," InParameter}* | Subselect) ")"

What I'm asking is to change the InExpression to

InExpression ::= (SingleValuedPathExpression | CaseExpression) ["NOT"] "IN" "(" (InParameter {"," InParameter}* | Subselect) ")"

Maybe it's not a bug, but an improvement request. Should I change the issue type? If so, how can I do that?

[lcobucci]

@Tony-Esales the main thing here is that we're redesigning many things for the ORM v3 and the parser is potentially in this list, which means that adding more features now will probably make things more complicated to get v3 released.

However you can use the HIDDEN field + HAVING clause as of now and deliver the things you need.

If you decide that you really believe that this is feature you're missing, we can close this bug report and open a feature request (which, if accepted, will be released on v3.0 or v3.1).

[Tony-Esales]

@Tony-Esales the main thing here is that we're redesigning many things for the ORM v3 and the parser is potentially in this list, which means that adding more features now will probably make things more complicated to get v3 released.

OK.

However you can use the HIDDEN field + HAVING clause as of now and deliver the things you need.

It doesn't work in Postgres. Actually, I think it doesn't work in any existing RDBMS since HAVING is supposed to be used to filter using aggregate functions.

[lcobucci]

@Tony-Esales you're correct, have you checked if the generated SQL doesn't handle things properly (hidden + where)?

[Tony-Esales]

@Tony-Esales you're correct, have you checked if the generated SQL doesn't handle things properly (hidden + where)?

Yes, I did. I stated it before:

Your solution using the HIDDEN field will only work for MySQL because using aliased field defined in the SELECT portion is not SQL standard. In my case, I'm using Postgres and it's not allowed.

DQL:
SELECT cg, CASE WHEN cg.id = 1 THEN 0 WHEN cg.id = 2 THEN 1 ELSE 3 END AS HIDDEN teste FROM \App\Entity\Common\CompanyGroup cg WHERE teste IN (3)

SQL (generated):
SELECT c0_.id AS id_0, c0_.acting AS acting_1, c0_.name AS name_2, c0_.about AS about_3, c0_.rntrc AS rntrc_4, c0_.active AS active_5, c0_.phone AS phone_6, c0_.email AS email_7, c0_.site AS site_8, CASE WHEN c0_.id = 1 THEN 0 WHEN c0_.id = 2 THEN 1 ELSE 3 END AS sclr_9, c0_.id_file_logo AS id_file_logo_10 FROM common.company_group c0_ WHERE sclr_9 IN (3)

Error from Postgres:

ERROR:  column "sclr_9" does not exist
LINHA 1: ..._file_logo_10 FROM common.company_group c0_ WHERE sclr_9 IN ...

[lcobucci]

Right, I'm sorry for the whole misunderstanding.

That DQL should work regardless of platform IMHO, since that's how things are documented. It should create a sub query or translate the expression to what the platform understands.

@morozov what do you think about this?

[Tony-Esales]

I write some code to solve the problem. In the file:
https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/Parser.php
replace the content in the line 2542 (just a "}") with

        } elseif ($token['type'] === Lexer::T_CASE || ($token['type'] === Lexer::T_OPEN_PARENTHESIS && $peek['type'] === Lexer::T_CASE)) {
            // Need to discover if it's a regular or an "in" comparison
            do {
                $token = $this->lexer->peek();
            } while ($token['type'] != Lexer::T_END);
            while ($token = $this->lexer->peek()) {
                if ($token['type'] !== Lexer::T_CLOSE_PARENTHESIS) {
                    break;
                }
            }
            $lookahead = $this->lexer->peek();
            $this->lexer->resetPeek();
        }

Can someone review this code and apply it to the current stable version (2.6.3)?

[greg0ire]

Please consider creating a pull request, it's the best way to get reviews, see if your changes break the test suite, and will allow you to keep authorship of that piece of code properly, in case it is accepted.

[morozov]

Right, I'm sorry for the whole misunderstanding.

That DQL should work regardless of platform IMHO, since that's how things are documented. It should create a sub query or translate the expression to what the platform understands.

@morozov what do you think about this?

From the pure SQL standpoint, regardless of the platform, the WHERE sclr_9 IN (3) expresion from https://github.com/doctrine/orm/issues/7605#issuecomment-462451566 is invalid. The column alias is usually applied to the resulting recordset and cannot be used in the WHERE clause. It should be WHERE CASE WHEN c0_.id = 1 THEN 0 WHEN c0_.id = 2 THEN 1 ELSE 3 END IN (3) instead.

[tonyfarney]

@greg0ire

I did a pull request for the branch 2.6 in order to be applied to 2.7 too.

Please let me know if I did something wrong (that is my first pull request)

https://github.com/doctrine/orm/pull/7606

[lcobucci]

I apologise for my wrong assumptions, all these years using MySQL have twisted my mind.

Since this is about extending DQL syntax, it's a new feature. Therefore the PR should target master.

The starting point should be a creating the test cases, we can totally help you there and we can begin with a functional test if it makes things easier. Then we proceed with the modification of the code that makes the tests pass. Finally we must update the documentation.

Once we merge this into master we can evaluate the feasibility of back porting it to 2.7.

Since you probably have to solve this in your software now, I'd recommend you to use the Native Query API.

Sorry again for the confusion and inconvenience and please let us know how we can help you with this implementation.

[tonyfarney]

@lcobucci
I tested my modification in my personal test case (real world situation). Should I implement some test case using PHPUnit? I never used test tools so... I need orientation on how to do that.

What should I do about the PR I did yesterday in the 2.6 branch?
https://github.com/doctrine/orm/pull/7606

[lcobucci]

Should I implement some test case using PHPUnit? I never used test tools so... I need orientation on how to do that.

Yes and don't worry we'll support you on this. Functional tests would be best thing to handle this since they are basically a real world situation. You can use https://github.com/doctrine/orm/blob/e8eaf8386de1497863cb400fbc4fddda5878db7e/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6141Test.php as example.

In that test you should provide a valid entity map, perform the operations you want, and assert that the the result matches your expectations.

What should I do about the PR I did yesterday in the 2.6 branch?

7606

If you're familiar to git rebase you can perform an interactive rebase on your branch on top of master, removing all commits that were not made by you. Also modifying the destination branch to master. Otherwise you can close that PR and open a new one :+1:

[tonyfarney]

@lcobucci
I'm creating the GH7605Teste.php. How can I check if my code is according to the Doctrine standards?

[tonyfarney]

@lcobucci My test and the modifications in the parser are done. Should I commit them togheter?

[lcobucci]

@tonyfarney awesome! You can commit them together and send the PR :tada:

[tonyfarney]

It's done!

Thank you so much for all the help @lcobucci

The new PR:
https://github.com/doctrine/orm/pull/7607