aarondfrancis / fast-paginate

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

Custom Selects with an additional order by or query constraint ar not working #57

Closed hebinet closed 5 months ago

hebinet commented 10 months ago

Hi Aaron,

thanks for this package! This has speed up our queries tremendously :)

We encountered a little inconvenience with custom selects used in order by or where clauses.

For example, if you have a query llike this:

User::query()
  ->selectRaw('(select 1 as computed_column)')
  ->orderBy('computed_column')
  ->fastPaginate()

or that

User::query()
  ->selectRaw('(select 1 as computed_column)')
  ->where('computed_column', 1)
  ->fastPaginate()

We get an error that the column doesn't exist. The problem here is, that the "Inner"-query is stripping away all the custom selects. I added some failing tests in this PR so you can see this in action.

I think its for performance reasons because in most of the cases you only need the ids for the inner query.

What do you think, is there any possibility this gets fixed/added?

King regards Markus

hebinet commented 10 months ago

@aarondfrancis Any updates on this?

d8vjork commented 9 months ago

Really need this in some of my projects and Laravel packages that integrates with this one!

d8vjork commented 9 months ago

Ok found the solution, you need to wrap your as column_name of the end with back quotes `

Like this:

$query->selectRaw('SQL_FUNCTION(column) as `another_column`')->order('another_column');

Another solution will be to add this str_contains($column, str_replace('', '', $order))` on FastPaginate.php:133 resulting on the following:

        return collect($base->columns)
            ->filter(function ($column) use ($orders, $base) {
                $column = $column instanceof Expression ? $column->getValue($base->grammar) : $base->grammar->wrap($column);

                foreach ($orders as $order) {
                    // If we're ordering by this column, then we need to
                    // keep it in the inner query.
                    if (str_contains($column, "as $order") || str_contains($column, str_replace('`', '', $order))) {
                        return true;
                    }
                }

                // Otherwise we don't.
                return false;
            })
aarondfrancis commented 5 months ago

Thanks for this! I've fixed the order by part, but I can't quite crack the where thing.

In your test you have:

select `users`.`id`, (select 1 as computed_column) from `users` where `computed_column` = 1

Simplified, I think that should be

select `users`.`id`, 1 as computed_column from `users` where `computed_column` = 1

Which doesn't work, even as plain ol SQL. I don't think you can use an aliased column like that. You can do it with a HAVING

select `users`.`id`, 1 as computed_column from `users` HAVING `computed_column` = 1

but that seems different than what you're doing.

I'm going to remove the where test and merge it. Please feel free to comment back here or open a new PR for the other part!