Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from May 15, 2024

Conversation

hebinet
Copy link
Contributor

@hebinet hebinet commented Dec 4, 2023

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
Copy link
Contributor Author

hebinet commented Dec 12, 2023

@aarondfrancis Any updates on this?

@d8vjork
Copy link

d8vjork commented Dec 27, 2023

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

@d8vjork
Copy link

d8vjork commented Dec 27, 2023

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
Copy link
Contributor

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!

@aarondfrancis aarondfrancis merged commit 8016238 into hammerstonedev:main May 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants