aarondfrancis / fast-paginate

A fast implementation of offset/limit pagination for Laravel.
MIT License
1.21k stars 56 forks source link

SQL query breaks with an aggregated column in 'having clause' #12

Closed dmanva closed 2 years ago

dmanva commented 2 years ago

Hello!

Using aggregated columns in 'having clause' currently breaks the query.

Example (default Laravel's migrations and model are used):

User::query()->selectRaw('*, CONCAT(name, email) as name_email')->having('name_email', '!=', '')->paginate();

But the same query with fastPaginate

User::query()->selectRaw('*, CONCAT(name, email) as name_email')->having('name_email', '!=', '')->fastPaginate();

doesn't work and causes an error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'name_email' in 'having clause'

P.S. Thank you for the package and the idea itself

adrianb93 commented 2 years ago

It makes sense why it doesn't work to me. You could submit a PR that keeps any selects with an " as " in it or has an open and closed parenthesis: https://github.com/hammerstonedev/fast-paginate/blob/7910d15d4c55fc5a67080ebed113e6aeb9d49cd8/src/BuilderMixin.php#L22-L30

aarondfrancis commented 2 years ago

Hmm yeah this not a use case I thought of. I'm wondering if I should just defer to paginate if there's a having? It gets a little messy trying to know what all is added to the select. And there are definitely cases where the select contains an expensive subselect that's not referenced in the where at all. Thoughts?

aarondfrancis commented 2 years ago

Ok, added a fix to skip it if there's a having https://github.com/hammerstonedev/fast-paginate/blob/757c71d8c2726f02f843694afeb666e3473c66c1/src/BuilderMixin.php#L19-L21

aarondfrancis commented 2 years ago

Let me know what yall think and I can cut a release that includes this

dmanva commented 2 years ago

Well, I don't have any better solution than proposed skip...

aarondfrancis commented 2 years ago

Yeah at least this makes it work while we consider potentially more sophisticated solutions in the future. Ok, tagging!