Orm: Ordering with arithmetic expression

[sgehrig]

Bug Report

| Q | A
|------------ | ------
| BC Break | no
| Version | v2.7.0

Summary

Ordering with arithmetic expression does not work as I'd expect it to work after reading the DQL EBNF.

Current behavior

This is a simplified example of the actual query.

SELECT 
    j,
    (SELECT SUM(i.amount) FROM Invoice i WHERE i.job = j) AS HIDDEN invoiced_amount,
    (SELECT COALESCE(SUM(t.orderedAmount), j.orderedAmount) FROM Task t WHERE t.job = j) AS HIDDEN current_ordered_amount
FROM Jobs j
ORDER BY  current_ordered_amount - invoiced_amount ASC

Running the query, [Syntax Error] line 0, col xxx: Error: Expected end of string, got '-' is thrown.

How to reproduce

Run a query similar so the one above with an arithmetic expression in the ORDER BY clause.

Expected behavior

I'd expect the query to work because of what's stated in the documentation:

OrderByClause ::= "ORDER" "BY" OrderByItem {"," OrderByItem}*
OrderByItem ::= (SimpleArithmeticExpression | ...) ["ASC" | "DESC"]
SimpleArithmeticExpression ::= ArithmeticTerm {("+" | "-") ArithmeticTerm}*
ArithmeticTerm ::= ArithmeticFactor {("*" | "/") ArithmeticFactor}*
ArithmeticFactor ::= [("+" | "-")] ArithmeticPrimary
ArithmeticPrimary ::= ... | ResultVariable | ...
/* ResultVariable identifier usage of mapped field aliases (the "total" of "COUNT(*) AS total") */
ResultVariable = identifier

Is there something I missed? Or misinterpreted?

[sgehrig]

I've created a test case:

https://github.com/sgehrig/orm/commit/c60272b4b8d5c125f3b0afc118f84fece39176d1

[SenseException]

Hi @sgehrig, if you like you can create an PR for your test case that can be later used for an implementation. But instead of letting the test count the results, I would expect the test to check the correct order of the result.

Have you tried a simpler query like SELECT f FROM Foo f ORDER BY f.id - 1?

[sgehrig]

Hi @SenseException, sure no problem. I've also added some more test cases and found some interesting things:

• ORDER BY p.id + p.id ASC ✅
• ORDER BY 1 + p.id ASC 🚫
• ORDER BY p.id + 1 ASC ✅
• ORDER BY s + 1 DESC 🚫 (where s is a ResultVariable)
• ORDER BY 1 + s DESC 🚫 (where s is a ResultVariable)
• ORDER BY s + p.id DESC 🚫 (where s is a ResultVariable)
• ORDER BY p.id + s DESC ✅ (where s is a ResultVariable)

These cases are all represented in the test case PR.

[sgehrig]

@SenseException please see https://github.com/doctrine/orm/pull/8012

[sgehrig]

I've been doing some debugging using the test cases provided in #8012. I'm not an expert in the DQL parser though, but I assume the bug somehow originates from

https://github.com/doctrine/orm/blob/fdad48278b6c0d0d7075de584e6982969bf430c6/lib/Doctrine/ORM/Query/Parser.php#L1497

When running the successful test cases $peek returns the arithmetic operator (+ for example) and therefore the arithmetic expression is detected correctly. When running the failing test cases $peek however returns the DESC keyword skipping the whole arithmetic expression.

Just my two cents.

[sgehrig]

As this issue becomes more and more critical for us, I'd really like to help here with a PR. But I'll need some guidance on the current source code to understand why it has been written the way it currently is.

[sgehrig]

I just found that enclosing the expression in double brackets (((...))) seems to trick the parser into correctly handling the order by item:

• ORDER BY p.id + p.id ASC ✅
• ORDER BY 1 + p.id ASC 🚫
  
  
  
  • ORDER BY ((1 + p.id)) ASC ✅
  
  
• ORDER BY p.id + 1 ASC ✅
• ORDER BY s + 1 DESC 🚫 (where s is a ResultVariable)
  
  
  
  • ORDER BY ((s + 1)) DESC ✅
  
  
• ORDER BY 1 + s DESC 🚫 (where s is a ResultVariable)
  
  
  
  • ORDER BY ((1 + s)) DESC ✅
  
  
• ORDER BY s + p.id DESC 🚫 (where s is a ResultVariable)
  
  
  
  • ORDER BY ((s + p.id)) DESC ✅
  
  
• ORDER BY p.id + s DESC ✅ (where s is a ResultVariable)

Still, I'd consider this a bug.