search

omines / datatables-bundle

DataTables bundle for Symfony
https://omines.github.io/datatables-bundle/
MIT License
255 stars 114 forks source link

Fix rows count when there is a groupBy #124

Closed simondaigre closed 4 years ago

curry684 commented 4 years ago

This doesn't seem right - the query explicitly runs a COUNT DISTINCT

https://github.com/omines/datatables-bundle/blob/c2f8cb7441e9e49e0c57b826f98e8940480f8be2/src/Adapter/Doctrine/ORMAdapter.php#L238-L241

It should always return a single row.

simondaigre commented 4 years ago

This issue happens for a table with a groupBy and a having conditions. In this case the getCount fails with a NonResultException from Doctrine.

curry684 commented 4 years ago

Mmmmmm ok yes in that case it could go wrong, but I'd daresay the query is completely wrong then. A HAVING clause would be run on the result of the grouping, and as such indeed reduce the result set to none, causing the mentioned exception. Can you share some more details on your specific query?

simondaigre commented 4 years ago

I'm using this request to only display orders who contains at least one product :

$builder
        ->select('o')
        ->from(Order::class, 'o')
        ->leftJoin('o.orderProducts', 'op')
        ->leftJoin('o.customer', 'customer')
        ->where('o.checkoutState = :completed')
        ->groupBy('o.id')
        ->having('COUNT(op) > 0')
        ->setParameter('completed', OrderCheckoutStateType::STATE_COMPLETED)
;
curry684 commented 4 years ago

Why not just join orderProducts with a regular inner join instead of a leftJoin?

I see your case but using a HAVING clause for this is both inefficient (you're producing too many rows to filter them after the act) and killing some of the magic of the ORM adapter as you noticed. Inner join would fix both issues, or a subquery which would also be more efficient.

In general I consider using HAVING in OLTP environments bad practice as it is most often applied as a band aid to repair issues better fixed elsewhere. For OLAP (or reporting in general) it obviously has many justifiable uses.

curry684 commented 4 years ago

I'm closing the PR btw as it is itself just a band aid that would cause unexpected results elsewhere.

We may need to consider either disabling HAVING clauses in the ORM adapter, or at least documenting its risks, but just ignoring the issues and returning 0 is not a fix.

simondaigre commented 4 years ago

Got it, thanks 👍